Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make constant_time_ops.rs and utils.rs panic-free #404

Merged
merged 7 commits into from
Jul 18, 2024
Merged

Conversation

mamonet
Copy link
Member

@mamonet mamonet commented Jul 16, 2024

This is an entry PR that makes sure particular modules of ML-KEM are panic-free, it adds assumptions and pre-conditions using hax_lib::requires to functions that have a potential to panic to get the extracted F* filed verifiable. For now, two modules have been added and others are yet to be followed.

@mamonet
Copy link
Member Author

mamonet commented Jul 16, 2024

I didn't get to add serialize.rs module today, hopefully will include it tomorrow.

@franziskuskiefer
Copy link
Member

Let's go one module at a time. So just drop serialize from this PR and we have something to get in.
It looks like the hax job failed though. Also, please run the typechecking on CI for the modules that tc.

@mamonet mamonet changed the title Make constant_time_ops.rs, utils.rs, and serialize.rs panic-free Make constant_time_ops.rs and utils.rs panic-free Jul 17, 2024
@mamonet
Copy link
Member Author

mamonet commented Jul 17, 2024

Ok. Let me see where it goes wrong.

@mamonet
Copy link
Member Author

mamonet commented Jul 17, 2024

Apparently, the process of lax-checking Libcrux_ml_kem.Ind_cca.fst has been killed in hax CI job. Maybe certain functions need to be divided up there, I will see which functions in that module take the longest to lax-check.

#[cfg_attr(hax, hax_lib::requires(
lhs.len() == rhs.len() &&
lhs.len() == SHARED_SECRET_SIZE
))]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's my observation, with this pre-condition applied, decapsulate function in Libcrux_ml_kem.Ind_cca.fst takes 2.5 mins to lax-check. Without the pre-condition, that function takes ~1 min to lax-check. encapsulate function lax-checks right away in both cases.

Copy link
Member Author

@mamonet mamonet Jul 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems F* has difficulty to handle decapsulate function, is splitting it up an option?

@mamonet
Copy link
Member Author

mamonet commented Jul 17, 2024

@franziskuskiefer It seems the last commit makes hax CI job green, could you review it please or assign someone who is aware of decapsulate logic to look at?

let implicit_rejection_shared_secret =
Scheme::kdf::<K, CIPHERTEXT_SIZE, Hasher>(&implicit_rejection_shared_secret, ciphertext);
let shared_secret = Scheme::kdf::<K, CIPHERTEXT_SIZE, Hasher>(shared_secret, ciphertext);

select_shared_secret_in_constant_time(
compare_ciphertexts_select_shared_secret_in_constant_time(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be fine. But I wonder why F* has trouble here. @karthikbhargavan do you understand what's going on here?

@mamonet mamonet marked this pull request as ready for review July 17, 2024 20:05
Copy link
Contributor

@karthikbhargavan karthikbhargavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will debug the lax thing, seems weird. But meanwhile, let's get the Makefile updated and get this in so the pipeline is in motion.

@jschneider-bensch
Copy link
Collaborator

SHA-3 C extraction should be fixed with the changes in #412.

@mamonet mamonet merged commit a7a83d2 into main Jul 18, 2024
52 checks passed
@mamonet mamonet deleted the mlkem-panic-free branch July 18, 2024 13:54
@mamonet mamonet restored the mlkem-panic-free branch July 18, 2024 13:54
franziskuskiefer added a commit that referenced this pull request Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants