GNOME Bugzilla – Bug 793712
segfault on a TLS connection without private key in g_tls_connection_set_certificate()
Last modified: 2018-03-05 02:40:42 UTC
Created attachment 368745 [details] reproducer Normally, g_tls_connection_set_certificate() expects a public/private key pair; if /tmp/cert-and-key.pem has both the cert and the key, things work fine: GTlsCertificate *cert = g_tls_certificate_new_from_file ("/tmp/cert-and-key.pem", &error); g_tls_connection_set_certificate (G_TLS_CONNECTION (tls_con), cert); But when the file only contains the certificate (and they key in a separate file), one gets a crash GTlsCertificate *cert = g_tls_certificate_new_from_file ("/tmp/cert.pem", &error); g_tls_connection_set_certificate (G_TLS_CONNECTION (tls_con), cert); Starting program: /home/martin/src/test/a.out [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib64/libthread_db.so.1". [New Thread 0x7ffff52ad700 (LWP 3065)] ** Message: successfully connected [New Thread 0x7fffef17f700 (LWP 3066)] Thread 3 "pool" received signal SIGSEGV, Segmentation fault.
+ Trace 238421
Thread 140737204713216 (LWP 3066)
I'm attaching a full reproducer; it has the correct cert-and-key line commented out, and the incorrect "only cert" line is active. It expects /tmp/cert.pem, /tmp/cert-and-key.pem, and /tmp/ca.pem to exist, and hardcodes the server to connect to, so for running it you need to adjust this accordingly. This should rather fail with a proper GError, not simply crash.
Indeed. Since certificates could be provided by users, this should be resilient to malformed/garbage input files too. This needs fuzzing (and the bug you’ve already got a backtrace for needs fixing).
Created attachment 368917 [details] [review] Fix crash when setting client cert without private key If, if g_tls_client_connection_gnutls_retrieve_function, we return to GnuTLS a gnutls_retr2_st with positive ncerts but no key, GnuTLS will crash. GnuTLS should probably become robust to this scenario, just as it must be robust to any manner of invalid input here. But it makes sense for us to handle it, as well. We must abort the handshake in this case. A new test is added that triggers this crash. Thanks to Martin Pitt for contributing a reproducer that formed the basis for this test.
Created attachment 368918 [details] [review] Add a couple garbage certificate tests Ensure that creating a GTlsCertificate using a JPEG astronaut returns NULL. Also ensure that a GTlsDatabase containing only a JPEG astronaut does not permit successful certificate verification. More such tests are possible, but these are easy and a good start.
Thanks Michael, patches look great. I like the idea of throwing space themed garbage at it :-)
Review of attachment 368917 [details] [review]: From a really quick look, I can’t see any obvious problems with this.
Review of attachment 368918 [details] [review]: A couple of nitpicks. If you put together a quick program which loads a certificate file from argv[1], you could fuzz test it using afl with these commands: CC=afl-gcc jhbuild make -ac mkdir -p /tmp/fuzz/input /tmp/fuzz/output # copy a few example valid certificates which are structurally different from each other into /tmp/fuzz/input AFL_SKIP_CPUFREQ=1 afl-fuzz -m 100 -i /tmp/fuzz/input/ -o /tmp/fuzz/output/ -f /tmp/fuzz/in.pem -- my-test-program /tmp/fuzz/in.pem # leave it running for quite a long time (a day or two) ::: tls/tests/certificate.c @@ +234,3 @@ + + cert = g_tls_certificate_new_from_file (tls_test_file_path ("garbage.pem"), &error); + g_assert (cert == NULL); g_assert_null (cert); g_assert() will be optimised out if compiled with G_DISABLE_ASSERT. g_assert_null() won’t (and will give a more informative error message). @@ +239,3 @@ + + cert = g_tls_certificate_new_from_pem ("I am not a very good certificate.", -1, &error); + g_assert (cert == NULL); Same. ::: tls/tests/connection.c @@ +2088,3 @@ + test->database = g_tls_file_database_new (tls_test_file_path ("garbage.pem"), &error); + g_assert_no_error (error); + g_assert (test->database); g_assert_nonnull (test->database); @@ +2093,3 @@ + test->client_connection = g_tls_client_connection_new (connection, test->identity, &error); + g_assert_no_error (error); + g_assert (test->client_connection); g_assert_nonnull (test->client_connection);
(In reply to Philip Withnall from comment #6) > Review of attachment 368918 [details] [review] [review]: > > A couple of nitpicks. > > If you put together a quick program which loads a certificate file from > argv[1], you could fuzz test it using afl with these commands: > > CC=afl-gcc jhbuild make -ac > mkdir -p /tmp/fuzz/input /tmp/fuzz/output > # copy a few example valid certificates which are structurally different > from each other into /tmp/fuzz/input > AFL_SKIP_CPUFREQ=1 afl-fuzz -m 100 -i /tmp/fuzz/input/ -o /tmp/fuzz/output/ > -f /tmp/fuzz/in.pem -- my-test-program /tmp/fuzz/in.pem > # leave it running for quite a long time (a day or two) This would be good to do, yes. If anyone wants to attempt it. ;) > ::: tls/tests/certificate.c > @@ +234,3 @@ > + > + cert = g_tls_certificate_new_from_file (tls_test_file_path > ("garbage.pem"), &error); > + g_assert (cert == NULL); > > g_assert_null (cert); > > g_assert() will be optimised out if compiled with G_DISABLE_ASSERT. > g_assert_null() won’t (and will give a more informative error message). True. Let me attach a follow-up patch to change the tests to g_assert_null() and g_assert_nonnull().
Review of attachment 368917 [details] [review]: ::: tls/gnutls/gtlsconnection-gnutls.c @@ -656,3 @@ - st->cert_type = GNUTLS_CRT_X509; - st->ncerts = 0; It's actually not right to delete this, since the return value (st) would then be unusable if g_tls_connection_get_certificate() returns null, and there would be no way to detect that using st.
(In reply to Michael Catanzaro from comment #7) > True. Let me attach a follow-up patch to change the tests to g_assert_null() > and g_assert_nonnull(). Reported bug #793856.
Created attachment 368960 [details] [review] Fix crash when setting client cert without private key If, if g_tls_client_connection_gnutls_retrieve_function, we return to GnuTLS a gnutls_retr2_st with positive ncerts but no key, GnuTLS will crash. GnuTLS should probably become robust to this scenario, just as it must be robust to any manner of invalid input here. But it makes sense for us to handle it, as well. We must abort the handshake in this case. A new test is added that triggers this crash. Thanks to Martin Pitt for contributing a reproducer that formed the basis for this test.
Created attachment 368961 [details] [review] Use gnutls_certificate_set_retrieve_function2() The GnuTLS manual says this is significantly more efficient than using gnutls_certificate_set_retrieve_function(). It certainly is a bit safer. It's easy to forget to initialize a member of the gnutls_retr2_st, but harder to forget about function parameters.
Created attachment 368962 [details] [review] Add a couple garbage certificate tests Ensure that creating a GTlsCertificate using a JPEG astronaut returns NULL. Also ensure that a GTlsDatabase containing only a JPEG astronaut does not permit successful certificate verification. More such tests are possible, but these are easy and a good start.
Created attachment 368963 [details] [review] Fix crash when setting client cert without private key If, if g_tls_client_connection_gnutls_retrieve_function, we return to GnuTLS a gnutls_retr2_st with positive ncerts but no key, GnuTLS will crash. GnuTLS should probably become robust to this scenario, just as it must be robust to any manner of invalid input here. But it makes sense for us to handle it, as well. We must abort the handshake in this case. A new test is added that triggers this crash. Thanks to Martin Pitt for contributing a reproducer that formed the basis for this test.
Created attachment 368964 [details] [review] Use gnutls_certificate_set_retrieve_function2() The GnuTLS manual says this is significantly more efficient than using gnutls_certificate_set_retrieve_function(). It certainly is a bit safer. It's easy to forget to initialize a member of the gnutls_retr2_st, but harder to forget about function parameters.
Created attachment 368965 [details] [review] Add a couple garbage certificate tests Ensure that creating a GTlsCertificate using a JPEG astronaut returns NULL. Also ensure that a GTlsDatabase containing only a JPEG astronaut does not permit successful certificate verification. More such tests are possible, but these are easy and a good start.
Created attachment 368966 [details] [review] Fix crash when setting client cert without private key If, if g_tls_client_connection_gnutls_retrieve_function, we return to GnuTLS a gnutls_retr2_st with positive ncerts but no key, GnuTLS will crash. GnuTLS should probably become robust to this scenario, just as it must be robust to any manner of invalid input here. But it makes sense for us to handle it, as well. We must abort the handshake in this case. A new test is added that triggers this crash. Thanks to Martin Pitt for contributing a reproducer that formed the basis for this test.
Created attachment 368967 [details] [review] Use gnutls_certificate_set_retrieve_function2() The GnuTLS manual says this is significantly more efficient than using gnutls_certificate_set_retrieve_function(). It certainly is a bit safer. It's easy to forget to initialize a member of the gnutls_retr2_st, but harder to forget about function parameters.
Created attachment 368968 [details] [review] Add a couple garbage certificate tests Ensure that creating a GTlsCertificate using a JPEG astronaut returns NULL. Also ensure that a GTlsDatabase containing only a JPEG astronaut does not permit successful certificate verification. More such tests are possible, but these are easy and a good start.
GitLab's killer feature will making it a lot less obvious when I need to resubmit a patch. ;)
Review of attachment 368966 [details] [review]: ::: tls/gnutls/gtlsconnection-gnutls.c @@ -656,3 @@ - st->cert_type = GNUTLS_CRT_X509; - st->ncerts = 0; I guess I did something wrong with git, because this is still here. OK, trying again....
Created attachment 368969 [details] [review] Fix crash when setting client cert without private key If, if g_tls_client_connection_gnutls_retrieve_function, we return to GnuTLS a gnutls_retr2_st with positive ncerts but no key, GnuTLS will crash. GnuTLS should probably become robust to this scenario, just as it must be robust to any manner of invalid input here. But it makes sense for us to handle it, as well. We must abort the handshake in this case. A new test is added that triggers this crash. Thanks to Martin Pitt for contributing a reproducer that formed the basis for this test.
Created attachment 368970 [details] [review] Use gnutls_certificate_set_retrieve_function2() The GnuTLS manual says this is significantly more efficient than using gnutls_certificate_set_retrieve_function(). It certainly is a bit safer. It's easy to forget to initialize a member of the gnutls_retr2_st, but harder to forget about function parameters.
Created attachment 368971 [details] [review] Add a couple garbage certificate tests Ensure that creating a GTlsCertificate using a JPEG astronaut returns NULL. Also ensure that a GTlsDatabase containing only a JPEG astronaut does not permit successful certificate verification. More such tests are possible, but these are easy and a good start.
Attachment 368969 [details] pushed as 15531b5 - Fix crash when setting client cert without private key Attachment 368970 [details] pushed as c158d7f - Use gnutls_certificate_set_retrieve_function2() Attachment 368971 [details] pushed as 985277a - Add a couple garbage certificate tests