Make poly_chknorm constant flow #2788

jakemas · 2025-10-30T19:47:53Z

Issues:

Resolves #V1982530715 and #V1982532566

Description of changes:

The reference implementation implements poly_chknorm in variables time. It argues that while the input coefficients itself are secret in some call sites, it is okay to leak which coefficient lead to rejection. It, hence, does absolute value computation in constant-time and then checks the bound using a conditional.

This approach appears safe, but somewhat unclean as it is still operating on secret data. When performing constant-time testing it also requires a number of declassifications.

This commit takes a more conservative approach and changes poly_chknorm to a constant-time implementation in the hope that the performance penalty is acceptable.

A minor change is that the API of poly_chknorm is changed to returning 0xFFFFFFFF in the case of failure to be able to re-use existing constant-time primitives.

Call-outs:

PR source adapted from: Upstream PR in pq-code-package/mldsa-native#392.

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.

github-actions

clang-tidy made some suggestions

crypto/fipsmodule/ml_dsa/ml_dsa_ref/sign.c

dkostic · 2025-10-30T20:11:30Z

crypto/fipsmodule/ml_dsa/ml_dsa_ref/polyvec.c

+    /* Reference: Leaks which polynomial violates the bound via a conditional.
+     * We are more conservative to reduce the number of declassifications in
+     * constant-time testing.
+     */


Suggested change

/* Reference: Leaks which polynomial violates the bound via a conditional.

* We are more conservative to reduce the number of declassifications in

* constant-time testing.

*/

crypto/fipsmodule/ml_dsa/ml_dsa_ref/sign.c

dkostic · 2025-10-30T20:22:33Z

crypto/fipsmodule/ml_dsa/ml_dsa_ref/sign.c

