From 7bb39b63e797b17b6b1cbf652e88b3b038eaf30e Mon Sep 17 00:00:00 2001 From: John Safranek Date: Mon, 14 Sep 2026 08:37:01 -0700 Subject: [PATCH] internal: re-find the channel after the callback The generic channel request callback may free the channel it was handed, so DoChannelRequest() looks it up again before the type handling reads it. Gone, the request ends there, and a reply the peer wanted fails on the missing channel the way one after a typed session callback does. - ssh.h says the callback may free its channel and what the request does from there - regress.c frees the channel from the callback on each of the three answers, and on a pty-req, the type that wrote to the channel outside a session request --- src/internal.c | 12 +++++- tests/regress.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++ wolfssh/ssh.h | 14 ++++++- 3 files changed, 125 insertions(+), 2 deletions(-) diff --git a/src/internal.c b/src/internal.c index 6e3eb28d..ea5c4dc1 100644 --- a/src/internal.c +++ b/src/internal.c @@ -13321,6 +13321,16 @@ static int DoChannelRequest(WOLFSSH* ssh, else if (decision == WOLFSSH_REQ_ACCEPT) { granted = 1; } + + /* A callback may free its own channel, so look it up again + * before the handling below reads it. Gone, the request ends + * here, and a wanted reply fails on the missing channel the + * way one after a typed callback does. */ + channel = ChannelFind(ssh, channelId, WS_CHANNEL_ID_SELF); + if (channel == NULL) { + WLOG(WS_LOG_DEBUG, + " channel request callback freed the channel."); + } } } @@ -13342,7 +13352,7 @@ static int DoChannelRequest(WOLFSSH* ssh, } #endif - if (ret == WS_SUCCESS && !rej) { + if (ret == WS_SUCCESS && !rej && channel != NULL) { if (ChannelRequestIs(type, typeSz, "env")) { char name[WOLFSSH_MAX_NAMESZ]; word32 nameSz; diff --git a/tests/regress.c b/tests/regress.c index b4513a79..1592f2b8 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -4345,6 +4345,106 @@ static void TestChannelReqCallbackGrantOverridesAppChannels(void) FreeChannelOpenHarness(&harness); } +/* A generic channel request callback that frees the channel it was handed, + * the way a policy that tears the channel down in place does, and answers + * anyReqCbReturn. */ +static int FreeingChannelReqAnyCb(WOLFSSH_CHANNEL* channel, const byte* type, + word32 typeSz, const byte* data, word32 dataSz, int wantReply, + void* ctx) +{ + RecordAnyReq(type, typeSz, data, dataSz, ctx); + anyReqCbWantReply = wantReply; + AssertIntEQ(wolfSSH_ChannelFree(channel), WS_SUCCESS); + + return anyReqCbReturn; +} + +/* Runs one request through a fresh harness whose generic callback frees the + * channel and answers anyReturn, and returns what DoReceive() made of it. + * The channel id is handed back, since the channel itself is gone. */ +static int RunRequestThroughFreeingCb(ChannelOpenHarness* harness, + word32* channelId, const char* type, byte wantReply, + const byte* tail, word32 tailSz, int anyReturn) +{ + WOLFSSH_CHANNEL* channel; + byte in[192]; + word32 inSz; + + ResetAnyReqCb(anyReturn); + typedReqCbCalls = 0; + InitChannelOpenHarness(harness, NULL, 0); + AssertIntEQ(wolfSSH_CTX_SetChannelReqAnyCb(harness->ctx, + FreeingChannelReqAnyCb), WS_SUCCESS); + AssertIntEQ(wolfSSH_CTX_SetChannelReqExecCb(harness->ctx, + CountingSessionReqCb), WS_SUCCESS); + channel = SeedConfirmedSessionChannel(harness); + *channelId = channel->channel; + + inSz = BuildChannelRequestPacket(*channelId, type, wantReply, + tail, tailSz, in, sizeof(in)); + RepointHarnessInput(harness, in, inSz); + + return DoReceive(harness->ssh); +} + +/* The generic callback may free the channel it was called on. Nothing below + * it reads the channel again after that, and the request ends there: the + * type is not handled, no typed callback runs, and a wanted reply fails on + * the channel that is gone. */ +static void TestChannelReqCallbackMayFreeChannel(void) +{ + ChannelOpenHarness harness; + word32 channelId; + byte tail[32]; + word32 tailSz; + + /* Left unhandled, the exec handling would read the channel next. */ + tailSz = AppendString(tail, sizeof(tail), 0, "ls"); + AssertIntEQ(RunRequestThroughFreeingCb(&harness, &channelId, "exec", 1, + tail, tailSz, WOLFSSH_REQ_UNHANDLED), WS_FATAL_ERROR); + AssertIntEQ(harness.ssh->error, WS_INVALID_CHANID); + AssertIntEQ(anyReqCbCalls, 1); + AssertIntEQ(typedReqCbCalls, 0); + AssertIntEQ(harness.io.outSz, 0); + AssertNull(ChannelFind(harness.ssh, channelId, WS_CHANNEL_ID_SELF)); + FreeChannelOpenHarness(&harness); + + /* Granted, the same request would have committed the session on it. */ + AssertIntEQ(RunRequestThroughFreeingCb(&harness, &channelId, "exec", 1, + tail, tailSz, WOLFSSH_REQ_ACCEPT), WS_FATAL_ERROR); + AssertIntEQ(harness.ssh->error, WS_INVALID_CHANID); + AssertIntEQ(typedReqCbCalls, 0); + AssertTrue(harness.ssh->clientState < CLIENT_DONE); + AssertNull(ChannelFind(harness.ssh, channelId, WS_CHANNEL_ID_SELF)); + FreeChannelOpenHarness(&harness); + + /* Refused, the handling was skipped whatever the channel did. */ + AssertIntEQ(RunRequestThroughFreeingCb(&harness, &channelId, "exec", 1, + tail, tailSz, WOLFSSH_REQ_REJECT), WS_FATAL_ERROR); + AssertIntEQ(harness.ssh->error, WS_INVALID_CHANID); + AssertIntEQ(typedReqCbCalls, 0); + FreeChannelOpenHarness(&harness); + +#ifdef WOLFSSH_TERM + /* With no reply wanted there is nothing left to fail, so the packet is + * taken and the session carries on. A pty-req is the case that wrote to + * the channel outside a session request. */ + tailSz = AppendString(tail, sizeof(tail), 0, "vt100"); + tailSz = AppendUint32(tail, sizeof(tail), tailSz, 80); + tailSz = AppendUint32(tail, sizeof(tail), tailSz, 24); + tailSz = AppendUint32(tail, sizeof(tail), tailSz, 0); + tailSz = AppendUint32(tail, sizeof(tail), tailSz, 0); + tailSz = AppendString(tail, sizeof(tail), tailSz, ""); + AssertIntEQ(RunRequestThroughFreeingCb(&harness, &channelId, "pty-req", 0, + tail, tailSz, WOLFSSH_REQ_UNHANDLED), WS_SUCCESS); + AssertIntEQ(harness.ssh->error, WS_SUCCESS); + AssertIntEQ(anyReqCbCalls, 1); + AssertIntEQ(harness.io.outSz, 0); + AssertNull(ChannelFind(harness.ssh, channelId, WS_CHANNEL_ID_SELF)); + FreeChannelOpenHarness(&harness); +#endif /* WOLFSSH_TERM */ +} + /* Both setters answer a NULL context, the only error either has. */ static void TestReqAnyCallbackSettersRejectNullCtx(void) { @@ -15809,6 +15909,7 @@ int main(int argc, char** argv) TestChannelReqCallbackSeesWholeType(); TestChannelReqCallbackSettlesSessionRequest(); TestChannelReqCallbackGrantOverridesAppChannels(); + TestChannelReqCallbackMayFreeChannel(); TestReqAnyCallbackSettersRejectNullCtx(); #ifdef WOLFSSH_TERM TestChannelReqCallbackKeepsTypeChecks(); diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index b034ce30..4a5405b6 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -507,7 +507,19 @@ typedef enum WS_ReqCbResult { * kept; a request that does not fit its type is refused whatever the * callback said. A type the library does not know is answered * CHANNEL_SUCCESS on ACCEPT, where it is otherwise refused. Shares the - * channel request context. */ + * channel request context. + * + * The callback may free the channel it was handed, with + * wolfSSH_ChannelFree(). The request ends there whatever the answer: the + * type is not handled, and a request wanting a reply has nothing left to + * answer on, so it fails with WS_INVALID_CHANID. + * + * type and data point into the session's input buffer and are good only + * for the length of the call, so a callback keeping either copies it. + * The packet is still being parsed, so the callback must not re-enter + * the receive side of the library on this session -- wolfSSH_worker(), + * wolfSSH_stream_read(), wolfSSH_accept(), the SFTP calls -- which may + * grow or compact that buffer and leave both pointers behind. */ typedef int (*WS_CallbackChannelReqAny)(WOLFSSH_CHANNEL* channel, const byte* type, word32 typeSz, const byte* data, word32 dataSz, int wantReply, void* ctx);