Follow up for PR 1218 - #1239
Conversation
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1239
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. Reported findings require changes before merge.
wolfSSH_CTX_SetSshProtoIdStr() now rejects a string that is not CRLF-terminated, exceeds 255 bytes counting the terminator, or carries a CR or LF in the body. DoKexInit() subtracts the two terminator bytes from the length when hashing it, so an unterminated string underflowed the hash length. Issue: F-10571 partial progress
985107a to
0b1405a
Compare
aidankeefe2022
assigned
ejohnstown and
aidankeefe2022 and unassigned
aidankeefe2022 and
ejohnstown
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1239
Scan targets checked: wolfssh-bugs, wolfssh-src
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
ejohnstown
left a comment
ejohnstown
left a comment
There was a problem hiding this comment.
Went through the Skoll report on this PR and checked each finding against the code. Built the branch with --enable-all under -Wall -Werror: clean, and tests/unit.test passes (DoProtoId: SUCCESS, binary exits 0). No correctness defects -- everything below is a test or naming point.
Suggested order: the rename first, then the single consistency assertion in the loop. The fixture indices and the KEX-coverage note are optional.
One thing not in the report, about the ssh.h change: caching the size is the only semantic change here. An embedder who rewrites the referenced buffer in place to something shorter now gets a stale length in SendProtoId()'s GrowBuffer/WMEMCPY, where before WSTRLEN() was recomputed at send time and stayed self-consistent. The new doc line ("valid and unmodified ... a later in-place rewrite of the buffer is not revalidated") covers exactly that -- worth knowing that line is load-bearing, not decoration.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1239
Scan targets checked: wolfssh-bugs, wolfssh-src
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
3cfc151 to
28de6e1
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1239
Scan targets checked: wolfssh-bugs, wolfssh-src
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
28de6e1 to
2ffcdc3
Compare
Added more test vectors as well as locking in behavior of VerifyProtoId Added round trip test for proto id Comment fixes New define for proto id min size
2ffcdc3 to
e2cb97b
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1239
Scan targets checked: wolfssh-bugs, wolfssh-src
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
4 participants
Added more tests, broke up ValidateProtoId conditions to make better errors and added more checks, added ProtoIdSz field to travel with the ProtoId buffer.