cred-encoding: Avoid potential use after free when caching encodings

The pattern currently is to call get_cache(), generate the encoding
if that failed and then store it with cache().  The latter adopts the
passed encoding and frees any stored encoding.  However, the latter means
that if two threads concurrently fail to get a cached encoding and then
both generate and store one, one of the threads might use an encoding
that was freed by the other thread.

Since encodings are not expected to change, we can avoid this issue by
not replacing an existing cache entry and instead return that (while
freeing the passed value instead of the cached one).

Closes strongswan/strongswan#1231

src/libstrongswan/credentials/cred_encoding.c

@@ -115,6 +115,34 @@ METHOD(cred_encoding_t, get_cache, bool,
 	return !!chunk;
 }
 
+METHOD(cred_encoding_t, cache, void,
+	private_cred_encoding_t *this, cred_encoding_type_t type, void *cache,
+	chunk_t *encoding)
+{
+	chunk_t *chunk;
+
+	if (type >= CRED_ENCODING_MAX || (int)type < 0)
+	{
+		free(encoding->ptr);
+		return;
+	}
+
+	this->lock->write_lock(this->lock);
+	chunk = this->cache[type]->get(this->cache[type], cache);
+	if (chunk)
+	{
+		free(encoding->ptr);
+		*encoding = *chunk;
+	}
+	else
+	{
+		chunk = malloc_thing(chunk_t);
+		*chunk = *encoding;
+		this->cache[type]->put(this->cache[type], cache, chunk);
+	}
+	this->lock->unlock(this->lock);
+}
+
 /**
  * Implementation of cred_encoding_t.encode
  */
@@ -160,43 +188,11 @@ static bool encode(private_cred_encoding_t *this, cred_encoding_type_t type,
 
 	if (success && cache)
 	{
-		chunk = malloc_thing(chunk_t);
-		*chunk = *encoding;
-		this->lock->write_lock(this->lock);
-		chunk = this->cache[type]->put(this->cache[type], cache, chunk);
-		this->lock->unlock(this->lock);
-		if (chunk)
-		{
-			free(chunk->ptr);
-			free(chunk);
-		}
+		_cache(this, type, cache, encoding);
 	}
 	return success;
 }
 
