Add checks for ML-KEM keys

[abhinav-thales]

This PR introduces the sanity checks for encapsulation and decapsulation keys introduced in the final specification of FIPS-203 in section 7.

The changes include:
- checking generated encaps and decaps key in the keypair step
- checking encaps key before the encapsulation step
- checking decaps key before the decapsulation step

Ideally this check should be in the ML-KEM source code but I am not sure if we are allowed to make changes to the upstream code. Kindly let me know otherwise and will move the code accordingly.

[baentsch]

Thanks for this improvement @abhinav-thales . It seems the issue tracking this is #1951:

if we allowed to make changes to the upstream code

We are allowed to do anything :-) The question is whether the upstream takes such changes. More seriously: liboqs has a patch mechanism to insert code not yet made available by the upstreams but we're not over-eager to use that facility as it's introducing more work in a place where it conceptually doesn't belong.

Anyway, please see the discussion in the issue above. This PR could serve as a test stopgap measure and as such imo is mergeable as-is. What's your take @bhess: Also do a patch or wait for the upstream?

[bhess]

I agree that including key validation tests makes sense. As you pointed out, the pq-crystals upstream doesn't include these validations, so we’d need to rely on our patch mechanism, which would increase maintenance overhead on the liboqs side.

One alternative to consider is using this as an opportunity to start adopting the https://github.com/pq-code-package/mlkem-native implementation from PQCP. It recently had its first (alpha) release and is actively maintained. This implementation already includes the required key validations [1][2], and the advantage is that it would eliminate the need for us to maintain custom patches.

[1] https://github.com/pq-code-package/mlkem-native/blob/112dbd3d3c42aa6558a448bb80affe5f8c158ece/mlkem/kem.c#L36
[2] https://github.com/pq-code-package/mlkem-native/blob/112dbd3d3c42aa6558a448bb80affe5f8c158ece/mlkem/kem.c#L64

[baentsch]

One alternative to consider is using this as an opportunity to start adopting the https://github.com/pq-code-package/mlkem-native implementation from PQCP

Why not -- not a new issue, though -- my thoughts stated half a year ago still stand/may be worth while addressing then: pq-code-package/tsc#15 (comment) : Technically, this could be simple, no? PQCP adds "copy-from-upstream" YML files and OQS adds it as another upstream (assuming some form of agreement on APIs, of course :) My strong preference would be to then drop other upstreams to indeed make life easier and not more complicated with this addition.

[baentsch]

Additional thought: As and when there are different algorithm code bases becoming available and if OQS would still want to extend its utility, wouldn't this be the perfect opportunity to finally introduce the notion of "upstream maintained" to the YML file as a way to signal code (bases) that someone may rely on for use beyond research & prototyping? Given the various professionally maintained code bases for standardized algorithms already available or with a clear plan to do so, I'm personally less and less convinced this goal is a sensible one for OQS to pursue, though -- but wouldn't want to rule it out and not lay foundations for that -- like with such distinction of actively supported upstream code bases that would make it into a "reliable build" (vs. the current "amalgam/best effort" one).

[abhinav-thales]

Hi @baentsch @bhess :
the final decision is not clear to me :(
can we do the patches(in crypto source, also reduces code complexity by using inbuilt 'poly' functions) OR
include these changes in the test file currently and wait for the upstream switch ?

[baentsch]

the final decision is not clear to me :(

There is no final decision yet (as to which upstreams OQS keeps supporting). As far as I'm concerned, there's a lot speaking for switching for ML-KEM from PQClean/reference code to PQCP code -- if OQS decides to keep supporting ML-KEM at all (see related discussion here).

include these changes in the test file currently and wait for the upstream switch ?

That would be my preference for now. Sub-optimal as it will not be available to users of the library but anything else has a high potential to be wasted effort.

[bhess]

I agree with @baentsch.

[abhinav-thales]

Thanks @baentsch . we could proceed with the PR as it is then.

@bhess can I request a review from you as well please ? I don't have permission to add 'request reviews' .

[SWilson4]

Thanks very much for these improvements.

[SWilson4]

I don't believe we can count on comparison operators being constant time. I suggest doing Barrett reduction here instead, similar to the reference implementation. (That code computes a centred representation; we'd just need an additional addition.)

I realize that this is overkill for testing code, but there's a possibility we use this file as a guide for patching the algorithm source later.

[abhinav-thales]

correct, added a simple mod function as this was a test file. will change it to proper mod function after few tests at my end.

[SWilson4]

Hi @abhinav-thales, as per discussion in today's OQS meeting, we're OK with this being non–constant time, as long as the comment stating that it is constant time is removed. Feel free to go ahead with that approach if it's simpler for you. (In that case my preference would be to simply use the % operator so that the operation does not appear to be constant time.)

[abhinav-thales]

Hi @SWilson4 , thanks for the update.
But any particular reason, why non-constant time is ok ? having it time constant would be the ideal scenario IMO.
in the meantime, I have tested modQ using "Barrett reduction" and another approach using shift operators as well.

[SWilson4]

Hi @SWilson4 , thanks for the update. But any particular reason, why non-constant time is ok ? having it time constant would be the ideal scenario IMO. in the meantime, I have tested modQ using "Barrett reduction" and another approach using shift operators as well.

I brought it up with the OQS team, and we decided that we're OK with a non–constant time function here because the code is limited to a test file, in the interest of not holding up the PR. If you prefer to submit a constant-time implementation, that's fine too :)

[nidhidamodaran]

@abhinav-thales wouldn't a check like below on public be more simpler ?

+int poly_frombytes(poly *r, const uint8_t a[KYBER_POLYBYTES])
 {
   unsigned int i;
   for(i=0;i<KYBER_N/2;i++) {
     r->coeffs[2*i]   = ((a[3*i+0] >> 0) | ((uint16_t)a[3*i+1] << 8)) & 0xFFF;
     r->coeffs[2*i+1] = ((a[3*i+1] >> 4) | ((uint16_t)a[3*i+2] << 4)) & 0xFFF;
+    if((r->coeffs[2*i] >= KYBER_Q) || (r->coeffs[2*i+1]  >= KYBER_Q))
+       return -1;
   }
+  return 0;
 }

[SWilson4]

@abhinav-thales wouldn't a check like below on public be more simpler ?

+int poly_frombytes(poly *r, const uint8_t a[KYBER_POLYBYTES])
 {
   unsigned int i;
   for(i=0;i<KYBER_N/2;i++) {
     r->coeffs[2*i]   = ((a[3*i+0] >> 0) | ((uint16_t)a[3*i+1] << 8)) & 0xFFF;
     r->coeffs[2*i+1] = ((a[3*i+1] >> 4) | ((uint16_t)a[3*i+2] << 4)) & 0xFFF;
+    if((r->coeffs[2*i] >= KYBER_Q) || (r->coeffs[2*i+1]  >= KYBER_Q))
+       return -1;
   }
+  return 0;
 }

See discussion here for why this patch is not a good idea.

[nidhidamodaran]

understood. thanks for the explanation. I was just trying to understand whether we really need to encode the key data, after decoding to reject keys.