wolfssh/client: reject unsanitized fields before known_hosts write#1045
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the wolfssh client’s known_hosts update path by validating untrusted name (CLI), type (peer-supplied), and key fields before they can be written as whitespace-delimited, newline-terminated records—preventing field/line injection into ~/.ssh/known_hosts.
Changes:
- Add
IsFieldStorable()and pre-validatename/type/keyinAppendKeyToFile()before performing thefprintfwrite. - Expose a
WOLFSSH_TEST_INTERNALtest hook (wolfSSH_TestAppendKeyToFile) without changing production symbols/behavior. - Add a regression test covering valid writes/appends and rejection of whitespace/control-byte injection attempts.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/wolfssh/common.c | Adds field sanitization before known_hosts append; adds internal-only test hook wrapper. |
| apps/wolfssh/common.h | Declares the internal-only wolfSSH_TestAppendKeyToFile hook under WOLFSSH_TEST_INTERNAL. |
| tests/regress.c | Adds TestAppendKeyToFile to verify correct acceptance/rejection behavior and that invalid inputs do not write. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
af1d7e0 to
b89cf93
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1045
Scan targets checked: wolfssh-bugs, wolfssh-src
No new issues found in the changed files. ✅
b89cf93 to
1c79137
Compare
1c79137 to
0c340e1
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1045
Scan targets checked: wolfssh-bugs, wolfssh-src
No new issues found in the changed files. ✅
Reject unsanitized fields before writing
known_hostsSummary
The wolfssh client wrote the destination host name (from
argv) and thepeer-supplied key-type verbatim into
~/.ssh/known_hostsviafprintf(f, "%s %s %s\n", name, type, key). Since entries arewhitespace-delimited and newline-terminated, an attacker-influenced value with
a newline could inject a forged trusted entry, and a space/tab could
forge fields.
Addressed by f_5136.
Fix (
apps/wolfssh/common.c)static IsFieldStorable(): rejects a field that isNULL, empty, orcontains a space/control byte (
(unsigned char)c <= ' ') or0x7f,returning
WS_BAD_ARGUMENT. High-bit (UTF-8/IDN) bytes are allowed.AppendKeyToFilevalidatesname,type, andkeybefore opening thefile, so a malformed value never reaches
fprintf.typematters becauseit is copied verbatim from the peer's host-key blob; the
keycheck is aNULL/empty guard (Base64 carries no separators).
AppendKeyToFilestaysstatic— no production symbol/behavior change.Testing (
tests/regress.c)New
TestAppendKeyToFile, reached via a#ifdef WOLFSSH_TEST_INTERNALwrapper(
wolfSSH_Test*convention; absent from production builds). Covers: cleanwrite + a second append that preserves the prior entry; rejection of
newline/space/tab/CR/DEL and
NULL/empty inname/type/key, eachasserting nothing is written; and a high-bit name accepted intact.
Verification
regress.testpasses; each guard confirmed with a negative control (revertingthe check fails the matching case); clean under ASan + UBSan (LeakSanitizer
n/a on macOS); production
wolfsshapp builds clean.