From 0011b7b376771843ec51233b61c2d552b5020de8 Mon Sep 17 00:00:00 2001 From: David Garske Date: Wed, 19 Aug 2020 15:11:17 -0700 Subject: [PATCH] Fix possible ECC curve cache leak for custom curves. Fix possible memory leak with `wc_DhKeyDecode` and `WOLFSSL_DH_EXTRA`. Fix leak in `dh_test` with new call to DH key import. --- wolfcrypt/src/asn.c | 14 +++++-- wolfcrypt/src/ecc.c | 88 +++++++++++++++++++++++++++---------------- wolfcrypt/test/test.c | 8 +++- 3 files changed, 71 insertions(+), 39 deletions(-) diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index d3d263e0e..986a747f6 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -4446,8 +4446,11 @@ int wc_DhKeyDecode(const byte* input, word32* inOutIdx, DhKey* key, word32 inSz) #endif #endif /* Assume input started after 1.2.840.113549.1.3.1 dhKeyAgreement */ - if (GetInt(&key->p, input, inOutIdx, inSz) < 0 || - GetInt(&key->g, input, inOutIdx, inSz) < 0) { + if (GetInt(&key->p, input, inOutIdx, inSz) < 0) { + ret = ASN_DH_KEY_E; + } + if (ret == 0 && GetInt(&key->g, input, inOutIdx, inSz) < 0) { + mp_clear(&key->p); ret = ASN_DH_KEY_E; } @@ -4476,8 +4479,11 @@ int wc_DhKeyDecode(const byte* input, word32* inOutIdx, DhKey* key, word32 inSz) if (GetSequence(input, inOutIdx, &length, inSz) < 0) return ASN_PARSE_E; - if (GetInt(&key->p, input, inOutIdx, inSz) < 0 || - GetInt(&key->g, input, inOutIdx, inSz) < 0) { + if (GetInt(&key->p, input, inOutIdx, inSz) < 0) { + return ASN_DH_KEY_E; + } + if (ret == 0 && GetInt(&key->g, input, inOutIdx, inSz) < 0) { + mp_clear(&key->p); return ASN_DH_KEY_E; } } diff --git a/wolfcrypt/src/ecc.c b/wolfcrypt/src/ecc.c index 6e4630816..b92d339c4 100644 --- a/wolfcrypt/src/ecc.c +++ b/wolfcrypt/src/ecc.c @@ -1265,41 +1265,58 @@ enum ecc_curve_load_mask { #define FREE_CURVE_SPECS() #endif /* ECC_CACHE_CURVE */ -static void _wc_ecc_curve_free(ecc_curve_spec* curve) +static void wc_ecc_curve_cache_free_spec_item(ecc_curve_spec* curve, mp_int* item, + byte mask) +{ + if (item) { + #ifdef HAVE_WOLF_BIGINT + wc_bigint_free(&item->raw); + #endif + mp_clear(item); + } + curve->load_mask &= ~mask; +} +static void wc_ecc_curve_cache_free_spec(ecc_curve_spec* curve) { if (curve == NULL) { return; } if (curve->load_mask & ECC_CURVE_FIELD_PRIME) - mp_clear(curve->prime); + wc_ecc_curve_cache_free_spec_item(curve, curve->prime, ECC_CURVE_FIELD_PRIME); if (curve->load_mask & ECC_CURVE_FIELD_AF) - mp_clear(curve->Af); + wc_ecc_curve_cache_free_spec_item(curve, curve->Af, ECC_CURVE_FIELD_AF); #ifdef USE_ECC_B_PARAM if (curve->load_mask & ECC_CURVE_FIELD_BF) - mp_clear(curve->Bf); + wc_ecc_curve_cache_free_spec_item(curve, curve->Bf, ECC_CURVE_FIELD_BF); #endif if (curve->load_mask & ECC_CURVE_FIELD_ORDER) - mp_clear(curve->order); + wc_ecc_curve_cache_free_spec_item(curve, curve->order, ECC_CURVE_FIELD_ORDER); if (curve->load_mask & ECC_CURVE_FIELD_GX) - mp_clear(curve->Gx); + wc_ecc_curve_cache_free_spec_item(curve, curve->Gx, ECC_CURVE_FIELD_GX); if (curve->load_mask & ECC_CURVE_FIELD_GY) - mp_clear(curve->Gy); + wc_ecc_curve_cache_free_spec_item(curve, curve->Gy, ECC_CURVE_FIELD_GY); curve->load_mask = 0; } static void wc_ecc_curve_free(ecc_curve_spec* curve) { - /* don't free cached curves */ -#ifndef ECC_CACHE_CURVE - _wc_ecc_curve_free(curve); -#endif - (void)curve; + if (curve) { + #ifdef ECC_CACHE_CURVE + /* only free custom curves (reset are globally cached) */ + if (curve->dp && curve->dp->id == ECC_CURVE_CUSTOM) { + wc_ecc_curve_cache_free_spec(curve); + XFREE(curve, NULL, DYNAMIC_TYPE_ECC); + } + #else + wc_ecc_curve_cache_free_spec(curve); + #endif + } } -static int wc_ecc_curve_load_item(const char* src, mp_int** dst, - ecc_curve_spec* curve, byte mask) +static int wc_ecc_curve_cache_load_item(ecc_curve_spec* curve, const char* src, + mp_int** dst, byte mask) { int err; @@ -1349,23 +1366,29 @@ static int wc_ecc_curve_load(const ecc_set_type* dp, ecc_curve_spec** pCurve, #endif /* make sure cache has been allocated */ - if (ecc_curve_spec_cache[x] == NULL) { - ecc_curve_spec_cache[x] = (ecc_curve_spec*)XMALLOC( - sizeof(ecc_curve_spec), NULL, DYNAMIC_TYPE_ECC); - if (ecc_curve_spec_cache[x] == NULL) { + if (ecc_curve_spec_cache[x] == NULL || dp->id == ECC_CURVE_CUSTOM) { + curve = (ecc_curve_spec*)XMALLOC(sizeof(ecc_curve_spec), NULL, DYNAMIC_TYPE_ECC); + if (curve == NULL) { #if defined(ECC_CACHE_CURVE) && !defined(SINGLE_THREADED) wc_UnLockMutex(&ecc_curve_cache_mutex); #endif return MEMORY_E; } - XMEMSET(ecc_curve_spec_cache[x], 0, sizeof(ecc_curve_spec)); + XMEMSET(curve, 0, sizeof(ecc_curve_spec)); + + /* set curve pointer to cache */ + if (dp->id != ECC_CURVE_CUSTOM) { + ecc_curve_spec_cache[x] = curve; + } } - - /* set curve pointer to cache */ - *pCurve = ecc_curve_spec_cache[x]; - -#endif /* ECC_CACHE_CURVE */ + else { + curve = ecc_curve_spec_cache[x]; + } + /* return new or cached curve */ + *pCurve = curve; +#else curve = *pCurve; +#endif /* ECC_CACHE_CURVE */ /* make sure the curve is initialized */ if (curve->dp != dp) { @@ -1389,30 +1412,29 @@ static int wc_ecc_curve_load(const ecc_set_type* dp, ecc_curve_spec** pCurve, curve->load_mask |= load_items; /* load items */ - x = 0; if (load_items & ECC_CURVE_FIELD_PRIME) - x += wc_ecc_curve_load_item(dp->prime, &curve->prime, curve, + ret += wc_ecc_curve_cache_load_item(curve, dp->prime, &curve->prime, ECC_CURVE_FIELD_PRIME); if (load_items & ECC_CURVE_FIELD_AF) - x += wc_ecc_curve_load_item(dp->Af, &curve->Af, curve, + ret += wc_ecc_curve_cache_load_item(curve, dp->Af, &curve->Af, ECC_CURVE_FIELD_AF); #ifdef USE_ECC_B_PARAM if (load_items & ECC_CURVE_FIELD_BF) - x += wc_ecc_curve_load_item(dp->Bf, &curve->Bf, curve, + ret += wc_ecc_curve_cache_load_item(curve, dp->Bf, &curve->Bf, ECC_CURVE_FIELD_BF); #endif if (load_items & ECC_CURVE_FIELD_ORDER) - x += wc_ecc_curve_load_item(dp->order, &curve->order, curve, + ret += wc_ecc_curve_cache_load_item(curve, dp->order, &curve->order, ECC_CURVE_FIELD_ORDER); if (load_items & ECC_CURVE_FIELD_GX) - x += wc_ecc_curve_load_item(dp->Gx, &curve->Gx, curve, + ret += wc_ecc_curve_cache_load_item(curve, dp->Gx, &curve->Gx, ECC_CURVE_FIELD_GX); if (load_items & ECC_CURVE_FIELD_GY) - x += wc_ecc_curve_load_item(dp->Gy, &curve->Gy, curve, + ret += wc_ecc_curve_cache_load_item(curve, dp->Gy, &curve->Gy, ECC_CURVE_FIELD_GY); /* check for error */ - if (x != 0) { + if (ret != 0) { wc_ecc_curve_free(curve); ret = MP_READ_E; } @@ -1441,7 +1463,7 @@ void wc_ecc_curve_cache_free(void) /* free all ECC curve caches */ for (x = 0; x < (int)ECC_SET_COUNT; x++) { if (ecc_curve_spec_cache[x]) { - _wc_ecc_curve_free(ecc_curve_spec_cache[x]); + wc_ecc_curve_cache_free_spec(ecc_curve_spec_cache[x]); XFREE(ecc_curve_spec_cache[x], NULL, DYNAMIC_TYPE_ECC); ecc_curve_spec_cache[x] = NULL; } diff --git a/wolfcrypt/test/test.c b/wolfcrypt/test/test.c index c4a877fd8..4af597a2d 100644 --- a/wolfcrypt/test/test.c +++ b/wolfcrypt/test/test.c @@ -14567,10 +14567,14 @@ int dh_test(void) } #endif - - /* Test DH key import / export */ #ifdef WOLFSSL_DH_EXTRA + wc_FreeDhKey(&key); + ret = wc_InitDhKey_ex(&key, HEAP_HINT, devId); + if (ret != 0) { + ERROR_OUT(-7949, done); + } + #if !defined(NO_ASN) && !defined(NO_FILESYSTEM) file = XFOPEN(dhKeyFile, "rb"); if (!file)