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.

[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.

[dstebila]

@bhess You are planning to change the ML-KEM implementation in liboqs to PQCP's mlkem-native. Will that include the desired checks or will it still require separate code?

[bhess]

Will that include the desired checks or will it still require separate code?

Yes, the checks are by default in mlkem-native. Test code as done in this PR may still be useful IMO.

[SWilson4]

LGTM. Tagging @bhess for a second opinion as our ML-KEM expert.