Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions examples/portfwd/portfwd.c
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,43 @@ static int portfwdReqFailureCb(WOLFSSH* ssh, void* buf, word32 sz, void* ctx)
}


/* Reports one open response. The id is our own: a failed open never learns
* the peer's, and it is the id portfwd_worker() tracks. */
static int wsChannelOpenRespCb(WOLFSSH_CHANNEL* channel, void* ctx,
const char* result)
{
word32 id = 0;
const char* str;

if (ctx != NULL) {
str = (const char*)ctx;
}
else {
str = "<BAD CONTEXT>";
}
if (wolfSSH_ChannelGetId(channel, &id, WS_CHANNEL_ID_SELF)
!= WS_SUCCESS) {
printf("Channel for %s, unknown id open %s.\n", str, result);
}
else {
printf("Channel for %s, %u open %s.\n", str, id, result);
}
return WS_SUCCESS;
}


static int wsChannelOpenConfCb(WOLFSSH_CHANNEL* channel, void* ctx)
{
return wsChannelOpenRespCb(channel, ctx, "confirmed");
}


static int wsChannelOpenFailCb(WOLFSSH_CHANNEL* channel, void* ctx)
{
return wsChannelOpenRespCb(channel, ctx, "failed");
}


/*
* fwdFromHost - address to bind the local listener socket to (default: any)
* fwdFromHostPort - port number to bind the local listener socket to
Expand Down Expand Up @@ -583,6 +620,9 @@ THREAD_RETURN WOLFSSH_THREAD portfwd_worker(void* args)

wolfSSH_CTX_SetPublicKeyCheck(ctx, wsPublicKeyCheck);
wolfSSH_SetPublicKeyCheckCtx(ssh, (void*)"You've been sampled.");
wolfSSH_CTX_SetChannelOpenRespCb(ctx,
wsChannelOpenConfCb, wsChannelOpenFailCb);
wolfSSH_SetChannelOpenCtx(ssh, (void*)"port forward");

ret = wolfSSH_SetUsername(ssh, username);
if (ret != WS_SUCCESS)
Expand Down
65 changes: 65 additions & 0 deletions src/agent.c
Original file line number Diff line number Diff line change
Expand Up @@ -1731,6 +1731,71 @@ 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;

WLOG_ENTER();

if (ssh == NULL)
ret = WS_SSH_NULL_E;
else if (!ssh->useAgent) {
/* Nothing asked for agent forwarding on this session. */
ret = WS_BAD_ARGUMENT;
}
else if (ssh->agent == NULL) {
/* Server side sets ssh->agent here and nowhere else, so a NULL one
* is the "not opened yet" test. Idempotent so a caller polling for
* the peer's request cannot end up with two agent channels. */
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) {
ret = SendChannelOpenSession(ssh, newChannel);

if (ret < WS_SUCCESS
&& ret != WS_WANT_WRITE && ret != WS_WANT_READ) {
ChannelDelete(newChannel, ssh->ctx->heap);
}
else {
/* Publish the agent even when the open is only queued, so
* a retry takes the already-open path above rather than
* opening a second channel. */
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);
}

if (ssh != NULL)
ssh->error = ret;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wolfSSH_AGENT_ChannelOpen() latches its benign "not requested yet" return into ssh-error · Incorrect error handling

Every return is written into ssh->error, including the WS_BAD_ARGUMENT reported while the peer has not asked for agent forwarding — the polling agent.h documents as safe. wolfSSH_accept() (src/ssh.c:598) clears only the want/auth-pending codes, so a poll poisons the session and later accepts return WS_INVALID_STATE_E. The success path likewise clears a latched WS_WANT_WRITE. Sibling wolfSSH_AGENT_Relay() sets ssh->error only on genuine failure.

Fix: Record ssh->error only for genuine failures, leaving it untouched on WS_SUCCESS and on the WS_BAD_ARGUMENT no-agent-requested return.


WLOG_LEAVE(ret);
return ret;
}


