Add internal APIs for ML-DSA #1999

jakemas · 2024-11-18T20:07:38Z

Issues:

Resolves #CryptoAlg-2723

Description of changes:

This PR adds the internal functions from FIPS 204: Module-Lattice-Based Digital Signature Standard. We base this implementation on the upstream reference implementation of ML-DSA pq-crystals/dilithium@444cdcc. However, the upstream commit only includes implementation of ML-DSA.Sign_internal and ML-DSA.Verify_internal, so we also include ML-DSA.KeyGen_internal to complete the implementation.

Changes:

Implementation of external ML-DSA algorithm functions:
- FIPS 204 Algorithm 1 ML-DSA.KeyGen as crypto_sign_keypair
- FIPS 204 Algorithm 2 ML-DSA.Sign as crypto_sign_signature
- FIPS 204 Algorithm 3 ML-DSA.Verify as crypto_sign_verify
Implementation of internal ML-DSA algorithm functions:
- FIPS 204 Algorithm 6 ML-DSA.KeyGen_internal as crypto_sign_keypair_internal
- FIPS 204 Algorithm 7 ML-DSA.Sign_internal as crypto_sign_signature_internal
- FIPS 204 Algorithm 8 ML-DSA.Verify_internal as crypto_sign_verify_internal
Addition of internal functions to the pqdsa API:
- ml_dsa_65_keypair_internal
- ml_dsa_65_sign_internal
- ml_dsa_65_verify_internal
Addition of KATs from seeds (KATs from https://github.com/post-quantum-cryptography/KAT/tree/main)
- New testing framework to run KATs from seeds (as in ACVP)
Removal of pq_custom_randombytes.{c/h}

Call-outs:

We can remove the testing mechanism for the KATs pq_custom_randombytes as we now support KATs that use the internal functions that provide randomness via an input seed.

Testing:

The KATs have been migrated to use the internal functions.

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.

codecov-commenter · 2024-11-18T20:31:05Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.90%. Comparing base (ab8953b) to head (33dee33).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1999      +/-   ##
==========================================
+ Coverage   78.89%   78.90%   +0.01%     
==========================================
  Files         595      594       -1     
  Lines      102451   102434      -17     
  Branches    14527    14524       -3     
==========================================
- Hits        80827    80825       -2     
+ Misses      20976    20958      -18     
- Partials      648      651       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

andrewhop · 2024-11-18T23:49:20Z

crypto/dilithium/ml_dsa.h

-                      uint8_t *private_key);
+                      uint8_t *secret_key);
+
+int ml_dsa_65_keypair_internal(uint8_t *public_key,


Why internal? We used deterministic for ML-KEM.

We used deterministic for ML-KEM as we wrote these functions before the final release of the standard came out, and we were working from the IPD and naming conventions from the PQC mailing list. Since the release of FIPS 203, 204 final, we now see these deterministic methods described as internal, so to make the implementation of the standard clearer I named them internal and added descriptors to the standard itself, e.g.:

/************************************************* * Name: crypto_sign_keypair_internal * * Description: FIPS 204: Algorithm 6 ML-DSA.KeyGen_internal. * Generates public and private key. Internal API. ...

To refer to:

andrewhop · 2024-11-18T23:52:27Z

crypto/dilithium/pqcrystals_dilithium_ref_common/README.md

 - `ntt.c`, `poly.c`, `reduce.c`, `reduce.h`: have been modified with a code refactor. The function `fqmul` has been added to bring mode code consistency with Kyber/ML-KEM. See https://github.com/aws/aws-lc/pull/1748 for more details on this change.
 - `reduce.c`: a small fix to documentation has been made on the bounds of `reduce32`.
 - `poly.c`: a small fix to documentation has been made on the bounds of `poly_reduce`.
 - `polyvec.c`: a small fix to documentation has been made on the bounds of `polyveck_reduce`.

 **Testing** 

-The KATs were obtained from https://github.com/pq-crystals/dilithium/tree/master/ref/nistkat.
-To compile the KAT programs on Linux or macOS, go to the `ref/` directory and run `make nistkat`. This will produce executables within `nistkat` which once executed will produce the KATs: `PQCsignKAT_Dilithium2.rsp`, `PQCsignKAT_Dilithium3.rsp`,`PQCsignKAT_Dilithium5.rsp`.
+The KATs were obtained from https://github.com/post-quantum-cryptography/KAT.


I know why, but please explain why our file MLDSA_65_hedged_pure.txt doesn't match https://github.com/post-quantum-cryptography/KAT/blob/main/MLDSA/kat_MLDSA_65_hedged_pure.rsp

andrewhop · 2024-11-18T23:54:31Z

crypto/dilithium/pqcrystals_dilithium_ref_common/sign.c

  uint8_t seedbuf[2*SEEDBYTES + CRHBYTES];
  uint8_t tr[TRBYTES];
  const uint8_t *rho, *rhoprime, *key;
  polyvecl mat[DILITHIUM_K_MAX];
  polyvecl s1, s1hat;
  polyveck s2, t1, t0;

-  /* Get randomness for rho, rhoprime and key */
-  pq_custom_randombytes(seedbuf, SEEDBYTES);
+  OPENSSL_memcpy(seedbuf, seed, SEEDBYTES);


At first I thought this looked weird and was wondering why not just use seed directly, but I see they put more stuff into seedbuf down below. Seems a bit weird and a fragile construction.

andrewhop · 2024-11-18T23:59:53Z

crypto/dilithium/pqcrystals_dilithium_ref_common/sign.c

  seedbuf[SEEDBYTES+0] = params->k;
  seedbuf[SEEDBYTES+1] = params->l;


How does this work? seedbuf is an array of uint8_t, so each index in the array can only hold 8 bits and k/l are both size_t which could be 64 bits each.

andrewhop · 2024-11-19T00:00:56Z

crypto/dilithium/pqcrystals_dilithium_ref_common/sign.c

+int crypto_sign_keypair(ml_dsa_params *params, uint8_t *pk, uint8_t *sk) {
+  uint8_t seed[SEEDBYTES];
+  /* Get randomness for rho, rhoprime and key */
+  RAND_bytes(seed, SEEDBYTES);


RAND_bytes can never fail, but you should still check the return code so static analysis tools don't complain about not checking the return value.

andrewhop · 2024-11-19T00:09:55Z

crypto/dilithium/pqcrystals_dilithium_ref_common/sign.c

+#ifdef DILITHIUM_RANDOMIZED_SIGNING
+  if (!RAND_bytes(rnd, RNDBYTES))
+    return -1;
+#else
+  for(i=0;i<RNDBYTES;i++)
+    rnd[i] = 0;
+#endif


Why is this a compile time option and not two normal APIs for sign and sign_deterministic?

andrewhop · 2024-11-19T00:11:26Z

crypto/dilithium/pqcrystals_dilithium_ref_common/sign.c

+  if(ctxlen > 255)
+    return -1;


Here and in other spots in this PR please update the code to always add the brackets even for single line statements.

Suggested change

if(ctxlen > 255)

return -1;

if(ctxlen > 255) {

return -1;

}

andrewhop · 2024-11-19T00:14:29Z

crypto/dilithium/p_pqdsa_test.cc

@@ -19,7 +18,6 @@

 #include "../test/file_test.h"
 #include "../test/test_util.h"
-#include "../rand_extra/pq_custom_randombytes.h"


andrewhop · 2024-11-19T00:16:50Z

crypto/dilithium/p_pqdsa_test.cc

+    for(i = 0; i < ctxstr.size(); i++) {
+      m_prime[2 + i] = ctxstr[i];
+    }


Can this just use memcpy to be simpler and maybe faster?

andrewhop · 2024-11-19T00:17:47Z

crypto/dilithium/pqcrystals_dilithium_ref_common/sign.c

+  for(i = 0; i < ctxlen; i++)
+    pre[2 + i] = ctx[i];


memcpy here too? Either way needs {}

added internal APIs for ml-dsa

33dee33

jakemas requested a review from a team as a code owner November 18, 2024 20:07

jakemas mentioned this pull request Nov 18, 2024

Adding internal functions for ML-DSA #1977

Closed

andrewhop reviewed Nov 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add internal APIs for ML-DSA #1999

Add internal APIs for ML-DSA #1999

jakemas commented Nov 18, 2024 •

edited

Loading

codecov-commenter commented Nov 18, 2024

andrewhop Nov 18, 2024

jakemas Nov 19, 2024

andrewhop Nov 18, 2024

andrewhop Nov 18, 2024

andrewhop Nov 18, 2024

andrewhop Nov 19, 2024

andrewhop Nov 19, 2024

andrewhop Nov 19, 2024

andrewhop Nov 19, 2024

andrewhop Nov 19, 2024

andrewhop Nov 19, 2024

		seedbuf[SEEDBYTES+0] = params->k;
		seedbuf[SEEDBYTES+1] = params->l;

Add internal APIs for ML-DSA #1999

Are you sure you want to change the base?

Add internal APIs for ML-DSA #1999

Conversation

jakemas commented Nov 18, 2024 • edited Loading

Issues:

Description of changes:

Call-outs:

Testing:

codecov-commenter commented Nov 18, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakemas commented Nov 18, 2024 •

edited

Loading