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
 	}

Reply via email to