From 96aa3032ed1b1a8e4ae45d1de92fd5ed24ddb8fd Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 2 Sep 2026 09:40:30 -0700 Subject: [PATCH 1/3] agent: let the application open the agent channel The one server-side site that opens auth-agent@openssh.com sits inside wolfSSH_accept(), so an application driving its own channels cannot reach it: the session records the request and no channel follows. - add wolfSSH_AGENT_ChannelOpen(), the same open lifted out of accept(), which still calls it - it reports WS_BAD_ARGUMENT until the peer asks and on a client session, and is idempotent after, so an application can poll it - publish the agent on a queued open too, so a retry after WS_WANT_WRITE finds it rather than opening a second channel and leaking the first - flush what is left of a queued open on the next call, rather than reporting a success the peer never saw - record ssh->error from the send alone, so neither a poll ahead of the request nor a failed allocation stops accept() continuing --- src/agent.c | 78 ++++++++++++++++++++++++++++++++++++ src/ssh.c | 42 +------------------- tests/regress.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++ wolfssh/agent.h | 12 ++++++ 4 files changed, 193 insertions(+), 41 deletions(-) diff --git a/src/agent.c b/src/agent.c index 33c0eb936..f46d72966 100644 --- a/src/agent.c +++ b/src/agent.c @@ -1731,6 +1731,84 @@ int wolfSSH_AGENT_enable(WOLFSSH* ssh, byte isEnabled) } +int wolfSSH_AGENT_ChannelOpen(WOLFSSH* ssh) +{ + WOLFSSH_AGENT_CTX* newAgent = NULL; + WOLFSSH_CHANNEL* newChannel = NULL; + int ret = WS_SUCCESS; + /* wolfSSH_accept() clears only want-read/want-write/auth-pending, so a + * WS_BAD_ARGUMENT latched by a poll kills the handshake. */ + int recordError = 0; + + WLOG_ENTER(); + + if (ssh == NULL) + ret = WS_SSH_NULL_E; + else if (ssh->ctx->side != WOLFSSH_ENDPOINT_SERVER) { + /* Server side only. wolfSSH_connect() sets ssh->agent too, so the + * checks below would report a channel a client never opened. */ + ret = WS_BAD_ARGUMENT; + } + else if (!ssh->useAgent) { + /* Nothing asked for agent forwarding on this session. */ + ret = WS_BAD_ARGUMENT; + } + else if (ssh->agent == NULL) { + /* Nothing else sets ssh->agent, so a NULL one means "not opened + * yet". Idempotent, so a poll cannot open a second channel. */ + WLOG(WS_LOG_AGENT, "Starting agent channel"); + + newAgent = wolfSSH_AGENT_new(ssh->ctx->heap); + if (newAgent == NULL) + ret = WS_MEMORY_E; + + if (ret == WS_SUCCESS) { + newChannel = ChannelNew(ssh, ID_CHANTYPE_AUTH_AGENT, + ssh->ctx->windowSz, ssh->ctx->maxPacketSz); + if (newChannel == NULL) + ret = WS_MEMORY_E; + } + + if (ret == WS_SUCCESS) { + recordError = 1; + ret = SendChannelOpenSession(ssh, newChannel); + + if (ret < WS_SUCCESS + && ret != WS_WANT_WRITE && ret != WS_WANT_READ) { + ChannelDelete(newChannel, ssh->ctx->heap); + } + else { + /* Publish on a queued open too, so a retry takes the + * already-open path rather than opening a second. */ + ChannelAppend(ssh, newChannel); + newAgent->channel = newChannel->channel; + ssh->agent = newAgent; + newAgent = NULL; + if (ssh->ctx->agentCb) { + ssh->ctx->agentCb(WOLFSSH_AGENT_LOCAL_SETUP, + ssh->agentCbCtx); + } + } + } + + if (newAgent != NULL) + wolfSSH_AGENT_free(newAgent); + } + else if (wolfSSH_OutputPending(ssh)) { + /* Any queued output, not just this open. Flush it rather than + * report a success the peer hasn't seen. */ + recordError = 1; + ret = wolfSSH_SendPacket(ssh); + } + + if (recordError) + ssh->error = ret; + + WLOG_LEAVE(ret); + return ret; +} + + int wolfSSH_AGENT_worker(WOLFSSH* ssh) { int ret = WS_SUCCESS; diff --git a/src/ssh.c b/src/ssh.c index c02609769..cf1ac32f6 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -764,52 +764,12 @@ int wolfSSH_accept(WOLFSSH* ssh) #endif /* WOLFSSH_SFTP and !NO_WOLFSSH_SERVER */ #ifdef WOLFSSH_AGENT if (ssh->useAgent) { - WOLFSSH_AGENT_CTX* newAgent; - WOLFSSH_CHANNEL* newChannel; - - WLOG(WS_LOG_AGENT, "Starting agent channel"); - - newAgent = wolfSSH_AGENT_new(ssh->ctx->heap); - if (newAgent == NULL) { - ssh->error = WS_MEMORY_E; - WLOG(WS_LOG_DEBUG, acceptError, - "SERVER_USERAUTH_ACCEPT_DONE", ssh->error); - return WS_ERROR; - } - - newChannel = ChannelNew(ssh, ID_CHANTYPE_AUTH_AGENT, - ssh->ctx->windowSz, ssh->ctx->maxPacketSz); - if (newChannel == NULL) { - wolfSSH_AGENT_free(newAgent); - ssh->error = WS_MEMORY_E; - WLOG(WS_LOG_DEBUG, acceptError, - "SERVER_USERAUTH_ACCEPT_DONE", ssh->error); - return WS_FATAL_ERROR; - } - - ssh->error = SendChannelOpenSession(ssh, newChannel); + ssh->error = wolfSSH_AGENT_ChannelOpen(ssh); if (ssh->error < WS_SUCCESS) { - if (ssh->error == WS_WANT_WRITE || - ssh->error == WS_WANT_READ) { - ChannelAppend(ssh, newChannel); - } - else { - ChannelDelete(newChannel, ssh->ctx->heap); - wolfSSH_AGENT_free(newAgent); - } WLOG(WS_LOG_DEBUG, acceptError, "SERVER_USERAUTH_ACCEPT_DONE", ssh->error); return WS_FATAL_ERROR; } - ChannelAppend(ssh, newChannel); - newAgent->channel = newChannel->channel; - if (ssh->ctx->agentCb) { - ssh->ctx->agentCb(WOLFSSH_AGENT_LOCAL_SETUP, - ssh->agentCbCtx); - } - if (ssh->agent != NULL) - wolfSSH_AGENT_free(ssh->agent); - ssh->agent = newAgent; } #endif /* WOLFSSH_AGENT */ ssh->acceptState = ACCEPT_CLIENT_SESSION_ESTABLISHED; diff --git a/tests/regress.c b/tests/regress.c index c87c888d7..3c248d991 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -4437,6 +4437,102 @@ static void TestAgentChannelNullAgentSendsOpenFail(void) FreeChannelOpenHarness(&harness); } + +/* Nothing asked for forwarding, so the open is refused rather than started. + * The refusal is the documented answer to a poll, so it must not land in + * ssh->error: wolfSSH_accept() would then abort with WS_INVALID_STATE_E. */ +static void TestAgentChannelOpenWithoutRequest(void) +{ + ChannelOpenHarness harness; + + InitChannelOpenHarness(&harness, NULL, 0); + + AssertIntEQ(wolfSSH_AGENT_ChannelOpen(harness.ssh), WS_BAD_ARGUMENT); + AssertNull(harness.ssh->agent); + AssertIntEQ(harness.io.outSz, 0); + AssertIntEQ(harness.ssh->error, WS_SUCCESS); + + /* The handshake survives the poll: no input, so accept only wants read. */ + AssertIntEQ(wolfSSH_accept(harness.ssh), WS_FATAL_ERROR); + AssertIntEQ(harness.ssh->error, WS_WANT_READ); + + FreeChannelOpenHarness(&harness); +} + +/* A queued open publishes the agent, so the caller's next poll must finish + * the send rather than report a success the peer never saw, and must not + * open a second channel. */ +static void TestAgentChannelOpenFlushesQueuedOpen(void) +{ + ChannelOpenHarness harness; + word32 outSz; + + InitChannelOpenHarness(&harness, NULL, 0); + harness.ssh->useAgent = 1; + harness.io.blockNext = 1; + + AssertIntEQ(wolfSSH_AGENT_ChannelOpen(harness.ssh), WS_WANT_WRITE); + AssertNotNull(harness.ssh->agent); + AssertIntEQ(harness.ssh->channelListSz, 1); + AssertIntEQ(harness.io.outSz, 0); + + AssertIntEQ(wolfSSH_AGENT_ChannelOpen(harness.ssh), WS_SUCCESS); + AssertIntEQ(harness.ssh->channelListSz, 1); + AssertTrue(harness.io.outSz > 0); + AssertIntEQ(ParseMsgId(harness.io.out, harness.io.outSz), + MSGID_CHANNEL_OPEN); + + /* The flushed open is the answer wolfSSH_accept() retries on: success, + * no second channel, no new packet, ssh->error untouched. */ + outSz = harness.io.outSz; + harness.ssh->error = WS_SUCCESS; + + AssertIntEQ(wolfSSH_AGENT_ChannelOpen(harness.ssh), WS_SUCCESS); + AssertIntEQ(harness.ssh->channelListSz, 1); + AssertIntEQ(harness.io.outSz, outSz); + AssertIntEQ(harness.ssh->error, WS_SUCCESS); + + FreeChannelOpenHarness(&harness); +} + +/* A send that fails outright, rather than blocking, leaves nothing behind, + * so a later poll starts the open over. */ +static void TestAgentChannelOpenSendFailureCleansUp(void) +{ + ChannelOpenHarness harness; + + InitChannelOpenHarness(&harness, NULL, 0); + harness.ssh->useAgent = 1; + /* No room, so MemSend reports a general error. */ + harness.io.outCap = 0; + + AssertIntEQ(wolfSSH_AGENT_ChannelOpen(harness.ssh), WS_SOCKET_ERROR_E); + AssertNull(harness.ssh->agent); + AssertIntEQ(harness.ssh->channelListSz, 0); + AssertIntEQ(harness.io.outSz, 0); + AssertIntEQ(harness.ssh->error, WS_SOCKET_ERROR_E); + + FreeChannelOpenHarness(&harness); +} + +#ifndef NO_WOLFSSH_CLIENT +/* Server-side call. A client has an ssh->agent of its own, so answering the + * poll from it would report a channel that was never opened. */ +static void TestAgentChannelOpenOnClientRefused(void) +{ + ChannelOpenHarness harness; + + InitChannelOpenHarnessClient(&harness, NULL, 0); + harness.ssh->useAgent = 1; + + AssertIntEQ(wolfSSH_AGENT_ChannelOpen(harness.ssh), WS_BAD_ARGUMENT); + AssertIntEQ(harness.ssh->channelListSz, 0); + AssertIntEQ(harness.io.outSz, 0); + AssertIntEQ(harness.ssh->error, WS_SUCCESS); + + FreeChannelOpenHarness(&harness); +} +#endif /* !NO_WOLFSSH_CLIENT */ #endif @@ -13427,6 +13523,12 @@ int main(int argc, char** argv) #endif #ifdef WOLFSSH_AGENT TestAgentChannelNullAgentSendsOpenFail(); + TestAgentChannelOpenWithoutRequest(); + TestAgentChannelOpenFlushesQueuedOpen(); + TestAgentChannelOpenSendFailureCleansUp(); +#ifndef NO_WOLFSSH_CLIENT + TestAgentChannelOpenOnClientRefused(); +#endif #endif #endif /* NO_WOLFSSH_SERVER */ #if defined(WOLFSSH_AGENT) && !defined(WOLFSSH_NO_ED25519) \ diff --git a/wolfssh/agent.h b/wolfssh/agent.h index 581e3eba9..8f57bc5df 100644 --- a/wolfssh/agent.h +++ b/wolfssh/agent.h @@ -181,6 +181,18 @@ WOLFSSH_API int wolfSSH_CTX_set_agent_cb(WOLFSSH_CTX* ctx, WOLFSSH_API int wolfSSH_set_agent_cb_ctx(WOLFSSH* ssh, void* ctx); WOLFSSH_API int wolfSSH_CTX_AGENT_enable(WOLFSSH_CTX* ctx, byte isEnabled); WOLFSSH_API int wolfSSH_AGENT_enable(WOLFSSH* ssh, byte isEnabled); +/* Server side. Opens the auth-agent@openssh.com channel to the client once + * the peer's auth-agent-req@openssh.com asks for forwarding. wolfSSH_accept() + * does it on the default path; an application driving its own channels polls + * this instead. Opens one channel, then flushes what of the open is queued. + * Returns WS_SUCCESS, WS_BAD_ARGUMENT before the peer asks or on a client + * session, WS_WANT_READ or WS_WANT_WRITE while output is still queued, + * WS_SSH_NULL_E, WS_MEMORY_E, or whatever the send reports. WS_SUCCESS says + * the open went out, not that the peer took it; a refusal reaches the + * channel-open-fail callback. + * Only the send records in ssh->error, so a poll ahead of the peer's request + * leaves the session fit for wolfSSH_accept(). */ +WOLFSSH_API int wolfSSH_AGENT_ChannelOpen(WOLFSSH* ssh); WOLFSSH_LOCAL int wolfSSH_AGENT_worker(WOLFSSH* ssh); WOLFSSH_API int wolfSSH_AGENT_Relay(WOLFSSH* ssh, const byte* msg, word32* msgSz, byte* rsp, word32* rspSz); From d166aa895c219362e6096de6e041c92f5c037606 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Fri, 4 Sep 2026 13:41:16 -0700 Subject: [PATCH 2/3] agent: refuse a channel open after a disconnect wolfSSH_AGENT_ChannelOpen() answers a poll on a session that is over with WS_FATAL_ERROR and WS_DISCONNECT in ssh->error, the shape every other public sender uses: no channel opened, nothing on the wire, RFC 4253 section 11.1. wolfSSH_accept() gates the open it drives, so the new public entry point is the only way in. - promote SendAfterDisconnect() to WOLFSSH_LOCAL so agent.c uses the same helper as every other public sender - leave an open queued before the disconnect unflushed, the rule wolfSSH_shutdown() applies to all but its own disconnect - keep WS_DISCONNECT in ssh->error at the accept() call site, which used to overwrite it with the status the open returns --- src/agent.c | 7 +++++++ src/ssh.c | 20 +++++++++----------- tests/regress.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ wolfssh/agent.h | 5 +++-- wolfssh/internal.h | 6 ++++++ 5 files changed, 69 insertions(+), 13 deletions(-) diff --git a/src/agent.c b/src/agent.c index f46d72966..ba8618112 100644 --- a/src/agent.c +++ b/src/agent.c @@ -1749,6 +1749,13 @@ int wolfSSH_AGENT_ChannelOpen(WOLFSSH* ssh) * checks below would report a channel a client never opened. */ ret = WS_BAD_ARGUMENT; } + else if (SendAfterDisconnect(ssh)) { + /* The session is over, so neither a new open nor the flush of one + * queued before the disconnect may go out. RFC 4253 section 11.1. + * WS_DISCONNECT is in ssh->error, where the rest of the API puts + * it. */ + ret = WS_FATAL_ERROR; + } else if (!ssh->useAgent) { /* Nothing asked for agent forwarding on this session. */ ret = WS_BAD_ARGUMENT; diff --git a/src/ssh.c b/src/ssh.c index cf1ac32f6..36646514c 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -567,10 +567,6 @@ static int DoReceiveHandshake(WOLFSSH* ssh) #endif /* !NO_WOLFSSH_SERVER || !NO_WOLFSSH_CLIENT */ -/* Defined below, ahead of both drivers; either can be the only one built. */ -static int SendAfterDisconnect(WOLFSSH* ssh); - - #ifndef NO_WOLFSSH_SERVER const char acceptError[] = "accept error: %s, %d"; @@ -764,8 +760,13 @@ int wolfSSH_accept(WOLFSSH* ssh) #endif /* WOLFSSH_SFTP and !NO_WOLFSSH_SERVER */ #ifdef WOLFSSH_AGENT if (ssh->useAgent) { - ssh->error = wolfSSH_AGENT_ChannelOpen(ssh); - if (ssh->error < WS_SUCCESS) { + int agentRet = wolfSSH_AGENT_ChannelOpen(ssh); + + if (agentRet < WS_SUCCESS) { + /* WS_FATAL_ERROR is the disconnect, which already + * recorded WS_DISCONNECT; keep that. */ + if (agentRet != WS_FATAL_ERROR) + ssh->error = agentRet; WLOG(WS_LOG_DEBUG, acceptError, "SERVER_USERAUTH_ACCEPT_DONE", ssh->error); return WS_FATAL_ERROR; @@ -1094,11 +1095,8 @@ int wolfSSH_connect(WOLFSSH* ssh) #endif /* NO_WOLFSSH_CLIENT */ -/* A disconnect, sent or received, ends the session, so nothing further may - * go out. RFC 4253 section 11.1. Reads are deliberately not gated on this: - * channel data that arrived before the disconnect is still the caller's. - * Call only after ssh has been checked for NULL. */ -static int SendAfterDisconnect(WOLFSSH* ssh) +/* See wolfssh/internal.h for the contract. */ +int SendAfterDisconnect(WOLFSSH* ssh) { if (ssh->disconnected) { WLOG(WS_LOG_DEBUG, "Send attempted after a disconnect"); diff --git a/tests/regress.c b/tests/regress.c index 3c248d991..233d3b2bb 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -4459,6 +4459,48 @@ static void TestAgentChannelOpenWithoutRequest(void) FreeChannelOpenHarness(&harness); } +/* A poll after the peer disconnects must not open a channel or put anything + * on the wire. RFC 4253 section 11.1: the session is over. */ +static void TestAgentChannelOpenAfterDisconnect(void) +{ + ChannelOpenHarness harness; + + InitChannelOpenHarness(&harness, NULL, 0); + harness.ssh->useAgent = 1; + harness.ssh->disconnected = 1; + + AssertIntEQ(wolfSSH_AGENT_ChannelOpen(harness.ssh), WS_FATAL_ERROR); + AssertNull(harness.ssh->agent); + AssertIntEQ(harness.ssh->channelListSz, 0); + AssertIntEQ(harness.io.outSz, 0); + AssertIntEQ(harness.ssh->error, WS_DISCONNECT); + + FreeChannelOpenHarness(&harness); +} + +/* An open queued before the disconnect is not flushed either: those bytes + * belong to a session that is over, the same rule wolfSSH_shutdown() applies + * to everything but its own queued disconnect. */ +static void TestAgentChannelOpenQueuedThenDisconnect(void) +{ + ChannelOpenHarness harness; + + InitChannelOpenHarness(&harness, NULL, 0); + harness.ssh->useAgent = 1; + harness.io.blockNext = 1; + + AssertIntEQ(wolfSSH_AGENT_ChannelOpen(harness.ssh), WS_WANT_WRITE); + AssertIntEQ(harness.io.outSz, 0); + + harness.ssh->disconnected = 1; + + AssertIntEQ(wolfSSH_AGENT_ChannelOpen(harness.ssh), WS_FATAL_ERROR); + AssertIntEQ(harness.io.outSz, 0); + AssertIntEQ(harness.ssh->error, WS_DISCONNECT); + + FreeChannelOpenHarness(&harness); +} + /* A queued open publishes the agent, so the caller's next poll must finish * the send rather than report a success the peer never saw, and must not * open a second channel. */ @@ -13525,6 +13567,8 @@ int main(int argc, char** argv) TestAgentChannelNullAgentSendsOpenFail(); TestAgentChannelOpenWithoutRequest(); TestAgentChannelOpenFlushesQueuedOpen(); + TestAgentChannelOpenAfterDisconnect(); + TestAgentChannelOpenQueuedThenDisconnect(); TestAgentChannelOpenSendFailureCleansUp(); #ifndef NO_WOLFSSH_CLIENT TestAgentChannelOpenOnClientRefused(); diff --git a/wolfssh/agent.h b/wolfssh/agent.h index 8f57bc5df..f2bad7fb2 100644 --- a/wolfssh/agent.h +++ b/wolfssh/agent.h @@ -187,11 +187,12 @@ WOLFSSH_API int wolfSSH_AGENT_enable(WOLFSSH* ssh, byte isEnabled); * this instead. Opens one channel, then flushes what of the open is queued. * Returns WS_SUCCESS, WS_BAD_ARGUMENT before the peer asks or on a client * session, WS_WANT_READ or WS_WANT_WRITE while output is still queued, + * WS_FATAL_ERROR with WS_DISCONNECT in ssh->error once the session is over, * WS_SSH_NULL_E, WS_MEMORY_E, or whatever the send reports. WS_SUCCESS says * the open went out, not that the peer took it; a refusal reaches the * channel-open-fail callback. - * Only the send records in ssh->error, so a poll ahead of the peer's request - * leaves the session fit for wolfSSH_accept(). */ + * Only that and the send record in ssh->error, so a poll ahead of the peer's + * request leaves the session fit for wolfSSH_accept(). */ WOLFSSH_API int wolfSSH_AGENT_ChannelOpen(WOLFSSH* ssh); WOLFSSH_LOCAL int wolfSSH_AGENT_worker(WOLFSSH* ssh); WOLFSSH_API int wolfSSH_AGENT_Relay(WOLFSSH* ssh, diff --git a/wolfssh/internal.h b/wolfssh/internal.h index a8001c5a6..dabc967b9 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1643,6 +1643,12 @@ enum ChannelOpenFailReasons { OPEN_RESOURCE_SHORTAGE }; +/* A disconnect, sent or received, ends the session, so nothing further may + * go out. RFC 4253 section 11.1. Returns 1 and records WS_DISCONNECT in + * ssh->error when the session is over, 0 otherwise. Reads are deliberately + * not gated on this: channel data that arrived before the disconnect is + * still the caller's. Call only after ssh has been checked for NULL. */ +WOLFSSH_LOCAL int SendAfterDisconnect(WOLFSSH* ssh); WOLFSSH_LOCAL int DoReceive(WOLFSSH* ssh); WOLFSSH_LOCAL int DoProtoId(WOLFSSH* ssh); WOLFSSH_LOCAL int wolfSSH_SendPacket(WOLFSSH* ssh); From f3d7e6ef5beecc9b859f6c06e1d3322734613478 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Tue, 8 Sep 2026 08:55:50 -0700 Subject: [PATCH 3/3] agent: commit the open on a completed flush wolfSSH_AGENT_ChannelOpen() settles its channel and agent on whether the open reached the peer, which SendPacketDelivered() answers from the flush count. The highwater callback runs after the last byte is out, so its failure arrives as this send's return, and the rollback read that as a send that never left. - give SendPacketDelivered() external linkage, contract in internal.h - note in agent.h that an error raised once the open is on the wire leaves the channel open - test a failing highwater callback: the channel and the agent stand, and the poll after it is the idempotent one --- src/agent.c | 8 ++++++-- src/internal.c | 15 ++------------ tests/regress.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++ wolfssh/agent.h | 4 +++- wolfssh/internal.h | 12 +++++++++++ 5 files changed, 74 insertions(+), 16 deletions(-) diff --git a/src/agent.c b/src/agent.c index ba8618112..7f8d5fe7d 100644 --- a/src/agent.c +++ b/src/agent.c @@ -1777,11 +1777,15 @@ int wolfSSH_AGENT_ChannelOpen(WOLFSSH* ssh) } if (ret == WS_SUCCESS) { + word32 flushes = ssh->txFlushCount; + recordError = 1; ret = SendChannelOpenSession(ssh, newChannel); - if (ret < WS_SUCCESS - && ret != WS_WANT_WRITE && ret != WS_WANT_READ) { + /* What commits is the open reaching the peer, not the return: + * a highwater callback failing after the flush is not a send + * that never left. */ + if (!SendPacketDelivered(ssh, flushes, ret)) { ChannelDelete(newChannel, ssh->ctx->heap); } else { diff --git a/src/internal.c b/src/internal.c index 182c43df4..b3728779a 100644 --- a/src/internal.c +++ b/src/internal.c @@ -18059,19 +18059,8 @@ int SendIgnore(WOLFSSH* ssh, const unsigned char* data, word32 dataSz) return ret; } -/* Will the packet just framed reach the peer? A completed flush says so; the - * return does not, since the highwater callback runs after the last byte goes - * out and the rekey it starts fails with the same codes a lost send does. - * Comparing the flush count across the send tells those apart. - * - * Short of a flush, WS_WANT_WRITE is the one outcome that keeps the packet - * framed for the next one; an interrupted send is retried inside - * wolfSSH_SendPacket() rather than reported. Anything else counts as not sent, - * which at worst leaves the peer holding a request this side did not register; - * guessing the other way would desync the reply queue for the life of the - * session. Call before anything else runs, since a later send flushes this - * packet and would read as this one's. */ -static INLINE int SendPacketDelivered(WOLFSSH* ssh, word32 flushes, int ret) +/* Contract in internal.h. */ +int SendPacketDelivered(WOLFSSH* ssh, word32 flushes, int ret) { return ssh->txFlushCount != flushes || ret == WS_WANT_WRITE; } diff --git a/tests/regress.c b/tests/regress.c index 233d3b2bb..c8c3016de 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -4557,6 +4557,56 @@ static void TestAgentChannelOpenSendFailureCleansUp(void) FreeChannelOpenHarness(&harness); } +/* Fails the session's first highwater check, and counts its calls. */ +static int AgentOpenHighwaterCb(byte side, void* ctx) +{ + int* calls = (int*)ctx; + + WOLFSSH_UNUSED(side); + + (*calls)++; + return WS_FATAL_ERROR; +} + +/* The highwater callback fails after the open is out, so its error arrives as + * the send's return. Rolling back on that deletes a channel the peer has, and + * its confirmation then names nothing. */ +static void TestAgentChannelOpenHighwaterErrorKeepsChannel(void) +{ + ChannelOpenHarness harness; + int calls = 0; + word32 outSz; + + InitChannelOpenHarness(&harness, NULL, 0); + harness.ssh->useAgent = 1; + + wolfSSH_SetHighwaterCb(harness.ctx, 1, AgentOpenHighwaterCb); + wolfSSH_SetHighwaterCtx(harness.ssh, &calls); + harness.ssh->highwaterMark = 1; + harness.ssh->txCount = 1; + + AssertIntEQ(wolfSSH_AGENT_ChannelOpen(harness.ssh), WS_FATAL_ERROR); + AssertIntEQ(calls, 1); + AssertIntEQ(harness.ssh->error, WS_FATAL_ERROR); + + /* The open went out, so the channel and the agent stand. */ + AssertNotNull(harness.ssh->agent); + AssertIntEQ(harness.ssh->channelListSz, 1); + AssertTrue(harness.io.outSz > 0); + AssertIntEQ(ParseMsgId(harness.io.out, harness.io.outSz), + MSGID_CHANNEL_OPEN); + + /* The mark has fired, so the next poll is the idempotent one. */ + outSz = harness.io.outSz; + + AssertIntEQ(wolfSSH_AGENT_ChannelOpen(harness.ssh), WS_SUCCESS); + AssertIntEQ(calls, 1); + AssertIntEQ(harness.ssh->channelListSz, 1); + AssertIntEQ(harness.io.outSz, outSz); + + FreeChannelOpenHarness(&harness); +} + #ifndef NO_WOLFSSH_CLIENT /* Server-side call. A client has an ssh->agent of its own, so answering the * poll from it would report a channel that was never opened. */ @@ -13570,6 +13620,7 @@ int main(int argc, char** argv) TestAgentChannelOpenAfterDisconnect(); TestAgentChannelOpenQueuedThenDisconnect(); TestAgentChannelOpenSendFailureCleansUp(); + TestAgentChannelOpenHighwaterErrorKeepsChannel(); #ifndef NO_WOLFSSH_CLIENT TestAgentChannelOpenOnClientRefused(); #endif diff --git a/wolfssh/agent.h b/wolfssh/agent.h index f2bad7fb2..0139cacd3 100644 --- a/wolfssh/agent.h +++ b/wolfssh/agent.h @@ -190,7 +190,9 @@ WOLFSSH_API int wolfSSH_AGENT_enable(WOLFSSH* ssh, byte isEnabled); * WS_FATAL_ERROR with WS_DISCONNECT in ssh->error once the session is over, * WS_SSH_NULL_E, WS_MEMORY_E, or whatever the send reports. WS_SUCCESS says * the open went out, not that the peer took it; a refusal reaches the - * channel-open-fail callback. + * channel-open-fail callback. An error raised after the open is on the wire, + * a failing highwater callback, leaves the channel open and the next poll + * answers WS_SUCCESS. * Only that and the send record in ssh->error, so a poll ahead of the peer's * request leaves the session fit for wolfSSH_accept(). */ WOLFSSH_API int wolfSSH_AGENT_ChannelOpen(WOLFSSH* ssh); diff --git a/wolfssh/internal.h b/wolfssh/internal.h index dabc967b9..5b50a019a 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1653,6 +1653,18 @@ WOLFSSH_LOCAL int DoReceive(WOLFSSH* ssh); WOLFSSH_LOCAL int DoProtoId(WOLFSSH* ssh); WOLFSSH_LOCAL int wolfSSH_SendPacket(WOLFSSH* ssh); WOLFSSH_LOCAL int wolfSSH_OutputPending(WOLFSSH* ssh); +/* Will the packet just framed reach the peer? A completed flush says so; the + * return does not, since the highwater callback runs after the last byte is + * out and fails with the same codes a lost send does. Take flushes from + * ssh->txFlushCount before the send, and call this before anything else runs: + * a later send flushes this packet and would read as this one's. + * + * Short of a flush, only WS_WANT_WRITE keeps the packet framed for the next + * one; an interrupt is retried inside wolfSSH_SendPacket(), not reported. + * Anything else counts as not sent, which at worst leaves the peer holding a + * request this side did not register; the other guess desyncs the reply queue + * for the life of the session. */ +WOLFSSH_LOCAL int SendPacketDelivered(WOLFSSH* ssh, word32 flushes, int ret); WOLFSSH_LOCAL int SendProtoId(WOLFSSH* ssh); WOLFSSH_LOCAL int ValidateProtoId(const char* protoIdStr, word32 len); WOLFSSH_LOCAL int SendKexInit(WOLFSSH* ssh);