Skip to content

Fix potential memory leak when copying into existing SHA contexts and zero init tmpSha#9829

Draft
night1rider wants to merge 7 commits intowolfSSL:masterfrom
night1rider:tmpSha-fixes
Draft

Fix potential memory leak when copying into existing SHA contexts and zero init tmpSha#9829
night1rider wants to merge 7 commits intowolfSSL:masterfrom
night1rider:tmpSha-fixes

Conversation

@night1rider
Copy link
Contributor

@night1rider night1rider commented Feb 24, 2026

Problem:

When WOLFSSL_HASH_KEEP is enabled, SHA contexts accumulate message data in a dynamically allocated msg buffer. The Copy functions perform an XMEMCPY(dst, src, sizeof(...)) which overwrites the entire destination struct, including the dst->msg pointer. If the destination context was previously used and had an allocated msg buffer, 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 using WC_ALLOC_VAR_EX which does not zero-initialize memory. This temporary context is then passed directly to Copy as the destination. If Copy or any callback tries to free dst->msg as part of cleanup, it operates on an uninitialized garbage pointer.

Example code path of potential leak:

  /* Scenario: Copying into a SHA context that already has accumulated message data */                                                                                                                             
                  
  wc_Sha256 src, dst;                                                                                                                                                                                              
                                                                                                                                                                                                                   
  wc_InitSha256(&src);
  wc_InitSha256(&dst);

  /* Both contexts accumulate msg data with WOLFSSL_HASH_KEEP */
  wc_Sha256Update(&src, data1, len1);  /* src->msg allocated */
  wc_Sha256Update(&dst, data2, len2);  /* dst->msg allocated */

  /* BUG: dst->msg pointer is overwritten by XMEMCPY without being freed first */
  wc_Sha256Copy(&src, &dst);  /* dst->msg from data2 is leaked */

  wc_Sha256Free(&dst);  /* Only frees src's msg (now in dst), data2 msg is lost */

Then after applying the free fix we would need to edit the GetHash function due to:

  /* Scenario: GetHash with uninitialized temp context */

  wc_Sha256 sha;
  byte hash[32];

  wc_InitSha256(&sha);
  wc_Sha256Update(&sha, data, len);

  /* Internally: temp = ALLOC (garbage msg pointer) → Copy(&sha, temp) */
  /* If Copy/callback tries to free temp->msg, it frees a garbage pointer */
  wc_Sha256GetHash(&sha, hash);  /* potential crash or undefined behavior */

Fixes:

  • Fix potential memory leak in SHA Copy functions (wc_Sha224Copy, wc_Sha256Copy, wc_Sha512Copy, wc_Sha384Copy) where dst->msg buffer was not freed before XMEMCPY overwrites the destination struct with source data, losing the old pointer when WOLFSSL_HASH_KEEP is enabled
  • Change WC_ALLOC_VAR_EX to WC_CALLOC_VAR_EX in SHA GetHash functions to zero-initialize temporary contexts before they are passed to Copy, preventing use of uninitialized msg pointer fields

…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.
@night1rider
Copy link
Contributor Author

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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why memset when replacing every byte in memcpy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants