Fix potential memory leak when copying into existing SHA contexts and zero init tmpSha#9829
Fix potential memory leak when copying into existing SHA contexts and zero init tmpSha#9829night1rider wants to merge 7 commits intowolfSSL:masterfrom
Conversation
021128f to
9bf7ceb
Compare
… zero-initialize temp GetHash contexts
…h contexts; zero HMAC dst hash before copy to prevent shared pointers
… (MD5, SHA, SHA2, SHA3, SHAKE) and add copy cleanup tests to prevent resource leaks when copying into previously-used contexts.
dc0bb05 to
47037dd
Compare
…xceeding stack frame limit.
… avoid exceeding stack frame limit." This reverts commit d99fe3b.
…oid stack frame overflow on small-stack builds.
|
Jenkins Retest This Please |
| /* Free dst resources before copy to prevent memory leaks (e.g., | ||
| * hardware contexts). XMEMCPY overwrites dst. */ | ||
| wc_Sha3Free(dst); | ||
| XMEMSET(dst, 0, sizeof(wc_Sha3)); |
There was a problem hiding this comment.
Why memset when replacing every byte in memcpy?
There was a problem hiding this comment.
I agree it is redundant, and I initially did not have the xmemset in earlier commits (the SHA and SHA-256 copy code paths). I decided to add it to follow the convention that if we free a context, we should probably re-init it in some capacity to a known state before operating with it. I originally thought about adding our InitSha after the free, but decided that seemed like too much. I followed the other code patterns where we use xmemset to get the context initialized. I wasn't sure if copying the state of another context via memcpy is considered as 'known good' compared to a zeroized struct. I am happy to remove the xmemset since the memcpy is there. The xmemset would likely be optimized away by a compiler anyways.
Problem:
When
WOLFSSL_HASH_KEEPis enabled, SHA contexts accumulate message data in a dynamically allocatedmsgbuffer. The Copy functions perform anXMEMCPY(dst, src, sizeof(...))which overwrites the entire destination struct, including thedst->msgpointer. If the destination context was previously used and had an allocatedmsgbuffer, that memory is leaked since the pointer is overwritten before being freed.Additionally, the GetHash functions (
wc_ShaGetHash,wc_Sha256GetHash, etc.) allocate a temporary SHA context usingWC_ALLOC_VAR_EXwhich does not zero-initialize memory. This temporary context is then passed directly to Copy as the destination. If Copy or any callback tries to freedst->msgas part of cleanup, it operates on an uninitialized garbage pointer.Example code path of potential leak:
Then after applying the free fix we would need to edit the GetHash function due to:
Fixes:
wc_Sha224Copy,wc_Sha256Copy,wc_Sha512Copy,wc_Sha384Copy) wheredst->msgbuffer was not freed beforeXMEMCPYoverwrites the destination struct with source data, losing the old pointer whenWOLFSSL_HASH_KEEPis enabledWC_ALLOC_VAR_EXtoWC_CALLOC_VAR_EXin SHA GetHash functions to zero-initialize temporary contexts before they are passed to Copy, preventing use of uninitializedmsgpointer fields