GNOME Bugzilla – Bug 765317
WebKit Network process hangs in GTlsConnectionGnutls claim_op
Last modified: 2016-05-09 16:02:42 UTC
See the WebKit bug https://bugs.webkit.org/show_bug.cgi?id=156686 for detailed backtrace. This could be a regression introduced in #736809 or #735754. When the network process is blocked in the poll, the status when claim_op was called is: op: CLOSE_BOTH, read closing/closed: no/no, write closing/closed: no/no, need handshake: no, handshaking: yes, need_finish_handshake: no, implicit_handshake: no, reading: yes, writing: yes
It seems the problem is actually a secondary thread blocked in a lock, see:
+ Trace 236195
Thread 2 (Thread 0x7f68a27fc700 (LWP 12192))
No, it's not blocked on a lock, but just in an infinite loop in convert_certificate_chain_to_gnutls() for (*gnutls_chain_length = 0, cert = G_TLS_CERTIFICATE (chain); cert; cert = g_tls_certificate_get_issuer (cert))
There's a cycle in the certificate chain we are building, and the problematic certificate comes from this URL: https://www.westminster.ac.uk/sites/default/files/styles/landscape_wide/public/event/image/libre-graphics.jpg?itok=SIQiRl61×tamp=1460021761 Chromium and firefox both show that without problems and say the certificate is valid, but epiphany hangs because of the infinite loop in the network process.
Created attachment 326477 [details] [review] gnutls: Prevent cycles when building the certificate chain
Review of attachment 326477 [details] [review]: ::: tls/gnutls/gtlscertificate-gnutls.c @@ +741,3 @@ } + if (issuer) { This should be indented GNU style, same as you did for the for loop.
Review of attachment 326477 [details] [review]: OK, I just realized I wrote this code... I agree this is the right place to fix this issue. ::: tls/gnutls/gtlscertificate-gnutls.c @@ +749,3 @@ + { + if (issuer == glib_certs->pdata[k]) + continue; The loop says you check if the issuer is not already in the chain, but here you skip over the issuer if it's in the chain. This is confusing because there are two different chains here: the array glib_certs that has already been formed, and the chain currently being built, formed by following g_tls_certificate_get_issuer() recursively starting from the first cert in glib_certs. Your loop here primarily exists to check for the issuer in the later chain, but first checks for the issuer in the former chain (where it is guaranteed to exist). So: 1) I think the comment needs to be clearer to account for this, because as written, the continue statement seems to be contradicting the comment. 2) I've been trying to figure out why the continue statement is there at all. Why should we skip the check in this case? Can't this lead to an issuer cycle if a cert sent earlier in the chain is self-signed, and signs a cert sent later in the chain? (Yes, a pretty strangely broken server if that happens, but this is code exists only to account for broken servers.) I suspect your code is right as usual and I'm just missing something here, but I've been thinking about this for too long, so you'll have to tell me what.
(In reply to Michael Catanzaro from comment #6) > (where it is guaranteed to exist). (Actually that's not true... doesn't affect the rest of my comments, though.)
I don't know why I did that either, but it seems to be wrong indeed, I'll submit a new patch.
Created attachment 326830 [details] [review] gnutls: Prevent cycles when building the certificate chain Updated patch
Review of attachment 326830 [details] [review]: LGTM now. I'm going to put this into Fedora now due to the severity of this issue. I think we should wait for review from Dan before committing upstream.
(In reply to Michael Catanzaro from comment #6) > 2) I've been trying to figure out why the continue statement is there at > all. Why should we skip the check in this case? Can't this lead to an issuer > cycle if a cert sent earlier in the chain is self-signed, and signs a cert > sent later in the chain? That won't actually cause a problem; self-signed certs normally have a NULL issuer, so the later cert will point to the earlier cert, but the earlier cert will point to NULL and there is no loop. The bug is that we don't explicitly check for self-signedness; we just skip over the cert itself when looking for its issuer (the "j != i" in the existing code) so issuer stays NULL by default. And that *normally* works, but in this case, the site is returning a chain with two copies of the same self-signed cert: C[0] = site, issued by ICA G2 C[1] = ICA G2, issued by Root CA 2 C[2] = Root CA 2, issued by Root CA 2 C[3] = Root CA 2, issued by Root CA 2 and so we decide that C[2] is signed by C[3], and C[3] is signed by C[2] and boom. But this is easily fixed without needing to loop over anything; just explicitly check if the cert issued itself, and give it a NULL issuer in that case, rather than relying on the failure of the issuer-finding loop to leave issuer NULL by default.
Created attachment 326915 [details] [review] gnutls: Prevent cycles when building the certificate chain Thanks for the explanation Dan, this is indeed simpler.
Review of attachment 326915 [details] [review]: ::: tls/gnutls/gtlscertificate-gnutls.c @@ +727,3 @@ + { + g_tls_certificate_gnutls_set_issuer (glib_certs->pdata[i], NULL); + continue; Surely you can just continue without the call to g_tls_certificate_gnutls_set_issuer?
(In reply to Michael Catanzaro from comment #13) > Review of attachment 326915 [details] [review] [review]: > > ::: tls/gnutls/gtlscertificate-gnutls.c > @@ +727,3 @@ > + { > + g_tls_certificate_gnutls_set_issuer (glib_certs->pdata[i], NULL); > + continue; > > Surely you can just continue without the call to > g_tls_certificate_gnutls_set_issuer? I thought about that, but Dan said: "But this is easily fixed without needing to loop over anything; just explicitly check if the cert issued itself, and give it a NULL issuer in that case, rather than relying on the failure of the issuer-finding loop to leave issuer NULL by default."
not calling set_issuer() is fine; we know the issuer is NULL to start with here
Created attachment 326942 [details] [review] gnutls: Prevent cycles when building the certificate chain
Attachment 326942 [details] pushed as 7db4dbf - gnutls: Prevent cycles when building the certificate chain
It was discovered that glib-networking's certificate verification code is vulnerable to a denial of service if a TLS server sends the same self-signed certificate multiple times in a certificate chain, which can hang the client. A DWF assignment has been requested for this issue.
*** Bug 766016 has been marked as a duplicate of this bug. ***