From 0a9b44a962fe63c22b88a782955412cf8c19f0e7 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 9 Sep 2026 14:43:24 -0700 Subject: [PATCH] internal: scrub the channel command on free The peer's exec or subsystem command line is wiped ahead of both frees that release it, ChannelDelete() and the GetStringAlloc() that replaces it on a repeat request, the way ChannelDelete() already wipes the decrypted inputBuffer just above. A command line can carry a password or a token among its arguments. - ScrubChannelCommand() leaves the free to GetStringAlloc(), so a parse that fails behind it holds no dangling pointer - cover both wipes with the retain-on-free allocator, the replacement through wolfSSH_TestDoChannelRequest() - release the test's hand-built channel on a setup failure Issue: F-8850 --- src/internal.c | 18 +++- tests/unit.c | 236 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 253 insertions(+), 1 deletion(-) diff --git a/src/internal.c b/src/internal.c index c558fdf6..fe3ab098 100644 --- a/src/internal.c +++ b/src/internal.c @@ -4200,8 +4200,11 @@ void ChannelDelete(WOLFSSH_CHANNEL* channel, void* heap) channel->channel); } ShrinkBuffer(&channel->extDataBuffer, 1); - if (channel->command) + /* Scrub the peer's command line, which can carry credentials. */ + if (channel->command != NULL) { + WS_FORCEZERO(channel->command, channel->commandSz); WFREE(channel->command, heap, DYNTYPE_STRING); + } WFREE(channel, heap, DYNTYPE_CHANNEL); } } @@ -13087,6 +13090,17 @@ static void SetTerminalSize(WOLFSSH* ssh, word32 widthChar, word32 heightRows, #endif /* WOLFSSH_TERM */ +/* Wipe the old command ahead of the GetStringAlloc() that frees it, so a + * repeat request leaves no credentials behind in the freed block. */ +static void ScrubChannelCommand(WOLFSSH_CHANNEL* channel) +{ + if (channel->command != NULL) { + WS_FORCEZERO(channel->command, channel->commandSz); + channel->commandSz = 0; + } +} + + static int DoChannelRequest(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) { @@ -13158,6 +13172,7 @@ static int DoChannelRequest(WOLFSSH* ssh, ssh->clientState = CLIENT_DONE; } else if (ChannelRequestIs(type, typeSz, "exec")) { + ScrubChannelCommand(channel); ret = GetStringAlloc(ssh->ctx->heap, &channel->command, &channel->commandSz, buf, len, &begin); @@ -13179,6 +13194,7 @@ static int DoChannelRequest(WOLFSSH* ssh, ssh->clientState = CLIENT_DONE; } else if (ChannelRequestIs(type, typeSz, "subsystem")) { + ScrubChannelCommand(channel); ret = GetStringAlloc(ssh->ctx->heap, &channel->command, &channel->commandSz, buf, len, &begin); diff --git a/tests/unit.c b/tests/unit.c index 06a142c1..91859255 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -17036,6 +17036,230 @@ out: return result; } + +/* Verify ChannelDelete wipes the peer's exec/subsystem command line before + * releasing it. A command can carry a password or token in its arguments, + * and the buffer sits right below the inputBuffer this function already + * scrubs. The retain-on-free allocator is installed just around + * ChannelDelete so the freed bytes can be read back without touching + * freed memory. */ +static int test_ChannelDelete_zeroesCommand(void) +{ + static const char command[] = "sh -c 'login --password hunter2'"; + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* channel = NULL; + const byte* commandBytes; + word32 commandSz; + word32 i; + int result = 0; + wolfSSL_Malloc_cb prevMf = NULL; + wolfSSL_Free_cb prevFf = NULL; + wolfSSL_Realloc_cb prevRf = NULL; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -710; + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { + result = -711; + goto out; + } + + channel = ChannelNew(ssh, ID_CHANTYPE_SESSION, + DEFAULT_WINDOW_SZ, DEFAULT_MAX_PACKET_SZ); + if (channel == NULL) { + result = -712; + goto out; + } + + commandSz = (word32)WSTRLEN(command); + channel->command = (char*)WMALLOC(commandSz + 1, NULL, DYNTYPE_STRING); + if (channel->command == NULL) { + result = -713; + goto out; + } + WMEMCPY(channel->command, command, commandSz + 1); + channel->commandSz = commandSz; + commandBytes = (const byte*)channel->command; + + wolfSSL_GetAllocators(&prevMf, &prevFf, &prevRf); + /* Allocators unchanged on failure; nothing to restore. */ + if (wolfSSL_SetAllocators(RetainMalloc, RetainFree, + RetainRealloc) != 0) { + result = -714; + goto out; + } + ChannelDelete(channel, NULL); + wolfSSL_SetAllocators(prevMf, prevFf, prevRf); + channel = NULL; + + if (!IsRetained((void*)commandBytes)) { + result = -715; + goto out; + } + + for (i = 0; i < commandSz; i++) { + if (commandBytes[i] != 0) { + result = -716; + goto out; + } + } + +out: + DrainRetained(); + /* Only the setup-failure paths reach here with a channel; it is never + * on ssh->channelList, so wolfSSH_free() would not release it. */ + if (channel != NULL) + ChannelDelete(channel, ssh->ctx->heap); + if (ssh != NULL) + wolfSSH_free(ssh); + if (ctx != NULL) + wolfSSH_CTX_free(ctx); + return result; +} + +#ifdef WOLFSSH_TEST_INTERNAL + +/* [uint32 channelId][string "exec"][byte wantReply][string command] */ +static word32 BuildExecRequestPayload(byte* out, word32 outSz, + const char* command) +{ + word32 commandSz = (word32)WSTRLEN(command); + word32 idx = 0; + + if (outSz < 17 + commandSz) + return 0; + + PutU32BE(out + idx, 0); idx += UINT32_SZ; + PutU32BE(out + idx, 4); idx += UINT32_SZ; + WMEMCPY(out + idx, "exec", 4); idx += 4; + out[idx++] = 1; + PutU32BE(out + idx, commandSz); idx += UINT32_SZ; + WMEMCPY(out + idx, command, commandSz); idx += commandSz; + + return idx; +} + + +/* Verify a repeat exec request wipes the command line it replaces. + * GetStringAlloc() frees the old buffer to take the new one, so without + * the scrub the earlier command, credentials and all, stays readable in + * the freed block. Only the last one ever reaches ChannelDelete(). The + * retain-on-free allocator is installed just around the second request + * so the freed bytes can be read back. */ +static int test_DoChannelRequest_zeroesReplacedCommand(void) +{ + static const char first[] = "sh -c 'login --password hunter2'"; + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch = NULL; + const byte* commandBytes; + byte payload[128]; + word32 payloadSz; + word32 commandSz; + word32 idx; + word32 i; + int result = 0; + wolfSSL_Malloc_cb prevMf = NULL; + wolfSSL_Free_cb prevFf = NULL; + wolfSSL_Realloc_cb prevRf = NULL; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -720; + wolfSSH_SetIOSend(ctx, CaptureIoSendChanReq); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { + result = -721; + goto out; + } + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, + DEFAULT_WINDOW_SZ, DEFAULT_MAX_PACKET_SZ); + if (ch == NULL) { + result = -722; + goto out; + } + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + result = -723; + goto out; + } + + idx = 0; + payloadSz = BuildExecRequestPayload(payload, sizeof(payload), first); + if (payloadSz == 0) { + result = -724; + goto out; + } + if (wolfSSH_TestDoChannelRequest(ssh, payload, payloadSz, &idx) + != WS_SUCCESS) { + result = -725; + goto out; + } + + commandSz = ch->commandSz; + commandBytes = (const byte*)ch->command; + if (commandBytes == NULL || commandSz != (word32)WSTRLEN(first)) { + result = -726; + goto out; + } + + idx = 0; + payloadSz = BuildExecRequestPayload(payload, sizeof(payload), "ls"); + if (payloadSz == 0) { + result = -727; + goto out; + } + + wolfSSL_GetAllocators(&prevMf, &prevFf, &prevRf); + /* Allocators unchanged on failure; nothing to restore. */ + if (wolfSSL_SetAllocators(RetainMalloc, RetainFree, + RetainRealloc) != 0) { + result = -728; + goto out; + } + result = wolfSSH_TestDoChannelRequest(ssh, payload, payloadSz, &idx); + wolfSSL_SetAllocators(prevMf, prevFf, prevRf); + if (result != WS_SUCCESS) { + result = -729; + goto out; + } + result = 0; + + if (!IsRetained((void*)commandBytes)) { + result = -730; + goto out; + } + + for (i = 0; i < commandSz; i++) { + if (commandBytes[i] != 0) { + result = -731; + goto out; + } + } + + /* The replacement arrived whole, so the scrub hit the old buffer + * rather than the one in use. */ + if (ch->command == NULL || WSTRCMP(ch->command, "ls") != 0 + || ch->commandSz != 2) { + result = -732; + goto out; + } + +out: + DrainRetained(); + if (ssh != NULL) + wolfSSH_free(ssh); + if (ctx != NULL) + wolfSSH_CTX_free(ctx); + return result; +} + +#endif /* WOLFSSH_TEST_INTERNAL */ + #endif /* WOLFSSH_TEST_CAPTURING_ALLOCATOR */ #ifndef WOLFSSH_NO_DH @@ -21464,6 +21688,18 @@ int wolfSSH_UnitTest(int argc, char** argv) printf("SshResourceFree_zeroesSecrets: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED")); testResult = testResult || unitResult; + + unitResult = test_ChannelDelete_zeroesCommand(); + printf("ChannelDelete_zeroesCommand: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + +#ifdef WOLFSSH_TEST_INTERNAL + unitResult = test_DoChannelRequest_zeroesReplacedCommand(); + printf("DoChannelRequest_zeroesReplacedCommand: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; +#endif #endif #ifndef WOLFSSH_NO_DH