Validate ECC curve in user auth, document auth callback contracts - #1141
Open
ejohnstown wants to merge 4 commits into
Open
Validate ECC curve in user auth, document auth callback contracts#1141ejohnstown wants to merge 4 commits into
ejohnstown wants to merge 4 commits into
Conversation
ejohnstown
requested review from
wolfSSL-Fenrir-bot
and
a lite review from Copilot
August 4, 2026 22:13
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request tightens authentication safety and clarifies callback contracts in wolfSSH, specifically addressing ECC public key algorithm/curve binding during user authentication and documenting “fail-closed” expectations for authentication-related callbacks.
Changes:
- Documented
WS_CallbackUserAuthandWS_CallbackPublicKeyCheckcontracts to prevent insecure “return 0” stubs from silently accepting users/host keys. - Hardened ECDSA publickey userauth parsing by requiring the curve name inside the key blob to match the declared algorithm and by importing points pinned to the declared curve.
- Added unit tests covering the byte-count highwater rekey trigger and ECC curve-mismatch userauth cases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| wolfssh/ssh.h | Adds explicit, security-focused callback contract documentation to prevent fail-open stubs (userauth + host key verification). |
| src/internal.c | Binds ECDSA userauth curve name and point import to the declared algorithm/curve to prevent curve-mismatch acceptance. |
| tests/unit.c | Adds unit coverage for byte-based highwater behavior and for ECC userauth curve/name/point mismatch rejection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1141
Scan targets checked: wolfssh-bugs, wolfssh-src
No new issues found in the changed files. ✅
- WOLFSSH_USERAUTH_SUCCESS is 0, the same value as WS_SUCCESS and the C "no error" idiom, so a bare "return 0", a forwarded WS_SUCCESS, or a fall-through default of 0 silently authenticates the client. - Spell out that the callback must fail closed: return WOLFSSH_USERAUTH_FAILURE for any authType or code path it does not explicitly handle. - Note that for WOLFSSH_USERAUTH_PUBLICKEY the library verifies the signature but not the key's authorization, so the callback has to check the offered key against the user's authorized keys. Issue: F-6815
- The callback is the client's only defense against a man in the middle, and 0 accepts the server host key, so a stub that defaults to "return 0" trusts whatever key is presented. - State that the callback must match the key against a trust store, and point at ClientPublicKeyCheck() in the examples. - Record that with no callback registered the host key is rejected with WS_PUBKEY_REJECTED_E. Issue: F-6976
- DoUserAuthRequestEcc() skipped the curve name in the public key blob and imported the point with wc_ecc_import_x963(), which picks the curve from the point length, so the key did not have to be on the curve the declared algorithm names. - Derive the curve from pk->publicKeyType with NameToId() and wcPrimeForId(), require the blob's curve name to equal PrimeNameForId() for that id, and import with wc_ecc_import_x963_ex() pinned to that curve. - The import's error check sat outside the success guard, so it rewrote any earlier error as WS_CRYPTO_FAILED. Scope it to the import itself so the parse and algorithm-match errors keep their own codes. - Add test_EccUserAuthCurveMismatch, which offers a blob naming one curve under another algorithm and expects the request to fail. Issue: F-6979
- The byte-count branch of HighwaterCheck() had no coverage. Exercise the boundary at the mark, the once-per-epoch flag that keeps the callback from firing a second time, the receive side, and a mark of 0 disabling the check. Issue: F-6978
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.
ssh.h: document theWS_CallbackUserAuthcontract (F-6815) --WOLFSSH_USERAUTH_SUCCESSis 0, so a stub returning 0 authenticates everyone. Say the callback must fail closed, and that publickey auth requires it to check the key against the user's authorized keys.ssh.h: document theWS_CallbackPublicKeyCheckcontract (F-6976) -- 0 accepts the host key, so a "return 0" stub trusts any server. Require a trust store check, point atClientPublicKeyCheck(), note that no callback meansWS_PUBKEY_REJECTED_E.internal: validate the ECC curve name in user auth (F-6979) --DoUserAuthRequestEcc()skipped the blob's curve name and letwc_ecc_import_x963()infer the curve from the point length, so the key need not be on the curve the algorithm names. Pin the import to the curve frompk->publicKeyTypeand require the name to match. Also scope the import's error check so parse errors keep their own codes. Addstest_EccUserAuthCurveMismatch.tests: addtest_ByteHighwater(F-6978) -- Covers the byte-count branch ofHighwaterCheck(): the mark boundary, the once-per-epoch flag, the receive side, and a mark of 0.