Skip to content

Follow up for PR 1218 #1239

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
aidankeefe2022 wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Follow up for PR 1218 #1239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0b1405a
Validate a custom SSH proto ID string
aidankeefe2022 Aug 31, 2026
28de6e1
Skoll review and Github review
aidankeefe2022 Sep 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 50 additions & 16 deletions src/internal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1337,6 +1337,7 @@ WOLFSSH_CTX* CtxInit(WOLFSSH_CTX* ctx, byte side, void* heap)
ctx->maxPacketSz = DEFAULT_MAX_PACKET_SZ;
ctx->maxAuthAttempts = DEFAULT_MAX_AUTH_ATTEMPTS;
ctx->sshProtoIdStr = sshProtoIdStr;
ctx->sshProtoIdStrSz = (word32)(sizeof(sshProtoIdStr) - 1);
ctx->algoListKex = cannedKexAlgoNames;
if (side == WOLFSSH_ENDPOINT_CLIENT) {
ctx->algoListKey = cannedKeyAlgoNamesHostKey;
Expand Down Expand Up @@ -6533,9 +6534,8 @@ static int DoKexInit(WOLFSSH* ssh, byte* buf, word32 len, word32* idx)
}

if (ret == WS_SUCCESS) {
byte SSH_PROTO_EOL_SZ = 2;

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;

@ejohnstown ejohnstown Sep 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@aidankeefe2022 aidankeefe2022 Sep 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created round trip test!

c32toa(strSz, scratchLen);
ret = HashUpdate(hash, hashId, scratchLen, LENGTH_SZ);
}
Expand Down Expand Up @@ -14185,21 +14185,50 @@ int DoProtoId(WOLFSSH* ssh)
/* Validates a locally configured proto ID string */
int ValidateProtoId(const char* protoIdStr, word32 len)
{
/* Length is checked first: the prefix, terminator, and body checks below
* index and subtract from len. The minimum is the "SSH-2.0-" prefix plus
* one body byte plus CRLF. */
if (protoIdStr == NULL || len < SSH_PROTO_SZ + 3 ||
protoIdStr[len-1] != '\n' || protoIdStr[len-2] != '\r' ||
len > WOLFSSH_PROTOID_LIMIT ||
WSTRNCMP(protoIdStr, sshProtoIdPrefix, SSH_PROTO_SZ) != 0 ||
WSTRNSTR(protoIdStr, "\n", len - 2) != NULL ||
WSTRNSTR(protoIdStr, "\r", len - 2) != NULL) {
word32 i;

/* The length check must stay first: every check below indexes into
* protoIdStr or subtracts from the unsigned len. The minimum is the
* "SSH-2.0-" prefix plus one body byte plus CRLF. */
if (protoIdStr == NULL ||
len < SSH_PROTO_MIN ||
len > WOLFSSH_PROTOID_LIMIT) {
WLOG(WS_LOG_ERROR, "Proto Id was invalid: it must be between %d and "
"%d bytes, counting the prefix and the terminator",
SSH_PROTO_MIN, WOLFSSH_PROTOID_LIMIT);
return WS_BAD_ARGUMENT;
}

if (WSTRNCMP(protoIdStr, sshProtoIdPrefix, SSH_PROTO_SZ) != 0) {
WLOG(WS_LOG_ERROR, "Proto Id was invalid: it must start with "
"\"SSH-2.0-\", end in \\r\\n, be no longer than %d bytes, "
"and must not contain \\r or \\n in the body of the line",
WOLFSSH_PROTOID_LIMIT);
"\"SSH-2.0-\"");
return WS_BAD_ARGUMENT;
}

/* RFC 4253 section 4.2 splits the line as "SSH-2.0-" softwareversion
* [SP comments] CRLF. A leading space would make softwareversion
* empty, so reject it. */
if (protoIdStr[SSH_PROTO_SZ] == ' ') {
WLOG(WS_LOG_ERROR, "Proto Id was invalid: the body must start with a "
"non-space character");
return WS_BAD_ARGUMENT;
}

if (protoIdStr[len - 1] != '\n' || protoIdStr[len - 2] != '\r') {
WLOG(WS_LOG_ERROR, "Proto Id was invalid: it must end in \\r\\n");
return WS_BAD_ARGUMENT;
}

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

@ejohnstown ejohnstown Sep 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@aidankeefe2022 aidankeefe2022 Sep 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comment

/* spaces are intetionally allowed */
if (c < 0x20 || c > 0x7e) {
WLOG(WS_LOG_ERROR, "Proto Id was invalid: byte %u is "
"not printable US-ASCII", i);
return WS_BAD_ARGUMENT;
}
}

