Skip to content

internal: gate auth-agent channel opens on the client's request - #1244

Open
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_13389
Open

internal: gate auth-agent channel opens on the client's request#1244
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_13389

Conversation

@yosuke-wolfssl

Copy link
Copy Markdown
Contributor

Problem

Finding f-13389. With WOLFSSH_AGENT, DoChannelOpen() admitted an inbound
auth-agent@openssh.com channel open on the single test ssh->agent != NULL.

That pointer is not a record of the request. The client allocates it during userauth,
because the same object serves agent-backed publickey authentication, and it sends
auth-agent-req@openssh.com only after its session channel is confirmed. In between,
wolfSSH_connect() sits in DoReceiveHandshake() processing packets, so a malicious
server can open an agent channel a round trip before the client has asked for
forwarding, and get a confirmation. Applications relay non-session channel data into
wolfSSH_AGENT_Relay() (as examples/client does), making the channel a signing path
to the user's local agent.

Nothing checked the endpoint role either: a server with ssh->agent set — which happens
once a client sends auth-agent-req — accepted an auth-agent open back from that
client, the wrong direction per OpenSSH's PROTOCOL, overwriting ssh->agent->channel
with a peer-chosen id.

The pre-authentication variant of this is not reachable: IsMessageAllowedClient()
already refuses message ids >= 80 until CONNECT_SERVER_USERAUTH_ACCEPT_DONE.

Fix (src/internal.c)

The ID_CHANTYPE_AUTH_AGENT case now decides in three steps, ahead of ChannelNew()
and any channelOpenCb, as the session and forwarded-tcpip gates beside it do:

Check Refuses when Answer
Direction endpoint is a server OPEN_ADMINISTRATIVELY_PROHIBITED
Request connectState < CONNECT_CLIENT_CHANNEL_AGENT_REQUEST_SENT OPEN_ADMINISTRATIVELY_PROHIBITED
Resource ssh->agent == NULL unchanged, WS_AGENT_NULL_E

No new session state: wolfSSH_connect() already advances connectState immediately
after SendChannelAgentRequest(), and returns without advancing if that send fails.
wolfSSH clients never open this channel type, so refusing it on a server is
interop-safe. Refusals log at WS_LOG_DEBUG, since they fire per peer open request.

Closes f-13389.

Tests (tests/regress.c)

Four cases on the existing in-memory channel-open harness: an open before the request is
refused; the gate outranks an accepting channelOpenCb; an open after the request is
confirmed and wires agent->channel; a server refuses one. TestAgentChannelNullAgentSendsOpenFail
moves to a client harness, where the ssh->agent check still decides the open, and now
asserts its fail reason.

Verification

  • 6/6 C test programs pass under --enable-all; regress also passes with --disable-agent.
  • Negative controls: dropping the request check, the direction check, or the fail-reason
    assertion each makes exactly the intended test fail; forcing the gate to always refuse
    fails the accepting test, so it is not vacuous.
  • gcc-13 -Werror preflight clean across 6 configs.

@yosuke-wolfssl yosuke-wolfssl self-assigned this Sep 8, 2026
Copilot AI lite review requested due to automatic review settings September 8, 2026 05:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

An auth-agent open currently logs ssh->agent at INFO with a raw pointer value per request, which can enable log spamming and unnecessarily exposes address data.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR hardens wolfSSH’s handling of auth-agent@openssh.com channel opens by ensuring agent-forwarding channels are only accepted when they are (1) directionally valid and (2) actually requested by the client, closing a server-driven channel-open signing path.

Changes:

  • Added endpoint-direction and client-request-state gating for ID_CHANTYPE_AUTH_AGENT in DoChannelOpen() before any channel allocation or callbacks run.
  • Added regression tests covering agent channel opens before/after the request state, callback precedence, null-agent behavior, and server-side refusal.
File summaries
File Description
src/internal.c Adds pre-ChannelNew() gating for auth-agent channel opens based on endpoint role and connectState.
tests/regress.c Adds an in-memory harness test matrix for agent channel open accept/refuse scenarios and moves the null-agent case into the client harness.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/internal.c

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #1244

Scan targets checked: wolfssh-bugs, wolfssh-src

Fenrir result: Approved ✅

No new issues found in the changed files.

Advisory only — this automated result does not count as a GitHub approval.

- DoChannelOpen refuses an auth-agent open on a server endpoint, and on
  a client with the agent disabled or connectState below
  CONNECT_CLIENT_CHANNEL_AGENT_REQUEST_SENT.
- Both answer OPEN_ADMINISTRATIVELY_PROHIBITED; the ssh->agent check
  stays as the resource check behind them.
- Cover the refusals before the request, with the agent disabled, over
  an accepting channelOpenCb, and on a server, plus the accepted open
  past the request; each refusal asserts ssh->error stays clean.
- Move TestAgentChannelNullAgentSendsOpenFail to a client harness, and
  have the agent tests set the agent flag explicitly.

Issue: F-13389
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants