Fix nine issues from a security audit - #1143
Open
ejohnstown wants to merge 8 commits into
Open
Conversation
ejohnstown
requested review from
wolfSSL-Fenrir-bot
and
a lite review from Copilot
August 4, 2026 23:50
Contributor
There was a problem hiding this comment.
Pull request overview
This PR bundles several targeted fixes across wolfSSH’s SCP support, key exchange/key derivation, channel request parsing, certificate-chain handling, client keyboard-interactive auth handling, and a few cleanup/hardening points; it also adds/extends regression tests to cover the reported failures.
Changes:
- Harden public-facing and protocol-parsing paths (e.g., reject NULL SCP error messages, enforce exact channel request-type matches, reject empty RFC 6187 chains).
- Fix correctness/robustness issues in core logic (e.g., preserve GenerateKey hash errors across loop iterations, handle select() errors and fd_set re-arming, free OpenSSH-certificate key IDs).
- Add/extend unit and API tests for the above behaviors, and adjust test build inputs to reuse example client helpers.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfssh/wolfscp.h | Documents NULL handling for wolfSSH_SetScpErrorMsg. |
| wolfssh/internal.h | Exposes a cert-chain leaf parsing test helper under cert/test builds. |
| src/wolfscp.c | Rejects NULL message in wolfSSH_SetScpErrorMsg. |
| src/internal.c | Fixes key cleanup for OpenSSH cert IDs; preserves GenerateKey errors; rejects empty cert chains; enforces exact channel request-type matches; tightens cert usage checks; zeroizes/frees auth keySig_ptr. |
| src/io.c | Cleans up Harmony send-size clamp with clearer signed/unsigned handling. |
| examples/client/common.c | Fixes keyboard-interactive response lifecycle across rounds; factors response freeing into a helper. |
| examples/client/client.c | Re-arms fd_sets each select() pass and handles select() errors/EINTR correctly. |
| tests/api.c | Adds API regression coverage for NULL args to wolfSSH_SetScpErrorMsg. |
| tests/unit.c | Adds unit coverage for empty cert chains, multi-block GenerateKey, exact channel request matching, and OpenSSH-cert key cleanup. |
| tests/auth.c | Adds keyboard-interactive regression tests (EOF handling and multi-round response reuse). |
| tests/include.am | Links tests/auth.test with example client common helpers needed by new tests. |
💡 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 #1143
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
ejohnstown
changed the title
- check message for NULL alongside ssh - drop the always-true valueSz guard - cover both NULL arguments in test_wolfSSH_SCP_CB Issue: F-7510
- gate the multi-block loop on ret so the first wc_HashFinal result is not discarded - add a SHA-256 two-block key expansion known-answer test Issue: F-7507
- add ChannelRequestIs() comparing the type length as well - use it for all nine handled request types, so truncated, empty and NUL-padded types are rejected - cover "sh", "sub", empty and "shell\0AAAA" in unit tests Issue: F-7217
- fail ParseCertChain when the peer sends a zero cert count - only set isCert when a leaf was actually parsed - add a ParseLeafCert test hook and zero-count unit test Issue: F-7517
- zero the response arrays and stop filling after the first failed read, tracking only the slots populated - release the previous round's responses on each INFO_REQUEST - add auth.test coverage for EOF stdin and a second round Issue: F-7515, F-7516
- store select()'s result in an int so an error is not read as a large positive count - retry on EINTR, bail out on any other error - re-arm the descriptor sets on every pass Issue: F-7508
- cover the ID_OSSH_CERT_* key IDs in wolfSSH_KEY_clean - zeroize the key signature struct before freeing it - add a unit test for the RSA OpenSSH-certificate ID Issue: F-7519
- read TCPIP_TCP_PutIsReady() before comparing, and cast so the clamp is not a signed/unsigned comparison - no behaviour change, the API returns uint16_t Issue: F-7509
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.
scp: reject NULL message in SetScpErrorMsg-- NULL deref in a public API (F-7510)kex: keep GenerateKey hash error in loop-- hash failure was clobbered by the next iteration (F-7507)ssh: match channel request types exactly-- prefix matches accepted truncated request types (F-7217)certs: reject an empty RFC 6187 cert chain-- zero cert count was accepted (F-7517)client: fix keyboard-interactive responses-- out-of-range slot walk and per-round leak (F-7515, F-7516)client: handle select() errors in readPeer-- unsigned return spun on error; fd sets not re-armed (F-7508)auth: free OpenSSH-certificate user keys-- OSSH cert key IDs missed by wolfSSH_KEY_clean (F-7519)io: clean up Harmony send size clamp-- signed/unsigned cleanup, not a bug fix (F-7509)