Added FIPS 204 documentation, cleanse intermediate values #2017

jakemas · 2024-11-26T20:45:15Z

Issues:

Resolves CryptoAlg-2799

Description of changes:

This PR addresses the following:

Documentation:

addition of documentation for FIPS 204. To maintain a clearer implementation of the specification, I have added call outs in the function descriptors that describe the respective algorithm identifiers (name and algorithm number) in FIPS 204.
For functions that were missing documentation, this has been added.

Clean up:

DBENCH internal benchmarking within poly.c has been removed.
Migrated the files touched over to AWS-LC style "loop" guidelines, that are encased in brackets.

New addition intermediate value cleanse:

FIPS 204 states: "Implementations of ML-DSA shall ensure that any potentially sensitive intermediate data is destroyed as soon as it is no longer needed." Two particular cases in which implementations may refrain from destroying intermediate data are:
- The seed xi generated in step 1 of ML-DSA.KeyGen (Algorithm 1) can be stored for the purpose of later expansion. As it can be used to generate the private key, it shall be treated with the same safeguards as a private key.
- The matrix \hat{A} generated in step 3 of ML-DSA.KeyGen_internal (Algorithm 6) can be stored so that it need not be recomputed in later operations. Likewise, the matrix \hat{A} generated in step 5 of ML-DSA.Verify_internal(Algorithm 8). This data is public and can be derived from the public key.

Callouts:

Pointers in crypto_sign_signature_internal namely, uint8_t *rho, *tr, *key, *mu, *rhoprime; have pointer addresses cleansed.

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-26T21:08:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.67%. Comparing base (412018d) to head (a8777cd).
Report is 23 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2017      +/-   ##
==========================================
+ Coverage   78.62%   78.67%   +0.05%     
==========================================
  Files         596      598       +2     
  Lines      103012   103350     +338     
  Branches    14661    14692      +31     
==========================================
+ Hits        80989    81313     +324     
- Misses      21373    21385      +12     
- Partials      650      652       +2

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

geedo0 · 2024-11-29T15:31:43Z

crypto/dilithium/pqcrystals_dilithium_ref_common/README.md

@@ -15,6 +15,8 @@ that initialize a given structure with values corresponding to a parameter set.
 - `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`.
+- Documentation has been added to `ntt.c`, `packing.c`, `poly.c`, `polyvec.c`, and `reduce.c` that outlines the algorithm specification (including algorithm number) in FIPS 204.


Refers to reduce.c instead of rounding.c

nice spot! fixed 0f43f79

crypto/dilithium/pqcrystals_dilithium_ref_common/sign.c

crypto/dilithium/pqcrystals_dilithium_ref_common/poly.c

geedo0 · 2024-12-04T16:22:30Z

crypto/dilithium/pqcrystals_dilithium_ref_common/sign.c

+  /* FIPS 204. Section 3.6.3 Destruction of intermediate values. */
+  OPENSSL_cleanse(seedbuf, sizeof(seedbuf));
+  OPENSSL_cleanse(tr, sizeof(tr));
+  OPENSSL_cleanse(&rho, SEEDBYTES);


