From 48e9267d7a0d961647114add028438d544447772 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Thu, 18 Aug 2022 12:04:39 +0200 Subject: [PATCH] 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 | 62 +++++++++---------- src/libstrongswan/credentials/cred_encoding.h | 8 ++- .../plugins/bliss/bliss_private_key.c | 2 +- .../plugins/bliss/bliss_public_key.c | 2 +- src/libstrongswan/plugins/botan/botan_util.c | 2 +- .../curve25519/curve25519_private_key.c | 2 +- .../curve25519/curve25519_public_key.c | 2 +- .../plugins/openssl/openssl_ed_public_key.c | 2 +- .../plugins/openssl/openssl_util.c | 2 +- .../plugins/pkcs11/pkcs11_public_key.c | 2 +- .../plugins/wolfssl/wolfssl_ec_public_key.c | 2 +- .../plugins/wolfssl/wolfssl_ed_public_key.c | 2 +- .../plugins/wolfssl/wolfssl_rsa_public_key.c | 2 +- 13 files changed, 45 insertions(+), 47 deletions(-) diff --git a/src/libstrongswan/credentials/cred_encoding.c b/src/libstrongswan/credentials/cred_encoding.c index 1754a47c8..c1223c6f2 100644 --- 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 fc60160ad..0b469d039 100644 --- 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 b6f07b3ac..9eb89c59e 100644 --- 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 caa46b6dd..17d59900e 100644 --- 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 575f8c1ab..dbd0c9f54 100644 --- 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 7e58b04aa..bf5c41486 100644 --- 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 3d75dd205..2c20549e9 100644 --- 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 c8dfc298a..7380f1c15 100644 --- 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 d72f7e30e..6c12c89c2 100644 --- 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 a2ef3f3be..62e754de7 100644 --- 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 110543762..97abe950b 100644 --- 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 ea0fb3dfc..f6facc639 100644 --- 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 da8899c2d..341455094 100644 --- 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);