return WS_SUCCESS;
}

Expand All @@ -14214,7 +14243,7 @@ int SendProtoId(WOLFSSH* ssh)

if (ret == WS_SUCCESS) {
WLOG(WS_LOG_DEBUG, "%s", ssh->ctx->sshProtoIdStr);
sshProtoIdStrSz = (word32)WSTRLEN(ssh->ctx->sshProtoIdStr);
sshProtoIdStrSz = ssh->ctx->sshProtoIdStrSz;
ret = GrowBuffer(&ssh->outputBuffer, sshProtoIdStrSz);
}

Expand Down Expand Up @@ -24251,6 +24280,11 @@ int wolfSSH_TestDoProtoId(WOLFSSH* ssh)
return DoProtoId(ssh);
}

int wolfSSH_TestSendProtoId(WOLFSSH* ssh)
{
return SendProtoId(ssh);
}

int wolfSSH_TestIsMessageAllowed(WOLFSSH* ssh, byte msg, byte state)
{
return IsMessageAllowed(ssh, msg, state);
Expand Down
8 changes: 6 additions & 2 deletions src/ssh.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3472,19 +3472,23 @@ int wolfSSH_GetMaxAuthAttempts(WOLFSSH* ssh)
int wolfSSH_CTX_SetSshProtoIdStr(WOLFSSH_CTX* ctx,
const char* protoIdStr)
{
word32 protoIdStrSz;
int ret;

WLOG(WS_LOG_DEBUG, "Entering wolfSSH_CTX_SetSshProtoIdStr()");

if (!ctx || !protoIdStr) {
return WS_BAD_ARGUMENT;
}

if ((ret = ValidateProtoId(protoIdStr, (word32)WSTRLEN(protoIdStr))) !=
WS_SUCCESS) {
protoIdStrSz = (word32)WSTRLEN(protoIdStr);
ret = ValidateProtoId(protoIdStr, protoIdStrSz);
if (ret != WS_SUCCESS) {
return ret;
}

ctx->sshProtoIdStr = protoIdStr;
ctx->sshProtoIdStrSz = protoIdStrSz;
return WS_SUCCESS;
}

Expand Down
140 changes: 133 additions & 7 deletions tests/unit.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,30 @@ static const ProtoIdScriptVector protoIdScriptVectors[] = {
WOLFSSH_ENDPOINT_CLIENT, WS_VERSION_E },
};

/* Capture-to-buffer send callback for the SendProtoId() vectors. The
* proto ID is the first thing on the wire, so everything the callback
* sees is the ID line itself. */
typedef struct ProtoIdSendState {
byte buf[WOLFSSH_PROTOID_LIMIT + 1];
word32 len;
} ProtoIdSendState;

static int ProtoIdCaptureSend(WOLFSSH* ssh, void* buf, word32 sz, void* ctx)
{
ProtoIdSendState* s = (ProtoIdSendState*)ctx;

WOLFSSH_UNUSED(ssh);

if (sz > sizeof(s->buf) - s->len)
return WS_CBIO_ERR_GENERAL;

WMEMCPY(s->buf + s->len, buf, sz);
s->len += sz;

return (int)sz;
}


/* DoProtoId() Unit Test */
static int test_DoProtoId(void)
{
Expand Down Expand Up @@ -557,24 +581,54 @@ static int test_DoProtoId(void)
}
}

