diff --git a/tests/api/test_evp_pkey.c b/tests/api/test_evp_pkey.c index feef27266c..2ab9e11e4d 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 92c28a1521..cae4894fc2 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 eaafd5f6ce..1862a0bad6 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) {