Added Public Key Validation for Decoded Private Host Keys - #1121
Conversation
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1121
Scan targets checked: wolfssh-bugs, wolfssh-src
No new issues found in the changed files. ✅
ejohnstown
left a comment
There was a problem hiding this comment.
Tested this on Ubuntu 26.04 against wolfSSL master (ac01707f5) built with
--enable-all --enable-experimental --enable-mldsa, wolfSSH configured
--enable-all --enable-sshd. Clean build (no new warnings), make check
passes 10/10 with 1 skip, and valgrind is clean on the new reject paths
(all heap blocks freed, 0 errors) as well as on the ECDSA warn path and a
successful ML-DSA handshake.
Confirmed the fix does what it says. Using a private-only ML-DSA-65 DER
(4060 bytes, from wc_MlDsaKey_PrivateKeyToDer), master starts the daemon
and then fails with SendKexDhReply(), ret = -173 and accept error -1001,
exactly as reported in #1120. With this PR, wolfsshd refuses at startup
with -1029 and the new message. Same story for a 48-byte private-only
Ed25519 key: -134 at KEX on master, clean load-time rejection here.
Worth recording that neither rejection is a regression. A private-only
Ed25519 key cannot sign at all (wc_ed25519_sign_msg returns -173), so it
was never a usable host key, and the ML-DSA advice in the message is
accurate: wc_MlDsaKey_PrivateKeyDecode handles a seed-encoded key via
MakeKeyFromSeed, which produces the public half. The repo's own
keys/server-key-mldsa*.der and server-key-ed25519.der all carry public
keys, so nothing existing is affected.
Two things I would like addressed before this goes in.
-
The
MLDSA_MAX_PUB_KEY_SIZEscratch buffer on the stack. It takes
IdentifyAsn1Key's frame from about 224 bytes to about 2736 (measured:
sub sp, sp, #0xa50; the compiler folds the ECC and Ed25519 buffers into
the ML-DSA one). Everywhere else in internal.c that needs a key block
this size heap-allocates it (see the sigKeyBlock allocations around
internal.c:6717 and internal.c:14653), which keepsWOLFSSH_SMALL_STACK
builds viable.WS_KeySignature.ks.mldsaholds only theMlDsaKeyand
has noqfield to reuse, so this wants a WMALLOC/WFREE pair around the
export. -
The ECDSA path is warn-only, so it does not actually meet the stated
goal. I built a SEC1 key with the optional public point omitted: behavior
is identical to master, stillSendKexDhReply(), ret = -246at handshake
time, just with a log line beforehand. The X.509 pairing rationale in the
comment is plausible but is not tested here and I did not verify it
either. Either confirm that claim and note the asymmetry, or reject
ECDSA the same way. Separately, the warning text ("this only works if
paired with an X.509 certificate") is misleading on the client user-auth
path, which goes through this same function.
Minor: no test covers the new behavior. A private-only ML-DSA DER is a few
lines to generate with wc_MlDsaKey_PrivateKeyToDer, and would lock in the
load-time rejection.
124d09f to
73d7d80
Compare
|
Fixed suggestion 1 and test coverage. For suggestion 2, when ECDSA is paired with a cert, we don't need to derive the public key. I did update the comment for suggestion 2. |
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1121
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 2
1 finding(s) posted as inline comments (see file-level comments below)
Low (1)
New ECDSA/Ed25519 no-pubkey branches lack test coverage
File: tests/unit.c:6008
Function: test_IdentifyAsn1Key
Category: Missing edge-case coverage on a function the PR also changed
The PR adds new pubkey-derivability checks to the ECDSA (warn-only) and Ed25519 (WS_CRYPTO_FAILED) branches of IdentifyAsn1Key, but only the analogous ML-DSA branch gets a new test; the ECDSA and Ed25519 branches are exercised only via happy-path keys that already embed a derivable public key.
Recommendation: Add private-only DER fixtures for ECDSA (SEC1 key without the public point) and Ed25519 to cover the new warn/WS_CRYPTO_FAILED paths.
Referenced code: tests/unit.c:6008-6012 (5 lines)
This review was generated automatically by Fenrir. Findings are non-blocking.
73d7d80 to
e97dbf4
Compare
5ff1c6c to
8fe98b8
Compare
d188b45 to
ebafb38
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1121
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
935ffcd to
51553f9
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1121
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
51553f9 to
f0b7ed3
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1121
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
f0b7ed3 to
3b5ec97
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1121
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
3b5ec97 to
2f43819
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1121
Scan targets checked: wolfssh-bugs, wolfssh-src
No new issues found in the changed files. ✅
#1120
ML-DA & Ed25519: If a private key is decoded, but public key is not exported, an error is logged.
ECDSA: If a private key lacks a derivable public key, a warning is output.