int wolfSSH_AGENT_worker(WOLFSSH* ssh)
{
int ret = WS_SUCCESS;
Expand Down
12 changes: 11 additions & 1 deletion src/internal.c
Original file line number Diff line number Diff line change
Expand Up @@ -1672,6 +1672,7 @@ WOLFSSH* SshInit(WOLFSSH* ssh, WOLFSSH_CTX* ctx)
ssh->highwaterMark = ctx->highwaterMark;
ssh->msgHighwaterMark = ctx->msgHighwaterMark;
ssh->maxAuthAttempts = ctx->maxAuthAttempts;
ssh->appChannels = ctx->appChannels;
ssh->highwaterCtx = (void*)ssh;
ssh->reqSuccessCtx = (void*)ssh;
ssh->fs = NULL;
Expand Down Expand Up @@ -12725,6 +12726,9 @@ static int DoChannelRequest(WOLFSSH* ssh,
if (ssh->ctx->channelReqShellCb) {
rej = ssh->ctx->channelReqShellCb(channel, ssh->channelReqCtx);
}
else {
rej = ssh->appChannels;
}
ssh->clientState = CLIENT_DONE;
}
else if (ChannelRequestIs(type, typeSz, "exec")) {
Expand All @@ -12734,6 +12738,9 @@ static int DoChannelRequest(WOLFSSH* ssh,
if (ssh->ctx->channelReqExecCb) {
rej = ssh->ctx->channelReqExecCb(channel, ssh->channelReqCtx);
}
else {
rej = ssh->appChannels;
}
ssh->clientState = CLIENT_DONE;

WLOG(WS_LOG_DEBUG, " command = %s", channel->command);
Expand All @@ -12745,6 +12752,9 @@ static int DoChannelRequest(WOLFSSH* ssh,
if (ssh->ctx->channelReqSubsysCb) {
rej = ssh->ctx->channelReqSubsysCb(channel, ssh->channelReqCtx);
}
else {
rej = ssh->appChannels;
}
ssh->clientState = CLIENT_DONE;

WLOG(WS_LOG_DEBUG, " subsystem = %s", channel->command);
Expand Down Expand Up @@ -12886,7 +12896,7 @@ static int DoChannelRequest(WOLFSSH* ssh,
int replyRet;

if (rej) {
WLOG(WS_LOG_DEBUG, "Callback rejecting channel request.");
WLOG(WS_LOG_DEBUG, "Rejecting channel request.");
}
replyRet = SendChannelSuccess(ssh, channelId,
(ret == WS_SUCCESS && !rej));
Expand Down
96 changes: 52 additions & 44 deletions src/ssh.c
Original file line number Diff line number Diff line change
Expand Up @@ -579,6 +579,8 @@ const char acceptState[] = "accept state: %s";

int wolfSSH_accept(WOLFSSH* ssh)
{
byte stopState;

WLOG(WS_LOG_DEBUG, "Entering wolfSSH_accept()");

if (ssh == NULL)
Expand All @@ -598,6 +600,15 @@ int wolfSSH_accept(WOLFSSH* ssh)
return WS_INVALID_STATE_E;
}

/* In application-driven mode the state machine stops as soon as the
* user is authenticated; everything past that is the application's.
* Only stop there if the session has not already gone by: the loop
* below tests the stop state exactly, so a state it has stepped over
* would never terminate it. */
stopState = (ssh->appChannels
&& ssh->acceptState <= ACCEPT_SERVER_USERAUTH_SENT) ?
ACCEPT_SERVER_USERAUTH_SENT : ACCEPT_CLIENT_SESSION_ESTABLISHED;

/* check if data pending to be sent */
if (ssh->outputBuffer.length > 0 &&
ssh->acceptState < ACCEPT_CLIENT_SESSION_ESTABLISHED) {
Expand All @@ -609,7 +620,11 @@ int wolfSSH_accept(WOLFSSH* ssh)
ssh->acceptState != ACCEPT_SERVER_USERAUTH_ACCEPT_SENT &&
ssh->acceptState != ACCEPT_SERVER_KEXINIT_SENT &&
ssh->acceptState != ACCEPT_KEYED &&
ssh->acceptState != ACCEPT_SERVER_CHANNEL_ACCEPT_SENT) {
ssh->acceptState != ACCEPT_SERVER_CHANNEL_ACCEPT_SENT &&
/* Never step over where this call is meant to stop. The
* loop below tests for that state exactly, and the SCP and
* SFTP re-entry states sort after it. */
ssh->acceptState != stopState) {
WLOG(WS_LOG_DEBUG, "Advancing accept state");
ssh->acceptState++;
}
Expand All @@ -631,7 +646,7 @@ int wolfSSH_accept(WOLFSSH* ssh)
}
}

while (ssh->acceptState != ACCEPT_CLIENT_SESSION_ESTABLISHED) {
while (ssh->acceptState != stopState) {
switch (ssh->acceptState) {

case ACCEPT_BEGIN:
Expand Down Expand Up @@ -721,6 +736,12 @@ int wolfSSH_accept(WOLFSSH* ssh)
}
ssh->acceptState = ACCEPT_SERVER_USERAUTH_SENT;
WLOG(WS_LOG_DEBUG, acceptState, "SERVER_USERAUTH_SENT");
if (stopState == ACCEPT_SERVER_USERAUTH_SENT) {
/* The application takes it from here. Tested through
* stopState so a callback that changed the flag during
* this call cannot half-apply it. */
break;
}
FALL_THROUGH;

case ACCEPT_SERVER_USERAUTH_SENT:
Expand Down Expand Up @@ -764,52 +785,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;
Expand Down Expand Up @@ -3963,7 +3944,8 @@ WOLFSSH_CHANNEL* wolfSSH_ChannelFwdNewRemote(WOLFSSH* ssh,
if (newChannel != NULL)
ChannelAppend(ssh, newChannel);

WLOG(WS_LOG_DEBUG, "Leaving wolfSSH_ChannelFwdNewRemote(), newChannel = %p, ret = %d",
WLOG(WS_LOG_DEBUG,
"Leaving wolfSSH_ChannelFwdNewRemote(), newChannel = %p, ret = %d",
newChannel, ret);
return newChannel;
}
Expand Down Expand Up @@ -4957,6 +4939,32 @@ int wolfSSH_CTX_SetChannelReqSubsysCb(WOLFSSH_CTX* ctx,
}


int wolfSSH_CTX_SetAppChannels(WOLFSSH_CTX* ctx, byte enable)
{
int ret = WS_SSH_CTX_NULL_E;

if (ctx != NULL) {
ctx->appChannels = (enable != 0);
ret = WS_SUCCESS;
}

return ret;
}


int wolfSSH_SetAppChannels(WOLFSSH* ssh, byte enable)
{
int ret = WS_SSH_NULL_E;

if (ssh != NULL) {
ssh->appChannels = (enable != 0);
ret = WS_SUCCESS;
}

return ret;
}


int wolfSSH_SetChannelOpenCtx(WOLFSSH* ssh, void* ctx)
{
int ret = WS_SSH_NULL_E;
Expand Down
8 changes: 6 additions & 2 deletions src/wolfsftp.c
Original file line number Diff line number Diff line change
Expand Up @@ -1383,8 +1383,12 @@ int wolfSSH_SFTP_accept(WOLFSSH* ssh)
if (ssh->error == WS_WANT_READ || ssh->error == WS_WANT_WRITE)
ssh->error = WS_SUCCESS;

/* check accept is done, if not call wolfSSH accept */
if (ssh->acceptState < ACCEPT_CLIENT_SESSION_ESTABLISHED) {
/* check accept is done, if not call wolfSSH accept. In
* application-driven mode accept() parks at ACCEPT_SERVER_USERAUTH_SENT
* and never advances, so that state counts as done here. */
if (ssh->acceptState < ACCEPT_CLIENT_SESSION_ESTABLISHED
&& !(ssh->appChannels

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wolfSSH_SFTP_accept() serves SFTP without a granted subsystem request in app-channels mode · Channel handling errors

The new guard drops into the SFTP state machine as soon as ssh->appChannels is set and userauth completed, with no requirement that the peer opened a channel or that a subsystem sftp request was granted. With appChannels on and no channelReqSubsysCb, DoChannelRequest replies CHANNEL_FAILURE (rej = ssh->appChannels) yet SFTP is still served on that channel. Adjacent to known finding #8852, which covers state committed in DoChannelRequest after a callback rejects; this is a separate gate removed in wolfSSH_SFTP_accept.

Fix: Gate the app-channels bypass on the session request having been granted (e.g. clientState >= CLIENT_DONE and session type SUBSYSTEM/sftp) rather than on userauth alone.

&& ssh->acceptState >= ACCEPT_SERVER_USERAUTH_SENT)) {
byte name[] = "sftp";

WLOG(WS_LOG_SFTP, "Trying to do SSH accept first");
Expand Down
Loading
Loading