Fix issue with ML-DSA key parsing (#2152)

This fixes a memory read misalignment in the ML-DSA public and private key parsing. I've also added new tests to verify that the test works. This should also fix the intermittent errors we've been having in our fuzz tests. The plan is to cut a release as soon as this is merged. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.
aws · Feb 1, 2025 · e1513c3 · e1513c3
1 parent 7965343
commit e1513c3
Show file tree

Hide file tree

Showing 5 changed files with 61 additions and 24 deletions.
diff --git a/crypto/evp_extra/p_pqdsa.c b/crypto/evp_extra/p_pqdsa.c
@@ -280,13 +280,9 @@ EVP_PKEY *EVP_PKEY_pqdsa_new_raw_public_key(int nid, const uint8_t *in, size_t l
     goto err;
   }
 
-  const PQDSA *pqdsa =  PQDSA_KEY_get0_dsa(ret->pkey.pqdsa_key);
-  if (pqdsa->public_key_len != len) {
-    OPENSSL_PUT_ERROR(EVP, EVP_R_INVALID_BUFFER_SIZE);
-    goto err;
-  }
-
-  if (!PQDSA_KEY_set_raw_public_key(ret->pkey.pqdsa_key, in)) {
+  CBS cbs;
+  CBS_init(&cbs, in, len);
+  if (!PQDSA_KEY_set_raw_public_key(ret->pkey.pqdsa_key, &cbs)) {
     // PQDSA_KEY_set_raw_public_key sets the appropriate error.
     goto err;
   }
@@ -316,7 +312,9 @@ EVP_PKEY *EVP_PKEY_pqdsa_new_raw_private_key(int nid, const uint8_t *in, size_t
     goto err;
   }
 
-  if (!PQDSA_KEY_set_raw_private_key(ret->pkey.pqdsa_key, in)) {
+  CBS cbs;
+  CBS_init(&cbs, in, len);
+  if (!PQDSA_KEY_set_raw_private_key(ret->pkey.pqdsa_key, &cbs)) {
     // PQDSA_KEY_set_raw_private_key sets the appropriate error.
     goto err;
   }

diff --git a/crypto/evp_extra/p_pqdsa_asn1.c b/crypto/evp_extra/p_pqdsa_asn1.c
@@ -89,18 +89,19 @@ static int pqdsa_get_pub_raw(const EVP_PKEY *pkey, uint8_t *out,
 }
 
 static int pqdsa_pub_decode(EVP_PKEY *out, CBS *params, CBS *key) {
-  // See https://datatracker.ietf.org/doc/draft-ietf-lamps-dilithium-certificates/ section 4.
-  // the only parameter that can be included is the OID which has length 9
+  // See https://datatracker.ietf.org/doc/draft-ietf-lamps-dilithium-certificates/
+  // section 4. the only parameter that can be included is the OID which has
+  // length 9
   if (CBS_len(params) != 9) {
     OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
     return 0;
   }
-  // set the pqdsa params on the fresh pkey
+  // Set the pqdsa params on |out|.
   if (!EVP_PKEY_pqdsa_set_params(out, OBJ_cbs2nid(params))) {
     OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
     return 0;
   }
-  return PQDSA_KEY_set_raw_public_key(out->pkey.pqdsa_key,CBS_data(key));
+  return PQDSA_KEY_set_raw_public_key(out->pkey.pqdsa_key, key);
 }
 
 static int pqdsa_pub_encode(CBB *out, const EVP_PKEY *pkey) {
@@ -138,21 +139,22 @@ static int pqdsa_pub_cmp(const EVP_PKEY *a, const EVP_PKEY *b) {
 }
 
 static int pqdsa_priv_decode(EVP_PKEY *out, CBS *params, CBS *key, CBS *pubkey) {
-  // See https://datatracker.ietf.org/doc/draft-ietf-lamps-dilithium-certificates/ section 6.
-  // the only parameter that can be included is the OID which has length 9
-  if (CBS_len(params) != 9 ) {
+  // See https://datatracker.ietf.org/doc/draft-ietf-lamps-dilithium-certificates/
+  // section 6. the only parameter that can be included is the OID which has
+  // length 9.
+  if (CBS_len(params) != 9) {
     OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
     return 0;
   }
 
-  // Set the pqdsa params on the fresh pkey
+  // Set the pqdsa params on |out|.
   if (!EVP_PKEY_pqdsa_set_params(out, OBJ_cbs2nid(params))) {
     OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
     return 0;
   }
 
   // Set the private key
-  if (!PQDSA_KEY_set_raw_private_key(out->pkey.pqdsa_key, CBS_data(key))) {
+  if (!PQDSA_KEY_set_raw_private_key(out->pkey.pqdsa_key, key)) {
     OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
     return 0;
   }
@@ -167,7 +169,8 @@ static int pqdsa_priv_decode(EVP_PKEY *out, CBS *params, CBS *key, CBS *pubkey)
   }
 
   // Construct the public key from the private key
-  if (!out->pkey.pqdsa_key->pqdsa->method->pqdsa_pack_pk_from_sk(public_key, CBS_data(key))) {
+  if (!out->pkey.pqdsa_key->pqdsa->method->pqdsa_pack_pk_from_sk(
+          public_key, CBS_data(key))) {
     OPENSSL_free(public_key);
     OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
     return 0;

diff --git a/crypto/evp_extra/p_pqdsa_test.cc b/crypto/evp_extra/p_pqdsa_test.cc
@@ -1854,3 +1854,27 @@ TEST_P(PerMLDSATest, ACVPSigVer) {
     }
   });
 }
+
+static const uint8_t mldsa87kPublicKeyInvalidLength[] = {
+    0x30, 0x11, 0x30, 0x0b, 0x06, 0x09, 0x60, 0x86, 0x48, 0x01,
+    0x65, 0x03, 0x04, 0x03, 0x13, 0x03, 0x02, 0x00, 0xe4};
+
+TEST(PQDSAParameterTest, ParsePublicKeyInvalidLength) {
+  CBS cbs;
+  CBS_init(&cbs, mldsa87kPublicKeyInvalidLength,
+           sizeof(mldsa87kPublicKeyInvalidLength));
+  bssl::UniquePtr<EVP_PKEY> pub_pkey_from_der(EVP_parse_public_key(&cbs));
+  ASSERT_FALSE(pub_pkey_from_der.get());
+}
+
+static const uint8_t mldsa44kPrivateKeyInvalidLength[] = {
+    0x30, 0x16, 0x02, 0x01, 0x00, 0x30, 0x0b, 0x06, 0x09, 0x60, 0x86, 0x48,
+    0x01, 0x65, 0x03, 0x04, 0x03, 0x11, 0x04, 0x04, 0x82, 0x45, 0x52, 0xd8};
+
+TEST(PQDSAParameterTest, ParsePrivateKeyInvalidLength) {
+  CBS cbs;
+  CBS_init(&cbs, mldsa44kPrivateKeyInvalidLength,
+           sizeof(mldsa44kPrivateKeyInvalidLength));
+  bssl::UniquePtr<EVP_PKEY> private_pkey_from_der(EVP_parse_private_key(&cbs));
+  ASSERT_FALSE(private_pkey_from_der.get());
+}
diff --git a/crypto/pqdsa/internal.h b/crypto/pqdsa/internal.h
@@ -76,8 +76,8 @@ PQDSA_KEY *PQDSA_KEY_new(void);
 void PQDSA_KEY_free(PQDSA_KEY *key);
 int EVP_PKEY_pqdsa_set_params(EVP_PKEY *pkey, int nid);
 
-int PQDSA_KEY_set_raw_public_key(PQDSA_KEY *key, const uint8_t *in);
-int PQDSA_KEY_set_raw_private_key(PQDSA_KEY *key, const uint8_t *in);
+int PQDSA_KEY_set_raw_public_key(PQDSA_KEY *key, CBS *in);
+int PQDSA_KEY_set_raw_private_key(PQDSA_KEY *key, CBS *in);
 #if defined(__cplusplus)
 }  // extern C
 #endif

diff --git a/crypto/pqdsa/pqdsa.c b/crypto/pqdsa/pqdsa.c
@@ -66,17 +66,29 @@ const PQDSA *PQDSA_KEY_get0_dsa(PQDSA_KEY* key) {
   return key->pqdsa;
 }
 
-int PQDSA_KEY_set_raw_public_key(PQDSA_KEY *key, const uint8_t *in) {
-  key->public_key = OPENSSL_memdup(in, key->pqdsa->public_key_len);
+int PQDSA_KEY_set_raw_public_key(PQDSA_KEY *key, CBS *in) {
+  // Check if the parsed length corresponds with the expected length.
+  if (CBS_len(in) != key->pqdsa->public_key_len) {
+    OPENSSL_PUT_ERROR(EVP, EVP_R_INVALID_BUFFER_SIZE);
+    return 0;
+  }
+
+  key->public_key = OPENSSL_memdup(CBS_data(in), key->pqdsa->public_key_len);
   if (key->public_key == NULL) {
     return 0;
   }
 
   return 1;
 }
 
-int PQDSA_KEY_set_raw_private_key(PQDSA_KEY *key, const uint8_t *in) {
-  key->private_key = OPENSSL_memdup(in, key->pqdsa->private_key_len);
+int PQDSA_KEY_set_raw_private_key(PQDSA_KEY *key, CBS *in) {
+  // Check if the parsed length corresponds with the expected length.
+  if (CBS_len(in) != key->pqdsa->private_key_len) {
+    OPENSSL_PUT_ERROR(EVP, EVP_R_INVALID_BUFFER_SIZE);
+    return 0;
+  }
+
+  key->private_key = OPENSSL_memdup(CBS_data(in), key->pqdsa->private_key_len);
   if (key->private_key == NULL) {
     return 0;
   }