From 757f866fd889f3b5c3a001b0f3737d270bed15a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Frauenschl=C3=A4ger?= Date: Mon, 3 Aug 2026 12:22:35 +0200 Subject: [PATCH 1/8] Fix buffer overrun in SignCert when sizing the signature wrapper SignCert() checked the output buffer with requestSz + MAX_SEQ_SZ * 2 + sigSz > buffSz before handing the buffer to AddSignature(). That accounts for the outer SEQUENCE header but not for the signatureAlgorithm AlgorithmIdentifier (OID plus optional NULL parameters) or the signatureValue BIT STRING header that AddSignature() also writes, an under-count of about 13 bytes. AddSignature() takes no buffer size of its own, so any certificate whose final encoding lands in that narrow band just under buffSz passed the check and was written past the end of the buffer. The same estimate was used in wc_SignCert_cb(). Both call sites now ask AddSignature() for the exact encoding size by passing a NULL buffer first, then compare that against buffSz. This is the two-pass idiom already used when signing CRLs in SignCrl(), in wolfssl_x509_make_der() and in wolfSSL_X509_CRL_sign(). Both the template and the original ASN.1 encoders support the NULL buffer sizing call. The comparison is made unsigned, matching the pre-flight in SignCrl(). Casting buffSz to int made a buffer larger than INT_MAX compare negative and rejected every signature for it. Both functions also now bound requestSz against buffSz up front. MakeSignature() and MakeSignatureCb() hash requestSz bytes out of buf before any size check runs, so a caller passing the two mismatched got an out of bounds read of up to requestSz - buffSz bytes before the function returned. Only a negative requestSz was rejected before. Reaching this needs the application to pass values that disagree, so it is API misuse rather than attacker controlled input, but the read side now carries the same guarantee as the write side. Reachable from the OpenSSL compatibility layer through wolfSSL_X509_sign() and wolfSSL_X509_REQ_sign(), where the caller controls the certificate contents that steer the encoded size into the band. Adds test_wc_SignCert_buffer_bounds(), which signs into buffers sized across the band below the exact encoding size and requires BUFFER_E and an untouched guard region for each, while still accepting the exact size. test_wc_SignCert_cb() gains the same check for the callback entry point, using its RSA half where the PKCS#1 v1.5 signature is fixed length, in both directions so that an over-conservative estimate is caught too. The bounds test covers ECDSA as well as RSA. IsSigAlgoNoParams() drops the NULL parameters from the AlgorithmIdentifier, so the width an estimate under-counts by differs between the two: 24 bytes of wrapper against the 12 byte estimate for RSA, but only 19 for ECDSA, putting the capacities that used to be accepted and overrun within 8 bytes of the exact size. An ECDSA encoding size cannot be measured once and reused, because the DER INTEGERs holding r and s change length with the leading zero bytes of each new signature. The sweep measures a fresh reference size every iteration and, rather than requiring BUFFER_E for a capacity that the next signature might genuinely fit, asserts what has to hold either way: the call returns BUFFER_E or a size within the capacity, and the guard region past the capacity is untouched. That covers the whole band instead of trading it away for a margin wide enough to absorb the jitter. The prerequisites are split into one condition macro per algorithm rather than one shared list. Gating the whole test on the RSA prerequisites would have compiled the ECDSA sweep out of a build without RSA, which is exactly where it is the only coverage that exists. Both tests set an explicit serial number. wc_InitCert() leaves serialSz at zero, so wc_MakeCert() generates a random serial, and GenerateInteger() does not shrink the length after dropping leading zero bytes, which lets the promoted byte carry the MSB and makes the encoder pad the INTEGER with an extra 0x00. Measured over 200000 generated bodies, 813 of them, 0.406 percent, came out one byte longer, which would have made the swept capacities disagree with the reference size for roughly one run in 128. --- tests/api.c | 57 ++++++++- tests/api/test_asn.c | 270 +++++++++++++++++++++++++++++++++++++++++++ tests/api/test_asn.h | 2 + wolfcrypt/src/asn.c | 35 ++++-- 4 files changed, 351 insertions(+), 13 deletions(-) diff --git a/tests/api.c b/tests/api.c index d13c0ff0147..06944507470 100644 --- a/tests/api.c +++ b/tests/api.c @@ -25740,11 +25740,14 @@ static int mockSignCbError(const byte* in, word32 inLen, byte* out, } #endif -#ifdef WOLFSSL_CERT_SIGN_CB +/* Bytes kept past the advertised capacity to catch a write past the end. */ +#define SIGN_CERT_CB_GUARD_SZ 16 + static int test_wc_SignCert_cb(void) { EXPECT_DECLS; -#if defined(WOLFSSL_CERT_GEN) && !defined(NO_ASN_TIME) +#if defined(WOLFSSL_CERT_SIGN_CB) && defined(WOLFSSL_CERT_GEN) && \ + !defined(NO_ASN_TIME) #ifdef HAVE_ECC /* Test with ECC key */ @@ -25851,6 +25854,10 @@ static int test_wc_SignCert_cb(void) MockSignCtx signCtx; DecodedCert decodedCert; int ret; + int bodySz = 0; + int exactSz = 0; + int i; + static const byte fixedSerial[] = { 0x01, 0x02, 0x03, 0x04 }; XMEMSET(&rng, 0, sizeof(WC_RNG)); XMEMSET(&key, 0, sizeof(RsaKey)); @@ -25913,6 +25920,45 @@ static int test_wc_SignCert_cb(void) /* Callback returning error */ ExpectIntEQ(wc_SignCert_cb(cert.bodySz, cert.sigType, der, FOURK_BUF, RSA_TYPE, mockSignCbError, &signCtx, &rng), BAD_STATE_E); + + /* Buffer bounds. wc_SignCert_cb() appends the signatureAlgorithm and + * signatureValue as well as the outer SEQUENCE, so a capacity below + * the exact encoding size has to be rejected rather than overrun. The + * serial is fixed so both rebuilt bodies encode identically. */ + XMEMCPY(cert.serial, fixedSerial, sizeof(fixedSerial)); + cert.serialSz = (int)sizeof(fixedSerial); + ExpectIntGT(bodySz = wc_MakeCert(&cert, der, FOURK_BUF, &key, NULL, + &rng), 0); + ExpectIntGT(exactSz = wc_SignCert_cb(bodySz, cert.sigType, der, + FOURK_BUF, RSA_TYPE, mockSignCb, &signCtx, &rng), 0); + /* The guard region below is written past the advertised capacity, so + * it has to stay inside der. */ + ExpectIntLT(exactSz + SIGN_CERT_CB_GUARD_SZ, FOURK_BUF); + + ExpectIntGT(bodySz = wc_MakeCert(&cert, der, FOURK_BUF, &key, NULL, + &rng), 0); + if (EXPECT_SUCCESS()) { + XMEMSET(der + exactSz - 1, 0xA5, SIGN_CERT_CB_GUARD_SZ); + } + ExpectIntEQ(wc_SignCert_cb(bodySz, cert.sigType, der, + (word32)(exactSz - 1), RSA_TYPE, mockSignCb, &signCtx, &rng), + WC_NO_ERR_TRACE(BUFFER_E)); + for (i = 0; i < SIGN_CERT_CB_GUARD_SZ; i++) { + ExpectIntEQ(der[exactSz - 1 + i], 0xA5); + } + + /* The check must not be over-conservative either, so require the exact + * size to be accepted and the bytes above it left alone. */ + ExpectIntGT(bodySz = wc_MakeCert(&cert, der, FOURK_BUF, &key, NULL, + &rng), 0); + if (EXPECT_SUCCESS()) { + XMEMSET(der + exactSz, 0xA5, SIGN_CERT_CB_GUARD_SZ); + } + ExpectIntEQ(wc_SignCert_cb(bodySz, cert.sigType, der, + (word32)exactSz, RSA_TYPE, mockSignCb, &signCtx, &rng), exactSz); + for (i = 0; i < SIGN_CERT_CB_GUARD_SZ; i++) { + ExpectIntEQ(der[exactSz + i], 0xA5); + } #ifdef HAVE_ECC /* Invalid keyType */ ExpectIntEQ(wc_SignCert_cb(cert.bodySz, cert.sigType, der, @@ -25931,10 +25977,11 @@ static int test_wc_SignCert_cb(void) } #endif /* !NO_RSA && WOLFSSL_KEY_GEN */ -#endif /* WOLFSSL_CERT_GEN && !NO_ASN_TIME */ +#endif /* WOLFSSL_CERT_SIGN_CB && WOLFSSL_CERT_GEN && !NO_ASN_TIME */ return EXPECT_RESULT(); } -#endif /* WOLFSSL_CERT_SIGN_CB */ + +#undef SIGN_CERT_CB_GUARD_SZ static int test_ERR_load_crypto_strings(void) { @@ -38766,9 +38813,7 @@ TEST_CASE testCases[] = { TEST_DECL(test_NameConstraints_SubtreeMinMax), TEST_DECL(test_ParseSerial0FixtureMatrix), TEST_DECL(test_MakeCertWithCaFalse), -#ifdef WOLFSSL_CERT_SIGN_CB TEST_DECL(test_wc_SignCert_cb), -#endif TEST_DECL(test_wc_SetAcmeIdentifierExt), TEST_DECL(test_wc_SetKeyUsage), TEST_DECL(test_wc_SetAuthKeyIdFromPublicKey_ex), diff --git a/tests/api/test_asn.c b/tests/api/test_asn.c index c42124a458e..33ad775ad16 100644 --- a/tests/api/test_asn.c +++ b/tests/api/test_asn.c @@ -2518,6 +2518,276 @@ int test_ToTraditional_ex_mldsa_bad_params(void) return EXPECT_RESULT(); } +/* What wc_SignCert() bounds testing needs regardless of the signing algorithm. + * Each algorithm then gates on its own key type and certificate buffers, so an + * RSA-less build still gets the ECDSA sweep and vice versa. */ +#if defined(WOLFSSL_CERT_GEN) && !defined(NO_SHA256) && !defined(WC_NO_RNG) && \ + !defined(NO_ASN_TIME) && !defined(NO_ASN_CRYPT) + #if !defined(NO_RSA) && defined(USE_CERT_BUFFERS_2048) + #define TEST_SIGN_CERT_BOUNDS_RSA + #endif + #if defined(HAVE_ECC) && defined(USE_CERT_BUFFERS_256) + #define TEST_SIGN_CERT_BOUNDS_ECC + #endif +#endif + +#if defined(TEST_SIGN_CERT_BOUNDS_RSA) || defined(TEST_SIGN_CERT_BOUNDS_ECC) + +#define SIGN_CERT_SCRATCH_SZ 4096 +/* Number of capacities below the exact encoding size to try. Has to be wider + * than the AlgorithmIdentifier plus BIT STRING header that a sequence-headers + * only estimate leaves out. */ +#define SIGN_CERT_BAND_SZ 24 +/* Bytes kept past the advertised capacity to catch a write past the end. */ +#define SIGN_CERT_GUARD_SZ 32 +#define SIGN_CERT_GUARD_BYTE 0xA5 + +#ifdef TEST_SIGN_CERT_BOUNDS_RSA +/* Build the certificate body used by test_wc_SignCert_buffer_bounds(). + * wc_SignCert() rewrites the buffer in place, so it has to be rebuilt for + * every capacity tried. Returns the body size or a negative error. + * + * The serial is fixed rather than left for wc_MakeCert() to generate. A + * generated serial is random, and GenerateInteger() does not shrink its length + * after dropping leading zero bytes, so the promoted byte can carry the MSB and + * make the encoder pad the INTEGER with an extra 0x00. That changes the body + * size for about one certificate in 250, which would make the swept capacities + * below disagree with the reference size. */ +static int test_wc_SignCert_makeBody(Cert* cert, RsaKey* key, WC_RNG* rng, + byte* out, word32 outSz) +{ + static const byte serial[] = { 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, + 0x08 }; + int ret; + + ret = wc_InitCert(cert); + if (ret != 0) + return ret; + + cert->sigType = CTC_SHA256wRSA; + cert->isCA = 0; + XMEMCPY(cert->serial, serial, sizeof(serial)); + cert->serialSz = (int)sizeof(serial); + XSTRNCPY(cert->subject.country, "US", CTC_NAME_SIZE); + XSTRNCPY(cert->subject.state, "MT", CTC_NAME_SIZE); + XSTRNCPY(cert->subject.org, "wolfSSL", CTC_NAME_SIZE); + XSTRNCPY(cert->subject.commonName, "signcert-bounds", CTC_NAME_SIZE); + + return wc_MakeCert(cert, out, outSz, key, NULL, rng); +} +#endif /* TEST_SIGN_CERT_BOUNDS_RSA */ + +#ifdef TEST_SIGN_CERT_BOUNDS_ECC +/* ECDSA r and s are DER INTEGERs whose length changes with the leading zero + * bytes of each new signature, so an ECDSA encoding size measured once does not + * repeat. The sweep copes with that by asserting an invariant that holds for + * either outcome rather than a fixed return, so only the accept case needs a + * capacity clear of anything the jitter can reach. */ +#define SIGN_CERT_ECC_SLACK 8 + +/* ECDSA counterpart of test_wc_SignCert_makeBody(). */ +static int test_wc_SignCert_makeBodyEcc(Cert* cert, ecc_key* key, WC_RNG* rng, + byte* out, word32 outSz) +{ + static const byte serial[] = { 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, + 0x08 }; + int ret; + + ret = wc_InitCert(cert); + if (ret != 0) + return ret; + + cert->sigType = CTC_SHA256wECDSA; + cert->isCA = 0; + XMEMCPY(cert->serial, serial, sizeof(serial)); + cert->serialSz = (int)sizeof(serial); + XSTRNCPY(cert->subject.country, "US", CTC_NAME_SIZE); + XSTRNCPY(cert->subject.state, "MT", CTC_NAME_SIZE); + XSTRNCPY(cert->subject.org, "wolfSSL", CTC_NAME_SIZE); + XSTRNCPY(cert->subject.commonName, "signcert-bounds-ecc", CTC_NAME_SIZE); + + return wc_MakeCert(cert, out, outSz, NULL, key, rng); +} +#endif /* TEST_SIGN_CERT_BOUNDS_ECC */ +#endif /* TEST_SIGN_CERT_BOUNDS_RSA || TEST_SIGN_CERT_BOUNDS_ECC */ + +/* + * wc_SignCert() must never write past the capacity it was given. + * + * SignCert() hands the buffer to AddSignature(), which appends the + * signatureAlgorithm AlgorithmIdentifier and the signatureValue BIT STRING as + * well as wrapping everything in the outer SEQUENCE. A size check that only + * accounts for sequence headers under-counts by the algorithm identifier and + * bit string header, so capacities in a narrow band just below the exact + * encoding size get accepted and overrun. + */ +int test_wc_SignCert_buffer_bounds(void) +{ + EXPECT_DECLS; +#if defined(TEST_SIGN_CERT_BOUNDS_RSA) || defined(TEST_SIGN_CERT_BOUNDS_ECC) + WC_RNG rng; + Cert cert; + byte* scratch = NULL; + byte* buf = NULL; + int rngInit = 0; + int bodySz = 0; + int cap; + int i; +#ifdef TEST_SIGN_CERT_BOUNDS_RSA + RsaKey key; + word32 idx = 0; + int keyInit = 0; + int exactSz = 0; +#endif +#ifdef TEST_SIGN_CERT_BOUNDS_ECC + ecc_key eccKey; + word32 eccIdx = 0; + int eccInit = 0; + int eccExactSz = 0; + int eccSignedSz = 0; + int k; +#endif + + XMEMSET(&rng, 0, sizeof(rng)); + XMEMSET(&cert, 0, sizeof(cert)); + + ExpectIntEQ(wc_InitRng(&rng), 0); + if (EXPECT_SUCCESS()) rngInit = 1; + + ExpectNotNull(scratch = (byte*)XMALLOC(SIGN_CERT_SCRATCH_SZ, HEAP_HINT, + DYNAMIC_TYPE_TMP_BUFFER)); + ExpectNotNull(buf = (byte*)XMALLOC(SIGN_CERT_SCRATCH_SZ, HEAP_HINT, + DYNAMIC_TYPE_TMP_BUFFER)); + +#ifdef TEST_SIGN_CERT_BOUNDS_RSA + XMEMSET(&key, 0, sizeof(key)); + ExpectIntEQ(wc_InitRsaKey_ex(&key, HEAP_HINT, testDevId), 0); + if (EXPECT_SUCCESS()) keyInit = 1; + ExpectIntEQ(wc_RsaPrivateKeyDecode(server_key_der_2048, &idx, &key, + sizeof_server_key_der_2048), 0); + + /* Sign into a roomy buffer once to learn the exact encoding size. */ + ExpectIntGT(bodySz = test_wc_SignCert_makeBody(&cert, &key, &rng, scratch, + SIGN_CERT_SCRATCH_SZ), 0); + ExpectIntGT(exactSz = wc_SignCert(bodySz, cert.sigType, scratch, + SIGN_CERT_SCRATCH_SZ, &key, NULL, &rng), 0); + ExpectIntLT(exactSz + SIGN_CERT_GUARD_SZ, SIGN_CERT_SCRATCH_SZ); + + /* Every capacity below the exact size has to be rejected, and rejected + * without touching a byte beyond it. */ + for (cap = exactSz - SIGN_CERT_BAND_SZ; cap < exactSz; cap++) { + if (!EXPECT_SUCCESS()) break; + + ExpectIntGT(bodySz = test_wc_SignCert_makeBody(&cert, &key, &rng, + scratch, SIGN_CERT_SCRATCH_SZ), 0); + if (!EXPECT_SUCCESS()) break; + + XMEMCPY(buf, scratch, (size_t)bodySz); + XMEMSET(buf + cap, SIGN_CERT_GUARD_BYTE, SIGN_CERT_GUARD_SZ); + + ExpectIntEQ(wc_SignCert(bodySz, cert.sigType, buf, (word32)cap, &key, + NULL, &rng), WC_NO_ERR_TRACE(BUFFER_E)); + + for (i = 0; i < SIGN_CERT_GUARD_SZ; i++) { + ExpectIntEQ(buf[cap + i], SIGN_CERT_GUARD_BYTE); + } + } + + /* The exact size still has to be accepted - the check must not be made + * conservative instead of correct. */ + ExpectIntGT(bodySz = test_wc_SignCert_makeBody(&cert, &key, &rng, scratch, + SIGN_CERT_SCRATCH_SZ), 0); + if (EXPECT_SUCCESS() && (buf != NULL)) { + XMEMCPY(buf, scratch, (size_t)bodySz); + XMEMSET(buf + exactSz, SIGN_CERT_GUARD_BYTE, SIGN_CERT_GUARD_SZ); + } + ExpectIntEQ(wc_SignCert(bodySz, cert.sigType, buf, (word32)exactSz, &key, + NULL, &rng), exactSz); + for (i = 0; i < SIGN_CERT_GUARD_SZ; i++) { + ExpectIntEQ(buf[exactSz + i], SIGN_CERT_GUARD_BYTE); + } +#endif /* TEST_SIGN_CERT_BOUNDS_RSA */ + +#ifdef TEST_SIGN_CERT_BOUNDS_ECC + /* Same sweep for ECDSA. IsSigAlgoNoParams() drops the NULL parameters from + * the AlgorithmIdentifier, so the under-count an estimate makes has a + * different width here than it does for RSA. */ + XMEMSET(&eccKey, 0, sizeof(eccKey)); + ExpectIntEQ(wc_ecc_init_ex(&eccKey, HEAP_HINT, testDevId), 0); + if (EXPECT_SUCCESS()) eccInit = 1; + ExpectIntEQ(wc_EccPrivateKeyDecode(ecc_key_der_256, &eccIdx, &eccKey, + sizeof_ecc_key_der_256), 0); + + for (k = 1; k < SIGN_CERT_BAND_SZ; k++) { + if (!EXPECT_SUCCESS()) break; + + /* Measured fresh every iteration: the previous signature's length + * says nothing about the next one's. */ + ExpectIntGT(bodySz = test_wc_SignCert_makeBodyEcc(&cert, &eccKey, &rng, + scratch, SIGN_CERT_SCRATCH_SZ), 0); + ExpectIntGT(eccExactSz = wc_SignCert(bodySz, cert.sigType, scratch, + SIGN_CERT_SCRATCH_SZ, NULL, &eccKey, &rng), 0); + if (!EXPECT_SUCCESS()) break; + + cap = eccExactSz - k; + ExpectIntGT(bodySz = test_wc_SignCert_makeBodyEcc(&cert, &eccKey, &rng, + scratch, SIGN_CERT_SCRATCH_SZ), 0); + if (!EXPECT_SUCCESS()) break; + + XMEMCPY(buf, scratch, (size_t)bodySz); + XMEMSET(buf + cap, SIGN_CERT_GUARD_BYTE, SIGN_CERT_GUARD_SZ); + + eccSignedSz = wc_SignCert(bodySz, cert.sigType, buf, (word32)cap, NULL, + &eccKey, &rng); + /* The signature this call produces is not the one the capacity was + * derived from, so a capacity below the measured size is not always + * too small. Both outcomes are legitimate; what must hold either way + * is that nothing was written past the capacity. */ + ExpectTrue((eccSignedSz == WC_NO_ERR_TRACE(BUFFER_E)) || + ((eccSignedSz > 0) && (eccSignedSz <= cap))); + + for (i = 0; i < SIGN_CERT_GUARD_SZ; i++) { + ExpectIntEQ(buf[cap + i], SIGN_CERT_GUARD_BYTE); + } + } + + /* A capacity above anything the signature length can reach still has to be + * accepted, so the check is not merely conservative. */ + ExpectIntGT(bodySz = test_wc_SignCert_makeBodyEcc(&cert, &eccKey, &rng, + scratch, SIGN_CERT_SCRATCH_SZ), 0); + ExpectIntGT(eccExactSz = wc_SignCert(bodySz, cert.sigType, scratch, + SIGN_CERT_SCRATCH_SZ, NULL, &eccKey, &rng), 0); + cap = eccExactSz + SIGN_CERT_ECC_SLACK; + ExpectIntGT(bodySz = test_wc_SignCert_makeBodyEcc(&cert, &eccKey, &rng, + scratch, SIGN_CERT_SCRATCH_SZ), 0); + if (EXPECT_SUCCESS() && (buf != NULL)) { + XMEMCPY(buf, scratch, (size_t)bodySz); + XMEMSET(buf + cap, SIGN_CERT_GUARD_BYTE, SIGN_CERT_GUARD_SZ); + } + ExpectIntGT(eccSignedSz = wc_SignCert(bodySz, cert.sigType, buf, + (word32)cap, NULL, &eccKey, &rng), 0); + ExpectIntLE(eccSignedSz, cap); + for (i = 0; i < SIGN_CERT_GUARD_SZ; i++) { + ExpectIntEQ(buf[cap + i], SIGN_CERT_GUARD_BYTE); + } +#endif /* TEST_SIGN_CERT_BOUNDS_ECC */ + + XFREE(buf, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); + XFREE(scratch, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); +#ifdef TEST_SIGN_CERT_BOUNDS_ECC + if (eccInit) + wc_ecc_free(&eccKey); +#endif +#ifdef TEST_SIGN_CERT_BOUNDS_RSA + if (keyInit) + wc_FreeRsaKey(&key); +#endif + if (rngInit) + wc_FreeRng(&rng); +#endif /* TEST_SIGN_CERT_BOUNDS_RSA || TEST_SIGN_CERT_BOUNDS_ECC */ + return EXPECT_RESULT(); +} + /* * MC/DC wave 2 - decision-targeted negative paths for PKCS#8 wrap/parse * and RSA key decode. Targets argument-check, short-buffer, and diff --git a/tests/api/test_asn.h b/tests/api/test_asn.h index 8798bbc6aad..243f5dfe65d 100644 --- a/tests/api/test_asn.h +++ b/tests/api/test_asn.h @@ -43,6 +43,7 @@ int test_ToTraditional_ex_handcrafted(void); int test_ToTraditional_ex_roundtrip(void); int test_ToTraditional_ex_negative(void); int test_ToTraditional_ex_mldsa_bad_params(void); +int test_wc_SignCert_buffer_bounds(void); int test_wc_AsnDecisionCoverage(void); int test_wc_AsnFeatureCoverage(void); @@ -66,6 +67,7 @@ int test_wc_AsnFeatureCoverage(void); TEST_DECL_GROUP("asn", test_ToTraditional_ex_roundtrip), \ TEST_DECL_GROUP("asn", test_ToTraditional_ex_negative), \ TEST_DECL_GROUP("asn", test_ToTraditional_ex_mldsa_bad_params), \ + TEST_DECL_GROUP("asn", test_wc_SignCert_buffer_bounds), \ TEST_DECL_GROUP("asn", test_wc_AsnDecisionCoverage), \ TEST_DECL_GROUP("asn", test_wc_AsnFeatureCoverage) diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index 198fdcbecf2..eb0ca1830e0 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -30951,6 +30951,7 @@ static int SignCert(int requestSz, int sType, byte* buf, word32 buffSz, { int sigSz = 0; int ret; + int totalSz; void* heap = NULL; /* The signature buffer is sized from the key at runtime. */ int maxSigSz; @@ -30962,6 +30963,11 @@ static int SignCert(int requestSz, int sType, byte* buf, word32 buffSz, if (requestSz < 0) return requestSz; + /* MakeSignature() hashes requestSz bytes out of buf below, so bound the + * read against the buffer before signing rather than only the write. */ + if ((word32)requestSz > buffSz) + return BUFFER_E; + /* Async crypto reuses the signing key's embedded CertSignCtx; only RSA and * ECC keys carry one. */ if (rsaKey) { @@ -31032,7 +31038,13 @@ static int SignCert(int requestSz, int sType, byte* buf, word32 buffSz, #endif if (sigSz >= 0) { - if (requestSz + MAX_SEQ_SZ * 2 + sigSz > (int)buffSz) + /* AddSignature() takes no buffer size, and writes the + * signatureAlgorithm and signatureValue on top of the outer SEQUENCE, + * so ask it for the exact size rather than estimating. */ + totalSz = AddSignature(NULL, requestSz, certSignCtx->sig, sigSz, sType); + if (totalSz < 0) + sigSz = totalSz; + else if ((word32)totalSz > buffSz) sigSz = BUFFER_E; else sigSz = AddSignature(buf, requestSz, certSignCtx->sig, sigSz, @@ -31343,6 +31355,7 @@ int wc_SignCert_cb(int requestSz, int sType, byte* buf, word32 buffSz, WC_RNG* rng) { int sigSz = 0; + int totalSz; word32 sigCap = MAX_ENCODED_CLASSIC_SIG_SZ; CertSignCtx certSignCtx_lcl; CertSignCtx* certSignCtx = &certSignCtx_lcl; @@ -31388,6 +31401,12 @@ int wc_SignCert_cb(int requestSz, int sType, byte* buf, word32 buffSz, return requestSz; } + /* MakeSignatureCb() hashes requestSz bytes out of buf below, so bound the + * read against the buffer before signing rather than only the write. */ + if ((word32)requestSz > buffSz) { + return BUFFER_E; + } + /* keyType is restricted to RSA_TYPE/ECC_TYPE above, so the signature is * a classic (non-PQC) one and fits MAX_ENCODED_CLASSIC_SIG_SZ. */ #ifndef WOLFSSL_NO_MALLOC @@ -31420,12 +31439,14 @@ int wc_SignCert_cb(int requestSz, int sType, byte* buf, word32 buffSz, #endif if (sigSz >= 0) { - /* Check buffer has room for signature structure. This is an estimate - * using MAX_SEQ_SZ * 2 to account for sequence headers and algorithm - * identifier overhead. For precise sizing, call AddSignature with - * NULL buffer first, but this estimate matches the existing pattern - * used in SignCert. */ - if (requestSz + MAX_SEQ_SZ * 2 + sigSz > (int)buffSz) { + /* AddSignature() takes no buffer size, and writes the + * signatureAlgorithm and signatureValue on top of the outer SEQUENCE, + * so ask it for the exact size rather than estimating. */ + totalSz = AddSignature(NULL, requestSz, certSignCtx->sig, sigSz, sType); + if (totalSz < 0) { + sigSz = totalSz; + } + else if ((word32)totalSz > buffSz) { sigSz = BUFFER_E; } else { From ea911b8c9b44673c3653977b5aa88e37c6009f49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Frauenschl=C3=A4ger?= Date: Mon, 3 Aug 2026 12:29:13 +0200 Subject: [PATCH 2/8] Fix buffer overrun in EVP_PKEY_keygen on a populated EVP_PKEY The RSA branch of wolfSSL_EVP_PKEY_keygen() passed &pkey->pkey.ptr directly to wolfSSL_i2d_RSAPrivateKey(): pkey->pkey_sz = wolfSSL_i2d_RSAPrivateKey(pkey->rsa, (unsigned char**)&pkey->pkey.ptr); Following the i2d convention, wolfSSL_RSA_To_Der_ex() treats a non-NULL *outBuf as a caller supplied buffer: it encodes into it with no size check and then advances the pointer past the encoding. When ppkey points at an EVP_PKEY that already carries a DER encoding, the generated private key is written into that older, typically smaller allocation and pkey.ptr is left pointing into the middle of it, which the eventual XFREE() then trips over. Decoding a 2048-bit public key and calling keygen on the same EVP_PKEY writes about 1190 bytes into a 294 byte buffer. The branch now installs the generated key on the pkey and hands the encoding to PopulateRSAEvpPkeyDer(), which is what wolfSSL_EVP_PKEY_set1_RSA() already does and what the sibling EC branch does through ECC_populate_EVP_PKEY(). That function sizes the encoding first and allocates its own buffer, so i2d is never shown a populated pkey, and the three copies of the free, encode and assign sequence become one. It also allocates against pkey->heap, which is the heap every site that later releases pkey.ptr passes, while i2d deliberately allocates with a NULL hint because its result is returned to the user. The old RSA key is released before the new one is installed, which fixes the previous unconditional overwrite of pkey->rsa leaking the old object, and success is no longer reported when the encoding fails. pkcs8HeaderSz is taken from the newly generated key rather than left as it was. It describes the DER currently held in pkey.ptr, and PopulateRSAEvpPkeyDer() adds a PKCS#8 wrapper only when the RSA key carries a header size. d2i_PrivateKey(), d2i_AutoPrivateKey() and PEM_read_bio_PrivateKey() set the field to 26 for a wrapped RSA key, and a pkey obtained that way and then reused for keygen kept the 26 while the encoding underneath was no longer wrapped. Every export path that trusts the pair then sliced 26 bytes off the front of the new key: wolfssl_i_evp_pkey_get_der() behind i2d_PrivateKey(), pkcs8_encode() behind i2d_PKCS8PrivateKey(), and wolfssl_pkey_encrypt() behind PEM_write_bio_PrivateKey(), returning a corrupt encoding under a success return. wolfSSL_EVP_PKEY_set1_RSA() already maintains this field. Adds test_wolfSSL_EVP_PKEY_keygen_reuse(), which runs keygen on an EVP_PKEY populated from a public key DER and requires the resulting encoding to decode back to the generated key, plus a second pass seeded from a PKCS#8 key since the public key seed leaves pkcs8HeaderSz at zero and cannot catch the stale header. The test is gated on OPENSSL_EXTRA rather than OPENSSL_ALL, since nothing it calls needs the latter, and on !NO_ASN and !NO_PWDBASED because wolfSSL_i2d_PrivateKey() is compiled only under those. The PKCS#8 pass additionally needs !NO_CERTS, which is what load_file() is gated on. The forward declaration is gated on the same condition as the definition rather than on WOLFSSL_KEY_GEN, which settings.h only happens to derive WOLFSSL_KEY_TO_DER from today. The sibling cases in the same switch are left alone deliberately. The DH case does not free a previous pkey->dh, and the EC case promotes a borrowed pkey->ecc to owned, both of which are the same ownership class this change fixes for RSA. They are pre-existing, they need their own tests, and folding them in here would widen a buffer overrun fix into a rework of EVP_PKEY_keygen ownership across four algorithms. --- tests/api/test_evp_pkey.c | 96 +++++++++++++++++++++++++++++++++++++++ tests/api/test_evp_pkey.h | 2 + wolfcrypt/src/evp.c | 22 +++++++-- 3 files changed, 115 insertions(+), 5 deletions(-) diff --git a/tests/api/test_evp_pkey.c b/tests/api/test_evp_pkey.c index 6fbfb281c2a..feef27266c0 100644 --- a/tests/api/test_evp_pkey.c +++ b/tests/api/test_evp_pkey.c @@ -1428,6 +1428,102 @@ int test_wolfSSL_EVP_PKEY_keygen(void) return EXPECT_RESULT(); } +/* + * wolfSSL_EVP_PKEY_keygen() has to replace whatever the caller supplied + * EVP_PKEY already holds. + * + * The RSA branch used to pass &pkey->pkey.ptr straight to + * wolfSSL_i2d_RSAPrivateKey(). A non-NULL pointer there means "caller supplied + * buffer" to i2d, so the freshly generated key DER was written into the DER + * already on the pkey without a size check, and the pointer was left pointing + * past the encoding. + */ +int test_wolfSSL_EVP_PKEY_keygen_reuse(void) +{ + EXPECT_DECLS; +/* wolfSSL_i2d_PrivateKey() needs OPENSSL_EXTRA plus !NO_ASN and !NO_PWDBASED, + * and nothing here is OPENSSL_ALL only. */ +#if defined(OPENSSL_EXTRA) && !defined(NO_RSA) && defined(WOLFSSL_KEY_GEN) && \ + defined(USE_CERT_BUFFERS_2048) && !defined(HAVE_SELFTEST) && \ + !defined(NO_ASN) && !defined(NO_PWDBASED) + WOLFSSL_EVP_PKEY* pkey = NULL; + WOLFSSL_EVP_PKEY* decoded = NULL; + EVP_PKEY_CTX* ctx = NULL; + const unsigned char* in; + unsigned char* der = NULL; + int derSz = 0; +/* load_file() is compiled only when certificates are available. */ +#if defined(HAVE_PKCS8) && !defined(NO_FILESYSTEM) && !defined(NO_CERTS) + byte* p8 = NULL; + size_t p8Sz = 0; +#endif + + /* Populate the pkey with a public key DER much smaller than the private + * key DER that keygen will produce. */ + in = client_keypub_der_2048; + ExpectNotNull(pkey = wolfSSL_d2i_PUBKEY(NULL, &in, + (long)sizeof_client_keypub_der_2048)); + + ExpectNotNull(ctx = EVP_PKEY_CTX_new(pkey, NULL)); + ExpectIntEQ(EVP_PKEY_keygen_init(ctx), WOLFSSL_SUCCESS); + ExpectIntEQ(EVP_PKEY_CTX_set_rsa_keygen_bits(ctx, 2048), WOLFSSL_SUCCESS); + ExpectIntEQ(EVP_PKEY_keygen(ctx, &pkey), WOLFSSL_SUCCESS); + + /* The DER on the pkey has to be the generated private key, held in a + * buffer that starts where pkey.ptr points. */ + ExpectIntGT(derSz = wolfSSL_i2d_PrivateKey(pkey, &der), 0); + in = der; + ExpectNotNull(decoded = wolfSSL_d2i_PrivateKey(EVP_PKEY_RSA, NULL, &in, + (long)derSz)); +#if defined(WOLFSSL_ERROR_CODE_OPENSSL) + ExpectIntEQ(EVP_PKEY_cmp(pkey, decoded), 1); +#else + ExpectIntEQ(EVP_PKEY_cmp(pkey, decoded), 0); +#endif + + XFREE(der, NULL, DYNAMIC_TYPE_OPENSSL); + der = NULL; + EVP_PKEY_free(decoded); + decoded = NULL; + EVP_PKEY_CTX_free(ctx); + ctx = NULL; + EVP_PKEY_free(pkey); + pkey = NULL; + +#if defined(HAVE_PKCS8) && !defined(NO_FILESYSTEM) && !defined(NO_CERTS) + /* Same again, seeded from a PKCS#8 wrapped key. That gives the pkey a + * non-zero pkcs8HeaderSz, which does not describe the bare PKCS#1 + * encoding keygen installs, so it has to be cleared. */ + ExpectIntEQ(load_file("./certs/server-keyPkcs8.der", &p8, &p8Sz), 0); + in = p8; + ExpectNotNull(pkey = wolfSSL_d2i_PrivateKey(EVP_PKEY_RSA, NULL, &in, + (long)p8Sz)); + + ExpectNotNull(ctx = EVP_PKEY_CTX_new(pkey, NULL)); + ExpectIntEQ(EVP_PKEY_keygen_init(ctx), WOLFSSL_SUCCESS); + ExpectIntEQ(EVP_PKEY_CTX_set_rsa_keygen_bits(ctx, 2048), WOLFSSL_SUCCESS); + ExpectIntEQ(EVP_PKEY_keygen(ctx, &pkey), WOLFSSL_SUCCESS); + + ExpectIntGT(derSz = wolfSSL_i2d_PrivateKey(pkey, &der), 0); + in = der; + ExpectNotNull(decoded = wolfSSL_d2i_PrivateKey(EVP_PKEY_RSA, NULL, &in, + (long)derSz)); +#if defined(WOLFSSL_ERROR_CODE_OPENSSL) + ExpectIntEQ(EVP_PKEY_cmp(pkey, decoded), 1); +#else + ExpectIntEQ(EVP_PKEY_cmp(pkey, decoded), 0); +#endif + + XFREE(der, NULL, DYNAMIC_TYPE_OPENSSL); + XFREE(p8, NULL, DYNAMIC_TYPE_TMP_BUFFER); + EVP_PKEY_free(decoded); + EVP_PKEY_CTX_free(ctx); + EVP_PKEY_free(pkey); +#endif /* HAVE_PKCS8 && !NO_FILESYSTEM && !NO_CERTS */ +#endif + return EXPECT_RESULT(); +} + int test_wolfSSL_EVP_SignInit_ex(void) { EXPECT_DECLS; diff --git a/tests/api/test_evp_pkey.h b/tests/api/test_evp_pkey.h index 60647ac0490..92c28a15216 100644 --- a/tests/api/test_evp_pkey.h +++ b/tests/api/test_evp_pkey.h @@ -51,6 +51,7 @@ int test_wolfSSL_EVP_PKEY_paramgen(void); int test_wolfSSL_EVP_PKEY_param_check(void); int test_wolfSSL_EVP_PKEY_keygen_init(void); int test_wolfSSL_EVP_PKEY_keygen(void); +int test_wolfSSL_EVP_PKEY_keygen_reuse(void); int test_wolfSSL_EVP_SignInit_ex(void); int test_wolfSSL_EVP_PKEY_sign_verify_rsa(void); int test_wolfSSL_EVP_PKEY_sign_verify_dsa(void); @@ -99,6 +100,7 @@ int test_wolfSSL_EVP_PKEY_encoded_public_key(void); TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_param_check), \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_keygen_init), \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_keygen), \ + TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_keygen_reuse), \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_SignInit_ex), \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_sign_verify_rsa), \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_sign_verify_dsa), \ diff --git a/wolfcrypt/src/evp.c b/wolfcrypt/src/evp.c index 3b70de2dbe8..9722a7b1c65 100644 --- a/wolfcrypt/src/evp.c +++ b/wolfcrypt/src/evp.c @@ -3803,6 +3803,9 @@ int wolfSSL_EVP_PKEY_keygen_init(WOLFSSL_EVP_PKEY_CTX *ctx) #ifdef HAVE_ECC static int ECC_populate_EVP_PKEY(WOLFSSL_EVP_PKEY* pkey, WOLFSSL_EC_KEY *key); #endif +#if !defined(NO_RSA) && defined(WOLFSSL_KEY_TO_DER) +static int PopulateRSAEvpPkeyDer(WOLFSSL_EVP_PKEY *pkey); +#endif int wolfSSL_EVP_PKEY_keygen(WOLFSSL_EVP_PKEY_CTX *ctx, WOLFSSL_EVP_PKEY **ppkey) @@ -3810,6 +3813,9 @@ int wolfSSL_EVP_PKEY_keygen(WOLFSSL_EVP_PKEY_CTX *ctx, int ret = WC_NO_ERR_TRACE(WOLFSSL_FAILURE); int ownPkey = 0; WOLFSSL_EVP_PKEY* pkey; +#if defined(WOLFSSL_KEY_GEN) && !defined(NO_RSA) + WOLFSSL_RSA* rsaTmp; +#endif WOLFSSL_ENTER("wolfSSL_EVP_PKEY_keygen"); @@ -3843,13 +3849,19 @@ int wolfSSL_EVP_PKEY_keygen(WOLFSSL_EVP_PKEY_CTX *ctx, switch (pkey->type) { #if defined(WOLFSSL_KEY_GEN) && !defined(NO_RSA) case WC_EVP_PKEY_RSA: - pkey->rsa = wolfSSL_RSA_generate_key(ctx->nbits, WC_RSA_EXPONENT, + rsaTmp = wolfSSL_RSA_generate_key(ctx->nbits, WC_RSA_EXPONENT, NULL, NULL); - if (pkey->rsa) { + if (rsaTmp != NULL) { + if (pkey->rsa != NULL && pkey->ownRsa == 1) + wolfSSL_RSA_free(pkey->rsa); + pkey->rsa = rsaTmp; pkey->ownRsa = 1; - pkey->pkey_sz = wolfSSL_i2d_RSAPrivateKey(pkey->rsa, - (unsigned char**)&pkey->pkey.ptr); - ret = WOLFSSL_SUCCESS; + /* PopulateRSAEvpPkeyDer() wraps the encoding in PKCS#8 only + * when the RSA key carries a header size, so take the new + * key's value. A size left over from a wrapped predecessor + * would make the export paths slice bytes off the encoding. */ + pkey->pkcs8HeaderSz = rsaTmp->pkcs8HeaderSz; + ret = PopulateRSAEvpPkeyDer(pkey); } break; #endif From 88bd09d1c4de66f06a5754d9acdcb8f8ff993d8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Frauenschl=C3=A4ger?= Date: Mon, 3 Aug 2026 12:33:54 +0200 Subject: [PATCH 3/8] Keep EVP_PKEY pkey_sz in step with pkey.ptr in PopulateRSAEvpPkeyDer PopulateRSAEvpPkeyDer() installs the newly allocated DER buffer on the EVP_PKEY before encoding into it, but only updates pkey_sz on the success path at the end. Every failure return in between left pkey_sz describing the previous encoding while pkey.ptr pointed at a buffer that holds no encoding at all and can be smaller than the old one. Callers such as wolfssl_i_evp_pkey_get_der() copy pkey_sz bytes out of pkey.ptr, so they would read past the new allocation. The reachable paths are wc_RsaKeyToDer() or wc_RsaKeyToPublicDer() failing after their size query succeeded, and, under HAVE_PKCS8, the PKCS#8 buffer allocation or wc_CreatePKCS8Key() failing. Reset pkey_sz when the new buffer is installed so a failure return leaves the pkey describing an empty encoding rather than a stale one. Verified by fault injection, having wc_RsaKeyToDer() fail whenever asked to write: wolfSSL_EVP_PKEY_set1_RSA() on a populated EVP_PKEY left pkey_sz at 1192 before this change and leaves it 0 after. pkcs8HeaderSz is cleared on the same return. It describes an offset into the encoding pkey_sz measures, and wolfssl_i_evp_pkey_get_der() already guards the subtraction of one from the other, but pkcs8_encode() and pkcs8_encrypt() in src/pk.c do not: with pkey_sz reset and a header size of 26 left over from a PKCS#8 wrapped predecessor, they would compute a length of 0 - 26 as a word32. --- wolfcrypt/src/evp.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/wolfcrypt/src/evp.c b/wolfcrypt/src/evp.c index 9722a7b1c65..eaafd5f6ce2 100644 --- a/wolfcrypt/src/evp.c +++ b/wolfcrypt/src/evp.c @@ -9307,8 +9307,11 @@ static int PopulateRSAEvpPkeyDer(WOLFSSL_EVP_PKEY *pkey) return WOLFSSL_FAILURE; } - /* Old pointer is invalid from this point on */ + /* Old pointer is invalid from this point on. The new buffer holds no + * encoding yet and can be smaller than the old one, so drop the old size + * rather than let a failure below return with the two out of step. */ pkey->pkey.ptr = (char*)derBuf; + pkey->pkey_sz = 0; if (rsa->type == RSA_PRIVATE) { ret = wc_RsaKeyToDer(rsa, derBuf, (word32)derSz); @@ -9346,6 +9349,9 @@ static int PopulateRSAEvpPkeyDer(WOLFSSL_EVP_PKEY *pkey) if (ret < 0) { WOLFSSL_MSG("PopulateRSAEvpPkeyDer failed"); + /* pkey_sz is zero here, so the header size cannot stay behind or the + * export paths subtract it from zero and underflow. */ + pkey->pkcs8HeaderSz = 0; return WOLFSSL_FAILURE; } else { From ecabfeb3013e1df7843e7aeca6d5644412df264e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Frauenschl=C3=A4ger?= Date: Mon, 3 Aug 2026 12:41:11 +0200 Subject: [PATCH 4/8] Fix buffer overrun in the NO_REALLOC EVP_PKEY populate paths Under WOLFSSL_NO_REALLOC, PopulateRSAEvpPkeyDer() and ECC_populate_EVP_PKEY() emulate XREALLOC by allocating a buffer sized for the NEW encoding and then copying pkey_sz bytes, the size of the OLD one, into it: derBuf = (byte*)XMALLOC((size_t)derSz, pkey->heap, DYNAMIC_TYPE_DER); if (derBuf != NULL) { XMEMCPY(derBuf, pkey->pkey.ptr, (size_t)pkey->pkey_sz); Whenever the replacement key encodes shorter than the one already on the EVP_PKEY the copy runs past the end of the new allocation. Putting a public key on a pkey holding a 2048-bit private key copies 1192 bytes into a 294 byte buffer. The copy serves no purpose: both functions fill the new buffer with a fresh encoding immediately afterwards. It is removed rather than bounded. ECC_populate_EVP_PKEY() also gains the pkey_sz reset that PopulateRSAEvpPkeyDer() already has, so a failure between the allocation and the encoding cannot leave the size describing a buffer that holds no encoding. The outgoing buffer is now wiped with ForceZero() before it is reallocated or freed. On a private key it holds a complete RSA or ECC DER, so returning it to the allocator intact leaves the key recoverable from the free pool through a later heap over-read, a core dump or a swap page. wolfSSL_RSA_To_Der_ex() establishes the same convention two frames away. wolfSSL_EVP_PKEY_free() gets the same treatment, since it releases that buffer on every normal teardown, as does the PKCS#8 branch of PopulateRSAEvpPkeyDer(), which frees the unwrapped PKCS#1 key on its success path once the wrapped copy has been built. In ECC_populate_EVP_PKEY() that covers all three sites which release the previous encoding, the two private-key branches as well as the public one. clearEVPPkeyKeys() leaves pkey.ptr in place, so a pkey decoded from a private key still carries that DER when a public-only key replaces it. The wipe there happens before the allocation, since XREALLOC consumes the old pointer, and pkey_sz and pkcs8HeaderSz are dropped with the contents so a failed allocation cannot leave either describing a buffer that no longer holds an encoding. Where the allocation of the new buffer fails, pkcs8HeaderSz is cleared along with pkey_sz for the reason given in the previous commit. ECC_populate_EVP_PKEY() clears pkcs8HeaderSz when it installs a public key. A SubjectPublicKeyInfo has no PKCS#8 wrapper, but neither wolfSSL_EVP_PKEY_set1_EC_KEY() nor clearEVPPkeyKeys() resets the field, so putting a public key on a pkey decoded from a PKCS#8 EC key left the export paths starting that many bytes inside the new encoding and returning it short under a success return. The traditional private-key branch needs the same reset. It runs whenever the incoming EC key carries no header size of its own, a generated key for instance, and writes a bare SEC1 ECPrivateKey. Seeding an EVP_PKEY from certs/ecc-keyPkcs8.der and then calling wolfSSL_EVP_PKEY_set1_EC_KEY() with a generated key made wolfSSL_i2d_PrivateKey() return 92 bytes beginning in the middle of the private scalar rather than the 121 byte encoding. Every export path is affected, including the PKCS#8 encryption in wolfSSL_PEM_write_bio_PKCS8PrivateKey(), which encrypts that same misaligned slice. Adds test_wolfSSL_EVP_PKEY_set1_shrinking_der(), which replaces the key on an EVP_PKEY with a public-only one for both RSA and ECC and requires the stored encoding to shrink. The smoke-test job opensslextra-norealloc-asan builds exactly this configuration under AddressSanitizer, which is where the over-copy is caught. The test gates each algorithm on its own prerequisites rather than on one shared list. WOLFSSL_KEY_TO_DER is defined by settings.h only when RSA is enabled, so requiring it for the whole test compiled the ECC half out of any build without RSA, and that half is the only coverage the ECC over-copy has. The ECC half is seeded from a PKCS#8 wrapped key so that pkcs8HeaderSz starts non-zero, and its size assertion is exact rather than a comparison against the previous size, so an export starting at a stale header shows up as a mismatch rather than passing. test_wolfSSL_EVP_PKEY_set1_EC_KEY_no_pkcs8() covers the private-key case. It compares the encoding exported after the replacement against the one a pkey that never held a wrapped key produces from the same EC key, so a carried over header size shows up as a size and content mismatch. --- tests/api/test_evp_pkey.c | 157 ++++++++++++++++++++++++++++++++++++++ tests/api/test_evp_pkey.h | 4 + wolfcrypt/src/evp.c | 95 +++++++++++++++++++++-- 3 files changed, 250 insertions(+), 6 deletions(-) diff --git a/tests/api/test_evp_pkey.c b/tests/api/test_evp_pkey.c index feef27266c0..2ab9e11e4dd 100644 --- a/tests/api/test_evp_pkey.c +++ b/tests/api/test_evp_pkey.c @@ -1524,6 +1524,163 @@ int test_wolfSSL_EVP_PKEY_keygen_reuse(void) return EXPECT_RESULT(); } +/* + * Replacing a PKCS#8 wrapped key on an EVP_PKEY with an EC key that carries no + * wrapper has to drop pkcs8HeaderSz along with the encoding it described. + * + * ECC_populate_EVP_PKEY() writes a bare SEC1 ECPrivateKey when the EC key has + * no header size of its own. Everything that exports the key, from + * wolfSSL_EVP_PKEY_get_der() to the PKCS#8 encryption in + * wolfSSL_PEM_write_bio_PKCS8PrivateKey(), skips pkcs8HeaderSz bytes of the + * stored buffer, so a size left from the previous key makes them start inside + * the new encoding and hand out a truncated one. + */ +int test_wolfSSL_EVP_PKEY_set1_EC_KEY_no_pkcs8(void) +{ + EXPECT_DECLS; +#if defined(OPENSSL_EXTRA) && defined(HAVE_ECC) && !defined(NO_FILESYSTEM) && \ + !defined(NO_CERTS) && !defined(NO_ASN) && !defined(NO_PWDBASED) + WOLFSSL_EVP_PKEY* wrapped = NULL; + WOLFSSL_EVP_PKEY* fresh = NULL; + WOLFSSL_EC_KEY* ec = NULL; + const unsigned char* in; + unsigned char* wrappedDer = NULL; + unsigned char* freshDer = NULL; + int wrappedSz = 0; + int freshSz = 0; + byte* buf = NULL; + size_t bufSz = 0; + + /* Seed a pkey from a PKCS#8 wrapped key so that pkcs8HeaderSz starts out + * non-zero. */ + ExpectIntEQ(load_file("./certs/ecc-keyPkcs8.der", &buf, &bufSz), 0); + in = buf; + ExpectNotNull(wrapped = wolfSSL_d2i_PrivateKey(EVP_PKEY_EC, NULL, &in, + (long)bufSz)); + + /* A generated key carries no PKCS#8 header, so setting it takes the + * traditional branch of ECC_populate_EVP_PKEY(). */ + ExpectNotNull(ec = wolfSSL_EC_KEY_new_by_curve_name(NID_X9_62_prime256v1)); + ExpectIntEQ(wolfSSL_EC_KEY_generate_key(ec), WOLFSSL_SUCCESS); + ExpectIntEQ(wolfSSL_EVP_PKEY_set1_EC_KEY(wrapped, ec), WOLFSSL_SUCCESS); + + /* The same key on a pkey that never held a wrapped one is the reference: + * both have to export the identical encoding. */ + ExpectNotNull(fresh = wolfSSL_EVP_PKEY_new()); + ExpectIntEQ(wolfSSL_EVP_PKEY_set1_EC_KEY(fresh, ec), WOLFSSL_SUCCESS); + + ExpectIntGT(freshSz = wolfSSL_i2d_PrivateKey(fresh, &freshDer), 0); + ExpectIntGT(wrappedSz = wolfSSL_i2d_PrivateKey(wrapped, &wrappedDer), 0); + ExpectIntEQ(wrappedSz, freshSz); + ExpectNotNull(wrappedDer); + ExpectNotNull(freshDer); + ExpectBufEQ(wrappedDer, freshDer, (size_t)freshSz); + + XFREE(wrappedDer, NULL, DYNAMIC_TYPE_OPENSSL); + XFREE(freshDer, NULL, DYNAMIC_TYPE_OPENSSL); + XFREE(buf, NULL, DYNAMIC_TYPE_TMP_BUFFER); + wolfSSL_EC_KEY_free(ec); + wolfSSL_EVP_PKEY_free(fresh); + wolfSSL_EVP_PKEY_free(wrapped); +#endif + return EXPECT_RESULT(); +} + +/* + * Replacing the key on an EVP_PKEY with one whose DER is SHORTER has to shrink + * the stored encoding without writing past the new buffer. + * + * Under WOLFSSL_NO_REALLOC, PopulateRSAEvpPkeyDer() and ECC_populate_EVP_PKEY() + * emulate XREALLOC by allocating a buffer sized for the new encoding and + * copying pkey_sz bytes, the size of the OLD one, into it. The CI job + * opensslextra-norealloc-asan builds exactly that configuration under ASan, so + * this test is where such an over-copy gets caught. + */ +int test_wolfSSL_EVP_PKEY_set1_shrinking_der(void) +{ + EXPECT_DECLS; +/* settings.h defines WOLFSSL_KEY_TO_DER only when RSA is enabled, so gating the + * whole test on it would compile the ECC half out of an RSA-less build, and + * that half is the only coverage for the ECC_populate_EVP_PKEY() over-copy. + * Gate on the union and keep the per-algorithm guards inside. */ +#if defined(OPENSSL_EXTRA) && !defined(NO_FILESYSTEM) && !defined(NO_CERTS) && \ + !defined(NO_ASN) && !defined(NO_PWDBASED) && \ + ((!defined(NO_RSA) && defined(WOLFSSL_KEY_TO_DER)) || defined(HAVE_ECC)) + const unsigned char* in; + byte* buf = NULL; + size_t bufSz = 0; +#if !defined(NO_RSA) && defined(WOLFSSL_KEY_TO_DER) + WOLFSSL_EVP_PKEY* rsaPkey = NULL; + WOLFSSL_RSA* rsaPub = NULL; + int rsaPrivSz = 0; +#endif +#ifdef HAVE_ECC + WOLFSSL_EVP_PKEY* ecPkey = NULL; + WOLFSSL_EC_KEY* ecPriv = NULL; + WOLFSSL_EC_KEY* ecPub = NULL; + int ecPrivSz = 0; +#endif + +#if !defined(NO_RSA) && defined(WOLFSSL_KEY_TO_DER) + /* Private key DER first, then a public-only key whose DER is about a + * quarter of the size. */ + ExpectIntEQ(load_file("./certs/client-key.der", &buf, &bufSz), 0); + in = buf; + ExpectNotNull(rsaPkey = wolfSSL_d2i_PrivateKey(EVP_PKEY_RSA, NULL, &in, + (long)bufSz)); + ExpectIntGT(rsaPrivSz = wolfSSL_i2d_PrivateKey(rsaPkey, NULL), 0); + XFREE(buf, NULL, DYNAMIC_TYPE_TMP_BUFFER); + buf = NULL; + + ExpectIntEQ(load_file("./certs/client-keyPub.der", &buf, &bufSz), 0); + in = buf; + ExpectNotNull(rsaPub = wolfSSL_d2i_RSAPublicKey(NULL, &in, (long)bufSz)); + ExpectIntEQ(wolfSSL_EVP_PKEY_set1_RSA(rsaPkey, rsaPub), WOLFSSL_SUCCESS); + /* Confirm the stored encoding really did shrink, so the test keeps + * exercising the direction that overruns. */ + ExpectIntLT(wolfSSL_i2d_PrivateKey(rsaPkey, NULL), rsaPrivSz); + + XFREE(buf, NULL, DYNAMIC_TYPE_TMP_BUFFER); + buf = NULL; + wolfSSL_RSA_free(rsaPub); + wolfSSL_EVP_PKEY_free(rsaPkey); +#endif + +#ifdef HAVE_ECC + /* Same shape for ECC: the private key encoding is longer than the public + * one for the same curve. The seed is PKCS#8 wrapped rather than a bare + * SEC1 key, so pkcs8HeaderSz starts non-zero and the exact size assertion + * below catches a header size carried over onto the public encoding. */ + ExpectIntEQ(load_file("./certs/ecc-keyPkcs8.der", &buf, &bufSz), 0); + in = buf; + ExpectNotNull(ecPkey = wolfSSL_d2i_PrivateKey(EVP_PKEY_EC, NULL, &in, + (long)bufSz)); + ExpectIntGT(ecPrivSz = wolfSSL_i2d_PrivateKey(ecPkey, NULL), 0); + ExpectNotNull(ecPriv = wolfSSL_EVP_PKEY_get1_EC_KEY(ecPkey)); + + /* A key holding only the public point takes ECC_populate_EVP_PKEY's + * public branch. */ + ExpectNotNull(ecPub = wolfSSL_EC_KEY_new_by_curve_name( + NID_X9_62_prime256v1)); + ExpectIntEQ(wolfSSL_EC_KEY_set_public_key(ecPub, + wolfSSL_EC_KEY_get0_public_key(ecPriv)), WOLFSSL_SUCCESS); + ExpectIntEQ(wolfSSL_EVP_PKEY_set1_EC_KEY(ecPkey, ecPub), WOLFSSL_SUCCESS); + ExpectIntLT(wolfSSL_i2d_PrivateKey(ecPkey, NULL), ecPrivSz); + /* Exact rather than "smaller", so that an export starting at a stale + * pkcs8HeaderSz shows up as a size mismatch instead of passing. */ + ExpectIntEQ(wolfSSL_i2d_PrivateKey(ecPkey, NULL), + wc_EccPublicKeyDerSize((ecc_key*)ecPub->internal, 1)); + + XFREE(buf, NULL, DYNAMIC_TYPE_TMP_BUFFER); + buf = NULL; + wolfSSL_EC_KEY_free(ecPub); + wolfSSL_EC_KEY_free(ecPriv); + wolfSSL_EVP_PKEY_free(ecPkey); +#endif +#endif /* OPENSSL_EXTRA && WOLFSSL_KEY_TO_DER && !NO_FILESYSTEM */ + return EXPECT_RESULT(); +} + int test_wolfSSL_EVP_SignInit_ex(void) { EXPECT_DECLS; diff --git a/tests/api/test_evp_pkey.h b/tests/api/test_evp_pkey.h index 92c28a15216..cae4894fc29 100644 --- a/tests/api/test_evp_pkey.h +++ b/tests/api/test_evp_pkey.h @@ -52,6 +52,8 @@ int test_wolfSSL_EVP_PKEY_param_check(void); int test_wolfSSL_EVP_PKEY_keygen_init(void); int test_wolfSSL_EVP_PKEY_keygen(void); int test_wolfSSL_EVP_PKEY_keygen_reuse(void); +int test_wolfSSL_EVP_PKEY_set1_EC_KEY_no_pkcs8(void); +int test_wolfSSL_EVP_PKEY_set1_shrinking_der(void); int test_wolfSSL_EVP_SignInit_ex(void); int test_wolfSSL_EVP_PKEY_sign_verify_rsa(void); int test_wolfSSL_EVP_PKEY_sign_verify_dsa(void); @@ -101,6 +103,8 @@ int test_wolfSSL_EVP_PKEY_encoded_public_key(void); TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_keygen_init), \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_keygen), \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_keygen_reuse), \ + TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_set1_EC_KEY_no_pkcs8), \ + TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_set1_shrinking_der), \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_SignInit_ex), \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_sign_verify_rsa), \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_sign_verify_dsa), \ diff --git a/wolfcrypt/src/evp.c b/wolfcrypt/src/evp.c index eaafd5f6ce2..1862a0bad60 100644 --- a/wolfcrypt/src/evp.c +++ b/wolfcrypt/src/evp.c @@ -9292,18 +9292,45 @@ static int PopulateRSAEvpPkeyDer(WOLFSSL_EVP_PKEY *pkey) } #ifdef WOLFSSL_NO_REALLOC + /* The new buffer can be smaller than the old encoding, and the encoding + * below fills it completely, so nothing is carried over. Allocate before + * the old buffer is touched, so that a failure here leaves the encoding + * held so far in place. */ derBuf = (byte*)XMALLOC((size_t)derSz, pkey->heap, DYNAMIC_TYPE_DER); if (derBuf != NULL) { - XMEMCPY(derBuf, pkey->pkey.ptr, (size_t)pkey->pkey_sz); + /* On a private key the outgoing buffer holds a full RSA DER, so wipe + * it before it is returned to the allocator. The size is dropped with + * the contents so a failure below cannot leave pkey_sz describing a + * buffer that no longer holds an encoding. */ + if (pkey->pkey.ptr != NULL && pkey->pkey_sz > 0) { + ForceZero(pkey->pkey.ptr, (word32)pkey->pkey_sz); + } XFREE(pkey->pkey.ptr, pkey->heap, DYNAMIC_TYPE_DER); pkey->pkey.ptr = NULL; + pkey->pkey_sz = 0; } #else + /* XREALLOC consumes the old pointer, so the buffer has to be wiped before + * the call: on a private key it holds a full RSA DER. Nothing is carried + * over, as the encoding below fills the new buffer completely. The size is + * dropped with the contents so a failure below cannot leave pkey_sz + * describing a buffer that no longer holds an encoding. */ + if (pkey->pkey.ptr != NULL && pkey->pkey_sz > 0) { + ForceZero(pkey->pkey.ptr, (word32)pkey->pkey_sz); + pkey->pkey_sz = 0; + } + derBuf = (byte*)XREALLOC(pkey->pkey.ptr, (size_t)derSz, pkey->heap, DYNAMIC_TYPE_DER); #endif if (derBuf == NULL) { WOLFSSL_MSG("PopulateRSAEvpPkeyDer malloc failed"); + if (pkey->pkey_sz == 0) { + /* No encoding is described any more, so the header size has to go + * as well or the export paths subtract it from zero and + * underflow. */ + pkey->pkcs8HeaderSz = 0; + } return WOLFSSL_FAILURE; } @@ -9329,10 +9356,15 @@ static int PopulateRSAEvpPkeyDer(WOLFSSL_EVP_PKEY *pkey) if (derBuf != NULL) { ret = wc_CreatePKCS8Key(derBuf, &sz, keyBuf, (word32)keySz, RSAk, NULL, 0); + /* keyBuf holds the unwrapped private key. */ + ForceZero(keyBuf, (word32)keySz); XFREE(keyBuf, pkey->heap, DYNAMIC_TYPE_DER); pkey->pkey.ptr = (char*)derBuf; } else { + /* The encoding is abandoned but keyBuf stays on the pkey, + * so do not leave the key material behind in it. */ + ForceZero(keyBuf, (word32)keySz); ret = MEMORY_E; } derSz = (int)sz; @@ -9349,8 +9381,7 @@ static int PopulateRSAEvpPkeyDer(WOLFSSL_EVP_PKEY *pkey) if (ret < 0) { WOLFSSL_MSG("PopulateRSAEvpPkeyDer failed"); - /* pkey_sz is zero here, so the header size cannot stay behind or the - * export paths subtract it from zero and underflow. */ + /* As above: pkey_sz is zero here, so the header size cannot stay. */ pkey->pkcs8HeaderSz = 0; return WOLFSSL_FAILURE; } @@ -9803,6 +9834,13 @@ static int ECC_populate_EVP_PKEY(WOLFSSL_EVP_PKEY* pkey, WOLFSSL_EC_KEY *key) if (derBuf) { if (wc_EccKeyToPKCS8(ecc, derBuf, (word32*)&derSz) >= 0) { if (pkey->pkey.ptr) { + /* The outgoing buffer can hold a private key + * encoding, so wipe it before it is returned to + * the allocator. */ + if (pkey->pkey_sz > 0) { + ForceZero(pkey->pkey.ptr, + (word32)pkey->pkey_sz); + } XFREE(pkey->pkey.ptr, pkey->heap, DYNAMIC_TYPE_OPENSSL); } pkey->pkey_sz = (int)derSz; @@ -9842,10 +9880,21 @@ static int ECC_populate_EVP_PKEY(WOLFSSL_EVP_PKEY* pkey, WOLFSSL_EC_KEY *key) if (derBuf) { if (wc_EccKeyToDer(ecc, derBuf, (word32)derSz) >= 0) { if (pkey->pkey.ptr) { + /* As above, the outgoing buffer can hold a + * private key encoding. */ + if (pkey->pkey_sz > 0) { + ForceZero(pkey->pkey.ptr, + (word32)pkey->pkey_sz); + } XFREE(pkey->pkey.ptr, pkey->heap, DYNAMIC_TYPE_OPENSSL); } pkey->pkey_sz = (int)derSz; pkey->pkey.ptr = (char*)derBuf; + /* The encoding carries no PKCS#8 wrapper, so a header + * size left from a wrapped predecessor has to go with + * it, or the export paths start inside the new + * encoding. */ + pkey->pkcs8HeaderSz = 0; return WOLFSSL_SUCCESS; } else { @@ -9859,13 +9908,44 @@ static int ECC_populate_EVP_PKEY(WOLFSSL_EVP_PKEY* pkey, WOLFSSL_EC_KEY *key) else if (ecc->type == ECC_PUBLICKEY) { if ((derSz = wc_EccPublicKeyDerSize(ecc, 1)) > 0) { #ifdef WOLFSSL_NO_REALLOC - derBuf = (byte*)XMALLOC((size_t)derSz, pkey->heap, DYNAMIC_TYPE_OPENSSL); + /* The encoding below fills the new buffer, so nothing is carried + * over from the old one. It can be smaller than the old encoding. + * Allocate before the old buffer is touched, so that a failure + * here leaves the encoding held so far in place. */ + derBuf = (byte*)XMALLOC((size_t)derSz, pkey->heap, + DYNAMIC_TYPE_OPENSSL); if (derBuf != NULL) { - XMEMCPY(derBuf, pkey->pkey.ptr, (size_t)pkey->pkey_sz); + /* The buffer being released can hold a private key encoding, + * so wipe it first. The size is dropped with the contents so + * a failure below cannot leave pkey_sz describing a buffer + * that no longer holds an encoding. A SubjectPublicKeyInfo + * carries no PKCS#8 wrapper, so the header size has to go + * with it as well, or a header size left from a wrapped + * predecessor would make the export paths start inside the + * new encoding. */ + if (pkey->pkey.ptr != NULL && pkey->pkey_sz > 0) { + ForceZero(pkey->pkey.ptr, (word32)pkey->pkey_sz); + } XFREE(pkey->pkey.ptr, pkey->heap, DYNAMIC_TYPE_OPENSSL); pkey->pkey.ptr = NULL; + pkey->pkey_sz = 0; + pkey->pkcs8HeaderSz = 0; } #else + /* XREALLOC consumes the old pointer, so the buffer has to be + * wiped before the call: it can hold a private key encoding. The + * size is dropped with the contents so a failure below cannot + * leave pkey_sz describing a buffer that no longer holds an + * encoding. A SubjectPublicKeyInfo carries no PKCS#8 wrapper, so + * the header size has to go with it as well, or a header size + * left from a wrapped predecessor would make the export paths + * start inside the new encoding. */ + if (pkey->pkey.ptr != NULL && pkey->pkey_sz > 0) { + ForceZero(pkey->pkey.ptr, (word32)pkey->pkey_sz); + } + pkey->pkey_sz = 0; + pkey->pkcs8HeaderSz = 0; + derBuf = (byte*)XREALLOC(pkey->pkey.ptr, (size_t)derSz, pkey->heap, DYNAMIC_TYPE_OPENSSL); #endif @@ -9876,7 +9956,6 @@ static int ECC_populate_EVP_PKEY(WOLFSSL_EVP_PKEY* pkey, WOLFSSL_EC_KEY *key) XFREE(derBuf, pkey->heap, DYNAMIC_TYPE_OPENSSL); derBuf = NULL; pkey->pkey.ptr = NULL; - pkey->pkey_sz = 0; } } } @@ -12281,8 +12360,12 @@ void wolfSSL_EVP_PKEY_free(WOLFSSL_EVP_PKEY* key) wc_FreeRng(&key->rng); if (key->pkey.ptr != NULL) { + /* Holds the private key DER for a private pkey. */ + if (key->pkey_sz > 0) + ForceZero(key->pkey.ptr, (word32)key->pkey_sz); XFREE(key->pkey.ptr, key->heap, DYNAMIC_TYPE_PUBLIC_KEY); key->pkey.ptr = NULL; + key->pkey_sz = 0; } switch(key->type) { From 5d2188f958562a2a793c8770494a3e27e5076a31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Frauenschl=C3=A4ger?= Date: Wed, 5 Aug 2026 12:05:57 +0200 Subject: [PATCH 5/8] CI: build the cert sign callback path WOLFSSL_CERT_SIGN_CB is set only by --enable-certsigncb, and that flag appears nowhere under .github/. wc_SignCert_cb() is therefore compiled by no CI job, and neither is test_wc_SignCert_cb(), the only test covering it. The buffer bounds check the preceding commits add to that function, and the test assertions that go with it, would have merged without anything building them. Added as its own entry in the os-check Linux config list, in sorted position. "minutes" is seeded at 8.3 from the sibling --enable-all entries rather than omitted: the omission defaults it to 1.0, which sorts an eight minute build last in a list scheduled longest-first and deals it into whichever shard is already fullest. It also suppresses the stale estimate annotation that would otherwise prompt the refresh. Replace it with the real number from the Minutes column of the first run. Verified by running the entry through the workflow's own driver, .github/scripts/parallel-make-check.py, with the CFLAGS the workflow applies at make time (-pedantic -Wdeclaration-after-statement -Wnull-dereference -Wno-overlength-strings -DTEST_LIBWOLFSSL_SOURCES_INCLUSION_SEQUENCE) and --private-dir=certs. The out-of-tree build produces no compiler warnings and make check reports 17 passed, 6 skipped, 0 failed. Both test_wc_SignCert_buffer_bounds and test_wc_SignCert_cb run rather than skip. --- .github/configs/os-check-linux.json | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/configs/os-check-linux.json b/.github/configs/os-check-linux.json index 23dfc3a4e24..35f4d15510f 100644 --- a/.github/configs/os-check-linux.json +++ b/.github/configs/os-check-linux.json @@ -16,6 +16,9 @@ "configure": ["--enable-all", "--enable-asn=original"]}, {"name": "all-certgencache", "minutes": 8.3, "configure": ["--enable-all", "--enable-certgencache"]}, +{"name": "all-certsigncb", "minutes": 8.3, + "comment": "Only --enable-certsigncb sets WOLFSSL_CERT_SIGN_CB, so without this entry wc_SignCert_cb() and its test are never compiled anywhere in CI. Minutes is seeded from the sibling --enable-all entries; refresh from a run's step summary.", + "configure": ["--enable-all", "--enable-certsigncb"]}, {"name": "all-dtls13-frag-ch-no-mlkem", "minutes": 8.2, "configure": ["--enable-all", "--enable-dtls13", "--enable-dtls-frag-ch", "--disable-mlkem"]}, From e266d46e1498b497e7409d0599a4f3a54e6646b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Frauenschl=C3=A4ger?= Date: Wed, 5 Aug 2026 14:58:57 +0200 Subject: [PATCH 6/8] Release the previous DH key in EVP_PKEY_keygen The DH case of wolfSSL_EVP_PKEY_keygen() assigned straight over pkey->dh: case WC_EVP_PKEY_DH: pkey->dh = wolfSSL_DH_new(); A caller supplied EVP_PKEY can already hold a DH object. wolfSSL_EVP_PKEY_set1_DH() takes a reference and sets ownDh, and wolfSSL_EVP_PKEY_assign_DH() installs one outright, so keygen on such a pkey dropped the only pointer the EVP_PKEY had to that object without releasing its reference, and nothing freed it afterwards. The case now generates into a temporary and frees the previous key when the pkey owned it, which is the shape the RSA case in the same switch uses. Adds test_wolfSSL_EVP_PKEY_keygen_dh_reuse(), which loads DH parameters, puts them on an EVP_PKEY with set1_DH so the pkey holds a reference, and then runs keygen on that same pkey. The leak itself is not asserted by the test: it needs an allocation tracker, and the smoke-test sanitize-asan job provides one, since it builds with AddressSanitizer and sets no ASAN_OPTIONS, so LeakSanitizer runs by default there. What the test does locally is drive the path and show it stays free of double frees under AddressSanitizer. --- tests/api/test_evp_pkey.c | 38 ++++++++++++++++++++++++++++++++++++++ tests/api/test_evp_pkey.h | 2 ++ wolfcrypt/src/evp.c | 12 ++++++++++-- 3 files changed, 50 insertions(+), 2 deletions(-) diff --git a/tests/api/test_evp_pkey.c b/tests/api/test_evp_pkey.c index 2ab9e11e4dd..217d0c75115 100644 --- a/tests/api/test_evp_pkey.c +++ b/tests/api/test_evp_pkey.c @@ -1681,6 +1681,44 @@ int test_wolfSSL_EVP_PKEY_set1_shrinking_der(void) return EXPECT_RESULT(); } +/* + * EVP_PKEY_keygen() on a pkey that already carries a DH object has to release + * it rather than overwrite the only reference to it. + */ +int test_wolfSSL_EVP_PKEY_keygen_dh_reuse(void) +{ + EXPECT_DECLS; +#if (defined(OPENSSL_ALL) || defined(WOLFSSL_QT) || \ + defined(WOLFSSL_OPENSSH)) && !defined(NO_DH) && \ + defined(WOLFSSL_DH_EXTRA) && !defined(NO_FILESYSTEM) && \ + !defined(NO_CERTS) && (!defined(HAVE_FIPS) || FIPS_VERSION_GT(2,0)) + WOLFSSL_EVP_PKEY* pkey = NULL; + WOLFSSL_DH* dh = NULL; + EVP_PKEY_CTX* ctx = NULL; + byte* buf = NULL; + size_t bufSz = 0; + + ExpectIntEQ(load_file("./certs/dh2048.der", &buf, &bufSz), 0); + ExpectNotNull(dh = wolfSSL_DH_new()); + ExpectIntEQ(wolfSSL_DH_LoadDer(dh, buf, (int)bufSz), WOLFSSL_SUCCESS); + + /* set1 leaves the pkey holding a reference of its own, which keygen has to + * release when it installs the generated key. */ + ExpectNotNull(pkey = wolfSSL_EVP_PKEY_new()); + ExpectIntEQ(wolfSSL_EVP_PKEY_set1_DH(pkey, dh), WOLFSSL_SUCCESS); + + ExpectNotNull(ctx = EVP_PKEY_CTX_new(pkey, NULL)); + ExpectIntEQ(EVP_PKEY_keygen_init(ctx), WOLFSSL_SUCCESS); + ExpectIntEQ(EVP_PKEY_keygen(ctx, &pkey), WOLFSSL_SUCCESS); + + EVP_PKEY_CTX_free(ctx); + wolfSSL_DH_free(dh); + XFREE(buf, NULL, DYNAMIC_TYPE_TMP_BUFFER); + wolfSSL_EVP_PKEY_free(pkey); +#endif + return EXPECT_RESULT(); +} + int test_wolfSSL_EVP_SignInit_ex(void) { EXPECT_DECLS; diff --git a/tests/api/test_evp_pkey.h b/tests/api/test_evp_pkey.h index cae4894fc29..3558b97ec93 100644 --- a/tests/api/test_evp_pkey.h +++ b/tests/api/test_evp_pkey.h @@ -54,6 +54,7 @@ int test_wolfSSL_EVP_PKEY_keygen(void); int test_wolfSSL_EVP_PKEY_keygen_reuse(void); int test_wolfSSL_EVP_PKEY_set1_EC_KEY_no_pkcs8(void); int test_wolfSSL_EVP_PKEY_set1_shrinking_der(void); +int test_wolfSSL_EVP_PKEY_keygen_dh_reuse(void); int test_wolfSSL_EVP_SignInit_ex(void); int test_wolfSSL_EVP_PKEY_sign_verify_rsa(void); int test_wolfSSL_EVP_PKEY_sign_verify_dsa(void); @@ -105,6 +106,7 @@ int test_wolfSSL_EVP_PKEY_encoded_public_key(void); TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_keygen_reuse), \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_set1_EC_KEY_no_pkcs8), \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_set1_shrinking_der), \ + TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_keygen_dh_reuse), \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_SignInit_ex), \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_sign_verify_rsa), \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_sign_verify_dsa), \ diff --git a/wolfcrypt/src/evp.c b/wolfcrypt/src/evp.c index 1862a0bad60..0785b7ad274 100644 --- a/wolfcrypt/src/evp.c +++ b/wolfcrypt/src/evp.c @@ -3816,6 +3816,9 @@ int wolfSSL_EVP_PKEY_keygen(WOLFSSL_EVP_PKEY_CTX *ctx, #if defined(WOLFSSL_KEY_GEN) && !defined(NO_RSA) WOLFSSL_RSA* rsaTmp; #endif +#if !defined(NO_DH) && (!defined(HAVE_FIPS) || FIPS_VERSION_GT(2,0)) + WOLFSSL_DH* dhTmp; +#endif WOLFSSL_ENTER("wolfSSL_EVP_PKEY_keygen"); @@ -3884,8 +3887,13 @@ int wolfSSL_EVP_PKEY_keygen(WOLFSSL_EVP_PKEY_CTX *ctx, #endif #if !defined(NO_DH) && (!defined(HAVE_FIPS) || FIPS_VERSION_GT(2,0)) case WC_EVP_PKEY_DH: - pkey->dh = wolfSSL_DH_new(); - if (pkey->dh) { + dhTmp = wolfSSL_DH_new(); + if (dhTmp != NULL) { + /* A caller supplied pkey may already carry a DH object, so + * release it rather than overwrite the only reference to it. */ + if (pkey->dh != NULL && pkey->ownDh == 1) + wolfSSL_DH_free(pkey->dh); + pkey->dh = dhTmp; pkey->ownDh = 1; /* load DH params from CTX */ ret = wolfSSL_DH_LoadDer(pkey->dh, From 009ded5b836fe6cce30017657089465a8f1d9c60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Frauenschl=C3=A4ger?= Date: Wed, 5 Aug 2026 14:59:32 +0200 Subject: [PATCH 7/8] Tighten EC key ownership on the EVP_PKEY This is hardening rather than a fix for a reachable defect. Both branches below are wrong as written, but no entry point tested reaches them: wolfSSL_d2i_PrivateKey() and wolfSSL_d2i_PUBKEY() both leave pkey->ecc populated, so wolfSSL_EVP_PKEY_get1_EC_KEY() always takes its up_ref path. wolfSSL_EVP_PKEY_get1_EC_KEY() has a branch that builds an EC_KEY when the pkey does not carry one, caches it on the pkey and returns it. It did that without taking a second reference and without setting ownEcc, so the single reference the key was created with was handed to the caller while the pkey kept an unowned pointer to it. A caller releasing what get1 gave it, as the contract requires, would leave pkey->ecc dangling. The pkey now keeps the reference the key was created with and the caller gets one of its own. The same branch freed the key when neither DER load succeeded but left pkey->ecc pointing at it. That pointer is now cleared. wolfSSL_EVP_PKEY_keygen() set ownEcc on the EC path whether or not it had created the key, so a key placed on the pkey by something that did not transfer ownership would gain a second owner. Ownership is now claimed where the key is created. Adds test_wolfSSL_EVP_PKEY_get1_EC_KEY_reuse(), which releases the reference get1 returns and then calls get1 again. It covers the path a decoded pkey actually takes and pins the reference contract; it passes with and without the change above, which the comment on the test says plainly so it is not mistaken for a regression test. --- tests/api/test_evp_pkey.c | 45 +++++++++++++++++++++++++++++++++++++++ tests/api/test_evp_pkey.h | 2 ++ wolfcrypt/src/evp.c | 17 ++++++++++++++- 3 files changed, 63 insertions(+), 1 deletion(-) diff --git a/tests/api/test_evp_pkey.c b/tests/api/test_evp_pkey.c index 217d0c75115..775ed9eeccd 100644 --- a/tests/api/test_evp_pkey.c +++ b/tests/api/test_evp_pkey.c @@ -1681,6 +1681,51 @@ int test_wolfSSL_EVP_PKEY_set1_shrinking_der(void) return EXPECT_RESULT(); } +/* + * wolfSSL_EVP_PKEY_get1_EC_KEY() hands the caller a reference of its own, so + * releasing it has to leave the copy the EVP_PKEY holds intact and usable. + * + * This covers the path where the pkey already carries the EC_KEY, which is + * what every decode entry point produces. It passes with and without the + * reference fix in the branch that builds the key instead, since no decode + * path reaches that branch; it is here to pin the reference contract rather + * than as a regression test for it. + */ +int test_wolfSSL_EVP_PKEY_get1_EC_KEY_reuse(void) +{ + EXPECT_DECLS; +#if defined(OPENSSL_EXTRA) && defined(HAVE_ECC) && !defined(NO_FILESYSTEM) && \ + !defined(NO_CERTS) && !defined(NO_ASN) && !defined(NO_PWDBASED) + WOLFSSL_EVP_PKEY* pkey = NULL; + WOLFSSL_EC_KEY* ec1 = NULL; + WOLFSSL_EC_KEY* ec2 = NULL; + const unsigned char* in; + byte* buf = NULL; + size_t bufSz = 0; + + /* A pkey decoded from DER carries no EC_KEY yet, so the first get1 is the + * call that builds and caches one. */ + ExpectIntEQ(load_file("./certs/ecc-client-key.der", &buf, &bufSz), 0); + in = buf; + ExpectNotNull(pkey = wolfSSL_d2i_PrivateKey(EVP_PKEY_EC, NULL, &in, + (long)bufSz)); + + ExpectNotNull(ec1 = wolfSSL_EVP_PKEY_get1_EC_KEY(pkey)); + wolfSSL_EC_KEY_free(ec1); + ec1 = NULL; + + /* The pkey still holds a live key, so this neither reads freed memory nor + * returns NULL. */ + ExpectNotNull(ec2 = wolfSSL_EVP_PKEY_get1_EC_KEY(pkey)); + ExpectNotNull(wolfSSL_EC_KEY_get0_public_key(ec2)); + wolfSSL_EC_KEY_free(ec2); + + XFREE(buf, NULL, DYNAMIC_TYPE_TMP_BUFFER); + wolfSSL_EVP_PKEY_free(pkey); +#endif + return EXPECT_RESULT(); +} + /* * EVP_PKEY_keygen() on a pkey that already carries a DH object has to release * it rather than overwrite the only reference to it. diff --git a/tests/api/test_evp_pkey.h b/tests/api/test_evp_pkey.h index 3558b97ec93..c679bd2db89 100644 --- a/tests/api/test_evp_pkey.h +++ b/tests/api/test_evp_pkey.h @@ -54,6 +54,7 @@ int test_wolfSSL_EVP_PKEY_keygen(void); int test_wolfSSL_EVP_PKEY_keygen_reuse(void); int test_wolfSSL_EVP_PKEY_set1_EC_KEY_no_pkcs8(void); int test_wolfSSL_EVP_PKEY_set1_shrinking_der(void); +int test_wolfSSL_EVP_PKEY_get1_EC_KEY_reuse(void); int test_wolfSSL_EVP_PKEY_keygen_dh_reuse(void); int test_wolfSSL_EVP_SignInit_ex(void); int test_wolfSSL_EVP_PKEY_sign_verify_rsa(void); @@ -106,6 +107,7 @@ int test_wolfSSL_EVP_PKEY_encoded_public_key(void); TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_keygen_reuse), \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_set1_EC_KEY_no_pkcs8), \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_set1_shrinking_der), \ + TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_get1_EC_KEY_reuse), \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_keygen_dh_reuse), \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_SignInit_ex), \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_sign_verify_rsa), \ diff --git a/wolfcrypt/src/evp.c b/wolfcrypt/src/evp.c index 0785b7ad274..37f6ba51557 100644 --- a/wolfcrypt/src/evp.c +++ b/wolfcrypt/src/evp.c @@ -3874,11 +3874,15 @@ int wolfSSL_EVP_PKEY_keygen(WOLFSSL_EVP_PKEY_CTX *ctx, * prior call to wolfSSL_EVP_PKEY_paramgen. */ if (pkey->ecc == NULL) { pkey->ecc = wolfSSL_EC_KEY_new_by_curve_name(ctx->curveNID); + /* Ownership is claimed where the key is created. Claiming it + * for a key that was already on the pkey would hand the free + * to a second owner. */ + if (pkey->ecc != NULL) + pkey->ownEcc = 1; } if (pkey->ecc) { ret = wolfSSL_EC_KEY_generate_key(pkey->ecc); if (ret == WOLFSSL_SUCCESS) { - pkey->ownEcc = 1; if (ECC_populate_EVP_PKEY(pkey, pkey->ecc) != WOLFSSL_SUCCESS) ret = WOLFSSL_FAILURE; } @@ -9628,6 +9632,17 @@ WOLFSSL_EC_KEY* wolfSSL_EVP_PKEY_get1_EC_KEY(WOLFSSL_EVP_PKEY* key) WOLFSSL_EC_KEY_LOAD_PUBLIC) != WOLFSSL_SUCCESS) { wolfSSL_EC_KEY_free(local); + /* The pkey must not keep a pointer to the freed key. */ + key->ecc = NULL; + local = NULL; + } + } + if (local != NULL) { + /* The key was created with a single reference. It stays on the + * pkey, so that one belongs to the pkey and the caller needs + * its own, which it releases as the get1 contract requires. */ + key->ownEcc = 1; + if (wolfSSL_EC_KEY_up_ref(local) != WOLFSSL_SUCCESS) { local = NULL; } } From 63dc0dc827681ca269d40382b752f479ebedc8c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Frauenschl=C3=A4ger?= Date: Thu, 6 Aug 2026 10:12:09 +0200 Subject: [PATCH 8/8] Reset stale key metadata when d2i reuses an EVP_PKEY d2i_make_pkey() replaces the key data, size and type of a caller-supplied WOLFSSL_EVP_PKEY, but left pkcs8HeaderSz, mldsaOID, pkey_curve and save_type describing the key the object held before. A stale pkcs8HeaderSz is the damaging one. pkcs8_encode() and wolfssl_i_evp_pkey_get_der() both encode from pkey.ptr + pkcs8HeaderSz, so after d2i_PrivateKey(EVP_PKEY_RSA, NULL, &p, pkcs8RsaDer); d2i_PrivateKey_EVP(&pkey, &q, traditionalEccDer); PEM_write_bio_PKCS8PrivateKey() reports success while wrapping the ECC key with its first 26 bytes cut off, and the resulting PEM cannot be read back. Only the d2i_PUBKEY and d2i_PrivateKey_EVP routes are affected; d2i_PrivateKey and d2i_AutoPrivateKey go through d2i_evp_pkey(), which allocates a fresh object and recomputes the header size. The same branch also drops the data and the key object of the previous key without releasing either. pkey.ptr is overwritten with a fresh allocation, and wolfSSL_EVP_PKEY_free() only disposes of the object matching the type currently set, so the object of a key whose type has since changed is never freed. The sequence above leaks the 1219 byte RSA encoding together with the WOLFSSL_RSA and its bignums, 13 allocations in all. The data is released after the new encoding has been copied in, since the caller may be decoding out of it. Clear the metadata and dispose of the previous key on the reuse branch, so a reused object decodes to the same state as a new one, and add a regression test comparing the PKCS#8 output of a reused key against a freshly decoded one. --- tests/api/test_evp_pkey.c | 88 +++++++++++++++++++++++++++++++++++++++ tests/api/test_evp_pkey.h | 4 +- wolfcrypt/src/evp_pk.c | 37 +++++++++++++++- 3 files changed, 127 insertions(+), 2 deletions(-) diff --git a/tests/api/test_evp_pkey.c b/tests/api/test_evp_pkey.c index 775ed9eeccd..f0d2b838d48 100644 --- a/tests/api/test_evp_pkey.c +++ b/tests/api/test_evp_pkey.c @@ -3448,3 +3448,91 @@ int test_wolfSSL_EVP_PKEY_encoded_public_key(void) return EXPECT_RESULT(); } + +int test_wolfSSL_d2i_PrivateKey_reuse_resets_state(void) +{ + EXPECT_DECLS; +/* wolfSSL_d2i_PrivateKey_EVP() and wolfSSL_PEM_write_bio_PKCS8PrivateKey() are + * both OPENSSL_ALL, and the latter also needs PKCS#8 and a password based key + * derivation. */ +#if defined(OPENSSL_ALL) && defined(HAVE_PKCS8) && !defined(NO_PWDBASED) && \ + defined(HAVE_ECC) && !defined(NO_RSA) && !defined(NO_FILESYSTEM) && \ + !defined(NO_BIO) && defined(WOLFSSL_DER_TO_PEM) + WOLFSSL_EVP_PKEY* pkey = NULL; + unsigned char* rsaDer = NULL; + unsigned char* eccDer = NULL; + unsigned char* freshPem = NULL; + const unsigned char* p = NULL; + unsigned char* q = NULL; + WOLFSSL_BIO* bio = NULL; + char* pem = NULL; + int rsaSz = 0; + int eccSz = 0; + int freshSz = 0; + XFILE f = XBADFILE; + + ExpectNotNull(rsaDer = (unsigned char*)XMALLOC(4096, NULL, + DYNAMIC_TYPE_TMP_BUFFER)); + ExpectNotNull(eccDer = (unsigned char*)XMALLOC(4096, NULL, + DYNAMIC_TYPE_TMP_BUFFER)); + + ExpectTrue((f = XFOPEN("./certs/server-keyPkcs8.der", "rb")) != XBADFILE); + ExpectIntGT(rsaSz = (int)XFREAD(rsaDer, 1, 4096, f), 0); + if (f != XBADFILE) { + XFCLOSE(f); + f = XBADFILE; + } + ExpectTrue((f = XFOPEN("./certs/ecc-key.der", "rb")) != XBADFILE); + ExpectIntGT(eccSz = (int)XFREAD(eccDer, 1, 4096, f), 0); + if (f != XBADFILE) { + XFCLOSE(f); + f = XBADFILE; + } + + /* Baseline: decode the traditional ECC key into a new object and record + * its PKCS#8 PEM. */ + q = eccDer; + ExpectNotNull(pkey = wolfSSL_d2i_PrivateKey_EVP(NULL, &q, (long)eccSz)); + ExpectIntEQ(wolfSSL_EVP_PKEY_id(pkey), WC_EVP_PKEY_EC); + ExpectNotNull(bio = wolfSSL_BIO_new(wolfSSL_BIO_s_mem())); + ExpectIntGT(wolfSSL_PEM_write_bio_PKCS8PrivateKey(bio, pkey, NULL, NULL, 0, + NULL, NULL), 0); + ExpectIntGT(freshSz = (int)wolfSSL_BIO_get_mem_data(bio, &pem), 0); + ExpectNotNull(freshPem = (unsigned char*)XMALLOC((size_t)freshSz, NULL, + DYNAMIC_TYPE_TMP_BUFFER)); + if (freshPem != NULL && pem != NULL) { + XMEMCPY(freshPem, pem, (size_t)freshSz); + } + wolfSSL_BIO_free(bio); + bio = NULL; + wolfSSL_EVP_PKEY_free(pkey); + pkey = NULL; + + /* A PKCS#8 RSA key records a non-zero pkcs8HeaderSz. */ + p = rsaDer; + ExpectNotNull(pkey = wolfSSL_d2i_PrivateKey(WC_EVP_PKEY_RSA, NULL, &p, + (long)rsaSz)); + ExpectIntGT((int)pkey->pkcs8HeaderSz, 0); + + /* Reusing that object for the traditional ECC key must clear it, so the + * re-encoded key is not sliced at the previous key's header offset. */ + q = eccDer; + ExpectNotNull(wolfSSL_d2i_PrivateKey_EVP(&pkey, &q, (long)eccSz)); + ExpectIntEQ(wolfSSL_EVP_PKEY_id(pkey), WC_EVP_PKEY_EC); + ExpectIntEQ((int)pkey->pkcs8HeaderSz, 0); + + /* The reused object must encode exactly like the fresh one. */ + ExpectNotNull(bio = wolfSSL_BIO_new(wolfSSL_BIO_s_mem())); + ExpectIntGT(wolfSSL_PEM_write_bio_PKCS8PrivateKey(bio, pkey, NULL, NULL, 0, + NULL, NULL), 0); + ExpectIntEQ((int)wolfSSL_BIO_get_mem_data(bio, &pem), freshSz); + ExpectBufEQ(pem, freshPem, freshSz); + + wolfSSL_BIO_free(bio); + wolfSSL_EVP_PKEY_free(pkey); + XFREE(freshPem, NULL, DYNAMIC_TYPE_TMP_BUFFER); + XFREE(eccDer, NULL, DYNAMIC_TYPE_TMP_BUFFER); + XFREE(rsaDer, NULL, DYNAMIC_TYPE_TMP_BUFFER); +#endif + return EXPECT_RESULT(); +} diff --git a/tests/api/test_evp_pkey.h b/tests/api/test_evp_pkey.h index c679bd2db89..65df74ce183 100644 --- a/tests/api/test_evp_pkey.h +++ b/tests/api/test_evp_pkey.h @@ -74,6 +74,7 @@ int test_wolfSSL_EVP_PKEY_ed448(void); int test_wolfSSL_EVP_PKEY_x25519(void); int test_wolfSSL_EVP_PKEY_x448(void); int test_wolfSSL_EVP_PKEY_encoded_public_key(void); +int test_wolfSSL_d2i_PrivateKey_reuse_resets_state(void); #define TEST_EVP_PKEY_DECLS \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_CTX_new_id), \ @@ -126,6 +127,7 @@ int test_wolfSSL_EVP_PKEY_encoded_public_key(void); TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_ed448), \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_x25519), \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_x448), \ - TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_encoded_public_key) + TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_encoded_public_key), \ + TEST_DECL_GROUP("evp_pkey", test_wolfSSL_d2i_PrivateKey_reuse_resets_state) #endif /* WOLFCRYPT_TEST_EVP_PKEY_H */ diff --git a/wolfcrypt/src/evp_pk.c b/wolfcrypt/src/evp_pk.c index 73b9ddc3f6a..7a81f0d94a4 100644 --- a/wolfcrypt/src/evp_pk.c +++ b/wolfcrypt/src/evp_pk.c @@ -51,11 +51,36 @@ static int d2i_make_pkey(WOLFSSL_EVP_PKEY** out, const unsigned char* mem, word32 memSz, int priv, int type) { WOLFSSL_EVP_PKEY* pkey; + char* prevData = NULL; + int prevSz = 0; int ret = 1; /* Get or create the EVP PKEY object. */ if (*out != NULL) { pkey = *out; + /* Hold on to the data of the key this object held before. It is + * disposed of once the new key data has been copied in, as the caller + * may be decoding out of it. */ + prevData = pkey->pkey.ptr; + prevSz = pkey->pkey_sz; + pkey->pkey.ptr = NULL; + pkey->pkey_sz = 0; + #ifdef OPENSSL_EXTRA + /* Dispose of the key object of the key this object held before. The + * type is about to change and wolfSSL_EVP_PKEY_free() only disposes of + * the object matching the type set. */ + clearEVPPkeyKeys(pkey); + #endif + /* Drop metadata describing the key this object held before, so a + * reused object decodes to the same state as a new one. */ + pkey->pkcs8HeaderSz = 0; + pkey->save_type = 0; + #ifdef HAVE_ECC + pkey->pkey_curve = 0; + #endif + #ifdef WOLFSSL_HAVE_MLDSA + WOLFSSL_ATOMIC_STORE(pkey->mldsaOID, 0); + #endif } else { pkey = wolfSSL_EVP_PKEY_new(); @@ -68,9 +93,11 @@ static int d2i_make_pkey(WOLFSSL_EVP_PKEY** out, const unsigned char* mem, /* Set the size and allocate memory for key data to be copied into. */ pkey->pkey_sz = (int)memSz; if (memSz > 0) { - pkey->pkey.ptr = (char*)XMALLOC((size_t)memSz, NULL, + pkey->pkey.ptr = (char*)XMALLOC((size_t)memSz, pkey->heap, priv ? DYNAMIC_TYPE_PRIVATE_KEY : DYNAMIC_TYPE_PUBLIC_KEY); if (pkey->pkey.ptr == NULL) { + /* No encoding held - do not describe one. */ + pkey->pkey_sz = 0; ret = 0; } if (ret == 1) { @@ -78,6 +105,14 @@ static int d2i_make_pkey(WOLFSSL_EVP_PKEY** out, const unsigned char* mem, XMEMCPY(pkey->pkey.ptr, mem, memSz); } } + /* The data of the key held before is no longer referenced. */ + if (prevData != NULL) { + if (prevSz > 0) { + ForceZero(prevData, (word32)prevSz); + } + XFREE(prevData, pkey->heap, + priv ? DYNAMIC_TYPE_PRIVATE_KEY : DYNAMIC_TYPE_PUBLIC_KEY); + } if (ret == 1) { /* Set key type passed in and return object. */ pkey->type = type;