Make poly_chknorm constant flow #2788
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /* Reference: Leaks which polynomial violates the bound via a conditional. | ||
| * We are more conservative to reduce the number of declassifications in | ||
| * constant-time testing. | ||
| */ |
There was a problem hiding this comment.
| /* Reference: Leaks which polynomial violates the bound via a conditional. | |
| * We are more conservative to reduce the number of declassifications in | |
| * constant-time testing. | |
| */ |
|
|
||
| /* FIPS 204: Algorithm 7 line 23 - Reject if either check fails (constant-time to avoid leaking | ||
| which check failed) */ | ||
| if(z_invalid | w0_invalid) { |
There was a problem hiding this comment.
I think the compiler can turn this into non-CT code, please use an appropriate constant_time_* function to get the branch condition.
There was a problem hiding this comment.
Removed this check, added documentation and references to https://pq-crystals.org/dilithium/data/dilithium-specification-round3-20210208.pdf section 5.5
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2788 +/- ##
==========================================
- Coverage 78.42% 78.42% -0.01%
==========================================
Files 683 683
Lines 116283 116286 +3
Branches 16405 16403 -2
==========================================
- Hits 91197 91195 -2
- Misses 24210 24216 +6
+ Partials 876 875 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| /* FIPS 204: Algorithm 7 line 23 - Reject if either check fails (constant-time to avoid leaking | ||
| which check failed) */ | ||
| if(z_invalid | w0_invalid) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
4 participants
Issues:
Resolves #V1982530715 and #V1982532566
Description of changes:
The reference implementation implements
poly_chknormin 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_chknormto a constant-time implementation in the hope that the performance penalty is acceptable.A minor change is that the API of
poly_chknormis changed to returning0xFFFFFFFFin 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.