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);