Add checks for ML-KEM keys #2009

abhinav-thales · 2024-12-02T11:21:50Z

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.

Signed-off-by: Abhinav Saxena <abhinav.saxena@thalesgroup.com>

baentsch · 2024-12-03T11:42:55Z

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 · 2024-12-03T12:34:36Z

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 · 2024-12-03T13:44:53Z

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 · 2024-12-03T14:19:03Z

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 · 2024-12-11T11:11:12Z

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 · 2024-12-11T11:59:40Z

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 · 2024-12-11T13:16:40Z

I agree with @baentsch.

abhinav-thales · 2024-12-13T11:05:19Z

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.

tests/vectors_kem.c

Signed-off-by: Abhinav Saxena <abhinav.saxena@thalesgroup.com>

nidhidamodaran · 2024-12-16T14:40:11Z

@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 · 2024-12-20T12:46:05Z

@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 · 2024-12-20T13:39:59Z

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 · 2025-01-07T21:10:29Z

@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 · 2025-01-08T09:54:56Z

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.

Signed-off-by: Abhinav Saxena <abhinav.saxena@thalesgroup.com>

SWilson4

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

abhinav-thales · 2025-02-03T08:02:22Z

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

Hello @bhess, could you please help to review the PR when you have a moment ?
Thanks

bhess · 2025-02-03T08:37:08Z

Thanks @abhinav-thales , I'll take a closer look this week.

baentsch · 2025-02-03T09:26:48Z

Can I suggest checking this PR against the code in #2041 (that should include the same logic if I'm not mistaken)?

abhinav-thales · 2025-02-04T04:31:41Z

Can I suggest checking this PR against the code in #2041 (that should include the same logic if I'm not mistaken)?

Thanks @baentsch for the suggestion. I tested the changes locally by fetching the branch from the mentioned PR and updating this PR changes over it. With these changes, the tests were run and they look fine.

dstebila · 2025-02-04T16:24:00Z

As discussed in today's OQS status call, tagging @ydoroz to see if the cuPQC code has the relevant checks.

abhinav-thales · 2025-02-07T08:58:43Z

NOTE: The failing test is unrelated to the PR changes.

It is due to #2056

bhess

Thanks @abhinav-thales for adding these tests! See the few comments/nits.

bhess · 2025-02-10T10:22:06Z

tests/vectors_kem.c

+	encaps key is of length 384K bytes(384K*8 bits). Grouped into 12-bit values, the buffer requires (384*K*8)/12 = 256*K entries of 12 bits */
+	uint16_t buffd[ML_KEM_N * ML_KEM_K_MAX] = {0};
+	/* buffer to hold encoded value */
+	uint8_t buffe[ML_KEM_1024_PK_SIZE] = {0};


Isn't the max value ML_KEM_1024_PK_SIZE - 32 (384*K), since the rho value isn't used in the check?

nice catch...changed to -32 now

bhess · 2025-02-10T10:31:29Z

tests/vectors_kem.c

 #include "system_info.c"

+#ifdef OQS_ENABLE_KEM_ML_KEM
+/* macros for sanity checks for encaps and decaps key */
+#define ML_KEM_BLOCKSIZE        384


nit: It might be better to name this ML_KEM_POLYBYTES, as that’s the naming convention most implementations I’m familiar with use.

changed to POLY_BYTES

bhess · 2025-02-10T10:35:05Z

tests/vectors_kem.c

+		return false;
+	}
+	/* buffer to hold public key hash */
+	uint8_t pkdig[SHA256_OP_LEN] = {0};


Better name this SHA3_256_OP_LEN to be clear that this is SHA3 and not SHA256.

bhess · 2025-02-10T10:40:40Z

tests/vectors_kem.c

+		return 0;  // Default/error case
+	}
+}
+/* sanity check for private/decaps key */


I'd add a reference to the spec to be clear which check is performed here (e.g. "hash check" in FIPS 203, 7.3).

bhess · 2025-02-10T10:41:18Z

tests/vectors_kem.c

+	}
+	return true;
+}
+/* sanity check for public/encaps key */


I'd add a reference to the spec to be clear which check is performed here (e.g. "modulus check" in FIPS 203, 7.2).

mkannwischer · 2025-02-13T05:57:08Z

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.

@bhess @dstebila
Some clarification: mlkem-native does indeed have the pk and sk validation. What we do not do by default is the pair-wise consistency check (PCT) that checks at the end of keygen that encaps+decaps works as expected.
We added this just now, but it is disabled by default because it is so expensive - you can enable it by defining MLK_KEYGEN_PCT.
This check is optional according to FIPS203, but it is mandatory according to FIPS 140-3 IG.
Due to the cost, one likely only wants to enable this in case FIPS 140-3 validation is needed.

I have yet to find a cryptographer who thinks the PCT is useful .... but that's another discussion.

baentsch · 2025-02-13T09:02:49Z

you can enable it by defining MLK_KEYGEN_PCT

