Discard a rejected CBC packet before failing - #1281
Open
ejohnstown wants to merge 1 commit into
Open
ejohnstown wants to merge 1 commit into
ejohnstown wants to merge 1 commit into
Conversation
Under AES-CBC, DoReceive() no longer fails as soon as the decrypted length or the MAC is rejected. It reads the packet out to MAX_PACKET_SZ bytes and runs the MAC over the rest first, as OpenSSH does, so neither the failure point nor the MAC work reveals the length. - the length checks and the MAC check all go through RejectPacket() - a PROCESS_DISCARD state resumes the discard after a want-read; a transport failure mid-discard reports the rejection - CTR, AEAD and unencrypted packets still fail immediately - the discard compiles out under WOLFSSH_NO_AES_CBC - tests/unit.c covers each rejection with short reads, a peer close, and a nonzero start offset Issue: F-12606, F-8807, F-11668
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The discard path masks local allocation failures as packet-rejection errors.
Review effort: Balanced
Findings: 1
What changed in this PR
Adds constant-work rejection handling for malformed AES-CBC packets to reduce timing leakage.
Changes:
- Introduces resumable rejected-packet discard state.
- Preserves immediate rejection for non-CBC modes.
- Adds CBC discard and CTR regression tests.
| File | Description |
|---|---|
wolfssh/internal.h |
Adds discard state and metadata. |
src/internal.c |
Implements CBC packet discard handling. |
tests/unit.c |
Tests discard behavior and immediate CTR failure. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+14614
to
+14619
| if (ret < 0) { | ||
| /* Past a want-read the discard is over; report the rejection. */ | ||
| if (ssh->error != WS_WANT_READ) | ||
| ssh->error = ssh->discardError; | ||
| return ret; | ||
| } |
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.
Under AES-CBC, a packet rejected on its length or its MAC no longer fails at once. DoReceive() reads it out to MAX_PACKET_SZ bytes and runs the MAC over the rest first, as OpenSSH does, so neither the failure point nor the MAC work reveals the length.