From 3cf9a6f33ae896b9aa43d7623488086107e95f3b Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 29 Oct 2026 14:19:04 -0700 Subject: [PATCH 1/2] ossh: reject a misaligned OpenSSH private section GetOpenSshKey checked the padding only when the private section was a whole number of blocks, so a misaligned section, or one ending in junk, loaded. It now rejects a section that is not a multiple of MIN_BLOCK_SZ, as OpenSSH does, and compares every pad byte before deciding. - tests: parse crafted Ed25519 keys with good, missing, junk and bad padding Issue: F-12551, F-13925 --- src/ossh.c | 34 +++++-------- tests/unit.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 153 insertions(+), 21 deletions(-) diff --git a/src/ossh.c b/src/ossh.c index 96f8316ee..795cac90f 100644 --- a/src/ossh.c +++ b/src/ossh.c @@ -670,28 +670,20 @@ int GetOpenSshKey(WS_KeySignature *key, /* key comment */ } } - /* Padding: Add increasing digits to pad to the nearest - * block size. Default block size is 8, but depends on - * the encryption algo. The private key chunk's length, - * and the length of the comment delimit the end of the - * encrypted blob. No added padding required. */ + /* Padding: the private section is a whole number of + * blocks (MIN_BLOCK_SZ for an unencrypted key), filled + * out with the bytes 1, 2, 3, ... Check every pad byte + * so the work depends only on the padding length. */ if (ret == WS_SUCCESS) { - if (strSz % MIN_BLOCK_SZ == 0) { - if (strSz > subIdx) { - /* The padding starts at 1. */ - check2 = strSz - subIdx; - for (check1 = 1; - check1 <= check2; - check1++, subIdx++) { - if (check1 != str[subIdx]) { - /* Bad pad: free key. */ - wolfSSH_KEY_clean(key); - key->keyId = ID_NONE; - ret = WS_KEY_FORMAT_E; - break; - } - } - } + byte padBad = (strSz % MIN_BLOCK_SZ != 0); + + for (check1 = 1; subIdx < strSz; check1++, subIdx++) + padBad |= (byte)check1 ^ str[subIdx]; + + if (padBad) { + wolfSSH_KEY_clean(key); + key->keyId = ID_NONE; + ret = WS_KEY_FORMAT_E; } } } diff --git a/tests/unit.c b/tests/unit.c index 4f078dd6a..0f3a9bbdb 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -1514,6 +1514,140 @@ static int test_OpenSshFormatNonCompositeRejected(void) #endif /* !WOLFSSH_NO_ECDSA_SHA2_NISTP256 */ +#if defined(WOLFSSH_TEST_INTERNAL) && !defined(WOLFSSH_NO_ED25519) +static word32 OsshTest_PutString(byte* out, const byte* s, word32 sSz) +{ + out[0] = (byte)(sSz >> 24); + out[1] = (byte)(sSz >> 16); + out[2] = (byte)(sSz >> 8); + out[3] = (byte)sSz; + if (sSz > 0) + WMEMCPY(out + UINT32_SZ, s, sSz); + return UINT32_SZ + sSz; +} + +/* Build an unencrypted openssh-key-v1 Ed25519 key. The private section + * is the checkints, the key and an empty comment (131 bytes) followed by + * tail, so the caller controls the padding. privSz shortens the k||ENC(A) + * string. Returns the blob size. */ +static word32 OsshTest_Ed25519Blob(byte* out, const byte* pub, + const byte* seed, word32 privSz, const byte* tail, word32 tailSz) +{ + static const char magic[] = "openssh-key-v1"; + static const char none[] = "none"; + static const char name[] = "ssh-ed25519"; + byte priv[ED25519_KEY_SIZE + ED25519_PUB_KEY_SIZE]; + byte pubBlob[UINT32_SZ + sizeof(name) - 1 + UINT32_SZ + + ED25519_PUB_KEY_SIZE]; + byte sect[256]; + word32 sectSz = 0, pubBlobSz = 0, sz = 0; + + WMEMCPY(priv, seed, ED25519_KEY_SIZE); + WMEMCPY(priv + ED25519_KEY_SIZE, pub, ED25519_PUB_KEY_SIZE); + + pubBlobSz += OsshTest_PutString(pubBlob, (const byte*)name, + sizeof(name) - 1); + pubBlobSz += OsshTest_PutString(pubBlob + pubBlobSz, pub, + ED25519_PUB_KEY_SIZE); + + WMEMSET(sect, 0x5A, 2 * UINT32_SZ); /* checkint1 == checkint2 */ + sectSz = 2 * UINT32_SZ; + sectSz += OsshTest_PutString(sect + sectSz, (const byte*)name, + sizeof(name) - 1); + sectSz += OsshTest_PutString(sect + sectSz, pub, ED25519_PUB_KEY_SIZE); + sectSz += OsshTest_PutString(sect + sectSz, priv, privSz); + sectSz += OsshTest_PutString(sect + sectSz, NULL, 0); /* comment */ + if (tailSz > 0) + WMEMCPY(sect + sectSz, tail, tailSz); + sectSz += tailSz; + + WMEMCPY(out, magic, sizeof(magic)); + sz = sizeof(magic); + sz += OsshTest_PutString(out + sz, (const byte*)none, sizeof(none) - 1); + sz += OsshTest_PutString(out + sz, (const byte*)none, sizeof(none) - 1); + sz += OsshTest_PutString(out + sz, NULL, 0); /* kdfoptions */ + out[sz++] = 0; out[sz++] = 0; out[sz++] = 0; out[sz++] = 1; + sz += OsshTest_PutString(out + sz, pubBlob, pubBlobSz); + sz += OsshTest_PutString(out + sz, sect, sectSz); + return sz; +} + +/* GetOpenSshKey on Ed25519 keys: the private section must be a whole + * number of blocks ending in the 1, 2, 3, ... padding. */ +static int test_OpenSshKeyEd25519Parse(void) +{ + static const byte seed[ED25519_KEY_SIZE] = { + 0x9d, 0x61, 0xb1, 0x9d, 0xef, 0xfd, 0x5a, 0x60, + 0xba, 0x84, 0x4a, 0xf4, 0x92, 0xec, 0x2c, 0xc4, + 0x44, 0x49, 0xc5, 0x69, 0x7b, 0x32, 0x69, 0x19, + 0x70, 0x3b, 0xac, 0x03, 0x1c, 0xae, 0x7f, 0x60 + }; + static const byte padGood[] = { 1, 2, 3, 4, 5 }; + static const byte padLong[] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13 }; + static const byte padBad2[] = { 1, 9, 3, 4, 5 }; + static const byte padBad5[] = { 1, 2, 3, 4, 9 }; + static const byte junk3[] = { 0xFF, 0xFF, 0xFF }; + static const struct { + const char* name; + const byte* tail; + word32 tailSz; + word32 privSz; + int expect; + } cases[] = { + { "aligned, good pad", padGood, sizeof(padGood), + ED25519_KEY_SIZE + ED25519_PUB_KEY_SIZE, WS_SUCCESS }, + { "aligned, an extra block of pad", padLong, sizeof(padLong), + ED25519_KEY_SIZE + ED25519_PUB_KEY_SIZE, WS_SUCCESS }, + { "misaligned, no pad", NULL, 0, + ED25519_KEY_SIZE + ED25519_PUB_KEY_SIZE, WS_KEY_FORMAT_E }, + { "misaligned, junk tail", junk3, sizeof(junk3), + ED25519_KEY_SIZE + ED25519_PUB_KEY_SIZE, WS_KEY_FORMAT_E }, + { "aligned, bad second pad byte", padBad2, sizeof(padBad2), + ED25519_KEY_SIZE + ED25519_PUB_KEY_SIZE, WS_KEY_FORMAT_E }, + { "aligned, bad last pad byte", padBad5, sizeof(padBad5), + ED25519_KEY_SIZE + ED25519_PUB_KEY_SIZE, WS_KEY_FORMAT_E }, + }; + ed25519_key edKey; + byte pub[ED25519_PUB_KEY_SIZE]; + byte blob[512]; + WS_KeySignature keySig; + word32 blobSz, idx, i; + int ret, result = 0; + + if (wc_ed25519_init(&edKey) != 0) + return -1720; + ret = wc_ed25519_import_private_only(seed, sizeof(seed), &edKey); + if (ret == 0) + ret = wc_ed25519_make_public(&edKey, pub, sizeof(pub)); + wc_ed25519_free(&edKey); + if (ret != 0) + return -1721; + + for (i = 0; i < (word32)(sizeof(cases) / sizeof(cases[0])); i++) { + blobSz = OsshTest_Ed25519Blob(blob, pub, seed, cases[i].privSz, + cases[i].tail, cases[i].tailSz); + WMEMSET(&keySig, 0, sizeof(keySig)); + keySig.keyId = ID_NONE; + idx = 0; + ret = GetOpenSshKey(&keySig, blob, blobSz, &idx); + if (ret != cases[i].expect) { + printf("OpenSshKeyEd25519Parse: %s: ret %d, expected %d\n", + cases[i].name, ret, cases[i].expect); + result = -1722; + } + else if (ret == WS_SUCCESS && keySig.keyId != ID_ED25519) { + printf("OpenSshKeyEd25519Parse: %s: keyId %d\n", + cases[i].name, keySig.keyId); + result = -1723; + } + wolfSSH_KEY_clean(&keySig); + } + + return result; +} +#endif /* WOLFSSH_TEST_INTERNAL && !WOLFSSH_NO_ED25519 */ + + #if defined(WOLFSSH_TEST_INTERNAL) && \ (!defined(WOLFSSH_NO_HMAC_SHA1) || \ !defined(WOLFSSH_NO_HMAC_SHA1_96) || \ @@ -27125,6 +27259,12 @@ int wolfSSH_UnitTest(int argc, char** argv) (unitResult == 0 ? "SUCCESS" : "FAILED")); testResult = testResult || unitResult; #endif +#if defined(WOLFSSH_TEST_INTERNAL) && !defined(WOLFSSH_NO_ED25519) + unitResult = test_OpenSshKeyEd25519Parse(); + printf("OpenSshKeyEd25519Parse: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; +#endif #if defined(WOLFSSH_TEST_INTERNAL) && !defined(NO_WOLFSSH_SERVER) unitResult = test_ShutdownPeerChannelId(); From 8a314ad369b612668736460bfba81735c77945fe Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 29 Oct 2026 14:19:54 -0700 Subject: [PATCH 2/2] ossh: keep decoder errors and the caller's heap GetOpenSshKeyEd25519 now returns its parse errors as they are and reports a wolfCrypt failure as WS_ED25519_E instead of WS_ECC_E. The OpenSSH key decoders take the WS_KeySignature heap and initialize the key with it, where they used NULL or the zeroed key's own heap field. - tests: check the Ed25519 decoder errors and the heap of a decoded key Issue: F-11653, F-11656 --- src/ossh.c | 60 +++++++++++++++++++++++++++------------------------- tests/unit.c | 48 +++++++++++++++++++++++++++++++++++++---- 2 files changed, 75 insertions(+), 33 deletions(-) diff --git a/src/ossh.c b/src/ossh.c index 795cac90f..9945897ef 100644 --- a/src/ossh.c +++ b/src/ossh.c @@ -72,14 +72,14 @@ /* * Utility for GetOpenSshKey() to read in RSA keys. */ -static int GetOpenSshKeyRsa(RsaKey* key, +static int GetOpenSshKeyRsa(RsaKey* key, void* heap, const byte* buf, word32 len, word32* idx) { const byte *n, *e, *d, *u, *p, *q; word32 nSz, eSz, dSz, uSz, pSz, qSz; int ret; - ret = wc_InitRsaKey(key, NULL); + ret = wc_InitRsaKey(key, heap); if (ret == WS_SUCCESS) ret = GetMpint(&nSz, &n, buf, len, idx); if (ret == WS_SUCCESS) @@ -130,12 +130,12 @@ static int GetMpintToMp(mp_int* mp, /* * Utility for GetOpenSshKey() to read in RSA keys. */ -static int GetOpenSshKeyRsa(RsaKey* key, +static int GetOpenSshKeyRsa(RsaKey* key, void* heap, const byte* buf, word32 len, word32* idx) { int ret; - ret = wc_InitRsaKey(key, NULL); + ret = wc_InitRsaKey(key, heap); if (ret == WS_SUCCESS) ret = GetMpintToMp(&key->n, buf, len, idx); if (ret == WS_SUCCESS) @@ -176,14 +176,14 @@ static int GetOpenSshKeyRsa(RsaKey* key, /* * Utility for GetOpenSshKey() to read in ECDSA keys. */ -static int GetOpenSshKeyEcc(ecc_key* key, +static int GetOpenSshKeyEcc(ecc_key* key, void* heap, const byte* buf, word32 len, word32* idx) { const byte *name = NULL, *priv = NULL, *pub = NULL; word32 nameSz = 0, privSz = 0, pubSz = 0; int ret; - ret = wc_ecc_init(key); + ret = wc_ecc_init_ex(key, heap, INVALID_DEVID); if (ret == WS_SUCCESS) ret = GetStringRef(&nameSz, &name, buf, len, idx); /* curve name */ if (ret == WS_SUCCESS) @@ -206,14 +206,16 @@ static int GetOpenSshKeyEcc(ecc_key* key, /* * Utility for GetOpenSshKey() to read in Ed25519 keys. */ -static int GetOpenSshKeyEd25519(ed25519_key* key, +static int GetOpenSshKeyEd25519(ed25519_key* key, void* heap, const byte* buf, word32 len, word32* idx) { const byte *priv = NULL, *pub = NULL; word32 privSz = 0, pubSz = 0; int ret; - ret = wc_ed25519_init_ex(key, key->heap, INVALID_DEVID); + ret = wc_ed25519_init_ex(key, heap, INVALID_DEVID); + if (ret != 0) + return WS_ED25519_E; /* OpenSSH key formatting stores the public key, ENC(A), and the * private key (k) concatenated with the public key, k || ENC(A). */ @@ -225,12 +227,12 @@ static int GetOpenSshKeyEd25519(ed25519_key* key, if (privSz < pubSz) ret = WS_KEY_FORMAT_E; - if (ret == WS_SUCCESS) + if (ret == WS_SUCCESS) { ret = wc_ed25519_import_private_key(priv, privSz - pubSz, pub, pubSz, key); - - if (ret != WS_SUCCESS) - ret = WS_ECC_E; + if (ret != 0) + ret = WS_ED25519_E; + } return ret; } @@ -238,7 +240,7 @@ static int GetOpenSshKeyEd25519(ed25519_key* key, #ifndef WOLFSSH_NO_MLDSA /* Parse OpenSSH ML-DSA private key blob; see GetOpenSshKeyPublicMlDsa. */ -static int GetOpenSshKeyMlDsa(MlDsaKey* key, +static int GetOpenSshKeyMlDsa(MlDsaKey* key, void* heap, const byte* buf, word32 len, word32* idx, byte level) { const byte *pub = NULL; @@ -247,7 +249,7 @@ static int GetOpenSshKeyMlDsa(MlDsaKey* key, word32 privSz = 0; int ret; - ret = wc_MlDsaKey_Init(key, key->heap, INVALID_DEVID); + ret = wc_MlDsaKey_Init(key, heap, INVALID_DEVID); if (ret == 0) { ret = wc_MlDsaKey_SetParams(key, level); if (ret != 0) { @@ -401,14 +403,14 @@ static int GetOpenSshKeyPublicEd25519(ed25519_key* key, const byte* buf, } #endif #ifndef WOLFSSH_NO_MLDSA -static int GetOpenSshKeyPublicMlDsa(MlDsaKey* key, const byte* buf, - word32 len, word32* idx, byte level) +static int GetOpenSshKeyPublicMlDsa(MlDsaKey* key, void* heap, + const byte* buf, word32 len, word32* idx, byte level) { int ret; const byte* pub = NULL; word32 pubSz = 0; - ret = wc_MlDsaKey_Init(key, key->heap, INVALID_DEVID); + ret = wc_MlDsaKey_Init(key, heap, INVALID_DEVID); if (ret == 0) { ret = wc_MlDsaKey_SetParams(key, level); if (ret != 0) { @@ -499,16 +501,16 @@ int GetOpenSshPublicKey(WS_KeySignature *key, #endif #ifndef WOLFSSH_NO_MLDSA case ID_MLDSA44: - ret = GetOpenSshKeyPublicMlDsa(&key->ks.mldsa.key, buf, len, - idx, WC_ML_DSA_44); + ret = GetOpenSshKeyPublicMlDsa(&key->ks.mldsa.key, + key->heap, buf, len, idx, WC_ML_DSA_44); break; case ID_MLDSA65: - ret = GetOpenSshKeyPublicMlDsa(&key->ks.mldsa.key, buf, len, - idx, WC_ML_DSA_65); + ret = GetOpenSshKeyPublicMlDsa(&key->ks.mldsa.key, + key->heap, buf, len, idx, WC_ML_DSA_65); break; case ID_MLDSA87: - ret = GetOpenSshKeyPublicMlDsa(&key->ks.mldsa.key, buf, len, - idx, WC_ML_DSA_87); + ret = GetOpenSshKeyPublicMlDsa(&key->ks.mldsa.key, + key->heap, buf, len, idx, WC_ML_DSA_87); break; #endif default: @@ -600,7 +602,7 @@ int GetOpenSshKey(WS_KeySignature *key, #ifndef WOLFSSH_NO_RSA case ID_SSH_RSA: ret = GetOpenSshKeyRsa(&key->ks.rsa.key, - str, strSz, &subIdx); + key->heap, str, strSz, &subIdx); break; #endif #ifndef WOLFSSH_NO_ECDSA @@ -608,19 +610,19 @@ int GetOpenSshKey(WS_KeySignature *key, case ID_ECDSA_SHA2_NISTP384: case ID_ECDSA_SHA2_NISTP521: ret = GetOpenSshKeyEcc(&key->ks.ecc.key, - str, strSz, &subIdx); + key->heap, str, strSz, &subIdx); break; #endif #ifndef WOLFSSH_NO_ED25519 case ID_ED25519: ret = GetOpenSshKeyEd25519(&key->ks.ed25519.key, - str, strSz, &subIdx); + key->heap, str, strSz, &subIdx); break; #endif #ifndef WOLFSSH_NO_MLDSA case ID_MLDSA44: ret = GetOpenSshKeyMlDsa( - &key->ks.mldsa.key, + &key->ks.mldsa.key, key->heap, str, strSz, &subIdx, WC_ML_DSA_44); /* Avoid double free. */ if (ret != WS_SUCCESS) @@ -628,14 +630,14 @@ int GetOpenSshKey(WS_KeySignature *key, break; case ID_MLDSA65: ret = GetOpenSshKeyMlDsa( - &key->ks.mldsa.key, + &key->ks.mldsa.key, key->heap, str, strSz, &subIdx, WC_ML_DSA_65); if (ret != WS_SUCCESS) key->keyId = ID_NONE; break; case ID_MLDSA87: ret = GetOpenSshKeyMlDsa( - &key->ks.mldsa.key, + &key->ks.mldsa.key, key->heap, str, strSz, &subIdx, WC_ML_DSA_87); if (ret != WS_SUCCESS) key->keyId = ID_NONE; diff --git a/tests/unit.c b/tests/unit.c index 0f3a9bbdb..d8647b4df 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -1573,7 +1573,8 @@ static word32 OsshTest_Ed25519Blob(byte* out, const byte* pub, } /* GetOpenSshKey on Ed25519 keys: the private section must be a whole - * number of blocks ending in the 1, 2, 3, ... padding. */ + * number of blocks ending in the 1, 2, 3, ... padding, a malformed key + * reports WS_KEY_FORMAT_E, and the key is set up on the caller's heap. */ static int test_OpenSshKeyEd25519Parse(void) { static const byte seed[ED25519_KEY_SIZE] = { @@ -1606,11 +1607,24 @@ static int test_OpenSshKeyEd25519Parse(void) ED25519_KEY_SIZE + ED25519_PUB_KEY_SIZE, WS_KEY_FORMAT_E }, { "aligned, bad last pad byte", padBad5, sizeof(padBad5), ED25519_KEY_SIZE + ED25519_PUB_KEY_SIZE, WS_KEY_FORMAT_E }, + /* 131 - 48 + 5 = 88 is aligned, so only privSz < pubSz fails */ + { "private string shorter than public", padGood, sizeof(padGood), + ED25519_KEY_SIZE + ED25519_PUB_KEY_SIZE - 48, WS_KEY_FORMAT_E }, + /* 131 - 8 + 5 = 128 is aligned; wolfCrypt rejects a 24-byte k */ + { "private key the wrong size", padGood, sizeof(padGood), + ED25519_KEY_SIZE + ED25519_PUB_KEY_SIZE - 8, WS_ED25519_E }, }; ed25519_key edKey; byte pub[ED25519_PUB_KEY_SIZE]; byte blob[512]; WS_KeySignature keySig; +#ifndef WOLFSSL_STATIC_MEMORY + /* Any non-NULL marker; a static-memory heap hint must be real. */ + int heapMark = 0; + void* heap = &heapMark; +#else + void* heap = NULL; +#endif word32 blobSz, idx, i; int ret, result = 0; @@ -1627,6 +1641,7 @@ static int test_OpenSshKeyEd25519Parse(void) blobSz = OsshTest_Ed25519Blob(blob, pub, seed, cases[i].privSz, cases[i].tail, cases[i].tailSz); WMEMSET(&keySig, 0, sizeof(keySig)); + keySig.heap = heap; keySig.keyId = ID_NONE; idx = 0; ret = GetOpenSshKey(&keySig, blob, blobSz, &idx); @@ -1635,14 +1650,39 @@ static int test_OpenSshKeyEd25519Parse(void) cases[i].name, ret, cases[i].expect); result = -1722; } - else if (ret == WS_SUCCESS && keySig.keyId != ID_ED25519) { - printf("OpenSshKeyEd25519Parse: %s: keyId %d\n", - cases[i].name, keySig.keyId); + else if (ret == WS_SUCCESS + && (keySig.keyId != ID_ED25519 + || keySig.ks.ed25519.key.heap != keySig.heap)) { + printf("OpenSshKeyEd25519Parse: %s: keyId %d, key heap is %s\n", + cases[i].name, keySig.keyId, + keySig.ks.ed25519.key.heap == keySig.heap ? + "the caller's" : "not the caller's"); result = -1723; } wolfSSH_KEY_clean(&keySig); } + /* Cut the 131-byte private section to 100 bytes, inside the k||ENC(A) + * string, so reading that string runs out of buffer. */ + blobSz = OsshTest_Ed25519Blob(blob, pub, seed, + ED25519_KEY_SIZE + ED25519_PUB_KEY_SIZE, NULL, 0); + blobSz -= 131 - 100; + blob[blobSz - 100 - 4] = 0; + blob[blobSz - 100 - 3] = 0; + blob[blobSz - 100 - 2] = 0; + blob[blobSz - 100 - 1] = 100; + WMEMSET(&keySig, 0, sizeof(keySig)); + keySig.heap = heap; + keySig.keyId = ID_NONE; + idx = 0; + ret = GetOpenSshKey(&keySig, blob, blobSz, &idx); + if (ret != WS_BUFFER_E) { + printf("OpenSshKeyEd25519Parse: truncated private string: ret %d, " + "expected %d\n", ret, WS_BUFFER_E); + result = -1724; + } + wolfSSH_KEY_clean(&keySig); + return result; } #endif /* WOLFSSH_TEST_INTERNAL && !WOLFSSH_NO_ED25519 */