Skip to content

Reject client conn messages before key exchange - #1147

Open
ejohnstown wants to merge 1 commit into
wolfSSL:masterfrom
ejohnstown:msgfilter
Open

Reject client conn messages before key exchange#1147
ejohnstown wants to merge 1 commit into
wolfSSL:masterfrom
ejohnstown:msgfilter

Conversation

@ejohnstown

Copy link
Copy Markdown
Contributor
  • IsMessageAllowedClient() gated the post-userauth rejection on connectState >= CONNECT_KEYED, so every state below it allowed msgid 80 and above.
  • A client in the pre-key-exchange DoReceive() loops accepted an unencrypted CHANNEL_OPEN, created the channel and replied with CHANNEL_OPEN_CONF.
  • Add a state sweep over the connection-protocol range and a full DoReceive() of a plaintext CHANNEL_OPEN from a pre-auth state.

Issue: F-7628

- IsMessageAllowedClient() gated the post-userauth rejection on
  connectState >= CONNECT_KEYED, so every state below it allowed
  msgid 80 and above.
- A client in the pre-key-exchange DoReceive() loops accepted an
  unencrypted CHANNEL_OPEN, created the channel and replied with
  CHANNEL_OPEN_CONF.
- Add a state sweep over the connection-protocol range and a full
  DoReceive() of a plaintext CHANNEL_OPEN from a pre-auth state.

Issue: F-7628
Copilot AI lite review requested due to automatic review settings August 6, 2026 17:35

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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@ejohnstown
ejohnstown requested review from wolfSSL-Fenrir-bot and a balanced review from Copilot August 6, 2026 18:19

@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 #1147

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@ejohnstown
ejohnstown requested a lite review from Copilot August 6, 2026 19:13

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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@ejohnstown
ejohnstown requested a lite review from Copilot August 6, 2026 20:12

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.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/internal.c:855

  • The comment says 'Is client userauth complete?' but the condition checks the opposite (userauth not yet complete). Please update the comment to match the logic (e.g., 'Is client userauth incomplete?') or invert the condition so comment and behavior stay consistent.
    /* Is client userauth complete? */
    if (ssh->connectState < CONNECT_SERVER_USERAUTH_ACCEPT_DONE) {

tests/regress.c:1692

  • MSGIDLIMIT_CONN_MAX looks like a limit/sentinel constant rather than a real on-the-wire message ID. Passing it into wolfSSH_TestIsMessageAllowed() could be invalid and (depending on implementation) risk out-of-range indexing or unintended paths. Consider replacing it with a concrete highest valid connection-protocol msgid, or explicitly testing the boundary as MSGIDLIMIT_CONN_MAX - 1 (and, if desired, a separate test for the sentinel/invalid value with clear intent).
    static const byte connMsgs[] = {
        MSGID_GLOBAL_REQUEST, MSGID_CHANNEL_OPEN, MSGID_CHANNEL_DATA,
        MSGID_CHANNEL_REQUEST, MSGIDLIMIT_CONN_MAX
    };

tests/regress.c:1744

  • This test relies on BuildChannelOpenPacket() fitting into a fixed 256-byte stack buffer, but it doesn't assert that the returned pktSz is non-zero and <= sizeof(pkt). Adding an assertion here will make failures clearer and prevent accidental truncation/overflow as packet formats evolve.
    byte pkt[256];
    byte out[256];
    word32 pktSz;

    pktSz = BuildChannelOpenPacket("session", 0, 131072, 16384, NULL, 0,
            pkt, sizeof(pkt));

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.

3 participants