gnutls28 in trusty no longer validates many valid certificate chains, such as google.com

[Anders Kaseorg]

[Impact]

Recently, due to some combination of the recent ca-certificate SRU and server certificate chain reconfigurations, the gnutls28 package in trusty was left unable to validate many valid certificate chains, such as that of google.com.

 0 s:/C=US/ST=California/L=Mountain View/O=Google Inc/CN=*.google.com
   i:/C=US/O=Google Inc/CN=Google Internet Authority G2
 1 s:/C=US/O=Google Inc/CN=Google Internet Authority G2
   i:/C=US/O=GeoTrust Inc./CN=GeoTrust Global CA
 2 s:/C=US/O=GeoTrust Inc./CN=GeoTrust Global CA
   i:/C=US/O=Equifax/OU=Equifax Secure Certificate Authority

The problem is that although GeoTrust Global CA is a trusted certificate, gnutls28 gives up after noting that Equifax Secure Certificate Authority is not. This bug was fixed upstream by these commits:

https://gitlab.com/gnutls/gnutls/commit/72a7b8e63f76c7f2faf482bdbf4e740b82a1fae9
https://gitlab.com/gnutls/gnutls/commit/9dbe3aab9e157ef8f7a67112a4619d4f028519dc
https://gitlab.com/gnutls/gnutls/commit/d1de36af91c5ac86dd2b1ab18b0b230a0b1e5d31

[Test Case]

One way to reproduce this is by building and running gnutls-cli:

$ apt-get build-dep gnutls28
$ apt-get source gnutls28
$ cd gnutls28-3.2.11
$ debian/rules build
$ ./src/gnutls-cli google.com
Processed 118 CA certificate(s).
Resolving 'google.com'...
Connecting to '2607:f8b0:4009:811::200e:443'...
- Certificate type: X.509
- Got a certificate list of 3 certificates.
- Certificate[0] info:
 - subject `C=US,ST=California,L=Mountain View,O=Google Inc,CN=*.google.com', issuer `C=US,O=Google Inc,CN=Google Internet Authority G2', EC key 256 bits, signed using RSA-SHA256, activated `2017-09-26 11:09:35 UTC', expires `2017-12-19 10:59:00 UTC', SHA-1 fingerprint `a2a8d7ae1097865469dd5cf830896b930b704c8c'
 Public Key ID:
  e3e4e591a11311b8c92f8cddbebbea025d0e2088
 Public key's random art:
  +--[ EC 256]----+
  |o .o. |
  |E . . . |
  | . . . o. . |
  | . = o o |
  | . B oS + |
  | . o =+o= . |
  | . oo . |
  | . . |
  | oo.++ |
  +-----------------+

- Certificate[1] info:
 - subject `C=US,O=Google Inc,CN=Google Internet Authority G2', issuer `C=US,O=GeoTrust Inc.,CN=GeoTrust Global CA', RSA key 2048 bits, signed using RSA-SHA256, activated `2017-05-22 11:32:37 UTC', expires `2018-12-31 23:59:59 UTC', SHA-1 fingerprint `a6120fc0b4664fad0b3b6ffd5f7a33e561ddb87d'
- Certificate[2] info:
 - subject `C=US,O=GeoTrust Inc.,CN=GeoTrust Global CA', issuer `C=US,O=Equifax,OU=Equifax Secure Certificate Authority', RSA key 2048 bits, signed using RSA-SHA1, activated `2002-05-21 04:00:00 UTC', expires `2018-08-21 04:00:00 UTC', SHA-1 fingerprint `7359755c6df9a0abc3060bce369564c8ec4542a3'
- Status: The certificate is NOT trusted. The certificate issuer is unknown.
*** Verifying server certificate failed...
*** Fatal error: Error in the certificate.
*** Handshake has failed
GnuTLS error: Error in the certificate.