-METHOD(cred_encoding_t, cache, void,
-	private_cred_encoding_t *this, cred_encoding_type_t type, void *cache,
-	chunk_t encoding)
-{
-	chunk_t *chunk;
-
-	if (type >= CRED_ENCODING_MAX || (int)type < 0)
-	{
-		return free(encoding.ptr);
-	}
-	chunk = malloc_thing(chunk_t);
-	*chunk = encoding;
-	this->lock->write_lock(this->lock);
-	chunk = this->cache[type]->put(this->cache[type], cache, chunk);
-	this->lock->unlock(this->lock);
-	/* free an encoding already associated to the cache */
-	if (chunk)
-	{
-		free(chunk->ptr);
-		free(chunk);
-	}
-}
-
 METHOD(cred_encoding_t, clear_cache, void,
 	private_cred_encoding_t *this, void *cache)
 {

src/libstrongswan/credentials/cred_encoding.h

@@ -202,14 +202,16 @@ struct cred_encoding_t {
 	 * Cache a credential encoding created externally.
 	 *
 	 * After calling cache(), the passed encoding is owned by the cred encoding
-	 * facility.
+	 * facility.  Note that if there already is a cached encoding for the same
+	 * key, the method will return that and free the passed value.
 	 *
 	 * @param type			format of the credential encoding
 	 * @param cache			key to use for caching, as given to encode()
-	 * @param encoding		encoding to cache, gets owned by this
+	 * @param encoding		encoding to cache, gets adopted, might get replaced
+	 *						with cached value
 	 */
 	void (*cache)(cred_encoding_t *this, cred_encoding_type_t type, void *cache,
-				  chunk_t encoding);
+				  chunk_t *encoding);
 
 	/**
 	 * Register a credential encoder function.

src/libstrongswan/plugins/bliss/bliss_private_key.c

@@ -646,7 +646,7 @@ METHOD(private_key_t, get_fingerprint, bool,
 										   this->set, type, fp);
 	if (success)
 	{
-		lib->encoding->cache(lib->encoding, type, this, *fp);
+		lib->encoding->cache(lib->encoding, type, this, fp);
 	}
 	return success;
 }

src/libstrongswan/plugins/bliss/bliss_public_key.c

@@ -267,7 +267,7 @@ METHOD(public_key_t, get_fingerprint, bool,
 										   this->set, type, fp);
 	if (success)
 	{
-		lib->encoding->cache(lib->encoding, type, this, *fp);
+		lib->encoding->cache(lib->encoding, type, this, fp);
 	}
 	return success;
 }

src/libstrongswan/plugins/botan/botan_util.c

@@ -200,7 +200,7 @@ bool botan_get_fingerprint(botan_pubkey_t pubkey, void *cache,
 
 	if (cache)
 	{
-		lib->encoding->cache(lib->encoding, type, cache, *fp);
+		lib->encoding->cache(lib->encoding, type, cache, fp);
 	}
 	return TRUE;
 }

src/libstrongswan/plugins/curve25519/curve25519_private_key.c

@@ -188,7 +188,7 @@ METHOD(private_key_t, get_fingerprint, bool,
 	success = curve25519_public_key_fingerprint(this->pubkey, type, fp);
 	if (success)
 	{
-		lib->encoding->cache(lib->encoding, type, this, *fp);
+		lib->encoding->cache(lib->encoding, type, this, fp);
 	}
 	return success;
 }

src/libstrongswan/plugins/curve25519/curve25519_public_key.c

@@ -186,7 +186,7 @@ METHOD(public_key_t, get_fingerprint, bool,
 	success = curve25519_public_key_fingerprint(this->pubkey, type, fp);
 	if (success)
 	{
-		lib->encoding->cache(lib->encoding, type, this, *fp);
+		lib->encoding->cache(lib->encoding, type, this, fp);
 	}
 	return success;
 }

src/libstrongswan/plugins/openssl/openssl_ed_public_key.c

@@ -175,7 +175,7 @@ bool openssl_ed_fingerprint(EVP_PKEY *key, cred_encoding_type_t type,
 		return FALSE;
 	}
 	hasher->destroy(hasher);
-	lib->encoding->cache(lib->encoding, type, key, *fp);
+	lib->encoding->cache(lib->encoding, type, key, fp);
 	return TRUE;
 }
 

src/libstrongswan/plugins/openssl/openssl_util.c

@@ -125,7 +125,7 @@ bool openssl_fingerprint(EVP_PKEY *key, cred_encoding_type_t type, chunk_t *fp)
 	}
 	free(enc.ptr);
 	hasher->destroy(hasher);
-	lib->encoding->cache(lib->encoding, type, key, *fp);
+	lib->encoding->cache(lib->encoding, type, key, fp);
 	return TRUE;
 }
 

src/libstrongswan/plugins/pkcs11/pkcs11_public_key.c

@@ -430,7 +430,7 @@ static bool fingerprint_ecdsa(private_pkcs11_public_key_t *this,
 	}
 	hasher->destroy(hasher);
 	chunk_clear(&asn1);
-	lib->encoding->cache(lib->encoding, type, this, *fp);
+	lib->encoding->cache(lib->encoding, type, this, fp);
 	return TRUE;
 }
 

src/libstrongswan/plugins/wolfssl/wolfssl_ec_public_key.c

@@ -253,7 +253,7 @@ bool wolfssl_ec_fingerprint(ecc_key *ec, cred_encoding_type_t type, chunk_t *fp)
 		return FALSE;
 	}
 	hasher->destroy(hasher);
-	lib->encoding->cache(lib->encoding, type, ec, *fp);
+	lib->encoding->cache(lib->encoding, type, ec, fp);
 	return TRUE;
 }
 

src/libstrongswan/plugins/wolfssl/wolfssl_ed_public_key.c

@@ -254,7 +254,7 @@ bool wolfssl_ed_fingerprint(wolfssl_ed_key *key, key_type_t key_type,
 	}
 	else
 	{
-		lib->encoding->cache(lib->encoding, type, key, *fp);
+		lib->encoding->cache(lib->encoding, type, key, fp);
 		success = TRUE;
 	}
 	DESTROY_IF(hasher);

src/libstrongswan/plugins/wolfssl/wolfssl_rsa_public_key.c

@@ -370,7 +370,7 @@ bool wolfssl_rsa_fingerprint(RsaKey *rsa, cred_encoding_type_t type,
 	}
 	else
 	{
-		lib->encoding->cache(lib->encoding, type, rsa, *fp);
+		lib->encoding->cache(lib->encoding, type, rsa, fp);
 		success = TRUE;
 	}
 	DESTROY_IF(hasher);