Package: libopenconnect2 Version: 5.03-1 Severity: important Tags: patch upstream X-Debbugs-CC: openconnect-de...@lists.infradead.org
The changes in gnutls.c from v5.01 to v5.02 concerning "support of CA certificates from PKCS#11 tokens (with GnuTLS 3.2.7+)" break functionality in openconnect at least if compiled with GnuTLS 2.12.x. Therefore, it also affects libopenconnect2 (= 5.02-1) in Ubuntu 14.04LTS.
I have tried to investigate on this issue with GDB and have come as far as to gnutls.c:1517 where err is not the return value of any call to gnutls_pkcs11_get_raw_issuer() or gnutls_x509_crt_import() within the code guarded by
#if defined(HAVE_P11KIT) && defined(HAVE_GNUTLS_PKCS11_GET_RAW_ISSUER)if compiled with GnuTLS 2.12.x as in Debian and Ubuntu Linux. So I thought to shift the lines 1517-1518 "if (err) break;" upwards to its original position, but then it crashes in gnutls.c:1522 invoking function gnutls_x509_crt_check_issuer(). Finally, I have given up and, although I know this is far from being smart, I reverted all changes in gnutls.c to v5.01 which works perfectly for me. The patch for reverting changes in gnutls.c is attached.
Could you please find a smarter fix or at least apply the given patch temporarily.
Thank you in advance! Thomas Uhle
--- openconnect-5.03/gnutls.c 2014-02-03 14:11:19 +0100 +++ openconnect-5.01/gnutls.c 2014-04-10 14:10:52 +0200 @@ -499,7 +499,8 @@ static int assign_privkey(struct opencon gnutls_privkey_t pkey, gnutls_x509_crt_t *certs, unsigned int nr_certs, - uint8_t *free_certs) + gnutls_x509_crt_t *extra_certs, + unsigned int nr_extra_certs) { int i; @@ -507,21 +508,28 @@ static int assign_privkey(struct opencon if (!vpninfo->my_certs) return GNUTLS_E_MEMORY_ERROR; - vpninfo->free_my_certs = gnutls_malloc(nr_certs); - if (vpninfo->free_my_certs) { - gnutls_free(vpninfo->my_certs); - vpninfo->my_certs = NULL; - return GNUTLS_E_MEMORY_ERROR; - } - - memcpy(vpninfo->free_my_certs, free_certs, nr_certs); memcpy(vpninfo->my_certs, certs, nr_certs * sizeof(*certs)); vpninfo->nr_my_certs = nr_certs; /* We are *keeping* the certs, unlike in GnuTLS 3 where our caller can free them after gnutls_certificate_set_key() has been called. - So wipe the 'free_certs' array. */ - memset(free_certs, 0, nr_certs); + So first wipe the 'certs' array (which is either '&cert' or + 'supporting_certs' in load_certificate())... */ + memset(certs, 0, nr_certs * sizeof(*certs)); + + /* ... and then also zero out the entries in extra_certs[] that + correspond to the certs that we're stealing. + We know certs[0] was already stolen by the load_certificate() + function so we might as well start at certs[1]. */ + for (i = 1; i < nr_certs; i++) { + int j; + for (j = 0; j < nr_extra_certs; j++) { + if (vpninfo->my_certs[i] == extra_certs[j]) { + extra_certs[j] = NULL; + break; + } + } + } gnutls_certificate_set_retrieve_function(vpninfo->https_cred, gtls_cert_cb); @@ -539,7 +547,8 @@ static int assign_privkey(struct opencon gnutls_privkey_t pkey, gnutls_x509_crt_t *certs, unsigned int nr_certs, - uint8_t *free_certs) + gnutls_x509_crt_t *extra_certs, + unsigned int nr_extra_certs) { gnutls_pcert_st *pcerts = calloc(nr_certs, sizeof(*pcerts)); int i, err; @@ -891,7 +900,7 @@ static int load_certificate(struct openc gnutls_x509_crt_t last_cert, cert = NULL; gnutls_x509_crt_t *extra_certs = NULL, *supporting_certs = NULL; unsigned int nr_supporting_certs = 0, nr_extra_certs = 0; - uint8_t *free_supporting_certs = NULL; + unsigned int certs_to_free = 0; /* How many of supporting_certs */ int err; /* GnuTLS error */ int ret; int i; @@ -1002,12 +1011,6 @@ static int load_certificate(struct openc else if (!ret) { if (nr_supporting_certs) { cert = supporting_certs[0]; - free_supporting_certs = gnutls_malloc(nr_supporting_certs); - if (!free_supporting_certs) { - ret = -ENOMEM; - goto out; - } - memset(free_supporting_certs, 1, nr_supporting_certs); goto got_key; } vpn_progress(vpninfo, PRG_ERR, @@ -1428,35 +1431,19 @@ static int load_certificate(struct openc choose the _right_ one. (RT#1942) Pick the right ones for ourselves and add them manually. */ - /* We may have already got a bunch of certs from PKCS#12 - file. Remember how many need to be freed when we're done, - since we'll expand the supporting_certs array with more - from the cafile and extra_certs[] array if we can, and - those extra certs must not be freed (twice). */ - if (!nr_supporting_certs) { - supporting_certs = gnutls_malloc(sizeof(*supporting_certs)); - if (!supporting_certs) { - vpn_progress(vpninfo, PRG_ERR, - _("Failed to allocate memory for certificate\n")); - ret = -ENOMEM; - goto out; - } - supporting_certs[0] = cert; - nr_supporting_certs = 1; - - free_supporting_certs = gnutls_malloc(1); - if (!free_supporting_certs) { - vpn_progress(vpninfo, PRG_ERR, - _("Failed to allocate memory for certificate\n")); - ret = -ENOMEM; - goto out; - } - free_supporting_certs[0] = 1; + if (nr_supporting_certs) { + /* We already got a bunch of certs from PKCS#12 file. Remember + how many need to be freed when we're done, since we'll + expand the supporting_certs array with more from the cafile + and extra_certs[] array if we can, and those extra certs + must not be freed (twice). */ + last_cert = supporting_certs[nr_supporting_certs-1]; + certs_to_free = nr_supporting_certs; + } else { + last_cert = cert; + certs_to_free = nr_supporting_certs = 1; } - last_cert = supporting_certs[nr_supporting_certs-1]; - while (1) { - uint8_t free_issuer; gnutls_x509_crt_t issuer; void *tmp; @@ -1470,89 +1457,54 @@ static int load_certificate(struct openc if (i < nr_extra_certs) { /* We found the next cert in the chain in extra_certs[] */ issuer = extra_certs[i]; - extra_certs[i] = NULL; - free_issuer = 1; } else { /* Look for it in the system trust cafile too. */ err = gnutls_certificate_get_issuer(vpninfo->https_cred, last_cert, &issuer, 0); + if (err) + break; + /* The check_issuer_sanity() function works fine as a workaround where it was used above, but when gnutls_certificate_get_issuer() returns a bogus cert, there's nothing we can do to fix it up. We don't get to iterate over all the available certs like we can over our own list. */ - if (!err && check_issuer_sanity(last_cert, issuer)) { + if (check_issuer_sanity(last_cert, issuer)) { vpn_progress(vpninfo, PRG_ERR, _("WARNING: GnuTLS returned incorrect issuer certs; authentication may fail!\n")); break; } - free_issuer = 0; - -#if defined(HAVE_P11KIT) && defined(HAVE_GNUTLS_PKCS11_GET_RAW_ISSUER) - if (err && cert_is_p11) { - gnutls_datum_t t; - - err = gnutls_pkcs11_get_raw_issuer(cert_url, last_cert, &t, GNUTLS_X509_FMT_DER, 0); - if (!err) { - err = gnutls_x509_crt_init(&issuer); - if (!err) { - err = gnutls_x509_crt_import(issuer, &t, GNUTLS_X509_FMT_DER); - if (err) - gnutls_x509_crt_deinit(issuer); - } - } - if (err) { - vpn_progress(vpninfo, PRG_ERR, - "Got no issuer from PKCS#11\n"); - } else { - get_cert_name(issuer, name, sizeof(name)); - - vpn_progress(vpninfo, PRG_ERR, - _("Got next CA '%s' from PKCS11\n"), name); - } - free_issuer = 1; - gnutls_free(t.data); - } -#endif - if (err) - break; - } - if (gnutls_x509_crt_check_issuer(issuer, issuer)) { + if (issuer == last_cert) { /* Don't actually include the root CA. If they don't already trust it, then handing it to them isn't going to help. But don't omit the original certificate if it's self-signed. */ - if (free_issuer) - gnutls_x509_crt_deinit(issuer); + if (nr_supporting_certs > 1) + nr_supporting_certs--; break; } /* OK, we found a new cert to add to our chain. */ tmp = supporting_certs; supporting_certs = gnutls_realloc(supporting_certs, - sizeof(cert) * (nr_supporting_certs+1)); + sizeof(cert) * (++nr_supporting_certs)); if (!supporting_certs) { - supporting_certs = tmp; - realloc_failed: + gnutls_free(tmp); vpn_progress(vpninfo, PRG_ERR, _("Failed to allocate memory for supporting certificates\n")); - if (free_issuer) - gnutls_x509_crt_deinit(issuer); - break; + /* The world is probably about to end, but try without them anyway */ + certs_to_free = 0; + ret = -ENOMEM; + goto out; } - tmp = free_supporting_certs; - free_supporting_certs = gnutls_realloc(free_supporting_certs, nr_supporting_certs+1); - if (!free_supporting_certs) { - free_supporting_certs = tmp; - goto realloc_failed; - } + /* First time we actually allocated an array? Copy the first cert into it */ + if (nr_supporting_certs == 2) + supporting_certs[0] = cert; /* Append the new one */ - supporting_certs[nr_supporting_certs] = issuer; - free_supporting_certs[nr_supporting_certs] = free_issuer; - nr_supporting_certs++; + supporting_certs[nr_supporting_certs-1] = issuer; last_cert = issuer; } for (i = 1; i < nr_supporting_certs; i++) { @@ -1572,9 +1524,9 @@ static int load_certificate(struct openc #if defined(HAVE_P11KIT) || defined(HAVE_TROUSERS) if (pkey) { err = assign_privkey(vpninfo, pkey, - supporting_certs, + supporting_certs ? supporting_certs : &cert, nr_supporting_certs, - free_supporting_certs); + extra_certs, nr_extra_certs); if (!err) { pkey = NULL; /* we gave it away, and potentially also some of extra_certs[] may have been zeroed. */ @@ -1582,7 +1534,7 @@ static int load_certificate(struct openc } else #endif /* P11KIT || TROUSERS */ err = gnutls_certificate_set_x509_key(vpninfo->https_cred, - supporting_certs, + supporting_certs ? supporting_certs : &cert, nr_supporting_certs, key); if (err) { @@ -1598,14 +1550,11 @@ static int load_certificate(struct openc if (key) gnutls_x509_privkey_deinit(key); if (supporting_certs) { - for (i = 0; i < nr_supporting_certs; i++) { + for (i = 0; i < certs_to_free; i++) { - /* We get here in an error case with !free_supporting_certs - and should free them all in that case */ - if (!free_supporting_certs || free_supporting_certs[i]) + if (supporting_certs[i]) gnutls_x509_crt_deinit(supporting_certs[i]); } gnutls_free(supporting_certs); - gnutls_free(free_supporting_certs); } else if (cert) { /* Not if supporting_certs. It's supporting_certs[0] then and was already freed. */ @@ -2056,12 +2005,9 @@ void openconnect_close_https(struct open if (vpninfo->my_certs) { int i; for (i = 0; i < vpninfo->nr_my_certs; i++) - if (vpninfo->free_my_certs[i]) - gnutls_x509_crt_deinit(vpninfo->my_certs[i]); + gnutls_x509_crt_deinit(vpninfo->my_certs[i]); gnutls_free(vpninfo->my_certs); - gnutls_free(vpninfo->free_my_certs); vpninfo->my_certs = NULL; - vpninfo->free_my_certs = NULL; } #endif }