Conversation
F-2139 Previously the plaintext private key DER buffer was freed via XFREE without a preceding ForceZero when no password encryption was requested. Track the actual allocation size and zeroize the buffer before release.
F-2140 wolfSSL_PEM_write_mem_DSAPrivateKey serializes the DSA private key to a heap DER buffer and freed it on five paths without ForceZero. Zeroize the buffer before each XFREE.
F-2141 The error path in wolfSSL_PEM_write_mem_ECPrivateKey freed the EC private key DER staging buffer without ForceZero. Zeroize before free.
F-2142 wolfSSL_RSA_To_Der could free a buffer holding RSA private key material when the DER encoding step failed. Record the allocation size and ForceZero the buffer before XFREE on the private key path.
F-2143 ssl_SetWatchKey_file loaded a private key file into a heap buffer and freed it without ForceZero on both error and success paths. Zeroize before XFREE.
F-2144 SetStaticEphemeralKey loaded a private key file into keyBuf and freed it without ForceZero. Static ephemeral keys are long-lived, so zeroize the buffer before XFREE.
F-2145 wolfSSL_CTX_use_RSAPrivateKey staged the RSA private key DER (PKCS#1: n, e, d, p, q, dP, dQ, qInv) in a heap buffer and freed it without ForceZero. Zeroize before XFREE.
F-2146 wolfSSL_d2i_RSAPrivateKey_bio read PKCS#1-encoded RSA private key DER from a BIO into a heap buffer and freed it without ForceZero. Zeroize before XFREE on both success and error paths.
F-2147 The error path in wolfSSL_i2d_ECPrivateKey could free an EC private key DER staging buffer that may contain a partial private scalar. Zeroize before XFREE.
F-2148 pem_write_mem_pkcs8privatekey stages the PKCS#8 DER encoded private key at the tail of the PEM buffer, then writes the shorter PEM output at the head of the same buffer. The DER tail is not overwritten, leaking the plaintext private key to heap memory after the callers free. Zero the DER staging area before returning.
F-2148 The prior fix zeroed the computed DER staging area, but PEM output from wc_DerToPemEx fills most of the buffer and overlaps that region, corrupting the valid PEM. Preserve the allocation size and zero only the bytes beyond the actual PEM length, or the whole buffer on failure.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR hardens key-handling paths by zeroing sensitive buffers before freeing them, reducing the chance of plaintext key material lingering in heap memory.
Changes:
- Add
ForceZero()calls beforeXFREE()for RSA/EC/DSA key DER buffers and key-file buffers. - Track allocation sizes in a few paths to wipe the full allocation (including any unused tail) before freeing.
- Add post-conversion cleanup for the PKCS#8 PEM-write path to clear DER staging remnants.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ssl_load.c | Zero RSA private key DER buffer before freeing in wolfSSL_CTX_use_RSAPrivateKey. |
| src/ssl.c | Zero ephemeral key-file buffer before freeing in SetStaticEphemeralKey. |
| src/sniffer.c | Zero watch-key buffers on both error and success paths before freeing. |
| src/pk_rsa.c | Zero temporary RSA DER buffers before freeing; introduce derAllocSz tracking. |
| src/pk_ec.c | Zero EC private key DER buffers on error paths before freeing. |
| src/pk.c | Zero DER buffers before freeing in PEM/PKCS#8 conversion paths and track allocation size for tail wiping. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| int ret = 1; | ||
| int derSz = 0; | ||
| int derAllocSz = 0; |
There was a problem hiding this comment.
derAllocSz is an int that is cast to word32 when passed to ForceZero(). If derSz can ever be negative (or otherwise invalid) on an error path, the cast can wrap to a very large value and cause an out-of-bounds wipe. Make derAllocSz an unsigned size type (e.g., word32/size_t) and only assign/call ForceZero() when the size is known to be > 0 (and ideally set it to the actual allocation size used for derBuf, not just the DER length).
| int derAllocSz = 0; | |
| word32 derAllocSz = 0; |
| } | ||
| } | ||
|
|
||
| derAllocSz = derSz; |
There was a problem hiding this comment.
derAllocSz is an int that is cast to word32 when passed to ForceZero(). If derSz can ever be negative (or otherwise invalid) on an error path, the cast can wrap to a very large value and cause an out-of-bounds wipe. Make derAllocSz an unsigned size type (e.g., word32/size_t) and only assign/call ForceZero() when the size is known to be > 0 (and ideally set it to the actual allocation size used for derBuf, not just the DER length).
| if ((derBuf != NULL) && (publicKey == 0)) { | ||
| ForceZero(derBuf, (word32)derAllocSz); | ||
| } |
There was a problem hiding this comment.
derAllocSz is an int that is cast to word32 when passed to ForceZero(). If derSz can ever be negative (or otherwise invalid) on an error path, the cast can wrap to a very large value and cause an out-of-bounds wipe. Make derAllocSz an unsigned size type (e.g., word32/size_t) and only assign/call ForceZero() when the size is known to be > 0 (and ideally set it to the actual allocation size used for derBuf, not just the DER length).
| char password[NAME_SZ]; | ||
| byte* key = NULL; | ||
| word32 keySz = 0; | ||
| word32 allocSz = 0; |
There was a problem hiding this comment.
The new wiping logic depends on allocSz being captured before *pemSz is updated, but allocSz is only set in one branch. If *pem is allocated and a failure occurs before that branch executes, allocSz may remain 0 and the buffer won’t be cleared even though it may contain sensitive staging data. Consider setting allocSz immediately after the PEM buffer allocation succeeds (at the point where the allocation size is definitively known) so all subsequent failure paths can reliably wipe the allocation.
| /* Remember the allocation size before *pemSz is updated to the | ||
| * actual PEM output length, so we can zero any unused tail that | ||
| * held the DER staging area. */ | ||
| allocSz = (word32)*pemSz; |
There was a problem hiding this comment.
The new wiping logic depends on allocSz being captured before *pemSz is updated, but allocSz is only set in one branch. If *pem is allocated and a failure occurs before that branch executes, allocSz may remain 0 and the buffer won’t be cleared even though it may contain sensitive staging data. Consider setting allocSz immediately after the PEM buffer allocation succeeds (at the point where the allocation size is definitively known) so all subsequent failure paths can reliably wipe the allocation.
| /* Zero any remnants of the DER staging area that persist after PEM | ||
| * conversion so plaintext private key material is not left in freed heap | ||
| * memory. On success, only the bytes past the actual PEM output need | ||
| * clearing; on failure, the whole buffer is zeroed since its state is | ||
| * indeterminate. */ | ||
| if (*pem != NULL) { | ||
| if (res == 1 && (word32)*pemSz < allocSz) { | ||
| ForceZero(*pem + *pemSz, allocSz - (word32)*pemSz); | ||
| } | ||
| else if (res != 1) { | ||
| ForceZero(*pem, allocSz); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new wiping logic depends on allocSz being captured before *pemSz is updated, but allocSz is only set in one branch. If *pem is allocated and a failure occurs before that branch executes, allocSz may remain 0 and the buffer won’t be cleared even though it may contain sensitive staging data. Consider setting allocSz immediately after the PEM buffer allocation succeeds (at the point where the allocation size is definitively known) so all subsequent failure paths can reliably wipe the allocation.
|
2139
2140
2141
2142
2143
2144
2145
2146
2147
2148