From 9f6388d27256ddb9245f46133fd0edcacb6e791d Mon Sep 17 00:00:00 2001 From: billphipps Date: Thu, 23 Mar 2023 15:46:38 -0400 Subject: [PATCH 1/2] Track SetDigest usage to avoid invalid free under error conditions. --- src/internal.c | 47 +++++++++++++++++++++++++++++++++++++++++----- wolfssl/internal.h | 1 + 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/src/internal.c b/src/internal.c index 8bca1bbd3..4ba102923 100644 --- a/src/internal.c +++ b/src/internal.c @@ -4377,24 +4377,28 @@ static void SetDigest(WOLFSSL* ssl, int hashAlgo) switch (hashAlgo) { #ifndef NO_SHA case sha_mac: + ssl->options.dontFreeDigest=1; ssl->buffers.digest.buffer = ssl->hsHashes->certHashes.sha; ssl->buffers.digest.length = WC_SHA_DIGEST_SIZE; break; #endif /* !NO_SHA */ #ifndef NO_SHA256 case sha256_mac: + ssl->options.dontFreeDigest=1; ssl->buffers.digest.buffer = ssl->hsHashes->certHashes.sha256; ssl->buffers.digest.length = WC_SHA256_DIGEST_SIZE; break; #endif /* !NO_SHA256 */ #ifdef WOLFSSL_SHA384 case sha384_mac: + ssl->options.dontFreeDigest=1; ssl->buffers.digest.buffer = ssl->hsHashes->certHashes.sha384; ssl->buffers.digest.length = WC_SHA384_DIGEST_SIZE; break; #endif /* WOLFSSL_SHA384 */ #ifdef WOLFSSL_SHA512 case sha512_mac: + ssl->options.dontFreeDigest=1; ssl->buffers.digest.buffer = ssl->hsHashes->certHashes.sha512; ssl->buffers.digest.length = WC_SHA512_DIGEST_SIZE; break; @@ -7530,9 +7534,12 @@ void FreeKeyExchange(WOLFSSL* ssl) /* Cleanup digest buffer */ if (ssl->buffers.digest.buffer) { - XFREE(ssl->buffers.digest.buffer, ssl->heap, DYNAMIC_TYPE_DIGEST); + if(!ssl->options.dontFreeDigest) { + XFREE(ssl->buffers.digest.buffer, ssl->heap, DYNAMIC_TYPE_DIGEST); + } ssl->buffers.digest.buffer = NULL; ssl->buffers.digest.length = 0; + ssl->options.dontFreeDigest=0; } /* Free handshake key */ @@ -26216,6 +26223,14 @@ static int HashSkeData(WOLFSSL* ssl, enum wc_HashType hashType, ssl->buffers.digest.length = (unsigned int)digest_sz; /* buffer for hash */ + if(!ssl->buffers.digest.buffer) { + if(!ssl->options.dontFreeDigest) { + XFREE(ssl->buffers.digest.buffer, ssl->heap, + DYNAMIC_TYPE_DIGEST); + } + } + ssl->options.dontFreeDigest=0; + ssl->buffers.digest.buffer = (byte*)XMALLOC(ssl->buffers.digest.length, ssl->heap, DYNAMIC_TYPE_DIGEST); if (ssl->buffers.digest.buffer == NULL) { @@ -30454,8 +30469,16 @@ exit_scv: #endif /* WOLFSSL_ASYNC_IO */ /* Digest is not allocated, so do this to prevent free */ + if(ssl->buffers.digest.buffer) { + if(!ssl->options.dontFreeDigest) { + /*This should not happen*/ + XFREE(ssl->buffers.digest.buffer, + ssl->heap, DYNAMIC_TYPE_DIGEST); + } + } ssl->buffers.digest.buffer = NULL; ssl->buffers.digest.length = 0; + ssl->options.dontFreeDigest=0; /* Final cleanup */ #ifdef WOLFSSL_ASYNC_IO @@ -31938,8 +31961,11 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, TypeHash(ssl->options.hashAlgo)); /* Replace sig buffer with new one */ - XFREE(ssl->buffers.digest.buffer, ssl->heap, - DYNAMIC_TYPE_DIGEST); + if(!ssl->options.dontFreeDigest) { + XFREE(ssl->buffers.digest.buffer, + ssl->heap, DYNAMIC_TYPE_DIGEST); + } + ssl->options.dontFreeDigest=0; ssl->buffers.digest.buffer = encodedSig; } @@ -32156,8 +32182,11 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, TypeHash(ssl->options.hashAlgo)); /* Replace sig buffer with new one */ - XFREE(ssl->buffers.digest.buffer, ssl->heap, - DYNAMIC_TYPE_DIGEST); + if(!ssl->options.dontFreeDigest) { + XFREE(ssl->buffers.digest.buffer, + ssl->heap, DYNAMIC_TYPE_DIGEST); + } + ssl->options.dontFreeDigest=0; ssl->buffers.digest.buffer = encodedSig; } break; @@ -34246,8 +34275,16 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, SendAlert(ssl, alert_fatal, bad_certificate); #endif /* Digest is not allocated, so do this to prevent free */ + if(ssl->buffers.digest.buffer) { + if(!ssl->options.dontFreeDigest) { + /*This should not happen*/ + XFREE(ssl->buffers.digest.buffer, + ssl->heap, DYNAMIC_TYPE_DIGEST); + } + } ssl->buffers.digest.buffer = NULL; ssl->buffers.digest.length = 0; + ssl->options.dontFreeDigest=0; #ifdef WOLFSSL_ASYNC_CRYPT /* Cleanup async */ diff --git a/wolfssl/internal.h b/wolfssl/internal.h index b9e164fb1..281b72b64 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -4312,6 +4312,7 @@ struct Options { word16 saveArrays:1; /* save array Memory for user get keys or psk */ word16 weOwnRng:1; /* will be true unless CTX owns */ + word16 dontFreeDigest:1; /* when true, we used SetDigest */ word16 haveEMS:1; /* using extended master secret */ #ifdef HAVE_POLY1305 word16 oldPoly:1; /* set when to use old rfc way of poly*/ From 2430f2377fac316509601778a5ba834b36f62f1a Mon Sep 17 00:00:00 2001 From: Bill Phipps Date: Mon, 10 Apr 2023 17:15:22 +0000 Subject: [PATCH 2/2] Corrected spacing and comments --- src/internal.c | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/src/internal.c b/src/internal.c index fba85a3a5..9349f48c9 100644 --- a/src/internal.c +++ b/src/internal.c @@ -4388,28 +4388,28 @@ static void SetDigest(WOLFSSL* ssl, int hashAlgo) switch (hashAlgo) { #ifndef NO_SHA case sha_mac: - ssl->options.dontFreeDigest=1; + ssl->options.dontFreeDigest = 1; ssl->buffers.digest.buffer = ssl->hsHashes->certHashes.sha; ssl->buffers.digest.length = WC_SHA_DIGEST_SIZE; break; #endif /* !NO_SHA */ #ifndef NO_SHA256 case sha256_mac: - ssl->options.dontFreeDigest=1; + ssl->options.dontFreeDigest = 1; ssl->buffers.digest.buffer = ssl->hsHashes->certHashes.sha256; ssl->buffers.digest.length = WC_SHA256_DIGEST_SIZE; break; #endif /* !NO_SHA256 */ #ifdef WOLFSSL_SHA384 case sha384_mac: - ssl->options.dontFreeDigest=1; + ssl->options.dontFreeDigest = 1; ssl->buffers.digest.buffer = ssl->hsHashes->certHashes.sha384; ssl->buffers.digest.length = WC_SHA384_DIGEST_SIZE; break; #endif /* WOLFSSL_SHA384 */ #ifdef WOLFSSL_SHA512 case sha512_mac: - ssl->options.dontFreeDigest=1; + ssl->options.dontFreeDigest = 1; ssl->buffers.digest.buffer = ssl->hsHashes->certHashes.sha512; ssl->buffers.digest.length = WC_SHA512_DIGEST_SIZE; break; @@ -7548,12 +7548,13 @@ void FreeKeyExchange(WOLFSSL* ssl) /* Cleanup digest buffer */ if (ssl->buffers.digest.buffer) { - if(!ssl->options.dontFreeDigest) { + /* Only free if digest buffer was not set using SetDigest */ + if (!ssl->options.dontFreeDigest) { XFREE(ssl->buffers.digest.buffer, ssl->heap, DYNAMIC_TYPE_DIGEST); } ssl->buffers.digest.buffer = NULL; ssl->buffers.digest.length = 0; - ssl->options.dontFreeDigest=0; + ssl->options.dontFreeDigest = 0; } /* Free handshake key */ @@ -26232,13 +26233,13 @@ static int HashSkeData(WOLFSSL* ssl, enum wc_HashType hashType, ssl->buffers.digest.length = (unsigned int)digest_sz; /* buffer for hash */ - if(!ssl->buffers.digest.buffer) { - if(!ssl->options.dontFreeDigest) { + if (!ssl->buffers.digest.buffer) { + if (!ssl->options.dontFreeDigest) { XFREE(ssl->buffers.digest.buffer, ssl->heap, DYNAMIC_TYPE_DIGEST); } } - ssl->options.dontFreeDigest=0; + ssl->options.dontFreeDigest = 0; ssl->buffers.digest.buffer = (byte*)XMALLOC(ssl->buffers.digest.length, ssl->heap, DYNAMIC_TYPE_DIGEST); @@ -30492,7 +30493,7 @@ exit_scv: /* Digest is not allocated, so do this to prevent free */ if(ssl->buffers.digest.buffer) { - if(!ssl->options.dontFreeDigest) { + if (!ssl->options.dontFreeDigest) { /*This should not happen*/ XFREE(ssl->buffers.digest.buffer, ssl->heap, DYNAMIC_TYPE_DIGEST); @@ -30500,7 +30501,7 @@ exit_scv: } ssl->buffers.digest.buffer = NULL; ssl->buffers.digest.length = 0; - ssl->options.dontFreeDigest=0; + ssl->options.dontFreeDigest = 0; /* Final cleanup */ #ifdef WOLFSSL_ASYNC_IO @@ -31983,11 +31984,11 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, TypeHash(ssl->options.hashAlgo)); /* Replace sig buffer with new one */ - if(!ssl->options.dontFreeDigest) { + if (!ssl->options.dontFreeDigest) { XFREE(ssl->buffers.digest.buffer, ssl->heap, DYNAMIC_TYPE_DIGEST); } - ssl->options.dontFreeDigest=0; + ssl->options.dontFreeDigest = 0; ssl->buffers.digest.buffer = encodedSig; } @@ -32204,11 +32205,11 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, TypeHash(ssl->options.hashAlgo)); /* Replace sig buffer with new one */ - if(!ssl->options.dontFreeDigest) { + if (!ssl->options.dontFreeDigest) { XFREE(ssl->buffers.digest.buffer, ssl->heap, DYNAMIC_TYPE_DIGEST); } - ssl->options.dontFreeDigest=0; + ssl->options.dontFreeDigest = 0; ssl->buffers.digest.buffer = encodedSig; } break; @@ -34298,7 +34299,7 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, #endif /* Digest is not allocated, so do this to prevent free */ if(ssl->buffers.digest.buffer) { - if(!ssl->options.dontFreeDigest) { + if (!ssl->options.dontFreeDigest) { /*This should not happen*/ XFREE(ssl->buffers.digest.buffer, ssl->heap, DYNAMIC_TYPE_DIGEST); @@ -34306,7 +34307,7 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, } ssl->buffers.digest.buffer = NULL; ssl->buffers.digest.length = 0; - ssl->options.dontFreeDigest=0; + ssl->options.dontFreeDigest = 0; #ifdef WOLFSSL_ASYNC_CRYPT /* Cleanup async */