Skip to content

Commit 96aa303

Browse files
ejohnstownejohnstown
committed
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
1 parent fb15a05 commit 96aa303

4 files changed

Lines changed: 193 additions & 41 deletions

File tree

‎src/agent.c‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1731,6 +1731,84 @@ int wolfSSH_AGENT_enable(WOLFSSH* ssh, byte isEnabled)
17311731
}
17321732

17331733

1734+
int wolfSSH_AGENT_ChannelOpen(WOLFSSH* ssh)
1735+
{
1736+
WOLFSSH_AGENT_CTX* newAgent = NULL;
1737+
WOLFSSH_CHANNEL* newChannel = NULL;
1738+
int ret = WS_SUCCESS;
1739+
/* wolfSSH_accept() clears only want-read/want-write/auth-pending, so a
1740+
* WS_BAD_ARGUMENT latched by a poll kills the handshake. */
1741+
int recordError = 0;
1742+
1743+
WLOG_ENTER();
1744+
1745+
if (ssh == NULL)
1746+
ret = WS_SSH_NULL_E;
1747+
else if (ssh->ctx->side != WOLFSSH_ENDPOINT_SERVER) {
1748+
/* Server side only. wolfSSH_connect() sets ssh->agent too, so the
1749+
* checks below would report a channel a client never opened. */
1750+
ret = WS_BAD_ARGUMENT;
1751+
}
1752+
else if (!ssh->useAgent) {
1753+
/* Nothing asked for agent forwarding on this session. */
1754+
ret = WS_BAD_ARGUMENT;
1755+
}
1756+
else if (ssh->agent == NULL) {
1757+
/* Nothing else sets ssh->agent, so a NULL one means "not opened
1758+
* yet". Idempotent, so a poll cannot open a second channel. */
1759+
WLOG(WS_LOG_AGENT, "Starting agent channel");
1760+
1761+
newAgent = wolfSSH_AGENT_new(ssh->ctx->heap);
1762+
if (newAgent == NULL)
1763+
ret = WS_MEMORY_E;
1764+
1765+
if (ret == WS_SUCCESS) {
1766+
newChannel = ChannelNew(ssh, ID_CHANTYPE_AUTH_AGENT,
1767+
ssh->ctx->windowSz, ssh->ctx->maxPacketSz);
1768+
if (newChannel == NULL)
1769+
ret = WS_MEMORY_E;
1770+
}
1771+
1772+
if (ret == WS_SUCCESS) {
1773+
recordError = 1;
1774+
ret = SendChannelOpenSession(ssh, newChannel);
1775+
1776+
if (ret < WS_SUCCESS
1777+
&& ret != WS_WANT_WRITE && ret != WS_WANT_READ) {
1778+
ChannelDelete(newChannel, ssh->ctx->heap);
1779+
}
1780+
else {
1781+
/* Publish on a queued open too, so a retry takes the
1782+
* already-open path rather than opening a second. */
1783+
ChannelAppend(ssh, newChannel);
1784+
newAgent->channel = newChannel->channel;
1785+
ssh->agent = newAgent;
1786+
newAgent = NULL;
1787+
if (ssh->ctx->agentCb) {
1788+
ssh->ctx->agentCb(WOLFSSH_AGENT_LOCAL_SETUP,
1789+
ssh->agentCbCtx);
1790+
}
1791+
}
1792+
}
1793+
1794+
if (newAgent != NULL)
1795+
wolfSSH_AGENT_free(newAgent);
1796+
}
1797+
else if (wolfSSH_OutputPending(ssh)) {
1798+
/* Any queued output, not just this open. Flush it rather than
1799+
* report a success the peer hasn't seen. */
1800+
recordError = 1;
1801+
ret = wolfSSH_SendPacket(ssh);
1802+
}
1803+
1804+
if (recordError)
1805+
ssh->error = ret;
1806+
1807+
WLOG_LEAVE(ret);
1808+
return ret;
1809+
}
1810+
1811+
17341812
int wolfSSH_AGENT_worker(WOLFSSH* ssh)
17351813
{
17361814
int ret = WS_SUCCESS;

‎src/ssh.c‎

Lines changed: 1 addition & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -764,52 +764,12 @@ int wolfSSH_accept(WOLFSSH* ssh)
764764
#endif /* WOLFSSH_SFTP and !NO_WOLFSSH_SERVER */
765765
#ifdef WOLFSSH_AGENT
766766
if (ssh->useAgent) {
767-
WOLFSSH_AGENT_CTX* newAgent;
768-
WOLFSSH_CHANNEL* newChannel;
769-
770-
WLOG(WS_LOG_AGENT, "Starting agent channel");
771-
772-
newAgent = wolfSSH_AGENT_new(ssh->ctx->heap);
773-
if (newAgent == NULL) {
774-
ssh->error = WS_MEMORY_E;
775-
WLOG(WS_LOG_DEBUG, acceptError,
776-
"SERVER_USERAUTH_ACCEPT_DONE", ssh->error);
777-
return WS_ERROR;
778-
}
779-
780-
newChannel = ChannelNew(ssh, ID_CHANTYPE_AUTH_AGENT,
781-
ssh->ctx->windowSz, ssh->ctx->maxPacketSz);
782-
if (newChannel == NULL) {
783-
wolfSSH_AGENT_free(newAgent);
784-
ssh->error = WS_MEMORY_E;
785-
WLOG(WS_LOG_DEBUG, acceptError,
786-
"SERVER_USERAUTH_ACCEPT_DONE", ssh->error);
787-
return WS_FATAL_ERROR;
788-
}
789-
790-
ssh->error = SendChannelOpenSession(ssh, newChannel);
767+
ssh->error = wolfSSH_AGENT_ChannelOpen(ssh);
791768
if (ssh->error < WS_SUCCESS) {
792-
if (ssh->error == WS_WANT_WRITE ||
793-
ssh->error == WS_WANT_READ) {
794-
ChannelAppend(ssh, newChannel);
795-
}
796-
else {
797-
ChannelDelete(newChannel, ssh->ctx->heap);
798-
wolfSSH_AGENT_free(newAgent);
799-
}
800769
WLOG(WS_LOG_DEBUG, acceptError,
801770
"SERVER_USERAUTH_ACCEPT_DONE", ssh->error);
802771
return WS_FATAL_ERROR;
803772
}
804-
ChannelAppend(ssh, newChannel);
805-
newAgent->channel = newChannel->channel;
806-
if (ssh->ctx->agentCb) {
807-
ssh->ctx->agentCb(WOLFSSH_AGENT_LOCAL_SETUP,
808-
ssh->agentCbCtx);
809-
}
810-
if (ssh->agent != NULL)
811-
wolfSSH_AGENT_free(ssh->agent);
812-
ssh->agent = newAgent;
813773
}
814774
#endif /* WOLFSSH_AGENT */
815775
ssh->acceptState = ACCEPT_CLIENT_SESSION_ESTABLISHED;

‎tests/regress.c‎

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4437,6 +4437,102 @@ static void TestAgentChannelNullAgentSendsOpenFail(void)
44374437

44384438
FreeChannelOpenHarness(&harness);
44394439
}
4440+
4441+
/* Nothing asked for forwarding, so the open is refused rather than started.
4442+
* The refusal is the documented answer to a poll, so it must not land in
4443+
* ssh->error: wolfSSH_accept() would then abort with WS_INVALID_STATE_E. */
4444+
static void TestAgentChannelOpenWithoutRequest(void)
4445+
{
4446+
ChannelOpenHarness harness;
4447+
4448+
InitChannelOpenHarness(&harness, NULL, 0);
4449+
4450+
AssertIntEQ(wolfSSH_AGENT_ChannelOpen(harness.ssh), WS_BAD_ARGUMENT);
4451+
AssertNull(harness.ssh->agent);
4452+
AssertIntEQ(harness.io.outSz, 0);
4453+
AssertIntEQ(harness.ssh->error, WS_SUCCESS);
4454+
4455+
/* The handshake survives the poll: no input, so accept only wants read. */
4456+
AssertIntEQ(wolfSSH_accept(harness.ssh), WS_FATAL_ERROR);
4457+
AssertIntEQ(harness.ssh->error, WS_WANT_READ);
4458+
4459+
FreeChannelOpenHarness(&harness);
4460+
}
4461+
4462+
/* A queued open publishes the agent, so the caller's next poll must finish
4463+
* the send rather than report a success the peer never saw, and must not
4464+
* open a second channel. */
4465+
static void TestAgentChannelOpenFlushesQueuedOpen(void)
4466+
{
4467+
ChannelOpenHarness harness;
4468+
word32 outSz;
4469+
4470+
InitChannelOpenHarness(&harness, NULL, 0);
4471+
harness.ssh->useAgent = 1;
4472+
harness.io.blockNext = 1;
4473+
4474+
AssertIntEQ(wolfSSH_AGENT_ChannelOpen(harness.ssh), WS_WANT_WRITE);
4475+
AssertNotNull(harness.ssh->agent);
4476+
AssertIntEQ(harness.ssh->channelListSz, 1);
4477+
AssertIntEQ(harness.io.outSz, 0);
4478+
4479+
AssertIntEQ(wolfSSH_AGENT_ChannelOpen(harness.ssh), WS_SUCCESS);
4480+
AssertIntEQ(harness.ssh->channelListSz, 1);
4481+
AssertTrue(harness.io.outSz > 0);
4482+
AssertIntEQ(ParseMsgId(harness.io.out, harness.io.outSz),
4483+
MSGID_CHANNEL_OPEN);
4484+
4485+
/* The flushed open is the answer wolfSSH_accept() retries on: success,
4486+
* no second channel, no new packet, ssh->error untouched. */
4487+
outSz = harness.io.outSz;
4488+
harness.ssh->error = WS_SUCCESS;
4489+
4490+
AssertIntEQ(wolfSSH_AGENT_ChannelOpen(harness.ssh), WS_SUCCESS);
4491+
AssertIntEQ(harness.ssh->channelListSz, 1);
4492+
AssertIntEQ(harness.io.outSz, outSz);
4493+
AssertIntEQ(harness.ssh->error, WS_SUCCESS);
4494+
4495+
FreeChannelOpenHarness(&harness);
4496+
}
4497+
4498+
/* A send that fails outright, rather than blocking, leaves nothing behind,
4499+
* so a later poll starts the open over. */
4500+
static void TestAgentChannelOpenSendFailureCleansUp(void)
4501+
{
4502+
ChannelOpenHarness harness;
4503+
4504+
InitChannelOpenHarness(&harness, NULL, 0);
4505+
harness.ssh->useAgent = 1;
4506+
/* No room, so MemSend reports a general error. */
4507+
harness.io.outCap = 0;
4508+
4509+
AssertIntEQ(wolfSSH_AGENT_ChannelOpen(harness.ssh), WS_SOCKET_ERROR_E);
4510+
AssertNull(harness.ssh->agent);
4511+
AssertIntEQ(harness.ssh->channelListSz, 0);
4512+
AssertIntEQ(harness.io.outSz, 0);
4513+
AssertIntEQ(harness.ssh->error, WS_SOCKET_ERROR_E);
4514+
4515+
FreeChannelOpenHarness(&harness);
4516+
}
4517+
4518+
#ifndef NO_WOLFSSH_CLIENT
4519+
/* Server-side call. A client has an ssh->agent of its own, so answering the
4520+
* poll from it would report a channel that was never opened. */
4521+
static void TestAgentChannelOpenOnClientRefused(void)
4522+
{
4523+
ChannelOpenHarness harness;
4524+
4525+
InitChannelOpenHarnessClient(&harness, NULL, 0);
4526+
harness.ssh->useAgent = 1;
4527+
4528+
AssertIntEQ(wolfSSH_AGENT_ChannelOpen(harness.ssh), WS_BAD_ARGUMENT);
4529+
AssertIntEQ(harness.ssh->channelListSz, 0);
4530+
AssertIntEQ(harness.io.outSz, 0);
4531+
AssertIntEQ(harness.ssh->error, WS_SUCCESS);
4532+
4533+
FreeChannelOpenHarness(&harness);
4534+
}
4535+
#endif /* !NO_WOLFSSH_CLIENT */
44404536
#endif
44414537