+
+  /* FIPS 204: Algorithm 7 line 23 - Reject if either check fails (constant-time to avoid leaking
+     which check failed) */
+  if(z_invalid | w0_invalid) {


I think the compiler can turn this into non-CT code, please use an appropriate constant_time_* function to get the branch condition.

Removed this check, added documentation and references to https://pq-crystals.org/dilithium/data/dilithium-specification-round3-20210208.pdf section 5.5

codecov-commenter · 2025-10-30T20:46:48Z

Codecov Report

❌ Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 78.60%. Comparing base (d826708) to head (d106952).

Files with missing lines	Patch %	Lines
crypto/fipsmodule/ml_dsa/ml_dsa_ref/poly.c	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2788      +/-   ##
==========================================
+ Coverage   78.42%   78.60%   +0.18%     
==========================================
  Files         683      683              
  Lines      116283   116287       +4     
  Branches    16404    16407       +3     
==========================================
+ Hits        91194    91409     +215     
+ Misses      24213    24002     -211     
  Partials      876      876

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

crypto/fipsmodule/ml_dsa/ml_dsa_ref/poly.c

hanno-becker · 2025-10-31T04:45:20Z

crypto/fipsmodule/ml_dsa/ml_dsa_ref/sign.c

+  /* FIPS 204: Algorithm 7 line 23 - Reject if either check fails (constant-time to avoid leaking
+     which check failed) */
+  if(z_invalid | w0_invalid) {


Similar comment as above. We may want to harden this as defense-in-depth, but separate checks are OK from all we know -- and what is explicitly stated in 5.5 of https://pq-crystals.org/dilithium/data/dilithium-specification-round3-20210208.pdf -- so the comment should be changed.

hanno-becker

I'm fine with defense-in-depth hardening of poly_chknorm as already done in mldsa-native, but we should change the comment so it's not suggested that the implementation would be insecure otherwise.

I have reservations towards consolidating the validity checks. As mentioned in the comments, they too are explicitly documented in the Dilithium documentation as safe to be performed separately, and merging them comes at a meaningful performance penalty (~5%); see the test PR pq-code-package/mldsa-native#572.

This reverts commit 5790986.

@jakemas

### Description of changes: Prepare release v1.64.0. ### What's Changed * Update max polyz value by @jakemas in #2787 * ECR Repositories for Android and Formal Verification Images by @skmcgrail in #2794 * Support more "openssl rsa" options by @justsmth in #2777 * Remove python codebuild patches by @WillChilds-Klein in #2793 * Additional options for "openssl c_client" by @justsmth in #2791 * GitHub-based Formal Verification Image Build by @skmcgrail in #2796 * Use C++11 atomics to update session stats by @justsmth in #2786 * Support "openssl dhparam" by @justsmth in #2790 * Add scrutinice pull permissions for aws-lc/amazonlinux repository by @skmcgrail in #2799 * Use GitHub-based Verification Images by @skmcgrail in #2798 * Remove dead code by @torben-hansen in #2797 * Rename snapsafe to VM UBE by @torben-hansen in #2800 * Bump MySQL version tag to 9.5.0 by @samuel40791765 in #2768 * Migrate to macos-15-intel by @samuel40791765 in #2802 * Use right compiler with ruby CI by @samuel40791765 in #2801 * Migrate analytics job to be GitHub triggered by @skmcgrail in #2779 * Support NetBSD by @justsmth in #2754 * Make poly_chknorm constant flow by @jakemas in #2788 * Rename fork to fork UBE by @torben-hansen in #2803 * Extend grv asan timeout for Golang to allow completion by @torben-hansen in #2805 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.

@jakemas

### Description of changes: Prepare release v1.64.0. ### What's Changed * Update max polyz value by @jakemas in aws#2787 * ECR Repositories for Android and Formal Verification Images by @skmcgrail in aws#2794 * Support more "openssl rsa" options by @justsmth in aws#2777 * Remove python codebuild patches by @WillChilds-Klein in aws#2793 * Additional options for "openssl c_client" by @justsmth in aws#2791 * GitHub-based Formal Verification Image Build by @skmcgrail in aws#2796 * Use C++11 atomics to update session stats by @justsmth in aws#2786 * Support "openssl dhparam" by @justsmth in aws#2790 * Add scrutinice pull permissions for aws-lc/amazonlinux repository by @skmcgrail in aws#2799 * Use GitHub-based Verification Images by @skmcgrail in aws#2798 * Remove dead code by @torben-hansen in aws#2797 * Rename snapsafe to VM UBE by @torben-hansen in aws#2800 * Bump MySQL version tag to 9.5.0 by @samuel40791765 in aws#2768 * Migrate to macos-15-intel by @samuel40791765 in aws#2802 * Use right compiler with ruby CI by @samuel40791765 in aws#2801 * Migrate analytics job to be GitHub triggered by @skmcgrail in aws#2779 * Support NetBSD by @justsmth in aws#2754 * Make poly_chknorm constant flow by @jakemas in aws#2788 * Rename fork to fork UBE by @torben-hansen in aws#2803 * Extend grv asan timeout for Golang to allow completion by @torben-hansen in aws#2805 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.

Make poly_chknorm constant flow

5e55c04

github-actions bot reviewed Oct 30, 2025

View reviewed changes

crypto/fipsmodule/ml_dsa/ml_dsa_ref/sign.c Show resolved Hide resolved

crypto/fipsmodule/ml_dsa/ml_dsa_ref/sign.c Show resolved Hide resolved

crypto/fipsmodule/ml_dsa/ml_dsa_ref/sign.c Show resolved Hide resolved

jakemas requested review from dkostic and manastasova October 30, 2025 19:52

Check results simultaniously

5790986

dkostic reviewed Oct 30, 2025

View reviewed changes

hanno-becker reviewed Oct 31, 2025

View reviewed changes

crypto/fipsmodule/ml_dsa/ml_dsa_ref/poly.c Outdated Show resolved Hide resolved

hanno-becker reviewed Oct 31, 2025

View reviewed changes

hanno-becker suggested changes Oct 31, 2025

View reviewed changes

jakemas added 2 commits November 3, 2025 11:46

Revert "Check results simultaniously"

434425b

This reverts commit 5790986.

Update comments

2ed8979

jakemas marked this pull request as ready for review November 3, 2025 19:53

jakemas requested a review from a team as a code owner November 3, 2025 19:53

jakemas added 2 commits November 3, 2025 11:54

Merge branch 'main' into mldsa-chknorm

783faa5

Add comment regarding signature rejection loop

0327643

dkostic approved these changes Nov 4, 2025

View reviewed changes

jakemas added 2 commits November 4, 2025 13:34

Merge branch 'main' into mldsa-chknorm

16e8e62

Merge branch 'main' into mldsa-chknorm

d106952

torben-hansen approved these changes Nov 7, 2025

View reviewed changes

jakemas merged commit 93f9f73 into aws:main Nov 7, 2025
381 of 388 checks passed

justsmth mentioned this pull request Nov 10, 2025

Prepare release v1.64.0 #2810

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make poly_chknorm constant flow #2788

Make poly_chknorm constant flow #2788

jakemas commented Oct 30, 2025 •

edited

Loading

Uh oh!

github-actions bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dkostic Oct 30, 2025

Uh oh!

jakemas Nov 3, 2025

Uh oh!

Uh oh!

dkostic Oct 30, 2025

Uh oh!

jakemas Nov 3, 2025

Uh oh!

codecov-commenter commented Oct 30, 2025 •

edited

Loading

Uh oh!

Uh oh!

hanno-becker Oct 31, 2025

Uh oh!

hanno-becker left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Make poly_chknorm constant flow #2788

Make poly_chknorm constant flow #2788

Conversation

jakemas commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issues:

Description of changes:

Call-outs:

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dkostic Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

jakemas Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dkostic Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

jakemas Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

hanno-becker Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

hanno-becker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jakemas commented Oct 30, 2025 •

edited

Loading

codecov-commenter commented Oct 30, 2025 •

edited

Loading