Specs for ML-KEM#1480
Conversation
3bb7365 to
fcefc79
Compare
fcefc79 to
ba4350d
Compare
ba4350d to
a4cfb1e
Compare
jschneider-bensch
left a comment
There was a problem hiding this comment.
Leaving some comments already before the rest of the review.
jschneider-bensch
left a comment
There was a problem hiding this comment.
I've left more comments, mostly around documentation and naming of functions. Is renaming a function okay in terms of updating the proofs, would you say?
-
The other set of suggestions concern the use of
FieldElementand its operations, where we could use the Rust type system more effectively to maintain the boundedness invariant, instead of mixing up the type's own operations with direct field accesses and allowing construction of the type independent of itsnewmethod. -
I think some of the documentation issues are inherited from the impl, where some documentation has outdated references that refer to the initial public draft of FIPS203, instead of the final version. While the documentation deviated from the published standard in some ways I found no issues with the implementation code.
-
Can you add test vector testing for the spec here, using
libcrux-kats, so we have the same test vector coverage here as on the impl side?
There was a problem hiding this comment.
These changes cause a rustfmt error.
There was a problem hiding this comment.
This file does not need to be changed at all.
| /// Output: encryption key ekₚₖₑ ∈ 𝔹^{384k+32}. | ||
| /// Output: decryption key dkₚₖₑ ∈ 𝔹^{384k}. | ||
| /// | ||
| /// d ←$ B |
There was a problem hiding this comment.
This is the key_generation_seed and is not sampled internally, but is an input.
| /// d ←$ B | |
| Input: randomness d ∈ 𝔹^{32} |
| /// Output: decryption key dkₚₖₑ ∈ 𝔹^{384k}. | ||
| /// | ||
| /// d ←$ B | ||
| /// (ρ,σ) ← G(d) |
There was a problem hiding this comment.
The rank is part of the input to G here.
| /// (ρ,σ) ← G(d) | |
| /// (ρ,σ) ← G(d‖k) |
| /// N ← 0 | ||
| /// for (i ← 0; i < k; i++) | ||
| /// for(j ← 0; j < k; j++) | ||
| /// Â[i,j] ← SampleNTT(XOF(ρ, j, i)) |
There was a problem hiding this comment.
This line deviates from the presentation in the standard.
| /// Â[i,j] ← SampleNTT(XOF(ρ, j, i)) | |
| /// Â[i,j] ← SampleNTT(ρ‖j‖i) |
While the current state is what actually happens at the beginning of SampleNTT, I prefer the presentation of the standard since it makes clear how (in particular in which order) the function arguments are to be combined to one input for SampleNTT.
|
|
||
| #[hax_lib::attributes] | ||
| impl FieldElement { | ||
| #[hax_lib::requires(val < FIELD_MODULUS)] |
There was a problem hiding this comment.
Why enforce this with a pre-condition here, instead of doing the reduction % FIELD_MODULUS in the constructor for FieldElement? The bound could be post-condition instead and making the val field private instead of pub would ensure that no out-of-bounds field element can be created in the Rust domain already as long as the operations preserve this too.
| /// `deserialize_then_decompress_u` function (which fuses the NTT into | ||
| /// the per-element decompress loop). | ||
| #[hax_lib::requires(RANK <= 4 && (du == 10 || du == 11) && ciphertext.len() == (RANK * COEFFICIENTS_IN_RING_ELEMENT * du) / 8)] | ||
| pub fn deserialize_then_decompress_u_then_ntt<const RANK: usize>( |
There was a problem hiding this comment.
It looks like this function is not used in ind_cpa::decrypt_unpacked?
| let ek: [u8; EK_SIZE] = serialize_public_key::<RANK, EK_SIZE>(&t_as_ntt, &seed_for_A); | ||
| let public_key_hash: [u8; 32] = H(&ek); | ||
|
|
||
| let mut implicit_rejection_value = [0u8; 32]; |
There was a problem hiding this comment.
This seems unnecessary, since z is already a reference to a [u8;32] and could be cloned below instead.
There was a problem hiding this comment.
Can you add a section on the F* extraction as well? I think there is no script to drive it and it's not clear what it should prove.
|
|
||
| ## Extraction via HAX | ||
|
|
||
| ### Lean |
There was a problem hiding this comment.
Is there anything that is proven in lean about this spec on its own, or is the extraction just used for the functional correctness proof of the impl?
This PR adds a hacspec-style reference implementation for ML-KEM. It includes cross-tests with the actual ML-KEM implementation, an F* and a Lean extraction. The PR builds on top of the SHA3-spec PR #1399.
The PR is structured into four clean commits:
This is the basis for the Lean verification of ML-KEM in
libcrux-iot.