Dispatch DoAsn1Key public keys on key type - #1137
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes incorrect ASN.1 public-key dispatch in DoAsn1Key() so non-RSA public keys (ECDSA/Ed25519 and ML-DSA under some build flags) no longer fall through the RSA encoding path and produce malformed/unsafe public-key blobs.
Changes:
- Dispatch ASN.1 public-key encoding by identified key ID (RSA vs ECDSA vs Ed25519 vs ML-DSA), and fail closed with
WS_UNIMPLEMENTED_Efor unsupported types. - Add unit tests validating ASN.1 SPKI → SSH public-key blob conversion for RSA, ECDSA P-256/384/521, and Ed25519.
- Adjust the public-key path to set
outType/outTypeSzconsistently before encoding.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/ssh.c |
Dispatch ASN.1 public-key encoding by key type and add ECDSA/Ed25519 handling; fail closed for unsupported types. |
tests/unit.c |
Add regression/unit tests for reading ASN.1 public keys into SSH-wire blobs for multiple algorithms. |
Suppressed comments (4)
src/ssh.c:2014
- The returned buffer from wolfSSH_ReadPublicKey_buffer() is typically allocated/freed as DYNTYPE_PRIVKEY in this codebase (see DoSshPubKey() and examples/client/common.c). This ECDSA ASN.1 path allocates the output blob with DYNTYPE_PUBKEY, which can cause mismatched free/memory tracking for callers following the existing convention.
*outSz = LENGTH_SZ + *outTypeSz + LENGTH_SZ + curveNameSz +
LENGTH_SZ + qSz;
newKey = (byte*)WMALLOC(*outSz, heap, DYNTYPE_PUBKEY);
if (newKey == NULL) {
tests/unit.c:9479
- wolfSSH_ReadPublicKey_buffer() output buffers are freed as DYNTYPE_PRIVKEY elsewhere in-tree (e.g., examples/client/common.c). Freeing this blob with DYNTYPE_PUBKEY can mismatch the allocator tag (and will also mismatch if the implementation allocates with DYNTYPE_PRIVKEY as other key-read paths do).
if (blob != NULL) {
WFREE(blob, NULL, DYNTYPE_PUBKEY);
}
tests/unit.c:9579
- wolfSSH_ReadPublicKey_buffer() output buffers are freed as DYNTYPE_PRIVKEY elsewhere in-tree (e.g., examples/client/common.c). Freeing this blob with DYNTYPE_PUBKEY can mismatch the allocator tag (and will also mismatch if the implementation allocates with DYNTYPE_PRIVKEY as other key-read paths do).
if (blob != NULL) {
WFREE(blob, NULL, DYNTYPE_PUBKEY);
}
src/ssh.c:2052
- The returned buffer from wolfSSH_ReadPublicKey_buffer() is typically allocated/freed as DYNTYPE_PRIVKEY in this codebase (see DoSshPubKey() and examples/client/common.c). This Ed25519 ASN.1 path allocates the output blob with DYNTYPE_PUBKEY, which can cause mismatched free/memory tracking for callers following the existing convention.
*outSz = LENGTH_SZ + *outTypeSz + LENGTH_SZ + qSz;
newKey = (byte*)WMALLOC(*outSz, heap, DYNTYPE_PUBKEY);
if (newKey == NULL) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0bda802 to
7573588
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1137
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
7573588 to
5300252
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1137
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.
Problem
DoAsn1Key()'s public-key branch had exactly two arms: an ML-DSA arm gated on an explicit key-ID test, and an unconditionalelsethat assumed RSA. ButIdentifyAsn1Key(in, inSz, 0, ...)also returnsID_ECDSA_SHA2_NISTP256/384/521andID_ED25519, which decode into theecc_keyanded25519_keymembers of theWS_KeySignature.ksunion.Those IDs fell into the RSA arm and reached
wc_RsaFlattenPublicKey(&key->ks.rsa.key, ...), walkingRsaKey.n/RsaKey.emp_intfields that overlay unrelated members of a different key type. Observed behavior: the call returnedWS_SUCCESSwith*outTypeset toecdsa-sha2-nistp256but a blob whose first string wasssh-rsa, built from unrelated memory. The encode/decode roundtrip cannot hold, and the read is out of the caller's control.Reachable through the public API
wolfSSH_ReadPublicKey_buffer(). The in-tree callerwolfSSH_TPM_InitKey()(examples/client/common.c) feeds a TPM public key exported as ASN.1, so a TPM-resident ECC key trips this.Fix (
src/ssh.c)Hoisted the
*outType/*outTypeSzassignment above the chain, gated the RSA arm on its own ID, and added the missing arms. Unknown types now fail closed withWS_UNIMPLEMENTED_Einstead of falling through to RSA.ID_SSH_RSAssh-rsa,e,n(unchanged)ID_ECDSA_SHA2_NISTP256/384/521ecdsa-sha2-nistpXXX,nistpXXX,Qviawc_ecc_export_x963()ID_ED25519ssh-ed25519,Aviawc_ed25519_export_public()ID_MLDSA44/65/87WS_UNIMPLEMENTED_EThe fallback also closes a second instance of the same defect:
IdentifyAsn1KeyassignsID_MLDSA44/65/87without per-level guards, while the ML-DSA arm is per-level guarded, so in aWOLFSSH_NO_MLDSA44build such a key previously hit the RSA arm too.Closes f-7504.
Tests (
tests/unit.c)test_ReadPublicKeyAsn1()covers RSA, ECDSA P-256/384/521, Ed25519 and ML-DSA 44/65/87. Each case reads a public key back throughwolfSSH_ReadPublicKey_buffer()and checksoutTypeplus every blob string against the key's own exported value, requiring the blob to be exactly consumed — content, not just framing. The RSA case reproduces the leading zero byteDoAsn1Keyprepends when the modulus MSB is set.Mutation-tested rather than assumed: corrupting a modulus byte, forcing
nMsb = 0, and corrupting the ML-DSA raw key are each caught by the corresponding case. The whole test fails before the fix.Verification
make check: 10 pass, 1 skip (external.test, needs network), 0 fail, in both a standard build and one against wolfSSL--enable-mldsa.--enable-smallstack.gcc-13 -Werror.src/ssh.ccompiles-Werrorclean across all eight on/off combinations ofWOLFSSH_NO_RSA/ECDSA/ED25519, and with ML-DSA compiled in.tests/unit.cwas checked over the same matrix and is clean for every configuration it supports, including an ML-DSA-only build with the other three key types disabled.Reviewer notes
Two points have been raised repeatedly by automated review. The rationale is recorded here so it does not need re-deriving.
The
WS_UNIMPLEMENTED_Efallback cannot be reached by any input.DoAsn1Keyguards the entire dispatch chain onret > 0. EverykeyIdthatIdentifyAsn1Keycan assign (ID_SSH_RSA, the three ECDSA curves, the three ML-DSA levels,ID_ED25519) has a matching arm, and anything unrecognised returns a negative error, so the block including the fallback is skipped.This was checked, not assumed. Feeding an ECC SPKI whose curve OID is an unsupported curve returns
WS_UNIMPLEMENTED_Ewithout == NULL; changing the fallback to return a different code and rebuilding yields the same result, proving the line never executes. Any test written against it would pass identically with theelsedeleted. The only configuration that can execute it isWOLFSSH_NO_MLDSA44(or the 65/87 equivalents) with ML-DSA otherwise enabled, reaching it through the unguarded level mapping described below, and CI does not build that combination. The branch is deliberate defence in depth against future drift between the two functions.Public-key blobs are allocated with
DYNTYPE_PRIVKEY. That reads oddly for a public key, but it matches the callers:examples/client/common.candapps/wolfssh/common.cboth freewolfSSH_ReadPublicKey_buffer()output withDYNTYPE_PRIVKEY, as doDoSshPubKey,DoPemKey,DoOpenSshKeyand the RSA arm. The tag is advisory rather than functional: withoutWOLFSSL_STATIC_MEMORYwolfSSL discards it before any custom allocator callback sees it, and with static memory the free path selects its bucket by size, consulting the type only for the IO-pool types, which wolfSSH's500-based enum can never collide with. Aligning the tag is a consistency fix, not a routing fix.Not in this PR
The unguarded
ID_MLDSA44/65/87assignment inIdentifyAsn1Keyreports a key type the build has disabled. This change makes it fail closed rather than confuse types, but the ID arguably should not be produced at all. Left for a separate finding.