Repository navigation
Hash the pad byte of K without a branch - #1283
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The security-sensitive padded hashing path lacks known-answer test coverage.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Refactors shared-secret mpint hashing to avoid branching on the pad byte.
Changes:
- Adds branchless mpint header hashing.
- Updates key exchange and key derivation paths.
- Adds
CreateMpint()edge-case tests.
| File | Description |
|---|---|
src/internal.c |
Implements branchless pad handling and hashing. |
wolfssh/internal.h |
Exposes the internal test wrapper. |
tests/unit.c |
Tests mpint normalization and validation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1283
Scan targets checked: wolfssh-src, wolfssh-bugs
Coverage: 1 of 3 in-scope changed file(s) opened by the reviewer; not opened: tests/unit.c, wolfssh/internal.h
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
The MSB of the shared secret K decides whether its mpint encoding takes a 0 pad byte. CreateMpint now derives the pad with masks, and HashMpintHeader hashes the length and the optional pad as one update of LENGTH_SZ + kPad bytes: no branch on the pad in wolfSSH, and no zero-length update, which the SE050 and TI hash ports reject. - apply to DoKexDhReply, SendKexDhReply, and the in-tree GenerateKey - add wolfSSH_TestCreateMpint and a unit test of the pad, leading-zero, empty-input and NULL-argument cases - add a GenerateKey known-answer vector with the MSB of K set Issue: F-8808
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1283
Scan targets checked: wolfssh-src, wolfssh-bugs
Coverage: 1 of 1 in-scope changed file(s) opened by the reviewer
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
The MSB of the shared secret K decides whether its mpint encoding takes a 0 pad byte. The pad is now derived with masks and hashed with the length in a single update, with no branch on it in wolfSSH (F-8808).