(Note that the gnutls-cli binary in trusty’s gnutls-bin package comes from gnutls26, which seems to have already received the necessary updates, although it requires the ‘--x509cafile /etc/ssl/certs/ca-certificates.crt’ option.)

[Regression Potential]

Most GnuTLS-dependent packages in trusty use gnutls26 rather than gnutls28, so potential regressions, if any, would likely manifest in self-compiled binaries and PPA packages that were specifically compiled against gnutls28. (I noticed this bug in the first place because vlc from ppa:jonathonf/vlc became unable to play YouTube videos.)

[Anders Kaseorg (andersk)]

Here is a patch for trusty that backports the relevant parts of the three upstream commits fixing this bug.

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Julian Andres Klode (juliank)]

Looking at it. First patch looks OK, have to compare the other patch with the upstream changes.

[Julian Andres Klode (juliank)]

Just to record my analysis of the debdiff: The changes are basically the same as the upstream commits, except for the PKCS#11 changes. This means that PKCS#11 certificates are still checked in full. I'm not sure where that would be used, but it is not a security problem (less is allowed than upstream, not more).

I have verified that xenial contains the same fixes by checking that _gnutls_check_if_same_key() exists there.

The changelog mentions trusty-updates, and does not close the bug report. I added (LP: #1722411)
as a final line and changed the distribution to trusty to match other uploads.

I'm building now, and will verify that the bug is fixed and upload afterwards.

[Julian Andres Klode (juliank)]

Built the package, run the command in the test, verified that it worked. popped the 2 patches, ran make again, verified that the command did not work. => tested OK

Uploaded.

[Roger Lipscombe (8-roger)]

The debdiff introduces a memory leak.

With the simple program at https://gist.github.com/rlipscombe/78d6e3bbfc67e010f1e7a9ddd8c87099, the previous version is fine, but this one leaks.

Valgrind reports the following:

==11134==
==11134== HEAP SUMMARY:
==11134== in use at exit: 1,014,363 bytes in 3,794 blocks
==11134== total heap usage: 978,656 allocs, 974,862 frees, 572,269,255 bytes allocated
==11134==
==11134== 53,462 bytes in 148 blocks are definitely lost in loss record 33 of 37
==11134== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==11134== by 0x4E6DF61: _gnutls_set_datum (in /usr/lib/x86_64-linux-gnu/libgnutls.so.28.30.1)
==11134== by 0x4E98A4C: _gnutls_x509_get_raw_dn2 (in /usr/lib/x86_64-linux-gnu/libgnutls.so.28.30.1)
==11134== by 0x4EBBDB8: gnutls_x509_crt_import (in /usr/lib/x86_64-linux-gnu/libgnutls.so.28.30.1)
==11134== by 0x4EC0C9D: gnutls_x509_crt_list_import (in /usr/lib/x86_64-linux-gnu/libgnutls.so.28.30.1)
==11134== by 0x4EC0EF6: gnutls_x509_crt_list_import2 (in /usr/lib/x86_64-linux-gnu/libgnutls.so.28.30.1)
==11134== by 0x4E7DCF3: gnutls_certificate_set_x509_trust_mem (in /usr/lib/x86_64-linux-gnu/libgnutls.so.28.30.1)
==11134== by 0x4E7E037: gnutls_certificate_set_x509_trust_file (in /usr/lib/x86_64-linux-gnu/libgnutls.so.28.30.1)
==11134== by 0x40107C: main (in /vagrant/gnutls-client)
==11134==
==11134== 294,000 bytes in 1,000 blocks are definitely lost in loss record 35 of 37
==11134== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==11134== by 0x4E6DF61: _gnutls_set_datum (in /usr/lib/x86_64-linux-gnu/libgnutls.so.28.30.1)
==11134== by 0x4E98A4C: _gnutls_x509_get_raw_dn2 (in /usr/lib/x86_64-linux-gnu/libgnutls.so.28.30.1)
==11134== by 0x4EBBDB8: gnutls_x509_crt_import (in /usr/lib/x86_64-linux-gnu/libgnutls.so.28.30.1)
==11134== by 0x4E81246: gnutls_pcert_import_x509_raw (in /usr/lib/x86_64-linux-gnu/libgnutls.so.28.30.1)
==11134== by 0x4EE0FC6: _gnutls_proc_crt (in /usr/lib/x86_64-linux-gnu/libgnutls.so.28.30.1)
==11134== by 0x4E67836: _gnutls_recv_server_certificate (in /usr/lib/x86_64-linux-gnu/libgnutls.so.28.30.1)
==11134== by 0x4E64B0F: gnutls_handshake (in /usr/lib/x86_64-linux-gnu/libgnutls.so.28.30.1)
==11134== by 0x401253: main (in /vagrant/gnutls-client)
==11134==
==11134== 294,000 bytes in 1,000 blocks are definitely lost in loss record 36 of 37
==11134== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==11134== by 0x4E6DF61: _gnutls_set_datum (in /usr/lib/x86_64-linux-gnu/libgnutls.so.28.30.1)
==11134== by 0x4E98A4C: _gnutls_x509_get_raw_dn2 (in /usr/lib/x86_64-linux-gnu/libgnutls.so.28.30.1)
==11134== by 0x4EBBDB8: gnutls_x509_crt_import (in /usr/lib/x86_64-linux-gnu/libgnutls.so.28.30.1)
==11134== by 0x4E81246: gnutls_pcert_import_x509_raw (in /usr/lib/x86_64-linux-gnu/libgnutls.so.28.30.1)
==11134== by 0x4EE427A: _gnutls_proc_dhe_signature (in /usr/lib/x86_64-linux-gnu/libgnutls.so.28.30.1)
==11134== by 0x4EEBB2C: proc_ecdhe_server_kx (in /usr/lib/x86_64-linux-gnu/libgnutls.so.28.30.1)
==11134== by 0x4E674B3: _gnutls_rec...

[Anders Kaseorg (andersk)]

8-roger: That’s probably because 3.2 is lacking
https://gitlab.com/gnutls/gnutls/commit/cdd60f7013d5e64702f3b04e6fe93218e88a2213.

[Roger Lipscombe (8-roger)]

I'm pretty sure that it's the "compare_ca_name_and_key.patch", which introduces "raw_spki", but doesn't seem to do anything to free it. I'll do some more investigation today.

[Roger Lipscombe (8-roger)]

"debdiff with memory leak fixed" appears to be identical to the original debdiff.

[Roger Lipscombe (8-roger)]

I'm trying it with the following extra patch:

diff -ruN gnutls28-3.2.11-orig/lib/x509/x509.c gnutls28-3.2.11/lib/x509/x509.c
--- gnutls28-3.2.11-orig/lib/x509/x509.c 2014-01-01 17:14:59.000000000 +0000
+++ gnutls28-3.2.11/lib/x509/x509.c 2018-01-18 14:52:52.617834001 +0000
@@ -136,6 +136,7 @@
                asn1_delete_structure(&cert->cert);
        gnutls_free(cert->raw_dn.data);
        gnutls_free(cert->raw_issuer_dn.data);
+ gnutls_free(cert->raw_spki.data);
        gnutls_free(cert);
 }

