From 9a58301ab174dbe77c488c913764fdb3164f0bb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Frauenschl=C3=A4ger?= Date: Thu, 23 May 2024 14:24:06 +0200 Subject: [PATCH] Fix PQC and hybrid certificate regressions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Due to recent changes in the logic to decode private keys and to parse the TLS1.3 CertificateVerify message, some regressions regarding PQC private keys and hybrid certificates have been introduced: * Decoding PQC private keys fails as the PKCS8 header of a decoded DER file is now already removed before parsing the key. * The key size wasn't properly stored in the context for PQC keys after decoding a certificate (always the maximum size) * The two 16-bit size values in case of a hybrid signature in the CertificateVerify message have been incorrectly decoded as 32-bit values instead of 16-bit values. This resulted in wrong values, leading to segmentation faults. All three regressions are fixed with the changes in this commit. Signed-off-by: Tobias Frauenschläger --- src/ssl_load.c | 32 ++++---------------------------- src/tls13.c | 7 +++++-- wolfcrypt/src/dilithium.c | 2 +- wolfcrypt/src/falcon.c | 2 +- wolfcrypt/src/sphincs.c | 2 +- 5 files changed, 12 insertions(+), 33 deletions(-) diff --git a/src/ssl_load.c b/src/ssl_load.c index 3aa449cd5e..a37273e006 100644 --- a/src/ssl_load.c +++ b/src/ssl_load.c @@ -1622,9 +1622,7 @@ static int ProcessBufferCertPublicKey(WOLFSSL_CTX* ctx, WOLFSSL* ssl, DecodedCert* cert, int checkKeySz) { int ret = 0; -#ifdef WOLF_PRIVATE_KEY_ID byte keyType = 0; -#endif int keySz = 0; #ifndef NO_RSA word32 idx; @@ -1637,9 +1635,7 @@ static int ProcessBufferCertPublicKey(WOLFSSL_CTX* ctx, WOLFSSL* ssl, case RSAPSSk: #endif case RSAk: - #ifdef WOLF_PRIVATE_KEY_ID keyType = rsa_sa_algo; - #endif /* Determine RSA key size by parsing public key */ idx = 0; ret = wc_RsaPublicKeyDecode_ex(cert->publicKey, &idx, @@ -1652,9 +1648,7 @@ static int ProcessBufferCertPublicKey(WOLFSSL_CTX* ctx, WOLFSSL* ssl, #endif /* !NO_RSA */ #ifdef HAVE_ECC case ECDSAk: - #ifdef WOLF_PRIVATE_KEY_ID keyType = ecc_dsa_sa_algo; - #endif /* Determine ECC key size based on curve */ #ifdef WOLFSSL_CUSTOM_CURVES if ((cert->pkCurveOID == 0) && (cert->pkCurveSize != 0)) { @@ -1676,9 +1670,7 @@ static int ProcessBufferCertPublicKey(WOLFSSL_CTX* ctx, WOLFSSL* ssl, #endif /* HAVE_ECC */ #if defined(WOLFSSL_SM2) && defined(WOLFSSL_SM3) case SM2k: - #ifdef WOLF_PRIVATE_KEY_ID keyType = sm2_sa_algo; - #endif /* Determine ECC key size based on curve */ keySz = WOLFSSL_SM2_KEY_BITS / 8; if (checkKeySz) { @@ -1690,9 +1682,7 @@ static int ProcessBufferCertPublicKey(WOLFSSL_CTX* ctx, WOLFSSL* ssl, #endif /* HAVE_ED25519 */ #ifdef HAVE_ED25519 case ED25519k: - #ifdef WOLF_PRIVATE_KEY_ID keyType = ed25519_sa_algo; - #endif /* ED25519 is fixed key size */ keySz = ED25519_KEY_SIZE; if (checkKeySz) { @@ -1703,9 +1693,7 @@ static int ProcessBufferCertPublicKey(WOLFSSL_CTX* ctx, WOLFSSL* ssl, #endif /* HAVE_ED25519 */ #ifdef HAVE_ED448 case ED448k: - #ifdef WOLF_PRIVATE_KEY_ID keyType = ed448_sa_algo; - #endif /* ED448 is fixed key size */ keySz = ED448_KEY_SIZE; if (checkKeySz) { @@ -1717,9 +1705,7 @@ static int ProcessBufferCertPublicKey(WOLFSSL_CTX* ctx, WOLFSSL* ssl, #if defined(HAVE_PQC) #if defined(HAVE_FALCON) case FALCON_LEVEL1k: - #ifdef WOLF_PRIVATE_KEY_ID keyType = falcon_level1_sa_algo; - #endif /* Falcon is fixed key size */ keySz = FALCON_LEVEL1_KEY_SIZE; if (checkKeySz) { @@ -1729,11 +1715,9 @@ static int ProcessBufferCertPublicKey(WOLFSSL_CTX* ctx, WOLFSSL* ssl, } break; case FALCON_LEVEL5k: - #ifdef WOLF_PRIVATE_KEY_ID keyType = falcon_level5_sa_algo; - #endif /* Falcon is fixed key size */ - keySz = FALCON_MAX_KEY_SIZE; + keySz = FALCON_LEVEL5_KEY_SIZE; if (checkKeySz) { ret = CHECK_KEY_SZ(ssl ? ssl->options.minFalconKeySz : ctx->minFalconKeySz, FALCON_MAX_KEY_SIZE, keySz, @@ -1743,11 +1727,9 @@ static int ProcessBufferCertPublicKey(WOLFSSL_CTX* ctx, WOLFSSL* ssl, #endif /* HAVE_FALCON */ #if defined(HAVE_DILITHIUM) case DILITHIUM_LEVEL2k: - #ifdef WOLF_PRIVATE_KEY_ID keyType = dilithium_level2_sa_algo; - #endif /* Dilithium is fixed key size */ - keySz = DILITHIUM_MAX_KEY_SIZE; + keySz = DILITHIUM_LEVEL2_KEY_SIZE; if (checkKeySz) { ret = CHECK_KEY_SZ(ssl ? ssl->options.minDilithiumKeySz : ctx->minDilithiumKeySz, DILITHIUM_MAX_KEY_SIZE, keySz, @@ -1755,11 +1737,9 @@ static int ProcessBufferCertPublicKey(WOLFSSL_CTX* ctx, WOLFSSL* ssl, } break; case DILITHIUM_LEVEL3k: - #ifdef WOLF_PRIVATE_KEY_ID keyType = dilithium_level3_sa_algo; - #endif /* Dilithium is fixed key size */ - keySz = DILITHIUM_MAX_KEY_SIZE; + keySz = DILITHIUM_LEVEL3_KEY_SIZE; if (checkKeySz) { ret = CHECK_KEY_SZ(ssl ? ssl->options.minDilithiumKeySz : ctx->minDilithiumKeySz, DILITHIUM_MAX_KEY_SIZE, keySz, @@ -1767,11 +1747,9 @@ static int ProcessBufferCertPublicKey(WOLFSSL_CTX* ctx, WOLFSSL* ssl, } break; case DILITHIUM_LEVEL5k: - #ifdef WOLF_PRIVATE_KEY_ID keyType = dilithium_level5_sa_algo; - #endif /* Dilithium is fixed key size */ - keySz = DILITHIUM_MAX_KEY_SIZE; + keySz = DILITHIUM_LEVEL5_KEY_SIZE; if (checkKeySz) { ret = CHECK_KEY_SZ(ssl ? ssl->options.minDilithiumKeySz : ctx->minDilithiumKeySz, DILITHIUM_MAX_KEY_SIZE, keySz, @@ -1786,7 +1764,6 @@ static int ProcessBufferCertPublicKey(WOLFSSL_CTX* ctx, WOLFSSL* ssl, break; } -#ifdef WOLF_PRIVATE_KEY_ID /* Store the type and key size as there may not be a private key set. */ if (ssl != NULL) { ssl->buffers.keyType = keyType; @@ -1796,7 +1773,6 @@ static int ProcessBufferCertPublicKey(WOLFSSL_CTX* ctx, WOLFSSL* ssl, ctx->privateKeyType = keyType; ctx->privateKeySz = keySz; } -#endif return ret; } diff --git a/src/tls13.c b/src/tls13.c index a120374021..34d314dd18 100644 --- a/src/tls13.c +++ b/src/tls13.c @@ -10088,10 +10088,13 @@ static int DoTls13CertificateVerify(WOLFSSL* ssl, byte* input, * with their size as 16-bit integeter prior in memory. Hence, * we can decode both lengths here now. */ word32 tmpIdx = args->idx; - ato32(input + tmpIdx, &args->sigSz); + word16 tmpSz = 0; + ato16(input + tmpIdx, &tmpSz); + args->sigSz = tmpSz; tmpIdx += OPAQUE16_LEN + args->sigSz; - ato32(input + tmpIdx, &args->altSignatureSz); + ato16(input + tmpIdx, &tmpSz); + args->altSignatureSz = tmpSz; if (args->sz != (args->sigSz + args->altSignatureSz + OPAQUE16_LEN + OPAQUE16_LEN)) { diff --git a/wolfcrypt/src/dilithium.c b/wolfcrypt/src/dilithium.c index f2e241b54a..f8559ccc56 100644 --- a/wolfcrypt/src/dilithium.c +++ b/wolfcrypt/src/dilithium.c @@ -488,7 +488,7 @@ static int parse_private_key(const byte* priv, word32 privSz, /* At this point, it is still a PKCS8 private key. */ if ((ret = ToTraditionalInline(priv, &idx, privSz)) < 0) { - return ret; + /* ignore error, did not have PKCS8 header */ } /* Now it is a octet_string(concat(priv,pub)) */ diff --git a/wolfcrypt/src/falcon.c b/wolfcrypt/src/falcon.c index eef982cad7..08406d38d3 100644 --- a/wolfcrypt/src/falcon.c +++ b/wolfcrypt/src/falcon.c @@ -469,7 +469,7 @@ static int parse_private_key(const byte* priv, word32 privSz, /* At this point, it is still a PKCS8 private key. */ if ((ret = ToTraditionalInline(priv, &idx, privSz)) < 0) { - return ret; + /* ignore error, did not have PKCS8 header */ } /* Now it is a octet_string(concat(priv,pub)) */ diff --git a/wolfcrypt/src/sphincs.c b/wolfcrypt/src/sphincs.c index 6556fdb5dc..5659de178a 100644 --- a/wolfcrypt/src/sphincs.c +++ b/wolfcrypt/src/sphincs.c @@ -431,7 +431,7 @@ static int parse_private_key(const byte* priv, word32 privSz, /* At this point, it is still a PKCS8 private key. */ if ((ret = ToTraditionalInline(priv, &idx, privSz)) < 0) { - return ret; + /* ignore error, did not have PKCS8 header */ } /* Now it is a octet_string(concat(priv,pub)) */