fix: protocol logic hardening in wolfSSH#1097
Open
MarkAtwood wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens several pieces of wolfSSH server-side protocol logic based on static-analysis findings, aiming to enforce stricter state/method gating and prevent subtle data-loss or policy-bypass behaviors.
Changes:
- Raises the default DH group-exchange minimum size to 2048 bits.
- Tightens server-side protocol handling: enforces advertised auth-method policy, rejects repeated channel program-start requests, and ignores post-auth USERAUTH_REQUEST per RFC 4252.
- Adds RSA raw-pubkey minimum size enforcement and fixes buffer appends to preserve unread data.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
wolfssh/internal.h |
Raises default GEX DH minimum to 2048 bits. |
src/internal.c |
Protocol/state hardening in auth gating, channel request handling, post-auth message handling, RSA key-size checks, and buffer append behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| WLOG(WS_LOG_DEBUG, | ||
| "DUAR: userauth type not advertised: %s", | ||
| IdToName(authNameId)); | ||
| authNameId = ID_UNKNOWN; |
| #ifdef WOLFSSH_KEYBOARD_INTERACTIVE | ||
| typeAllowed |= WOLFSSH_USERAUTH_KEYBOARD; | ||
| #endif | ||
| #if !defined(WOLFSSH_NO_RSA) || !defined(WOLFSSH_NO_ECDSA) |
Comment on lines
+10844
to
+10849
| channel->sessionType = WOLFSSH_SESSION_SHELL; | ||
| if (ssh->ctx->channelReqShellCb) { | ||
| rej = ssh->ctx->channelReqShellCb(channel, | ||
| ssh->channelReqCtx); | ||
| } | ||
| ssh->clientState = CLIENT_DONE; |
Comment on lines
+10859
to
+10863
| ret = GetStringAlloc(ssh->ctx->heap, &channel->command, NULL, | ||
| buf, len, &begin); | ||
| channel->sessionType = WOLFSSH_SESSION_EXEC; | ||
| if (ssh->ctx->channelReqExecCb) { | ||
| rej = ssh->ctx->channelReqExecCb(channel, |
Comment on lines
+10878
to
+10882
| ret = GetStringAlloc(ssh->ctx->heap, &channel->command, NULL, | ||
| buf, len, &begin); | ||
| channel->sessionType = WOLFSSH_SESSION_SUBSYSTEM; | ||
| if (ssh->ctx->channelReqSubsysCb) { | ||
| rej = ssh->ctx->channelReqSubsysCb(channel, |
- DoUserAuthRequest: gate on the advertised auth-type set before running the application userAuthCb, rejecting methods the server never advertised (Fenrir 4579). - DoChannelRequest: only honor shell/exec/subsystem when sessionType is still UNKNOWN, enforcing "at most one succeeds" (Fenrir 6271). - Silently discard USERAUTH_REQUEST received after authentication completes, per RFC 4252 section 5.1, instead of disconnecting (Fenrir 6515). - DoChannelExtendedData/PutBuffer: preserve unread buffered data instead of overwriting it (Fenrir 6520).
MarkAtwood
force-pushed
the
fix/wolfssh-protocol_logic
branch
from
0b8748e to
89aeeeb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Consolidated protocol-logic hardening for wolfSSH (reported by Fenrir static analysis):
DoUserAuthRequestnow gates on the advertised auth-type set before running the applicationuserAuthCb, so a method the server never advertised is rejected withUSERAUTH_FAILUREinstead of being silently handled.DoChannelRequestonly honorsshell/exec/subsystemwhilesessionTypeis stillUNKNOWN, enforcing "at most one succeeds".USERAUTH_REQUESTreceived after authentication completes is now silently discarded per RFC 4252 §5.1, instead of tearing down the connection.DoChannelExtendedData/PutBufferpreserve unread buffered data instead of overwriting it.Build-verified (
--enable-all) against wolfSSL; all four applied together compile and link clean.