Static Analysis Fixes#983
Open
ejohnstown wants to merge 8 commits into
Open
Conversation
- Replace mid-function return so end-of-function cleanup runs. Issue: F-3204
- otherMatch is set to 1 immediately above; guard always true. Issue: F-3205
Contributor
There was a problem hiding this comment.
Pull request overview
Hardening-focused PR addressing several static-analysis findings across core protocol parsing/negotiation, certificate-chain verification, terminal escape parsing, and client-side sensitive-buffer cleanup.
Changes:
- Tighten protocol message parsing to better match RFC requirements (e.g., NEWKEYS payload rejection, KEXINIT reserved-field handling, case-sensitive version comparison).
- Harden certificate verification input validation (explicitly reject
certsCount == 0). - Improve client safety by securely zeroing sensitive buffers during cleanup.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/wolfterm.c | Adds an additional bounds check during CSI escape-sequence parsing. |
| src/internal.c | Tightens SSH identification matching and hardens KEXINIT/NEWKEYS parsing behavior. |
| src/certman.c | Rejects zero-certificate chains up front in buffer verification. |
| apps/wolfssh/common.c | Removes redundant conditional output and zeroes sensitive client buffers on free. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ejohnstown
force-pushed
the
sf5
branch
2 times, most recently
from
fd6dd00 to
c0048ed
Compare
- Per RFC 4253 7.1 the reserved field is uint32 0, not a length prefix; reject non-zero values with WS_PARSE_E. - Add regression test. Issue: F-869
- Zero private key and password buffers in both apps/wolfssh and examples client. - Also zero keyboard-interactive response buffers in the example client. Issue: F-249
- Guard escBuf[i] read when getArgs consumes all input without a command. - Cap WMEMCPY against WOLFSSL_MAX_ESCBUF; escBufSz can exceed 16. - Update escBufSz on WS_WANT_READ resume so CSI leftovers survive multi-fill reads. Issue: F-250
- Without the guard, inner loops short-circuit and WS_SUCCESS is returned without any verification. - Add test case covering the new and existing bad-arg paths. Issue: F-409
- Reject SSH_MSG_NEWKEYS when len != 0 per RFC 4253 7.3. - Update wolfSSH_TestDoNewKeys to take buf/len/idx instead of assuming NULL/0, and add a non-zero len test case. Issue: F-2079
- Per RFC 4253 4.2 use WSTRNCMP instead of WSTRNCASECMP for prefix and full-ID comparisons. - Update client tests; add server test vectors expecting rejection of lowercase/mixed case. Issue: F-2865
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Small hardening pass across protocol parsing, cert verification, and client cleanup.