Eh, this line:
"""
But it turns out that openssl verifies it fine as does firefox and chromium.
Apparently they just reoder the chain if needed inside
gtlsdatabase-gnutls.c:build_certificate_chain().
"""

Should of course read:
"""
But it turns out that openssl verifies it fine as does firefox and chromium.
Apparently they just reoder the chain if needed.

It would be nice if glib-networking could also do a reodering inside e.g. gtlsdatabase-gnutls.c:build_certificate_chain()
"""

Sorry for this misleading sentence.

(In reply to comment #1)
> It would be nice if glib-networking could also do a reodering inside e.g.
> gtlsdatabase-gnutls.c:build_certificate_chain()

That chain is passed in g_tls_database_gnutls_verify_chain() into gnutls_x509_crt_list_verify(), which is supposed to handle unordered certs automatically. [1] (For older versions of GnuTLS you have to pass that GNUTLS_VERIFY_ALLOW_UNSORTED_CHAIN, which glib-networking does not, but my GnuTLS is new enough to not require that flag, and adding it does not change the failing result.)

[1] http://gnutls.org/manual/html_node/Verifying-a-certificate-in-the-context-of-TLS-session.html#gnutls_005fcertificate_005fverify_005fflags

I think the problem is in gtlsconnection-gnutls.c get_peer_certificate_from_session(), which creates a chain of GTlsCertificates, setting their issuer fields incorrectly if the next cert in the chain is not the issuer.  I think build_certificate_chain() could perhaps work as-is if that's fixed, but I'm not sure.

*** Bug 702998 has been marked as a duplicate of this bug. ***

Created attachment 265351 [details] [review]
Accept unordered certificate chains

Some TLS servers improperly send an unordered chain of certificates,
where the next certificate in the chain is not the issuer of the current
certificate. Recent versions of GnuTLS will verify the chain anyway to
help reduce unnecessary validation failures (since there is no security
risk in doing so).

When the certificates are unordered, get_peer_certificate_from_session()
will construct GTlsCertificates with incorrect issuer fields, causing
no end of trouble.

FYI: this fix is tested on wiki.ubuntu.com and libreoffice.org, both of which fail without this patch.

The testcase in the first post uses securesite.co.uk, which is currently correctly failing because that site is sending a cert that's only valid for secure1.securesite.co.uk.

Created attachment 265352 [details] [review]
Accept unordered certificate chains

Some TLS servers improperly send an unordered chain of certificates,
where the next certificate in the chain is not the issuer of the current
certificate. Recent versions of GnuTLS will verify the chain anyway to
help reduce unnecessary validation failures (since there is no security
risk in doing so).

When the certificates are unordered, get_peer_certificate_from_session()
will construct GTlsCertificates with incorrect issuer fields, causing
trouble with unordered chains even though GnuTLS would otherwise handle
these fine.

Comment on attachment 265352 [details] [review]
Accept unordered certificate chains

>+      if (gnutls_x509_crt_init (&gnutls_certs[i]) < 0 ||
>+          gnutls_x509_crt_import (gnutls_certs[i], &raw_certs[i], GNUTLS_X509_FMT_DER) < 0)
>+        {
>+          cert = NULL;
>+          goto out;
>+        }

hm... ok, all this stuff really ought to be in gtlscertificategnutls.c... I think what I'd like is to have get_peer_certificate_from_session() just get the raw_certs array from the session, and then pass that to some new method, eg, g_tls_certificate_gnutls_build_chain(), which will return the correctly-sorted certificate chain. (ie, that function will contain all of the rest of the new code you wrote).

>+  for (i = 0; i < num_certs; i++)
>+    g_ptr_array_add (glib_certs,
>+                     g_tls_certificate_gnutls_new (&raw_certs[i], NULL));

style nitpick; use {}s around multi-line bodies, even if it's only a single statement.


Also, can you add a test case to tls/tests/certificate.c as well? (This will presumably require adding a new certificate file as well... feel free to just grab the certs from some real web server that's currently failing. That's what we did for some of the other tests.)

Created attachment 266759 [details] [review]
Accept unordered certificate chains

Updated except for the test, looking into that now

I'm probably going to need help to complete a test case for this.

The natural point to test would be g_tls_certificate_gnutls_build_chain(), but it looks like I can only access the generic gio interface within the tests. (Or should I try dlopening the loadable module? Will bad things happen if we do that twice in the same process?)

So the next step up to test would be g_tls_connection_handshake() itself. So looking in tests/connection.c to see how to set that up, I guess we'd need a more elaborate (unordered) chain of trust for our test server. I don't think I know enough to set that up myself.

It'd be easy to test by connecting to a real server, like in the head post, but I doubt you want a test that's sure to break in a couple of years.

Hm... I was thinking you could just modify test_basic_connection() in connection.c, to have the server use a different cert/key file, but it looks like GTlsServerConnectionGnutls only ever sends a single certificate anyway, not the whole chain, so there'd be no way to get it to send an out-of-order chain.

I'll have to think some more about how to test this... anyway, I guess maybe it can go in without a test for now.

(I still need to re-review the code though.)

Review of attachment 266759 [details] [review]:

::: tls/gnutls/gtlscertificate-gnutls.c
@@ +586,3 @@
+        {
+          for (j = i - 1; j >= 0; j--)
+            gnutls_x509_crt_deinit (gnutls_certs[j]);

Small bug here: if gnutls_x509_crt_init() succeeds but gnutls_x509_crt_import() fails, one cert will be leaked.

Created attachment 276711 [details] [review]
Accept unordered certificate chains

No big changes, just fixed that silly memory leak.

Comment on attachment 276711 [details] [review]
Accept unordered certificate chains

Don't use g_ptr_array_index(); it's a silly function. Just say "glib_certs->pdata[j]"

If there are extra certificates in the chain, this will leak their GTlsCertificates, because you're only unreffing certificates if they become the issuer of another certificate. I think what you need to do is something like:

  glib_certs = g_ptr_array_new_full (num_certs, g_object_unref); // <-- using a free_func
  // fill in glib_certs...

  for (i = 0; i < num_certs; i++) {
      // find issuer...
      if (issuer)
          g_tls_certificate_gnutls_set_issuer (glib_certs->pdata[i], issuer)
  }

  result = g_object_ref (glib_certs->pdata[0]);
  g_ptr_array_unref (glib_certs);

So, every element of glib_certs gets unreffed at the end when you free glib_certs, but you add an extra ref to the initial element to keep it alive.


Also, now that this can be called independently of handshaking, it should be easy to add tests for it, that read in a file and then call g_tls_certificate_gnutls_build_chain() and verify the result. We'd want to test with (a) missing certificates, (b) out-of-order certificates, (c) duplicate certificates.

Thanks for the review, Dan!

(In reply to comment #14)
> Don't use g_ptr_array_index(); it's a silly function. Just say
> "glib_certs->pdata[j]"

That's easier to read, thanks.

> If there are extra certificates in the chain, this will leak their
> GTlsCertificates, because you're only unreffing certificates if they become the
> issuer of another certificate.

After a bit of squinting, I see you are right. Will fix that as you suggested.

> Also, now that this can be called independently of handshaking, it should be
> easy to add tests for it, that read in a file and then call
> g_tls_certificate_gnutls_build_chain() and verify the result. We'd want to test
> with (a) missing certificates, (b) out-of-order certificates, (c) duplicate
> certificates.

Good point, I think that would be much easier. I'll work on adding these.

Created attachment 279488 [details] [review]
Accept unordered certificate chains

Dan, apologies for the delay on this patch. I believe I addressed the concerns in your review and it'd be great if you could take another look at the code changes now.

I also include two test cases; however, they aren't ready for review as they have a few serious problems:

* To gain access to g_tls_certificate_gnutls_build_chain() I linked the tests with libgiognutls.la. libtool warns that this is not portable, since it's a dynamic shared object.
* I had to #include "gtlscertificate-gnutls.c", not .h, which is horrible.
* It doesn't even work: glib complains that I'm registering GTlsCertificateGnutls twice, since I am.

I'm not sure how to get around this, besides to give up and test g_tls_connection_handshake() instead, like I suggested in comment #10. (Do you have a suggestion?) I think I'm going to try this now.

Another issue: you'll have to comment out your new create-list test to even get as far as mine, since it is failing, though I'm not sure why since creating lists works great in my tests. This error is not introduced by my patch:

GLib-Net:ERROR:certificate.c:235:test_create_list: assertion failed (g_list_length (list) == 8): (0 == 8)

(In reply to comment #16)
> Another issue: you'll have to comment out your new create-list test to even get
> as far as mine, since it is failing, though I'm not sure why since creating
> lists works great in my tests. This error is not introduced by my patch:

Er just kidding, I solved this by rebuilding glib. Huh.

Created attachment 279508 [details]
updated testcase

(In reply to comment #16)
> I'm not sure how to get around this, besides to give up and test
> g_tls_connection_handshake() instead, like I suggested in comment #10. (Do you
> have a suggestion?) I think I'm going to try this now.

I think this can only happen if GTlsServerConnection gains support for sending a chain of certs, like you wrote in comment #11. So I have no clue how to create a proper test case for this. :/  I think it's important to do a test case if possible, since it would otherwise be difficult to notice if this is broken, but I'd like to make sure this makes 2.42 regardless.

You can use the attached test and do a simple before/after, though; it's the same as the attachment in the first post, except the URL in the first test is now broken. This one tests libreoffice.org, which sends its own cert twice at the start of the list, and also gentoo.org, which is simply out of order.

Created attachment 279509 [details] [review]
Accept unordered certificate chains

Same as the patch above, but without the broken attempts at testing.

Matthias, since you asked why this is on the blocker list, it's because Epiphany just started to block untrusted connections (which we do not want to delay -- it's stupid insecure otherwise), and without this fix we block too much and will train users to ignore our "this site might be evil" warnings.

Comment on attachment 279509 [details] [review]
Accept unordered certificate chains

>+convert_data_to_gnutls_certs (const gnutls_datum_t *certs,
>+                              guint                 num_certs,
>+                              gnutls_x509_crt_fmt_t format)

>+g_tls_certificate_gnutls_build_chain (const gnutls_datum_t *certs,
>+                                      guint                 num_certs,
>+                                      gnutls_x509_crt_fmt_t format)

tiny style nitpick: add an extra space between the types and variable names (so there's one completely empty column between the two blocks)

otherwise the code looks good. I would prefer it if the code did not get committed until we had tests for it too, since otherwise there's a good chance the tests will never get written. But if this is a 3.14 blocker then I guess we have to get it in now anyway. Just don't close the bug.

(In reply to comment #18)
> (In reply to comment #16)
> > I'm not sure how to get around this, besides to give up and test
> > g_tls_connection_handshake() instead, like I suggested in comment #10. (Do you
> > have a suggestion?) I think I'm going to try this now.
> 
> I think this can only happen if GTlsServerConnection gains support for sending
> a chain of certs, like you wrote in comment #11.

oh, actually, this functionality did get added (bug 724708), so it might be possible to test the code this way now

Created attachment 284902 [details] [review]
Accept unordered certificate chains

Hey Aleix, can you provide some guidance on how to use the glib API to construct a chain of certificates?  We should document this new feature and add a test case for it as well.

I spent some time working on the tests today, but when I realized that I would need to create the intermediate CA's certificate and set all of its properties manually with g_object_new() (since issuer is a construct-only property), I took another look at bug #729739 and realized that bug already has test cases with an intermediate CA, and that everything I did today (mostly playing around with the script to generate the certificates) will conflict badly with the tests in that bug.  The right approach is to wait until the tests in that bug are pushed, as then it will be pretty simple to write tests for this bug. But I guess since we've passed API freeze, that won't happen prior to GNOME 3.14, so we'd better push this patch now.  I do intend to finish test cases for this soon after bug #729739 is resolved, so I'll leave this bug open and assigned to me in the meantime.

Comment on attachment 284902 [details] [review]
Accept unordered certificate chains

Attachment 284902 [details] pushed as 0e08f17 - Accept unordered certificate chains

(In reply to comment #24)
> Hey Aleix, can you provide some guidance on how to use the glib API to
> construct a chain of certificates?  We should document this new feature and add
> a test case for it as well.

Hi Michael. Not sure if you still need this information considering comment 25. Just in case, bug #729739 adds functionality to the following functions:

   g_tls_certificate_new_from_pem
   g_tls_certificate_new_from_file
   g_tls_certificate_new_from_files

Basically, they work as before but now, they will try to load all the certificates in the file and create a certificate chain. If they can't create a certificate chain they will still return the first certificate found (as before). The certificates should be in order, root CA at the bottom.

Everything is documented in the documentation of each function in the patch.

I just addressed Dan's comments, let's see if I can commit soon.

Yeah, I hadn't seen bug #729739 when I wrote that comment. That looks good. Thanks for working on this!

See: bug #754129.