Uh oh!
There was an error while loading. Please reload this page.
F-8824: enable config files without trailing newline - #1156
Conversation
81fef31 to
601d645CompareThere was a problem hiding this comment.
Pull request overview
Enables known_hosts parsing to handle files whose last line does not end with a trailing newline, and adds regression coverage to prevent reintroducing the issue.
Changes:
- Make
load_der_file()allocate an extra byte and NUL-terminate the buffer past the file contents. - Update
ClientPublicKeyCheck()parsing to avoid clobbering the last byte and to normalize CRLF line endings. - Add a regression test that exercises last-entry matching with
\n, no trailing newline, and\r\n.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/wolfssh/common.c | NUL-terminate loaded file buffers and normalize CRLF in known_hosts parsing. |
| tests/regress.c | Add regression test for known_hosts last-line handling (with/without trailing newline and CRLF). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1156
Scan targets checked:wolfssh-bugs, wolfssh-src
Findings: 5
5 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
601d645 to
bf2d332CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1156
Scan targets checked:wolfssh-bugs, wolfssh-src
Findings: 5
5 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
| "other.example.com ssh-rsa AAAA%s%s ssh-rsa %s%s", | ||
| cases[i].sep, targetName, wrongKey, cases[i].tail); | ||
| WriteKnownHosts(hostsPath, contents); | ||
| AssertTrue(ClientPublicKeyCheck(pubKey, (word32)sizeof(pubKey), |
There was a problem hiding this comment.
🔵 [Low] Rejection assertion cannot distinguish a bad-key rejection from an unparsed entry · Weak or missing assertions
ClientPublicKeyCheck returns -1 both for the badMatch path and for the unknown-host path, where GetConfirmation() reads EOF from the redirected stdin and declines. The != 0 assertion therefore passes even if the last entry is never parsed, so it does not pin the behaviour its comment describes.
Fix: Capture the prompt's stdout or add a distinguishable return/observable so the bad-key rejection is asserted separately from the trust-on-first-use decline.
| size_t lineSz = WSTRLEN(line); | ||
| /* Remove trailing CR if present for comparison below */ | ||
| if (lineSz > 0 && line[lineSz - 1] == '\r') { |
There was a problem hiding this comment.
⚪ [Info] Redundant lineSz 0 guard is always true · Dead error handling
The enclosing if (line != NULL && *line) already guarantees a non-empty string, so WSTRLEN(line) is at least 1 and the lineSz > 0 sub-condition can never be false.
Related known finding #8812 (similar but distinct): Both findings identify an always-true condition caused by prior string-length/termination guarantees, but this operates on a known_hosts line in ClientPublicKeyCheck while issue 8812 operates on a path buffer in wolfSSH_GetPath. The faulting checks and immediate causes differ, and each requires a separate patch.
Fix: Drop the lineSz > 0 term and test line[lineSz - 1] == '\r' alone.
| AssertIntEQ(WFOPEN(NULL, &f, path, "wb"), 0); | ||
| AssertTrue(f != WBADFILE); | ||
| AssertIntEQ((word32)WFWRITE(NULL, contents, 1, sz, f), sz); |
There was a problem hiding this comment.
🔵 [Low] WriteKnownHosts dereferences a failed file handle when WOLFSSH_NO_ABORT is set · NULL pointer dereference
Assert expands to if (!(test)) Fail(...), and Fail reduces to a bare printf when WOLFSSH_NO_ABORT is defined (regress.c:50-67). If WFOPEN fails, f remains WBADFILE (NULL on POSIX) and execution falls through to WFWRITE/WFCLOSE, dereferencing it. LoadFileBuffer at regress.c:474 uses a real if guard instead.
Fix: Guard the write and close with if (f != WBADFILE) rather than relying on Assert for control flow.
| "other.example.com ssh-rsa AAAA%s%s ssh-rsa %s%s", | ||
| cases[i].sep, targetName, wrongKey, cases[i].tail); | ||
| WriteKnownHosts(hostsPath, contents); | ||
| AssertTrue(ClientPublicKeyCheck(pubKey, (word32)sizeof(pubKey), |
There was a problem hiding this comment.
🔵 [Low] Negative known_hosts assertion cannot distinguish a hard key-mismatch reject from a TOFU prompt reading EOF · Weak or missing assertions
ClientPublicKeyCheck returns -1 both for the intended badMatch hard reject (common.c:459) and for the unknown-host TOFU path where GetConfirmation() reads EOF from the redirected /dev/null stdin (common.c:486-491). The != 0 check passes either way, so a regression that downgrades a known-host key mismatch into a user prompt would not be caught.
Fix: Assert the exact return value and distinguish the reject path from the prompt path, e.g. by feeding a 'Y' on stdin so an unintended prompt would return success.
| /* An entry for a different host goes first, so the match lands on the | ||
| * last line, the one the terminator used to overwrite. */ | ||
| WSNPRINTF(contents, sizeof(contents), |
There was a problem hiding this comment.
⚪ [Info] otherMatch branch newly reachable under CRLF known_hosts is not covered · Missing edge-case coverage on a function the PR also changed
The CR strip changes reachability of the otherMatch branch at common.c:440-449: under CRLF a same-key/different-host entry previously never matched, and now it does, driving the fingerprint print plus AppendKeyToFile prompt at common.c:467-469. All three fixture cases use a non-matching AAAA key for the other host, so that branch is never entered.
Fix: Add a case where the non-target host line carries the same encoded key so the otherMatch branch is exercised under CRLF and no-trailing-newline inputs.
No description provided.