{
static char tooLongProtoId[WOLFSSH_PROTOID_LIMIT + 2];
static char justRightProtoId[WOLFSSH_PROTOID_LIMIT + 1];
/* Ensure a malformed local protoId cannot be loaded. */
{
static const struct {
const char* name;
const char* id;
int expectSuccess;
} protoIds[] = {
{ "conforming custom ID", "SSH-2.0-this_is_my_app\r\n", 1 },
{ "wrong version prefix", "SSH-2-this_is_my_app\r\n", 0 },
{ "LF terminator only", "SSH-2.0-this_is_my_app\n", 0 },
{ "CR terminator only", "SSH-2.0-this_is_my_app\r", 0 },
{ "empty string", "", 0 },
{ "prefix with no body", "SSH-2.0-\r\n", 0 },
{ "missing prefix", "hello\r\n", 0 },
{ "conforming custom ID", "SSH-2.0-this_is_my_app\r\n", 1 },
{ "shortest valid Id", "SSH-2.0-t\r\n", 1 },
{ "exact len custom ID", justRightProtoId, 1 },
/* Pin the printable-ASCII range as inclusive at both ends: an
* interior 0x20 (the RFC 4253 "SP comments" suffix) and a
* 0x7e must both be accepted. */
{ "body w/ SP comments", "SSH-2.0-app comment\r\n", 1 },
{ "body w/ tilde", "SSH-2.0-app~1\r\n", 1 },
/* Failing Tests */
{ "wrong version prefix", "SSH-2-this_is_my_app\r\n", 0 },
{ "bad casing prefix", "sSH-2.0-this_is_my_app\r\n", 0 },
{ "LF terminator only", "SSH-2.0-this_is_my_app\n", 0 },
{ "CR terminator only", "SSH-2.0-this_is_my_app\r", 0 },
{ "empty string", "", 0 },
{ "prefix with no body", "SSH-2.0-\r\n", 0 },
{ "missing prefix", "hello-this-is\r\n", 0 },
{ "non ascii char", "SSH-2.0-\x90s\r\n", 0 },
{ "Body End in CR", "SSH-2.0-s\r\r\n", 0 },
{ "Body End in TAB", "SSH-2.0-s\t\r\n", 0 },
{ "body starts w/ space", "SSH-2.0-\x20-a-b\r\n", 0 },
{ "body has embedded TAB", "SSH-2.0-\x7e-a\t\r\n", 0 },
{ "too long id", tooLongProtoId, 0 },
{ "null pointer", NULL, 0 },
};
int pc = (int)(sizeof(protoIds) / sizeof(protoIds[0]));
WMEMSET(tooLongProtoId, 'a', sizeof(tooLongProtoId));
WMEMCPY(tooLongProtoId, "SSH-2.0-", sizeof("SSH-2.0-") - 1);
tooLongProtoId[WOLFSSH_PROTOID_LIMIT + 1] = '\0';
tooLongProtoId[WOLFSSH_PROTOID_LIMIT] = '\n';
tooLongProtoId[WOLFSSH_PROTOID_LIMIT - 1] = '\r';
WMEMSET(justRightProtoId, 'a', sizeof(justRightProtoId));
WMEMCPY(justRightProtoId, "SSH-2.0-", sizeof("SSH-2.0-") - 1);
justRightProtoId[WOLFSSH_PROTOID_LIMIT] = '\0';
justRightProtoId[WOLFSSH_PROTOID_LIMIT - 1] = '\n';
justRightProtoId[WOLFSSH_PROTOID_LIMIT - 2] = '\r';

for (i = 0; i < pc; i++) {
const char* prevId = clientCtx->sshProtoIdStr;
ret = wolfSSH_CTX_SetSshProtoIdStr(clientCtx, protoIds[i].id);
Comment thread
aidankeefe2022 marked this conversation as resolved.
if ((ret == WS_SUCCESS) != protoIds[i].expectSuccess) {
fprintf(stderr,
Expand All @@ -584,8 +638,80 @@ static int test_DoProtoId(void)
: "WS_BAD_ARGUMENT");
failures++;
}
if (!protoIds[i].expectSuccess &&
clientCtx->sshProtoIdStr != prevId) {
fprintf(stderr,
"\t[protoId %d] \"%s\" FAIL: invalid proto id "
"was stored\n",
i, protoIds[i].name);
failures++;
}
if (clientCtx->sshProtoIdStrSz !=
(word32)WSTRLEN(clientCtx->sshProtoIdStr)) {
fprintf(stderr,
"\t[protoId %d] \"%s\" FAIL: stored sshProtoIdStrSz "
"was not retained\n",
i, protoIds[i].name);
failures++;
}
}
}

/* A configured proto ID must reach the wire byte for byte. */
{
static const char* const sendIds[] = {
"SSH-2.0-this_is_my_app\r\n",
"SSH-2.0-t\r\n",
"SSH-2.0-app comment\r\n",
justRightProtoId,
};
int sc = (int)(sizeof(sendIds) / sizeof(sendIds[0]));

wolfSSH_SetIOSend(clientCtx, ProtoIdCaptureSend);

for (i = 0; i < sc; i++) {
ProtoIdSendState sendState;
word32 expectSz = (word32)WSTRLEN(sendIds[i]);

ret = wolfSSH_CTX_SetSshProtoIdStr(clientCtx, sendIds[i]);
if (ret != WS_SUCCESS) {
fprintf(stderr,
"\t[send %d] FAIL: set proto id returned %d\n",
i, ret);
failures++;
continue;
}

ssh = wolfSSH_new(clientCtx);
if (ssh == NULL) {
fprintf(stderr,
"\t[send %d] FAIL: wolfSSH_new returned NULL\n", i);
failures++;
continue;
}

WMEMSET(&sendState, 0, sizeof(sendState));
wolfSSH_SetIOWriteCtx(ssh, &sendState);

ret = wolfSSH_TestSendProtoId(ssh);
if (ret != WS_SUCCESS) {
fprintf(stderr,
"\t[send %d] FAIL: SendProtoId returned %d\n",
i, ret);
failures++;
}
else if (sendState.len != expectSz ||
WMEMCMP(sendState.buf, sendIds[i], expectSz) != 0) {
fprintf(stderr,
"\t[send %d] FAIL: wrote %u bytes, expected the "
"%u byte proto id back verbatim\n",
i, sendState.len, expectSz);
failures++;
}
wolfSSH_free(ssh);
}
}
}