@@ -202,6 +203,7 @@
                asn1_delete_structure(&cert->cert);
                _gnutls_free_datum(&cert->raw_dn);
                _gnutls_free_datum(&cert->raw_issuer_dn);
+ _gnutls_free_datum(&cert->raw_spki);

                result = asn1_create_element(_gnutls_get_pkix(),
                                             "PKIX1.Certificate",
@@ -252,6 +262,7 @@
                _gnutls_free_datum(&_data);
        _gnutls_free_datum(&cert->raw_dn);
        _gnutls_free_datum(&cert->raw_issuer_dn);
+ _gnutls_free_datum(&cert->raw_spki);
        return result;
 }

[Roger Lipscombe (8-roger)]

In my testing, that patch appears to fix the memory leak. I'm attaching it properly.

I have no idea how to get it in a .debdiff. Also, does that require a version bump to "ubuntu1.3"?

[Anders Kaseorg (andersk)]

Er, right, I uploaded the wrong file, here’s the right one.

I’ll leave it to the sponsor to figure out whether the version needs to be bumped again, as the previous upload is still unapproved (https://launchpad.net/ubuntu/trusty/+queue?queue_state=1).

[Roger Lipscombe (8-roger)]

> I'm pretty sure that it's the "compare_ca_name_and_key.patch", which introduces "raw_spki", but doesn't seem to do anything to free it.

Oh, now I see how https://gitlab.com/gnutls/gnutls/commit/cdd60f7013d5e64702f3b04e6fe93218e88a2213 fixes the leak -- by avoiding the allocation in the first place. I wasn't paying attention this morning :)

I'll try with the updated debdiff. Thanks.

[Roger Lipscombe (8-roger)]

> I'll try with the updated debdiff. Thanks.

Looks good.

[Julian Andres Klode (juliank)]

Uploaded the updated debdiff

[Łukasz Zemczak (sil2100)]

Hello Anders, or anyone else affected,

Accepted gnutls28 into trusty-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/gnutls28/3.2.11-2ubuntu1.2 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed.Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-trusty to verification-done-trusty. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-trusty. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

[Julian Andres Klode (juliank)]

Hey, Anders, don't you want to verify it?

[Anders Kaseorg (andersk)]

Er, yeah, this has been working for me. Thanks for the reminder to actually set the tags.

[Łukasz Zemczak (sil2100)]

Which version of the package is working for you? Test verification should include package version information for us to be sure that we're releasing the same version of the package that has been tested.

[Anders Kaseorg (andersk)]

Łukasz: 3.2.11-2ubuntu1.2 in trusty-proposed. (What else could I be verifying? I’ve never encountered a requirement to explicitly state the version number when it’s already clear, but I’ll be sure to do so in the future.)

[Łukasz Zemczak (sil2100)]

@andersk The requirement has been around either since always or at least since a very long time, please see SRU acceptance comment #18:

"(...)
If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-trusty to verification-done-trusty.
(...)"

It's been multiple times where people were testing versions from PPAs instead of the -proposed pocket. Also, the SRU team by accepting a package validation needs to have some level of certainty that the tester actually performed the required tests on the package. We had countless cases of testers just marking packages as verified without doing anything, or not going through all the required test cases. Having a version number at least gives us some information and a better sense that the test result can be trusted. Of course, people can just copy-paste and cheat anyway, but that's one additional step they need to perform at least.

In most cases we're not even accepting test results without mentioning what specific tests have been performed. The more verbosity the better, since we have more proof. If we'd believe blindly in whatever anyone just says we'd have more broken packages for no reason. Anyone can say "works for me", and many people do, but then subtle things like: "whoops, I actually tested the wrong version" pop up here and there because the tested PPA-built package that seemingly had the same contents could be busted in the -proposed archives due to different package dependencies being available.

[Launchpad Janitor (janitor)]

This bug was fixed in the package gnutls28 - 3.2.11-2ubuntu1.2

---------------
gnutls28 (3.2.11-2ubuntu1.2) trusty; urgency=medium

  * debian/patches/check_same_certificate_not_only_issuer.patch: when
    verifying, check for the same certificate in the trusted list,
    not only the issuer.
  * debian/patches/compare_ca_name_and_key.patch: when comparing a CA
    certificate with the trusted list, compare the name and key.
    (LP: #1722411)

 -- Anders Kaseorg <email address hidden> Wed, 17 Jan 2018 16:23:47 -0500

[Łukasz Zemczak (sil2100)]

The verification of the Stable Release Update for gnutls28 has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.