diff --git a/src/agent.c b/src/agent.c index 33c0eb936..7f8d5fe7d 100644 --- a/src/agent.c +++ b/src/agent.c @@ -1731,6 +1731,95 @@ 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 (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; + } + 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) { + word32 flushes = ssh->txFlushCount; + + recordError = 1; + ret = SendChannelOpenSession(ssh, newChannel); + + /* 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 { + /* 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/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/src/ssh.c b/src/ssh.c index c02609769..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,52 +760,17 @@ 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; - } + int agentRet = wolfSSH_AGENT_ChannelOpen(ssh); - 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; + 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; } - - ssh->error = SendChannelOpenSession(ssh, newChannel); - 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; @@ -1134,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 c87c888d7..c8c3016de 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -4437,6 +4437,194 @@ 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 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. */ +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); +} + +/* 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. */ +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 +13615,15 @@ int main(int argc, char** argv) #endif #ifdef WOLFSSH_AGENT TestAgentChannelNullAgentSendsOpenFail(); + TestAgentChannelOpenWithoutRequest(); + TestAgentChannelOpenFlushesQueuedOpen(); + TestAgentChannelOpenAfterDisconnect(); + TestAgentChannelOpenQueuedThenDisconnect(); + TestAgentChannelOpenSendFailureCleansUp(); + TestAgentChannelOpenHighwaterErrorKeepsChannel(); +#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..0139cacd3 100644 --- a/wolfssh/agent.h +++ b/wolfssh/agent.h @@ -181,6 +181,21 @@ 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_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. 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); 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); diff --git a/wolfssh/internal.h b/wolfssh/internal.h index a8001c5a6..5b50a019a 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1643,10 +1643,28 @@ 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); 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);