wolfSSH_CTX_free(serverCtx);
wolfSSH_CTX_free(clientCtx);
Expand Down
6 changes: 6 additions & 0 deletions wolfssh/internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,6 +576,10 @@ enum NameIdType {
#define UINT32_SZ 4
#define LENGTH_SZ UINT32_SZ
#define SSH_PROTO_SZ 8 /* "SSH-2.0-" */
#define SSH_PROTO_EOL_SZ 2 /* "\r\n" */
/* Minimum size for a valid proto id *
* "SSH-2.0-" <at least one body char> "\r\n" */
#define SSH_PROTO_MIN (SSH_PROTO_SZ + 1 + SSH_PROTO_EOL_SZ)
#define TERMINAL_MODE_SZ 5 /* opcode byte + argument uint32 */
#define TERMINAL_MODES_MAX_SZ 4096
#define TERMINAL_WIDTH_DEFAULT 80 /* used when there is no terminal */
Expand Down Expand Up @@ -862,6 +866,7 @@ struct WOLFSSH_CTX {
const char* algoListMac;
const char* algoListKeyAccepted;
word32 bannerSz;
word32 sshProtoIdStrSz; /* validated, counting the CRLF */
word32 windowSz;
word32 maxPacketSz;
word32 maxAuthAttempts; /* server cap on failed userauth */
Expand Down Expand Up @@ -1936,6 +1941,7 @@ enum WS_MessageIdLimits {

#ifdef WOLFSSH_TEST_INTERNAL
WOLFSSH_API int wolfSSH_TestDoProtoId(WOLFSSH* ssh);
WOLFSSH_API int wolfSSH_TestSendProtoId(WOLFSSH* ssh);
WOLFSSH_API int wolfSSH_TestIsMessageAllowed(WOLFSSH* ssh, byte msg,
byte state);
WOLFSSH_API int wolfSSH_TestDoReceive(WOLFSSH* ssh);
Expand Down
12 changes: 9 additions & 3 deletions wolfssh/ssh.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -656,11 +656,17 @@ WOLFSSH_API int wolfSSH_CTX_SetBanner(WOLFSSH_CTX* ctx, const char* newBanner);
* MUST be between 11 and 255 bytes in length, counting the "SSH-2.0-"
* prefix and the trailing "\r\n"
* MUST end with '\r\n'
* MUST NOT contain '\r' or '\n' in the body
* MUST carry only printable US-ASCII (0x20 - 0x7e) in the body, which
* rules out an embedded '\r' or '\n'
* MUST NOT begin the body with a space; RFC 4253 section 4.2 reads the
* body as softwareversion [SP comments], so a leading space would
* make softwareversion empty. A space later in the body is accepted
* and starts the optional comments field.
* If these are not adhered to the function will return WS_BAD_ARGUMENT
* and not load the ProtoId in to the WOLFSSH_CTX struct.
* and not load the ProtoId into the WOLFSSH_CTX struct.
* ProtoIdStr is stored by reference and is not copied, so it must remain
* valid for the lifetime of the WOLFSSH_CTX. */
* valid and unmodified for the lifetime of the WOLFSSH_CTX. It is validated
* once, here; a later in-place rewrite of the buffer is not revalidated. */
WOLFSSH_API int wolfSSH_CTX_SetSshProtoIdStr(WOLFSSH_CTX* ctx,
const char* protoIdStr);
/* Set the server-side limit on failed userauth attempts per connection. The
Expand Down
Loading