Thanks for letting us know -- this seems like a very sensible approach -- and also one that could be used to make me overcome a basic "unhappiness" with this PR: It's adding code of value only to a single algorithm and under specific circumstances (liboqs was meant to be algorithm-agnostic such as to ease the support effort for the library for the few people actively contributing).

So what about this proposal: We create a new config option, say OQS_STRICT_FIPS that would guard/enable both, these new checks as well as MLK_KEYGEN_PCT? It should be OFF by default so users have to select it in full acceptance of the impact. OQS supports/tests it in CI for as long as there's a committer in liboqs actively supporting this algorithm & feature (@mkannwischer would you be willing to take this on in case @bhess couldn't?).

Signed-off-by: Abhinav Saxena <abhinav.saxena@thalesgroup.com>

abhinav-thales · 2025-02-18T10:47:04Z

Thanks @abhinav-thales for adding these tests! See the few comments/nits.

Hi @bhess , thanks for the review. have addressed the comments now. Tests look good.
Please review at your convenience

add checks for ML-KEM keys

1e40f58

Signed-off-by: Abhinav Saxena <abhinav.saxena@thalesgroup.com>

abhinav-thales requested review from dstebila and baentsch as code owners December 2, 2024 11:21

abhinav-thales added 2 commits December 3, 2024 11:02

fix build issues

5005d7f

Signed-off-by: Abhinav Saxena <abhinav.saxena@thalesgroup.com>

deadcode removal

faf5669

Signed-off-by: Abhinav Saxena <abhinav.saxena@thalesgroup.com>

baentsch requested a review from a team December 13, 2024 11:09

SWilson4 reviewed Dec 13, 2024

View reviewed changes

tests/vectors_kem.c Show resolved Hide resolved

tests/vectors_kem.c Outdated Show resolved Hide resolved

tests/vectors_kem.c Outdated Show resolved Hide resolved

tests/vectors_kem.c Outdated Show resolved Hide resolved

tests/vectors_kem.c Outdated Show resolved Hide resolved

fix the review comments

f9bc387

Signed-off-by: Abhinav Saxena <abhinav.saxena@thalesgroup.com>

abhinav-thales added 4 commits January 21, 2025 10:25

add mod(3329) using barrett reduction

82c6ab4

Signed-off-by: Abhinav Saxena <abhinav.saxena@thalesgroup.com>

fix build issues

6b37a38

Signed-off-by: Abhinav Saxena <abhinav.saxena@thalesgroup.com>

fix build issues

83e5d42

Signed-off-by: Abhinav Saxena <abhinav.saxena@thalesgroup.com>

Merge branch 'open-quantum-safe:main' into kemkeyschk

e2b09a5

SWilson4 approved these changes Jan 23, 2025

View reviewed changes

Merge branch 'open-quantum-safe:main' into kemkeyschk

5f1478f

SWilson4 requested a review from a team February 7, 2025 12:01

dstebila approved these changes Feb 7, 2025

View reviewed changes

bhess reviewed Feb 10, 2025

View reviewed changes

address review comments

2d51456

Signed-off-by: Abhinav Saxena <abhinav.saxena@thalesgroup.com>

Add checks for ML-KEM keys #2009

Are you sure you want to change the base?

Add checks for ML-KEM keys #2009

Conversation

abhinav-thales commented Dec 2, 2024 • edited Loading

baentsch commented Dec 3, 2024

bhess commented Dec 3, 2024

baentsch commented Dec 3, 2024

baentsch commented Dec 3, 2024

abhinav-thales commented Dec 11, 2024

baentsch commented Dec 11, 2024

bhess commented Dec 11, 2024

abhinav-thales commented Dec 13, 2024

SWilson4 left a comment

Choose a reason for hiding this comment

nidhidamodaran commented Dec 16, 2024 • edited Loading

SWilson4 commented Dec 20, 2024

nidhidamodaran commented Dec 20, 2024 • edited Loading

dstebila commented Jan 7, 2025

bhess commented Jan 8, 2025 • edited Loading

SWilson4 left a comment

Choose a reason for hiding this comment

abhinav-thales commented Feb 3, 2025

bhess commented Feb 3, 2025

baentsch commented Feb 3, 2025

abhinav-thales commented Feb 4, 2025

dstebila commented Feb 4, 2025

abhinav-thales commented Feb 7, 2025 • edited Loading

bhess left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkannwischer commented Feb 13, 2025 • edited Loading

baentsch commented Feb 13, 2025

abhinav-thales commented Feb 18, 2025

abhinav-thales commented Dec 2, 2024 •

edited

Loading

nidhidamodaran commented Dec 16, 2024 •

edited

Loading

nidhidamodaran commented Dec 20, 2024 •

edited

Loading

bhess commented Jan 8, 2025 •

edited

Loading

abhinav-thales commented Feb 7, 2025 •

edited

Loading

mkannwischer commented Feb 13, 2025 •

edited

Loading