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

diff --git a/src/libstrongswan/credentials/cred_encoding.c b/src/libstrongswan/credentials/cred_encoding.c
index 1754a47c8378eadd47b971adfeead311d095d077..c1223c6f2b97afb332c42d030faf7ad0ebd45457 100644 (file)
--- a/src/libstrongswan/credentials/cred_encoding.c
+++ b/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)
 {

diff --git a/src/libstrongswan/credentials/cred_encoding.h b/src/libstrongswan/credentials/cred_encoding.h
index fc60160ad3483b4b73f2466084270ba4621e6320..0b469d039afd136b4236d30f7033a073f5e7338e 100644 (file)
--- a/src/libstrongswan/credentials/cred_encoding.h
+++ b/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.

diff --git a/src/libstrongswan/plugins/bliss/bliss_private_key.c b/src/libstrongswan/plugins/bliss/bliss_private_key.c
index b6f07b3acf38def21503aa41e2383f5b2617a0c8..9eb89c59ec03dc1dceae8cf56ce6d3c59b8cb84d 100644 (file)
--- a/src/libstrongswan/plugins/bliss/bliss_private_key.c
+++ b/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;
 }

diff --git a/src/libstrongswan/plugins/bliss/bliss_public_key.c b/src/libstrongswan/plugins/bliss/bliss_public_key.c
index caa46b6dd6ce1bda9e7c5dbb204c0b5a55c69605..17d59900ea43b490e4c5064ae39556ce7f83510b 100644 (file)
--- a/src/libstrongswan/plugins/bliss/bliss_public_key.c
+++ b/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;
 }

diff --git a/src/libstrongswan/plugins/botan/botan_util.c b/src/libstrongswan/plugins/botan/botan_util.c
index 575f8c1abdeabff2d9cb6c35a4e8c08c89027460..dbd0c9f54aff969c4e2c4414d0e06508b6bc174a 100644 (file)
--- a/src/libstrongswan/plugins/botan/botan_util.c
+++ b/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;
 }

diff --git a/src/libstrongswan/plugins/curve25519/curve25519_private_key.c b/src/libstrongswan/plugins/curve25519/curve25519_private_key.c
index 7e58b04aa000b8b40955c1839082d5fc33442f01..bf5c41486dc623655803f34219fe76b4100a6af1 100644 (file)
--- a/src/libstrongswan/plugins/curve25519/curve25519_private_key.c
+++ b/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;
 }

diff --git a/src/libstrongswan/plugins/curve25519/curve25519_public_key.c b/src/libstrongswan/plugins/curve25519/curve25519_public_key.c
index 3d75dd205d8e6cf3c9d2ebb2f9e1b8b73def2130..2c20549e984f44da9bc99169c74bbdb51aa2b496 100644 (file)
--- a/src/libstrongswan/plugins/curve25519/curve25519_public_key.c
+++ b/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;
 }

diff --git a/src/libstrongswan/plugins/openssl/openssl_ed_public_key.c b/src/libstrongswan/plugins/openssl/openssl_ed_public_key.c
index c8dfc298a9de6ff2e9294a10d99bffbf1bba9661..7380f1c15b14562fc3f71601b91826c5b0be10d7 100644 (file)
--- a/src/libstrongswan/plugins/openssl/openssl_ed_public_key.c
+++ b/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;
 }
 

diff --git a/src/libstrongswan/plugins/openssl/openssl_util.c b/src/libstrongswan/plugins/openssl/openssl_util.c
index d72f7e30ee13cb5379f21672be588a49f548ebcc..6c12c89c27cc4c747a83d805fafe92b877cb2af2 100644 (file)
--- a/src/libstrongswan/plugins/openssl/openssl_util.c
+++ b/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;
 }
 

diff --git a/src/libstrongswan/plugins/pkcs11/pkcs11_public_key.c b/src/libstrongswan/plugins/pkcs11/pkcs11_public_key.c
index a2ef3f3be5a6264e83ad48c304f38e87acd24971..62e754de7b860f32e573eae7a93df6ffd0820f70 100644 (file)
--- a/src/libstrongswan/plugins/pkcs11/pkcs11_public_key.c
+++ b/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;
 }
 

diff --git a/src/libstrongswan/plugins/wolfssl/wolfssl_ec_public_key.c b/src/libstrongswan/plugins/wolfssl/wolfssl_ec_public_key.c
index 110543762f213acd80358b4daf7cd1d18939481c..97abe950b91f5f01d574d5f35a7c79e41b4be99f 100644 (file)
--- a/src/libstrongswan/plugins/wolfssl/wolfssl_ec_public_key.c
+++ b/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;
 }
 

diff --git a/src/libstrongswan/plugins/wolfssl/wolfssl_ed_public_key.c b/src/libstrongswan/plugins/wolfssl/wolfssl_ed_public_key.c
index ea0fb3dfc7709bb5655812e359fb8d019c2b9848..f6facc639609f3400448cf87ac1548598eae5df4 100644 (file)
--- a/src/libstrongswan/plugins/wolfssl/wolfssl_ed_public_key.c
+++ b/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);

diff --git a/src/libstrongswan/plugins/wolfssl/wolfssl_rsa_public_key.c b/src/libstrongswan/plugins/wolfssl/wolfssl_rsa_public_key.c
index da8899c2d8ce79940bad1d1c5e66e1eb7928103b..3414550944013403e68d42b03fa6759cf262440b 100644 (file)
--- a/src/libstrongswan/plugins/wolfssl/wolfssl_rsa_public_key.c
+++ b/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);