Fix mldsa constant time issues#1386
Open
lukaszobernig wants to merge 2 commits into
Open
Conversation
Algorithm 15 (CoeffFromHalfByte) is called during ExpandS in key generation to sample the secret signing key vectors s1 and s2 from rho'. Rejection sampling inherently leaks accept/reject decisions, but data-dependent branches within the accepted range leak secret coefficient values via branch timing and predictor state. Using ctutils ensures coefficient reduction and sign selection are branch-free.
Polynomial::infinity_norm and Vector::infinity_norm are called during the signing loop to check rejection bounds on secret-derived vectors. Replacing Iterator::max() with a constant-time fold reduction via ctutils eliminates data-dependent comparison branches.
lukaszobernig
force-pushed
the
fix_mldsa_constant_time
branch
from
6d25679 to
73325cc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I saw that these two spots still used non-constant time code, let's replace this with branchless implementations instead.
We only call
coeff_from_half_byteduring key generation, which means it is a lot harder for an attacker to collect enough execution traces. But since we generally use private key seeds, it's usually not just a one-shot for a fixed input and there is a chance to collect more traces. So it's definitely worth fixing in my opinion.The change to
Vector::infinity_normis not absolutely required, since by https://pq-crystals.org/dilithium/data/dilithium-specification-round3.pdf, section 5.5, it is OK to leak which coefficient violates the bound as long as we do not leak the coefficient itself or its sign. The change toPolynomial::infinity_normalthough is important since it fixes that potential leak of information about the individual coefficients. Let me know if you would want to keepVector::infinity_normas is, this would probably save a tiny amount of instruction cycles.