Fixing fuzz testing found bugs#1022
Conversation
ejohnstown
changed the title
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 tightens SSH parsing to prevent non-canonical/unsafe encodings by rejecting negative mpints, validating ECDSA host key blob fields against the negotiated algorithm/curve, and fixing RSA signature parsing to treat the signature blob as a string (per RFC 4253).
Changes:
- Update
GetMpint()to parse like a string and reject values with the sign bit set. - Harden
ParseECCPubKey()by validating blob algorithm + curve names against negotiated values and derive curve from negotiation. - Parse RSA signature blobs as strings (not mpints) and add a new
GetMpint()unit test.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tests/unit.c | Adds unit coverage for GetMpint() (positive, leading-zero, zero-length, negative, bounds). |
| src/internal.c | Hardens mpint parsing, ECDSA host key blob validation, and RSA signature blob parsing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1022
Scan targets checked: wolfssh-bugs, wolfssh-src
No new issues found in the changed files. ✅
- GetMpint now rejects values with the sign bit set as non-canonical (RFC 4251 Section 5) and is rewritten in terms of GetStringRef. - Parse RSA signature blobs with GetStringRef; they are strings of raw signature bytes (RFC 4253 Section 6.6), not mpints, and often have the high bit set. - Add unit test covering GetMpint parsing and rejection. - Add a DoUserAuthRequestRsa test pinning the string parse of the signature blob with a fixed signature whose leading byte has the high bit set; an mpint parse would reject it as negative. Issue: wolfSSL#1013
- Validate blob algorithm name against handshake pubKeyId - Derive curve from negotiated algo, not the key blob - Check curve name instead of skipping it - Add a ParseECCPubKey test checking the key blob algorithm and curve names are validated against the negotiated host key algorithm. Issue: wolfSSL#1012
- Parse each field of the key blob in ParseECCPubKey with its own scoped variables instead of reusing q/qSz for the algorithm name, curve name, and public key point.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1022
Scan targets checked: wolfssh-bugs, wolfssh-src
No new issues found in the changed files. ✅
Set keyAllocated right after wc_ecc_init_ex so FreePubKey frees the initialized key on the early error exits, not only after a successful import.
Run the DoUserAuthRequestRsa vectors through the X.509 path to pin that a raw signature with a high leading bit is accepted via the RFC 6187 branch. keys/ has no RSA certificate, so the test embeds a self-signed cert made from keys/hansel-key-rsa.pem (valid until May 2048).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
tests/unit.c:1
- The embedded certificate DER includes a
notBeforeUTCTime of260612174509Z(June 12, 2026). If any build/config enables certificate time validation along theDoUserAuthRequestRsaCert()path, this can make the unit test time-dependent/flaky (failing before that timestamp). Prefer regenerating the test certificate with anotBeforefar in the past to keep tests deterministic.
/* unit.c
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1022
Scan targets checked: wolfssh-bugs, wolfssh-src
No new issues found in the changed files. ✅
Issues: #1012, #1013