From fd4d462510d411d853b2fe19738388eeedb3f612 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Tue, 15 Sep 2026 19:57:06 -0700 Subject: [PATCH] scp: match the scp command as a whole token wolfSSH_ChannelCommandIsScp() reports whether a channel's session command starts an SCP transfer, taking "scp" only as its own token. The divert in wolfSSH_accept() and an application's exec callback both ask it, so the two cannot disagree about what the SCP server is handed. - an exec of "scpbackup foo" runs as an ordinary exec - a command carrying a NUL within the recorded command size is not an SCP command; ParseScpCommand() walks a C string, so a NUL would drop whatever follows it - wolfsshd's session callback asks through the same helper --- apps/wolfsshd/wolfsshd.c | 17 +++------- src/ssh.c | 3 +- src/wolfscp.c | 30 ++++++++++++----- tests/regress.c | 73 ++++++++++++++++++++++++++++++++++++++++ wolfssh/internal.h | 1 - wolfssh/wolfscp.h | 14 ++++++++ 6 files changed, 116 insertions(+), 22 deletions(-) diff --git a/apps/wolfsshd/wolfsshd.c b/apps/wolfsshd/wolfsshd.c index 218b4b19..a46a3ae2 100644 --- a/apps/wolfsshd/wolfsshd.c +++ b/apps/wolfsshd/wolfsshd.c @@ -502,18 +502,11 @@ static int SessionRequestCb(WOLFSSH_CHANNEL* channel, void* vCtx) break; } #ifdef WOLFSSH_SCP - { - word32 cmdSz = wolfSSH_ChannelGetSessionCommandSz(channel); - - /* "scp" must stand as its own token; a prefix match - * grants "scpbackup", and a NUL makes it another. */ - if (cmdSz >= (word32)WSTRLEN("scp") - && WSTRNCMP(cmd, "scp", 3) == 0 - && (cmdSz == (word32)WSTRLEN("scp") - || cmd[3] == ' ')) { - rej = 0; - break; - } + /* Shared with the SCP divert in wolfSSH_accept(), so the two + * cannot disagree about what starts a transfer. */ + if (wolfSSH_ChannelCommandIsScp(channel) == 1) { + rej = 0; + break; } #endif #ifdef WOLFSSH_SHELL diff --git a/src/ssh.c b/src/ssh.c index a38af96d..dee524dd 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -831,7 +831,8 @@ int wolfSSH_accept(WOLFSSH* ssh) #ifdef WOLFSSH_SCP if (ssh->channelList != NULL && ssh->channelList->sessionGranted - && ChannelCommandIsScp(ssh)) { + && wolfSSH_ChannelCommandIsScp(ssh->channelList) + == 1) { ssh->acceptState = ACCEPT_INIT_SCP_TRANSFER; WLOG(WS_LOG_DEBUG, acceptState, "ACCEPT_INIT_SCP_TRANSFER"); return WS_SCP_INIT; diff --git a/src/wolfscp.c b/src/wolfscp.c index cbbeb704..7d69cd5d 100644 --- a/src/wolfscp.c +++ b/src/wolfscp.c @@ -1040,20 +1040,34 @@ WOLFSSH_API int wolfSSH_SetScpErrorMsg(WOLFSSH* ssh, const char* message) return ret; } -/* Determine if channel command sent in initial negotiation is scp. - * Return 1 if yes, 0 if no */ -int ChannelCommandIsScp(WOLFSSH* ssh) +/* Determine if the channel's session command is scp. See wolfscp.h for + * the contract; "scp" must stand as its own token. */ +int wolfSSH_ChannelCommandIsScp(const WOLFSSH_CHANNEL* channel) { const char* cmd; + word32 cmdSz; + word32 scpSz = (word32)WSTRLEN("scp"); + word32 i; int ret = 0; - if (ssh == NULL) + if (channel == NULL) return WS_BAD_ARGUMENT; - cmd = wolfSSH_GetSessionCommand(ssh); - if (cmd != NULL && WSTRLEN(cmd) >= 3) { - if (WSTRNCMP(cmd, "scp", 3) == 0) - ret = 1; + cmd = wolfSSH_ChannelGetSessionCommand(channel); + cmdSz = wolfSSH_ChannelGetSessionCommandSz(channel); + + if (cmd != NULL && cmdSz >= scpSz + && WSTRNCMP(cmd, "scp", scpSz) == 0 + && (cmdSz == scpSz || cmd[scpSz] == ' ')) { + ret = 1; + } + + /* The parse that follows is a C string walk, so a NUL inside the + * command would drop the rest of it. Refuse rather than transfer + * something other than what was asked for. */ + for (i = 0; ret == 1 && i < cmdSz; i++) { + if (cmd[i] == '\0') + ret = 0; } return ret; diff --git a/tests/regress.c b/tests/regress.c index 1592f2b8..2edf881e 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -5788,6 +5788,78 @@ static void TestAcceptDivertNeedsScpGrantAlone(void) FreeChannelOpenHarness(&harness); } + +/* The SCP divert reads only the command, so "scp" has to stand as its own + * token: a bare prefix match takes "scpbackup" into the built-in SCP + * server. A NUL anywhere in the command hides what follows it from the + * parse that serves the transfer, whether it sits where the space belongs + * or past a good token boundary. wolfsshd's exec callback asks the same + * question with the same helper, so the two cannot answer it + * differently. */ +static void TestAcceptDivertMatchesScpCommandToken(void) +{ + static const struct { + const char* cmd; + word32 cmdSz; + int divert; + } cases[] = { + { "scp -f /tmp/a", 13, 1 }, /* what the real client sends */ + { "scp", 3, 1 }, /* the token alone */ + { "scpbackup foo", 13, 0 }, /* the token is only a prefix */ + { "scp\0 -f /tmp/a", 14, 0 }, /* a NUL where the space belongs */ + { "scp -t /d\0x", 11, 0 }, /* a NUL hiding a tail past the token */ + { "scp -t /d\0", 10, 0 }, /* a NUL counted at the end */ + { "sc", 2, 0 }, /* shorter than the token */ + { "ls", 2, 0 } + }; + ChannelOpenHarness harness; + WOLFSSH_CHANNEL* channel; + byte payload[128]; + byte in[128]; + word32 idx; + word32 inSz; + word32 i; + + AssertIntEQ(wolfSSH_ChannelCommandIsScp(NULL), WS_BAD_ARGUMENT); + + for (i = 0; i < sizeof(cases) / sizeof(cases[0]); i++) { + InitChannelOpenHarness(&harness, NULL, 0); + channel = SeedUnconfirmedChannel(&harness); + AssertIntEQ(ChannelUpdatePeer(channel, 5, 1024, 1024), WS_SUCCESS); + channel->openConfirmed = 1; + + idx = 0; + idx = AppendUint32(payload, sizeof(payload), idx, channel->channel); + idx = AppendString(payload, sizeof(payload), idx, "exec"); + idx = AppendByte(payload, sizeof(payload), idx, 1); + idx = AppendUint32(payload, sizeof(payload), idx, cases[i].cmdSz); + idx = AppendData(payload, sizeof(payload), idx, + (const byte*)cases[i].cmd, cases[i].cmdSz); + inSz = WrapPacket(MSGID_CHANNEL_REQUEST, payload, idx, in, sizeof(in)); + RepointHarnessInput(&harness, in, inSz); + + /* Neither app-channels nor a callback, so the request is granted + * and the session is the one wolfSSH_accept() goes on to serve. */ + AssertIntEQ(DoReceive(harness.ssh), WS_SUCCESS); + AssertIntEQ(channel->commandSz, cases[i].cmdSz); + AssertIntEQ(wolfSSH_ChannelCommandIsScp(channel), cases[i].divert); + RepointHarnessInput(&harness, NULL, 0); + + harness.ssh->acceptState = ACCEPT_SERVER_CHANNEL_ACCEPT_SENT; + if (cases[i].divert) { + AssertIntEQ(wolfSSH_accept(harness.ssh), WS_SCP_INIT); + AssertIntEQ(harness.ssh->acceptState, ACCEPT_INIT_SCP_TRANSFER); + } + else { + AssertIntEQ(wolfSSH_accept(harness.ssh), WS_SUCCESS); + AssertIntEQ(harness.ssh->acceptState, + ACCEPT_CLIENT_SESSION_ESTABLISHED); + } + + FreeChannelOpenHarness(&harness); + } +} + #endif /* WOLFSSH_SCP */ /* A username change after the first userauth request must end the session. */ @@ -15955,6 +16027,7 @@ int main(int argc, char** argv) TestAcceptDivertNeedsSftpGrantAlone(); #endif #ifdef WOLFSSH_SCP + TestAcceptDivertMatchesScpCommandToken(); TestAcceptDivertNeedsScpGrant(); TestAcceptDivertNeedsScpGrantAlone(); #endif diff --git a/wolfssh/internal.h b/wolfssh/internal.h index a18c8b42..d41e9330 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -2259,7 +2259,6 @@ enum WS_ScpDirection { WOLFSSH_SCP_FROM }; -WOLFSSH_LOCAL int ChannelCommandIsScp(WOLFSSH* ssh); WOLFSSH_LOCAL int DoScpRequest(WOLFSSH* ssh); WOLFSSH_LOCAL int DoScpSink(WOLFSSH* ssh); WOLFSSH_LOCAL int DoScpSource(WOLFSSH* ssh); diff --git a/wolfssh/wolfscp.h b/wolfssh/wolfscp.h index 1ebeb339..84767aae 100644 --- a/wolfssh/wolfscp.h +++ b/wolfssh/wolfscp.h @@ -170,6 +170,20 @@ WOLFSSH_API int wolfSSH_SCP_from(WOLFSSH* ssh, const char* src, * done; call it again on the same session until it completes. */ WOLFSSH_API int wolfSSH_SCP_accept(WOLFSSH* ssh); +/* Tells an SCP request from an ordinary exec, for use from an exec + * channel-request callback and by wolfSSH_accept() itself, so the two + * cannot disagree about what starts a transfer. + * + * Returns 1 when the channel's session command starts an SCP transfer, 0 + * when it does not, and WS_BAD_ARGUMENT when channel is NULL. "scp" must + * stand as its own token: a bare prefix match would take "scpbackup" as a + * transfer. The test runs over the recorded command size rather than the + * string length, and a command carrying a NUL anywhere within that size + * is not an SCP command: the parse that serves the transfer walks a C + * string, so a NUL would silently drop whatever follows it. */ +WOLFSSH_API int wolfSSH_ChannelCommandIsScp( + const WOLFSSH_CHANNEL* channel); + #ifdef __cplusplus }