44424538

@@ -13427,6 +13523,12 @@ int main(int argc, char** argv)
1342713523
#endif
1342813524
#ifdef WOLFSSH_AGENT
1342913525
TestAgentChannelNullAgentSendsOpenFail();
13526+
TestAgentChannelOpenWithoutRequest();
13527+
TestAgentChannelOpenFlushesQueuedOpen();
13528+
TestAgentChannelOpenSendFailureCleansUp();
13529+
#ifndef NO_WOLFSSH_CLIENT
13530+
TestAgentChannelOpenOnClientRefused();
13531+
#endif
1343013532
#endif
1343113533
#endif /* NO_WOLFSSH_SERVER */
1343213534
#if defined(WOLFSSH_AGENT) && !defined(WOLFSSH_NO_ED25519) \

‎wolfssh/agent.h‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,18 @@ WOLFSSH_API int wolfSSH_CTX_set_agent_cb(WOLFSSH_CTX* ctx,
181181
WOLFSSH_API int wolfSSH_set_agent_cb_ctx(WOLFSSH* ssh, void* ctx);
182182
WOLFSSH_API int wolfSSH_CTX_AGENT_enable(WOLFSSH_CTX* ctx, byte isEnabled);
183183
WOLFSSH_API int wolfSSH_AGENT_enable(WOLFSSH* ssh, byte isEnabled);
184+
/* Server side. Opens the auth-agent@openssh.com channel to the client once
185+
* the peer's auth-agent-req@openssh.com asks for forwarding. wolfSSH_accept()
186+
* does it on the default path; an application driving its own channels polls
187+
* this instead. Opens one channel, then flushes what of the open is queued.
188+
* Returns WS_SUCCESS, WS_BAD_ARGUMENT before the peer asks or on a client
189+
* session, WS_WANT_READ or WS_WANT_WRITE while output is still queued,
190+
* WS_SSH_NULL_E, WS_MEMORY_E, or whatever the send reports. WS_SUCCESS says
191+
* the open went out, not that the peer took it; a refusal reaches the
192+
* channel-open-fail callback.
193+
* Only the send records in ssh->error, so a poll ahead of the peer's request
194+
* leaves the session fit for wolfSSH_accept(). */
195+
WOLFSSH_API int wolfSSH_AGENT_ChannelOpen(WOLFSSH* ssh);
184196
WOLFSSH_LOCAL int wolfSSH_AGENT_worker(WOLFSSH* ssh);
185197
WOLFSSH_API int wolfSSH_AGENT_Relay(WOLFSSH* ssh,
186198
const byte* msg, word32* msgSz, byte* rsp, word32* rspSz);

0 commit comments

Comments
 (0)