ml-kem: fix potential kyberslash attack

[supinie]

Updates the compress method to remove division of secret values by public value (q) as described in the kyberslash attack

Also updates tests to use inequality as per 4.7 in FIPS 203 draft to not use floating point arithmetic.

[tarcieri]

You should be able to enable the integer_division_remainder_used clippy lint to ban the use of division entirely (though it seems there's still some / 2 you may need to convert to >> 1): rust-lang/rust-clippy#12451

[supinie]

I have updated the / 2 occurances in compress.rs, but did not add the integer_division_remainder_used as % is used in a number of other places, as well as some other integer divisions, although these divisions are only on fully public values to generate consts.

I am still working to understand why the nist tests failed with deterministic feature set.

[bifurcation]

Couple of nits, but overall LGTM. Thanks!

[tarcieri]

but did not add the integer_division_remainder_used as % is used in a number of other places

You can explicitly annotate the places which operate on public values with allow(integer_division_remainder_used) or I can add it in a followup PR.

[supinie]

I have updated the initial commit as per @bifurcation 's comments as well as adding the integer_division_remainder_used lint but am still stuck on why the KAT nist tests fail whilst no others do.

[supinie]

in response to @bifurcation 's comment (I am unable to in a thread):

Yes, of course, I don't know how I didn't see that...

I've done a little bit of testing and it appears that it is not always true that x >= Decompress(Compress(x)), for example when x = 833 when d = 1. This is the case for both the new proposed and old compress. I do believe that this is expected output, and Compress should output 1 for 833 <= x < 2497 with 0 otherwise, and Decompress returning q/2 for 1 and 0 otherwise (when d = 1).

I have also done some testing of the preservation of input under Compress(Decompress(y)), and again, for both versions of compress, this does not hold.

Here is a link to a playground demonstrating my findings on the above. I will continue to do work on this, but hope you will be able to give some insight on the issue.

[tarcieri]

re: integer_division_remainder_used you'll need to bump the clippy version used in CI. You can bump it to the latest stable unless there's additional lint failures.

[supinie]

Will bumping the clippy version in tests also affect the MSRV? I've also been having trouble running that lint on my local on stable (despite my stable being v1.78.0) and only having it recognised on nightly/beta.

[tarcieri]

Nope, clippy is just a linter and doesn’t impact MSRV

[tarcieri]

Sidebar: it doesn't look like we're impacted by this? https://groups.google.com/a/list.nist.gov/g/pqc-forum/c/hqbtIGFKIpU/m/cnE3pbueBgAJ

[bifurcation]

OK, I played around with this a little:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=08afd3b14736eee45eeb1cbf59a70406

1. It would be good to make test cases that do a strict known-answer test, using num_rational::Ratio as ground-truth.
2. In order to pass that test, I had to change the definition of Q_HALF to be (Q+1)/2 (not (Q-1)/2).
3. Correctly computing $\mod^{\pm}$ and taking the absolute value, we do in fact pass the Equation 4.7 test.

[bifurcation]

Sidebar: it doesn't look like we're impacted by this? https://groups.google.com/a/list.nist.gov/g/pqc-forum/c/hqbtIGFKIpU/m/cnE3pbueBgAJ

Doesn't seem obvious to me. Filed #25 to track this.

[supinie]

Thanks @bifurcation for that, I have updated all the tests and the clippy version in the workspace actions.

[bifurcation]

A few minor / aesthetic comments, but overall, this is getting pretty much done.

[tarcieri]

Build is green as of 3a205fd

[supinie]

pushed changes re. @bifurcation comments

[bifurcation]

@tarcieri I'm good with merging this if you are.

[tarcieri]

We should probably cut a release with this fix.

Should we file a RUSTSEC advisory as well?

[tarcieri]

Released as v0.1.1 in #28