Follow up for PR 1218 by aidankeefe2022 · Pull Request #1239 · wolfSSL/wolfssh

aidankeefe2022 · 2026-09-03T17:35:01Z

Added more tests, broke up ValidateProtoId conditions to make better errors and added more checks, added ProtoIdSz field to travel with the ProtoId buffer.

wolfSSL-Fenrir-bot

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

wolfSSL-Fenrir-bot

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

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.

ejohnstown · 2026-09-04T01:35:33Z

+            if ((ret == WS_SUCCESS) && clientCtx->sshProtoIdStrSz !=
+                    WSTRLEN(protoIds[i].id)) {
+                fprintf(stderr,
+                        "\t[protoId %d] \"%s\" FAIL: stored sshProtoIdSz "


The field this PR adds is sshProtoIdStrSz (see wolfssh/internal.h), but this message prints sshProtoIdSz. A grep for the name a failing test prints should land on the field it is talking about. The PR description has the same slip.

ejohnstown · 2026-09-04T01:35:33Z

+            if ((ret == WS_SUCCESS) && clientCtx->sshProtoIdStrSz !=
+                    WSTRLEN(protoIds[i].id)) {
+                fprintf(stderr,
+                        "\t[protoId %d] \"%s\" FAIL: stored sshProtoIdSz "
+                        "was not retained\n",
+                        i, protoIds[i].name);
+                failures++;
+            }


This checks the size only on the success path. A future edit that assigns ctx->sshProtoIdStrSz before ValidateProtoId() runs, or that clears one field without the other on rejection, would leave the CTX with a size that no longer matches the string, and this test would still pass. That desync is exactly what DoKexInit() can no longer catch now that it does not recompute WSTRLEN().

Cheapest fix is one unconditional assertion after each call, outside the ret == WS_SUCCESS guard:

if (clientCtx->sshProtoIdStrSz != (word32)WSTRLEN(clientCtx->sshProtoIdStr)) { /* fields are out of sync */ }

That proves the pair is consistent regardless of outcome, and the table already sets it up: "exact len custom ID" succeeds and every vector after it is expected to fail, so the tail is a ready-made "a reject must not clobber the stored ID" sequence. Safe to add -- this block is the last use of clientCtx before the frees.

added check that a failing proto id does not write to the ctx struct and added the size compare check

ejohnstown · 2026-09-04T01:35:33Z

+        WMEMSET(tooLongProtoId, 'a', sizeof(tooLongProtoId));
+        WMEMCPY(tooLongProtoId, "SSH-2.0-", sizeof("SSH-2.0-") - 1);
+        tooLongProtoId[256] = '\0';
+        tooLongProtoId[255] = '\n';
+        tooLongProtoId[254] = '\r';
+        WMEMSET(justRightProtoId, 'a', sizeof(justRightProtoId));
+        WMEMCPY(justRightProtoId, "SSH-2.0-", sizeof("SSH-2.0-") - 1);
+        justRightProtoId[255] = '\0';
+        justRightProtoId[254] = '\n';
+        justRightProtoId[253] = '\r';


These literal indices restate the [257]/[256] declarations above and WOLFSSH_PROTOID_LIMIT; change any one of the three and the terminator lands in the wrong place. Because the arrays are WMEMSET to 'a', a misplaced NUL still yields a well-formed-looking C string, so the test would keep passing while no longer sitting on the boundary it is meant to test.

WOLFSSH_PROTOID_LIMIT is already visible here through wolfssh/internal.h, so declaring tooLongProtoId[WOLFSSH_PROTOID_LIMIT + 2] and justRightProtoId[WOLFSSH_PROTOID_LIMIT + 1], then writing the terminators at sizeof(buf) - 1 / - 2 / - 3, makes "exactly at the limit" and "one past it" self-evident and self-maintaining.

ejohnstown · 2026-09-04T01:35:33Z

-
-            strSz = (word32)WSTRLEN(ssh->ctx->sshProtoIdStr) - SSH_PROTO_EOL_SZ;
+            /* The ID is hashed without its terminator. */
+            strSz = ssh->ctx->sshProtoIdStrSz - SSH_PROTO_EOL_SZ;


Nothing in the tree drives a handshake with a custom proto ID. wolfSSH_CTX_SetSshProtoIdStr() has exactly one caller, the unit test, and that only checks the accept/reject verdict and the stored size. So the one path this field exists for has no KEX coverage, and a desync between sshProtoIdStr and sshProtoIdStrSz would surface only as an exchange-hash mismatch, which is a miserable failure to diagnose.

No live defect -- CtxInit() and the setter both assign the string and the size from the same source in the same breath -- so this is optional rather than owed. If it does get written, the max-length case is the interesting one, and it should pass: a 255-byte ID is 253 body bytes plus CRLF, and the peer's GetInputLine() reads all 255 in one go and finds the LF before the length >= WOLFSSH_PROTOID_LIMIT cap fires. The accept boundary this PR adds does round-trip.

Created round trip test!

ejohnstown · 2026-09-04T01:35:33Z

+    }
+
+    for (i = 0; i < len - SSH_PROTO_EOL_SZ; i++) {
+        byte c = (byte)protoIdStr[i];


For the record, no change needed. RFC 4253 Section 4.2 has softwareversion as printable US-ASCII excluding whitespace and the minus sign, so accepting all of 0x20-0x7e is looser than the spec -- but it is a tightening over the old check, and the mandatory SSH-2.0- prefix contains -, so a strict split would have to exempt the prefix. It also cannot affect interop: ValidateProtoId() only ever sees the locally configured string, and a peer's ID goes through DoProtoId() untouched by this change.

If anything, a one-line comment noting that SP is permitted on purpose (it separates the RFC 4253 comments field) would keep a later reader from taking the leniency for an oversight.

Added comment

wolfSSL-Fenrir-bot

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.

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

aidankeefe2022 self-assigned this Sep 3, 2026

aidankeefe2022 requested a review from wolfSSL-Fenrir-bot September 3, 2026 17:35

wolfSSL-Fenrir-bot previously requested changes Sep 3, 2026

View reviewed changes

Comment thread tests/unit.c

aidankeefe2022 commented Sep 3, 2026

View reviewed changes

Comment thread tests/unit.c

aidankeefe2022 force-pushed the follow-up-for-PR-1218 branch from 985107a to 0b1405a Compare September 3, 2026 18:04

aidankeefe2022 assigned ejohnstown and aidankeefe2022 and unassigned aidankeefe2022 and ejohnstown Sep 3, 2026

aidankeefe2022 requested a review from wolfSSL-Fenrir-bot September 3, 2026 19:19

wolfSSL-Fenrir-bot reviewed Sep 3, 2026

View reviewed changes

aidankeefe2022 assigned wolfSSL-Bot and unassigned aidankeefe2022 Sep 3, 2026

ejohnstown reviewed Sep 4, 2026

View reviewed changes

ejohnstown assigned aidankeefe2022 and unassigned wolfSSL-Bot Sep 4, 2026

aidankeefe2022 requested a review from wolfSSL-Fenrir-bot September 4, 2026 18:54

wolfSSL-Fenrir-bot reviewed Sep 4, 2026

View reviewed changes

Skoll review and Github review

28de6e1

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

aidankeefe2022 force-pushed the follow-up-for-PR-1218 branch from 3cfc151 to 28de6e1 Compare September 4, 2026 19:43

Uh oh!

Conversation

aidankeefe2022 commented Sep 3, 2026

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #1239

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #1239

Uh oh!

ejohnstown left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aidankeefe2022 Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #1239

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aidankeefe2022 Sep 4, 2026 •

edited

Loading