Fix memory-safety, zeroization, and error-handling findings - #1136
Fix memory-safety, zeroization, and error-handling findings#1136ejohnstown wants to merge 15 commits into
Conversation
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1136
Scan targets checked: wolfssh-bugs, wolfssh-src
No new issues found in the changed files. ✅
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tests/api.c:1441
- wolfSSH_ReadKey_buffer() allocates SSH-format key outputs with DYNTYPE_PRIVKEY (via DoSshPubKey), but this test frees them with DYNTYPE_FILE. With memory tracking enabled, freeing with the wrong dyn type can break accounting/debug checks.
WFREE(keyRef, NULL, DYNTYPE_FILE);
WFREE(keyTrim, NULL, DYNTYPE_FILE);
examples/portfwd/portfwd.c:714
- When appBufferUsed == appBufferSz, appFd remains in the read fdset, so select() will return immediately as long as the socket stays readable. Because the recv() is skipped in that state, the loop can busy-spin at 100% CPU while waiting for wolfSSH_ChannelSend() to drain the buffer. Consider clearing appFd from rxFds while the buffer is full and still running the flush logic on select() timeout.
}
/* Skip the read when the buffer is full, a zero length recv() returns
* 0 and would be mistaken for the peer closing. The buffer drains at
* the bottom of the loop. */
if (appFdSet && appBufferUsed < appBufferSz &&
There was a problem hiding this comment.
Looks good, just 1 question.
Also, copilot suppressed this comment, but I can't figure out if actually valid:
When appBufferUsed == appBufferSz, appFd remains in the read fdset, so select() will return immediately as long as the socket stays readable. Because the recv() is skipped in that state, the loop can busy-spin at 100% CPU while waiting for wolfSSH_ChannelSend() to drain the buffer. Consider clearing appFd from rxFds while the buffer is full and still running the flush logic on select() timeout.
kSz can change (e.g. after rekeying), so it may no longer cover the full extent of previously stored key material. Use sizeof(ssh->k) instead to ensure the whole buffer is wiped, and reset kSz to 0. Issue: F-7219
heap was used throughout certman.c (WMALLOC/WFREE calls, and the free at wolfSSH_CERTMAN_free) but was never stored on the struct during _CertMan_init, so it was always NULL. Issue: F-7206
- wolfSSH_ChannelSend() can return fewer bytes than requested, but appBufferUsed was decremented without moving the unsent tail to the front of appBuffer, corrupting subsequent sends. Shift the remaining data down with WMEMMOVE() when the send is short. - Skip the recv() when appBuffer is full. The length argument would be 0, and a zero length recv() returns 0, which the loop read as the peer closing; the buffered data was dropped and the forward torn down. Issue: F-7207
c[inSz-1] = 0 clobbered the last byte of the copied key data instead of terminating the string after it, truncating public keys by one character. Issue: F-7205
- Test wolfSSH_ReadKey_buffer() and wc_CertPemToDer() results and skip the entry; a malformed file gave PwMapNew() a NULL buf and an indeterminate length. - NULL-check both WMALLOC() results, and guard the load_file() size so a missing file no longer reaches WMALLOC(0). - Fall through to the loop's existing WFREE()/advance so both buffers are freed on every path. Issue: F-7204
- Zero-initialize struct tm t; it was read indeterminate when the parse failed. - Fail the profile when wc_GetDateAsCalendarTime() returns non-zero, and gate the two date-format comparisons on valid. Issue: F-7208
- IdentifyAsn1Key() was called with a literal 1, so a public PEM decoded by wc_PubKeyPemToDer() was run through the private-key decoders and never identified. - Add test_wolfSSH_ReadPublicKey_pem() covering a public RSA PEM read through wolfSSH_ReadPublicKey_buffer(). The read itself only compiles with WOLFSSH_TPM, so the test also asserts the isPrivate 0 vs 1 difference in IdentifyAsn1Key() directly, which every build runs. Issue: F-7209
- Track userPublicKey allocations with a userPublicKeyAlloc flag - Free on that flag, not pubKeyName; -J with no -j leaked the cert - Restore userPublicKeyBuf when a key load fails, not a stale pointer - Tag load_der_file() allocations DYNTYPE_PRIVKEY to match the frees - Free the CA cert in ClientLoadCA() with the heap it came from - Pass the caller's heap into wolfSSH_TPM_InitKey() so the TPM public key is allocated from the pool ClientFreeBuffers() frees it with Issue: F-7210, F-7211
- crypt() returns a pointer into a static buffer that keeps the hashed password after CheckPasswordHashUnix() returns; wipe it once the comparison is done. Issue: F-7220
- The zero check ran WSTRCMP() on the caller's buffer, which is a length-bounded slice of the config line and not NUL terminated, so it read past inSz and rejected valid "0" values whose slice had trailing text. - Compare num, the NUL-terminated copy that atol() was given. Issue: F-7213
- wolfSSH_MakeRsaKey and wolfSSH_MakeEcdsaKey called wc_FreeRsaKey and wc_ecc_free unconditionally, so a failed init left the free operating on an indeterminate stack struct. - Track init with a keyInit flag, as wolfSSH_MakeMlDsaKey does, and free only when the init succeeded. - Guard wc_ed25519_free in wolfSSH_MakeEd25519Key the same way. - Replace the tab indent on the ed25519 free with spaces. Issue: F-7212
- wolfSSH_AGENT_worker() dereferenced ssh->agent to set the DONE state without checking it, so a call before the agent was set up took a null dereference. - Return WS_AGENT_NULL_E instead. Issue: F-7214
- ret held the read length from wolfSSH_SFTP_buffer_read(), and was only reset at the bottom of the per-entry loop, so a NAME response with a count of 0 skipped the loop and logged a read error. - Set ret to WS_SUCCESS after the count is parsed. The NULL return and untouched ssh->error are unchanged; only the bogus log goes away. Issue: F-7215
- AeadIncrementExpIv() returned as soon as a byte incremented without wrapping, so the iteration count depended on the counter, which starts out as KDF output. Carry through all 8 bytes instead. - Same counter values and the same instruction count once unrolled; the built object no longer has the data-dependent exits. Issue: F-7216
The send at the end of the loop only ran when select() reported a readable descriptor, but appBuffer can hold data across iterations. Mask appFd out of the read set while the buffer is full, and fall through the select() timeout to the send. Issue: F-7207
LoadPubKeyList()now checks thewolfSSH_ReadKey_buffer(),wc_CertPemToDer(), andWMALLOC()results so a malformed or missing key file skips the entry instead of handingPwMapNew()a NULL buffer and an indeterminate length.DoSshPubKey()wrote its null terminator atc[inSz-1], clobbering the last byte of the copied key; it now writes atc[inSz]._CertMan_init()never storedheapon the struct, so everyWMALLOC/WFREEin certman.c ran with a NULL heap; it is now set during init.portfwd_worker()dropped the unsent tail after a shortwolfSSH_ChannelSend(), and mistook a full buffer's zero-lengthrecv()for the peer closing; both are fixed.CheckProfile()ignored thewc_GetDateAsCalendarTime()return and compared an indeterminatestruct tm; it now zero-initializes, checks the return, and fails the profile on a parse error.DoPemKey()hardcodedisPrivate = 1when identifying the decoded key; it now passes the caller's flag through.userPublicKeyAllocflag rather thanpubKeyName(which leaked the cert buffer for-Jwithout-j), and frees the CA cert and the TPM public key from the heap they were allocated from.wolfSSH_MakeRsaKey(),MakeEcdsaKey(), andMakeEd25519Key()freed their key structs unconditionally, so a failed init freed an indeterminate stack struct; frees are now gated on an init flag.GetConfigInt()ranWSTRCMP()on the caller's non-terminated slice, rejecting valid zero values likeLoginGraceTime 0m; it now compares the NUL-terminated copy thatatol()was given.wolfSSH_AGENT_worker()dereferencedssh->agentwithout a null check; it now returnsWS_AGENT_NULL_E.wolfSSH_SFTP_DoName()leftretholding the read length, so a NAME response with zero entries took the error path;retis cleared before the loop.AeadIncrementExpIv()returned early on the first non-zero byte, making the nonce increment data-dependent; the carry now runs the full width with no early exit.SshResourceFree()wiped onlykSzbytes ofssh->k, which can under-cover after a rekey; it now zeroessizeof(ssh->k)and resetskSz.CheckPasswordHashUnix()left the hashed password incrypt()'s static buffer; it is wiped once the comparison is done.