fix: zeroize Falcon secret polynomial temporaries and encoded key buffers - #1061
fix: zeroize Falcon secret polynomial temporaries and encoded key buffers#1061Jr-kenny wants to merge 4 commits into
Conversation
|
Automated check (CONTRIBUTING.md) Findings:
Next steps:
|
db8fe3d to
b4e995a
Compare
| buffer.push(LOG_N); | ||
| buffer.extend_from_slice(&self.to_bytes()); | ||
| // Bind the serialized key so the temporary holding it is wiped, not just `buffer`. | ||
| let sk_bytes = Zeroizing::new(self.to_bytes()); |
There was a problem hiding this comment.
This handles the generate_seed copy, but the same full-key serialization still happens in PartialEq just below: self.to_bytes().ct_eq(&other.to_bytes()).
Both calls allocate encoded secret keys and drop them without Zeroizing, so comparing two Falcon secret keys can still leave the same kind of buffer this PR is trying to wipe. Could those two serialized values be bound with Zeroizing too, or compared without serializing?
There was a problem hiding this comment.
Yeah, that path slips the wipe too. PartialEq serializes both keys and drops them in the clear on every compare. Bound both to_bytes() in Zeroizing in 53a0f03 so they get wiped after the compare instead. ct_eq still runs over the bound bytes, so the comparison is unchanged and stays constant time.
Swept the rest while I was in there: this was the last unwiped to_bytes() path in Falcon, write_into and generate_seed were already covered. The same pattern showed up in the sibling secret keys though. The k256 ECDSA and ed25519 EdDSA SecretKeys are both ZeroizeOnDrop and their PartialEq had the identical un-wiped to_bytes().ct_eq(), so I folded the same fix into those two as well. Falcon, ecdsa and eddsa suites all pass locally.
huitseeker
left a comment
huitseeker
left a comment
There was a problem hiding this comment.
Looks good, a rebase would be great!
53a0f03 to
fc12005
Compare
|
just did ser |
fc12005 to
733ff85
Compare
FalconFelt gets a Zeroize impl so Polynomial<FalconFelt> is wipeable through the existing generic impl. The ZeroizeOnDrop marker on Polynomial is removed: it had no Drop behind it (a conditional Drop impl is rejected by E0367 and bounding the struct would exclude Polynomial<Complex64>), so it promised a wipe that never ran. Wipe-on-drop is now explicit at the call sites. SecretKey::read_from wraps the decoded polynomials and every FFT-domain intermediate of the big_g computation in Zeroizing, inlining hadamard_div as hadamard_inv + hadamard_mul so the inverse is reachable, and negates the basis polynomials through references so the un-negated copies are wiped. SecretKey::write_into wipes its encode buffer and the encoded chunks, fixing the stale note that claimed write_bytes consumes the buffer. generate_seed binds the serialized key in Zeroizing so the per-signature copy is wiped.
compute_pub_key_poly cloned f and g into FalconFelt polynomials on every signature and dropped them intact; they and their FFT forms are now bound in Zeroizing, with hadamard_div inlined so the inverse is wiped too. The h = g/f result is the public key and stays unwrapped. ntru_gen wraps the sampled f and g and the converted F and G so rejected candidates are wiped on every retry, moves the kept polynomials out of their wrappers, and negates through references so the un-negated copies are wiped. gen_poly wipes its raw 4096-sample buffer. Out of reach without new machinery and left as is: the BigInt arithmetic inside ntru_solve, and the Complex64 FFT domain (gram_schmidt_norm_squared, to_complex_fft, sign_helper, ffsampling), which would need the volatile treatment LdlTree uses.
733ff85 to
6a39e31
Compare
|
This should be ported to the |
|
Yeah, this needs to live in |
4 participants
Follow-up to #1057, closes 0xMiden/miden-vm#3533.
#1057 wiped the byte-level read buffers, but the secret material doesn't stop there. On the Falcon path the decoded coefficients get turned into
Polynomialtemporaries that nothing wipes,Polynomialcarries aZeroizeOnDropmarker with noDropbehind it so it never actually runs, and bothwrite_intoandgenerate_seedleave encoded copies of the secret key on the heap, the second one on every signature. 0xMiden/miden-vm#3533 has the full walkthrough.What's in here:
ZeroizeforFalconFelt, soPolynomial<FalconFelt>becomes wipeable through the existing generic impl.ZeroizeOnDropmarker onPolynomial. It's unimplementable as declared: aDropbounded onF: Zeroizefails E0367, and bounding the struct itself breaksPolynomial<Complex64>on the signing path. Made wipe-on-drop explicit at the use sites withZeroizinginstead, the same pattern feat: add zeroizing read helper for sensitive deserialization #1057 uses for the byte buffers.read_from: wrappedf/g/big_f/big_gand bound the fft chain stepwise so the intermediates get wiped too, inlininghadamard_divashadamard_inv+hadamard_mulso the inverse lands in a wrapped value instead of an internal allocation. Negated the basis polynomials through references so the un-negated copies stay wrapped.write_into: wiped the encode buffer and the encoded chunks, and fixed the stale comment that claimedwrite_bytesconsumes the buffer (it borrows it).generate_seed: bound the serialized key inZeroizingso the per-signature copies are covered.compute_pub_key_polyclonesf/gon every signature andntru_gendrops rejected candidates on every retry, both wiped now.The
BigIntarithmetic inntru_solveand theComplex64fft domain are left as documented residue. Wiping those needs the volatile-write treatmentLdlTreealready does rather thanZeroize, and that's called out in 0xMiden/miden-vm#3533.This sits on top of #1057, so it should land after that one. The first commits here are #1057's.
Test plan:
cargo test -p miden-crypto dsa(49 pass, including the reference-impl signature determinism check),cargo fmt --check,cargo clippy -p miden-crypto --all-features, and the no-std build.