This still doesn't seem right. You're passing in the address of rho (which is pointer type whose size is sizeof(uint_8*) and clearing SEEDBYTES from that address. I believe this is a buffer overrun on the stack frame. Can someone check my work? I think what you want is to either not do any cleansing at all and assume that the pointer values are not sensitive, or you want to replace SEEDBYTES with sizeof(rho) to basically set the pointer to 0. Same goes for the other pointers here.

Ahh I see you are correct. For reference, check what we did for ML-KEM for the same requirement, I was following this same methodology: #1883

Removed the cleansing of pointers in dafaa7e

crypto/dilithium/pqcrystals_dilithium_ref_common/sign.c

dkostic · 2024-12-05T22:31:28Z

general comment: please try to do one thing per PR. For example, this PR should be 3 separate ones:

adding documentation,
cleanse intermediate values,
refactoring you did (adding {})

jakemas · 2024-12-05T22:49:27Z

general comment: please try to do one thing per PR. For example, this PR should be 3 separate ones:

adding documentation,

cleanse intermediate values,

refactoring you did (adding {})

Thank for you that feedback!

geedo0

The only thing that concerns me here is the treatment of signs. I'm indifferent on the topic of clearing pointer values.

crypto/dilithium/pqcrystals_dilithium_ref_common/poly.c

geedo0 · 2024-12-06T15:48:44Z

crypto/dilithium/pqcrystals_dilithium_ref_common/sign.c

+  OPENSSL_cleanse(&rho, sizeof(rho));
+  OPENSSL_cleanse(&rhoprime, sizeof(rhoprime));
+  OPENSSL_cleanse(&key, sizeof(key));


Pointer values are no more sensitive than integer offsets. Don't think we need these anymore.

geedo0 · 2024-12-06T15:49:56Z

crypto/dilithium/pqcrystals_dilithium_ref_common/sign.c

+  OPENSSL_cleanse(&rho, sizeof(rho));
+  OPENSSL_cleanse(&tr, sizeof(tr));
+  OPENSSL_cleanse(&key, sizeof(key));
+  OPENSSL_cleanse(&mu, sizeof(mu));
+  OPENSSL_cleanse(&rhoprime, sizeof(rhoprime));


See comment above on pointer values.

Thank you! I agree with you on signs, cleansed in 015ccd6. To be consistant wit ML-KEM I'll leave the cleansing of the pointers in to see what other reviewers think.

Gerardo talks about these
uint8_t *rho, *tr, *key, *mu, *rhoprime;

Thank you! Just wanted your perspective before making the change! fixed in a8777cd

dkostic · 2024-12-06T21:45:54Z

crypto/dilithium/pqcrystals_dilithium_ref_common/polyvec.c

+*              coefficients are in [-eta, eta].
+*
+* Arguments:   - ml_dsa_params: parameter struct
+*              - polyvecl v: pointer to input vector


Suggested change

* - polyvecl v: pointer to input vector

* - polyvecl v: pointer to output vector

fixed in a8777cd

dkostic · 2024-12-06T21:46:22Z

crypto/dilithium/pqcrystals_dilithium_ref_common/polyvec.c

+*              coefficients are in [-gamma1 + 1, gamma1].
+*
+* Arguments:   - ml_dsa_params: parameter struct
+*              - polyvecl v: pointer to input vector


Suggested change

* - polyvecl v: pointer to input vector

* - polyvecl v: pointer to output vector

fixed in a8777cd

dkostic · 2024-12-06T21:46:55Z

crypto/dilithium/pqcrystals_dilithium_ref_common/polyvec.c

+*              coefficients are in [-eta, eta].
+*
+* Arguments:   - ml_dsa_params: parameter struct
+*              - polyveck v: pointer to input vector


Suggested change

* - polyveck v: pointer to input vector

* - polyveck v: pointer to output vector

fixed in a8777cd

dkostic · 2024-12-06T21:56:00Z

crypto/dilithium/pqcrystals_dilithium_ref_common/sign.c

+  OPENSSL_cleanse(&rho, sizeof(rho));
+  OPENSSL_cleanse(&tr, sizeof(tr));
+  OPENSSL_cleanse(&key, sizeof(key));
+  OPENSSL_cleanse(&mu, sizeof(mu));
+  OPENSSL_cleanse(&rhoprime, sizeof(rhoprime));


Gerardo talks about these
uint8_t *rho, *tr, *key, *mu, *rhoprime;

jakemas added 2 commits November 26, 2024 12:08

added documentation and clean up code

69b56b7

intermediate cleanse

f4c32a4

jakemas requested a review from a team as a code owner November 26, 2024 20:45

geedo0 reviewed Nov 29, 2024

View reviewed changes

cleanse fixes

0f43f79

geedo0 requested changes Dec 4, 2024

View reviewed changes

jakemas added 2 commits December 5, 2024 09:51

dont attempt to cleanse pointers

dafaa7e

cleanse everything

c33b520

geedo0 previously approved these changes Dec 5, 2024

View reviewed changes

cleanse like we cleanse ml-kem

d04999b

jakemas dismissed geedo0’s stale review via d04999b December 5, 2024 22:47

geedo0 reviewed Dec 6, 2024

View reviewed changes

cleanse signs

015ccd6

geedo0 previously approved these changes Dec 6, 2024

View reviewed changes

dkostic reviewed Dec 6, 2024

View reviewed changes

cr fixes

a8777cd

jakemas dismissed geedo0’s stale review via a8777cd December 6, 2024 22:59

geedo0 approved these changes Dec 9, 2024

View reviewed changes

dkostic approved these changes Dec 10, 2024

View reviewed changes

geedo0 merged commit 18cc07d into aws:main Dec 10, 2024
119 of 124 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added FIPS 204 documentation, cleanse intermediate values #2017

Added FIPS 204 documentation, cleanse intermediate values #2017

jakemas commented Nov 26, 2024 •

edited

Loading

codecov-commenter commented Nov 26, 2024 •

edited

Loading

geedo0 Nov 29, 2024

jakemas Dec 2, 2024

geedo0 Dec 4, 2024

jakemas Dec 4, 2024 •

edited

Loading

jakemas Dec 5, 2024

dkostic commented Dec 5, 2024

jakemas commented Dec 5, 2024

geedo0 left a comment

geedo0 Dec 6, 2024

geedo0 Dec 6, 2024

jakemas Dec 6, 2024

dkostic Dec 6, 2024

jakemas Dec 6, 2024

dkostic Dec 6, 2024

jakemas Dec 6, 2024

dkostic Dec 6, 2024

jakemas Dec 6, 2024

dkostic Dec 6, 2024

jakemas Dec 6, 2024

dkostic Dec 6, 2024

	* - polyvecl v: pointer to input vector
	* - polyvecl v: pointer to output vector

	* - polyveck v: pointer to input vector
	* - polyveck v: pointer to output vector

Added FIPS 204 documentation, cleanse intermediate values #2017

Added FIPS 204 documentation, cleanse intermediate values #2017

Conversation

jakemas commented Nov 26, 2024 • edited Loading

Issues:

Description of changes:

codecov-commenter commented Nov 26, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakemas Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkostic commented Dec 5, 2024

jakemas commented Dec 5, 2024

geedo0 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakemas commented Nov 26, 2024 •

edited

Loading

codecov-commenter commented Nov 26, 2024 •

edited

Loading

jakemas Dec 4, 2024 •

edited

Loading