PQ: fix timing sidechannels and add IPDWing #243

bwesterb · 2024-06-24T19:45:19Z

Fix three potential timing sidechannels. These don't affect ephemeral usage of Kyber as in TLS, but it's good practice to get rid of them anyway.

Also adds IPDWing, a preliminary version of X-Wing using the initial public draft (IPD) of ML-KEM. Don't use it.

Fix three potential timing sidechannels. These don't affect ephemeral usage of Kyber as in TLS, but it's good practice to get rid of them anyway. Also adds IPDWing, a preliminary version of X-Wing using the initial public draft (IPD) of ML-KEM. Don't use it.

cjpatton

I'm working my way down from the TLS bits. Everything looks good so far. My next step is to look at what's been modified in fips202.c, then ipdwing.c itself. Shouldn't be too bad.

A couple of questions:

Can you provide a source for the side channel mitigations? I want to make sure I understand what they're doing and that they've been implemented properly.
I think it would be worth pulling in the test vectors from the xwing draft. Would you mind adding these to the patch, as well as code to exercise them in tests?

boring-sys/patches/boring-pq.patch

cjpatton · 2024-06-27T00:30:41Z

boring-sys/patches/boring-pq.patch

+};
+struct IPDWING_public_key {
+    struct KYBER768_public_key m;
+    uint8_t x[32];


nit: I find it odd that the curve point is named x here but named xpub as part of the private key. In the private key, x is the name of the secret scalar. Could we go with xpub here instead so that we have one name for each thing?

cjpatton · 2024-06-27T00:33:14Z

boring-sys/patches/boring-pq.patch

+// IPDWING_generate_key is a deterministic function that outputs a public and
+// private key based on the given entropy.


Following boringSSL comment conventions, I think you'd want to say here what values are written to which variables.

cjpatton · 2024-06-27T00:34:20Z

boring-sys/patches/boring-pq.patch

+// session key from the given entropy, writing those values to |out_shared_key|
+// and |out_ciphertext|, respectively.
+OPENSSL_EXPORT void IPDWING_encap(uint8_t out_ciphertext[IPDWING_CIPHERTEXT_BYTES],
+                              uint8_t out_shared_key[IPDWING_KEY_BYTES],


Unless boringSSL's variable naming conventions dictate otehrwise, I'd go with "shared_secret" here in order to align with the draft.

boring-sys/patches/boring-pq.patch

cjpatton

I've reviewed up to IPDWing and everything looks good. Again, I would strongly reocommend pulling in the test vectors from the draft.

What remains to check:

It looks like this there is a new abosrb-then-squeeze API for SHAKE25.
The side channel mitigations. Can you please provide a reference for this? I don't really know what I'm looking at. An in-line comment with a pointer would be most helpful.

cjpatton · 2024-06-27T21:45:45Z

boring-sys/patches/boring-pq.patch

-+void encap(uint8_t out_ciphertext[KYBER_CIPHERTEXTBYTES],
+// Internal version that allows us to select between initial public draft
+// (when ipd=1) and round3 kyber (ipd=0).
+void encap2(uint8_t out_ciphertext[KYBER_CIPHERTEXTBYTES],


"encap2" is confusing. Did you name this way because "encap" was already taken?

Likewise for "decap2" below.

Suggested change

+void encap2(uint8_t out_ciphertext[KYBER_CIPHERTEXTBYTES],

+void encap_for_version(uint8_t out_ciphertext[KYBER_CIPHERTEXTBYTES],

cjpatton · 2024-06-27T21:53:38Z

boring-sys/patches/boring-pq.patch

 +        uint8_t ss[KYBER_KEY_BYTES],
 +        const struct public_key *in_pub,
-+        const uint8_t seed[KYBER_ENCAP_BYTES])
+        const uint8_t seed[KYBER_ENCAP_BYTES],
+        int ipd)
 +{
 +  const uint8_t *pk = &in_pub->opaque[0];
 +  uint8_t *ct = out_ciphertext;


nit: There are spurious white spaces on the blankline below here. This is not clear in this PR, but is evident after applying the patch (kyber.c:1593).

cjpatton · 2024-06-27T22:28:11Z

boring-sys/patches/boring-pq.patch

+        &out_pub->m,
+        &out_priv->m,
+        seed);
+    memcpy(out_priv->x, seed+64, 32);


Suggested change

+ memcpy(out_priv->x, seed+64, 32);

+ memcpy(out_priv->x, seed+KYBER_GENERATE_KEY_BYTES, 32);

cjpatton · 2024-06-27T22:31:41Z

boring-sys/patches/boring-pq.patch

+    memcpy(out_priv->x, seed+64, 32);
+    X25519_public_from_private(out_pub->x, out_priv->x);
+    memcpy(out_priv->xpub, out_pub->x, 32);


pedantic: as far as I've seen, boringSSL's convention is to use named constants in these types of situations. I think in this case it makes the code more readable to non-domain experts and is therfore worth doing.

Likewise elsewhere.

Suggested change

+ memcpy(out_priv->x, seed+64, 32);

+ X25519_public_from_private(out_pub->x, out_priv->x);

+ memcpy(out_priv->xpub, out_pub->x, 32);

+ memcpy(out_priv->x, seed+64, X25519_PRIVATE_KEY_LEN);

+ X25519_public_from_private(out_pub->x, out_priv->x);

+ memcpy(out_priv->xpub, out_pub->x, X25519_PUBLIC_KEY_LEN);

cjpatton · 2024-06-27T22:33:59Z

boring-sys/patches/boring-pq.patch

+        uint8_t ss[IPDWING_KEY_BYTES],
+        const struct IPDWING_public_key *in_pub,
+        const uint8_t seed[IPDWING_ENCAP_BYTES]) {
+


Spurious newline?

Suggested change

+

cjpatton · 2024-06-27T22:43:44Z

boring-sys/patches/boring-pq.patch

+void IPDWING_decap(
+        uint8_t out_shared_key[IPDWING_KEY_BYTES],
+        const struct IPDWING_private_key *in_priv,
+        const uint8_t *ct) {


nit: consistency with out_ciphertext for encapsulation

Suggested change

+ const uint8_t *ct) {

+ const uint8_t *ciphertext) {

cjpatton · 2024-06-27T22:45:52Z

boring-sys/patches/boring-pq.patch

-+  uint32_t t,d;
-+  int16_t a,b;
+void IPDWING_decap(
+        uint8_t out_shared_key[IPDWING_KEY_BYTES],


Consistency with the same parameter for encapsulation

Suggested change

+ uint8_t out_shared_key[IPDWING_KEY_BYTES],

+ uint8_t ss[IPDWING_KEY_BYTES],

cjpatton · 2024-06-27T22:47:55Z

boring-sys/patches/boring-pq.patch

-+    d += (t>>1) & 0x55555555;
+void IPDWING_marshal_public_key(
+        uint8_t out[IPDWING_PUBLIC_KEY_BYTES],
+        const struct IPDWING_public_key *in) {


Consistency

Suggested change

+ const struct IPDWING_public_key *in) {

+ const struct IPDWING_public_key *in_pub) {

cjpatton · 2024-06-27T22:48:46Z

boring-sys/patches/boring-pq.patch

+    KYBER768_parse_public_key(&out->m, in);
+    memcpy(out->x, in + KYBER768_PUBLIC_KEY_BYTES, 32);
+}
+


Spurious newline

Suggested change

+

cjpatton · 2024-06-27T23:01:38Z

boring-sys/patches/boring-pq.patch

 src/crypto/kyber/keccak.c         |  204 --
- src/crypto/kyber/kyber.c          | 2865 ++++++++++++++++++++-------
+ src/crypto/kyber/kyber.c          | 2319 +++++++++++++++-------


Reviewer note: Most of the changes here are moving SHA-3 out of this file into fips202.c. Also, some functions have been added for SHAKE256 (needed for IPD); and some unused functions have been pruned.

bwesterb · 2024-06-28T07:51:17Z

I've reviewed up to IPDWing and everything looks good. Again, I would strongly reocommend pulling in the test vectors from the draft.

I cross tested connections with the Go implementation, which tests against the test vectors.

What remains to check:

It looks like this there is a new abosrb-then-squeeze API for SHAKE25.

It's re-introduced. Upstream always contained the functions, but didn't use it, so I pruned it for this patch.

The side channel mitigations. Can you please provide a reference for this? I don't really know what I'm looking at. An in-line comment with a pointer would be most helpful.

For 2/3 division is replaced by Barrett reduction, which is the exact same fix as upstream (1, 2).

For the third, we use BoringSSL's internal value barrier to prevent the compiler from using the fact that an intermediate is {0,1}-valued. Upstream fixes that in a less robust way that presumes that one does not use LTO.

cjpatton

Everything looks correct to me. Readability could be improved (see inline comments), but if you want you can ignore those.

I've reviewed up to IPDWing and everything looks good. Again, I would strongly reocommend pulling in the test vectors from the draft.

I cross tested connections with the Go implementation, which tests against the test vectors.

Fair enough, but implementations can drift. In my opinion, you should either add that test to CI here or add consume the test vectors here as well.

What remains to check:

It looks like this there is a new abosrb-then-squeeze API for SHAKE25.

It's re-introduced. Upstream always contained the functions, but didn't use it, so I pruned it for this patch.

Thanks! I confirmed the code is the same.

The side channel mitigations. Can you please provide a reference for this? I don't really know what I'm looking at. An in-line comment with a pointer would be most helpful.

For 2/3 division is replaced by Barrett reduction, which is the exact same fix as upstream (1, 2).

For the third, we use BoringSSL's internal value barrier to prevent the compiler from using the fact that an intermediate is {0,1}-valued. Upstream fixes that in a less robust way that presumes that one does not use LTO.

I'm able to verify most of the changes, except those to polyvec_compress(), as there is no corresponding change in pq-crystals as far as I can tell. I'll just have to trust you that you got the magic constants right. (And one day I'll learn Kyber!)

cjpatton · 2024-06-28T21:39:10Z

boring-sys/patches/boring-pq.patch

 +
-+static void polyvec_tobytes(uint8_t r[KYBER_POLYVECBYTES], const polyvec *a);
-+static void polyvec_frombytes(polyvec *r, const uint8_t a[KYBER_POLYVECBYTES]);
+/*************************************************


This PR adds keccak_init, keccak_absorb, keccak_finalize, and keccak_squeeze from https://github.com/pq-crystals/kyber/blob/main/ref/fips202.c#L637. The code is the same.

nit: keccak_squeeze now appears out of order in this file: in the source it follows keccak_finalize.

Likewise for shake256 below.

cjpatton · 2024-06-28T21:51:49Z

boring-sys/patches/boring-pq.patch

-+}
+  unsigned int i,j;
+  int16_t u;
+  uint32_t d0;


You use uint64_t for the same variable below in polyvec_compress(). Is uint32_t large enough here?

From the name (taken from upstream) you would think polyvec_compress is just a component-wise version of poly_vec, which it's not. For Kyber768 the former leaves 10 bits per coefficient (the d_u parameter), whereas the latter leaves 4 (the d_v parameter). Because the numbers are bigger, we also need a more accurate Barrett approximant for the division by Q. The intermediate values in the latter can grow as large as 2^42, whereas they stay below 2^32 in the former.

bwesterb · 2024-06-30T10:18:36Z

Thanks for the review!

I'm able to verify most of the changes, except those to polyvec_compress(), as there is no corresponding change in pq-crystals as far as I can tell. I'll just have to trust you that you got the magic constants right. (And one day I'll learn Kyber!)

The patch is here

Here is an explanation of the magic constants. That implementation chooses different ones, but the idea is the same.

rushilmehra · 2024-07-08T17:47:00Z

boring-sys/patches/boring-pq.patch

@@ -20,43 +20,55 @@ This patch adds:
   key agreement should only be used for testing: to see if the smaller
   keyshare makes a difference.

+4. Supportfor IPDWing under codepoint 0xfe41. This key agreement


Suggested change

4. Supportfor IPDWing under codepoint 0xfe41. This key agreement

4. Support for IPDWing under codepoint 0xfe41. This key agreement

This is the successor of X25519Kyber768Draft00. Spec: https://datatracker.ietf.org/doc/draft-kwiatkowski-tls-ecdhe-mlkem/02/ IANA has assigned the codepoint. https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-parameters-8 Upstream BoringSSL support landed in. google/boringssl@7fb4d3d The version of BoringSSL we patch does not include it, so we add it manually. Chrome and Firefox are planning to enable in October. This PR is based on the IPD-Wing patch reviewed here: #243 There are two changes. First we simplify the patch a bit as we do not need IPD-Wing. Secondly, we perform the encapsulation key check, which was a last minute addition of NIST. We perform this check also for Kyber.

bwesterb force-pushed the bas/ipdwing branch from 0c473bb to f4cf0c2 Compare June 24, 2024 19:55

bwesterb requested a review from inikulin June 24, 2024 19:56

bwesterb force-pushed the bas/ipdwing branch from f4cf0c2 to 9660f86 Compare June 26, 2024 15:35

cjpatton reviewed Jun 27, 2024

View reviewed changes

cjpatton approved these changes Jun 28, 2024

View reviewed changes

rushilmehra approved these changes Jul 8, 2024

View reviewed changes

inikulin approved these changes Jul 8, 2024

View reviewed changes

inikulin merged commit 4725a93 into master Jul 8, 2024
23 checks passed

bwesterb mentioned this pull request Sep 17, 2024

Add post-quantum key agreement X25519MLKEM768 #271

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PQ: fix timing sidechannels and add IPDWing #243

PQ: fix timing sidechannels and add IPDWing #243

bwesterb commented Jun 24, 2024

cjpatton left a comment

cjpatton Jun 27, 2024

cjpatton Jun 27, 2024

cjpatton Jun 27, 2024

cjpatton left a comment

cjpatton Jun 27, 2024

bwesterb Jun 28, 2024

cjpatton Jun 27, 2024

cjpatton Jun 27, 2024

cjpatton Jun 27, 2024

cjpatton Jun 27, 2024

cjpatton Jun 27, 2024

cjpatton Jun 27, 2024

cjpatton Jun 27, 2024

cjpatton Jun 27, 2024

cjpatton Jun 27, 2024

bwesterb commented Jun 28, 2024 •

edited

Loading

cjpatton left a comment

cjpatton Jun 28, 2024

cjpatton Jun 28, 2024

bwesterb Jun 30, 2024 •

edited

Loading

bwesterb commented Jun 30, 2024 •

edited

Loading

rushilmehra Jul 8, 2024

		+// IPDWING_generate_key is a deterministic function that outputs a public and
		+// private key based on the given entropy.

	+void encap2(uint8_t out_ciphertext[KYBER_CIPHERTEXTBYTES],
	+void encap_for_version(uint8_t out_ciphertext[KYBER_CIPHERTEXTBYTES],

	+ memcpy(out_priv->x, seed+64, 32);
	+ memcpy(out_priv->x, seed+KYBER_GENERATE_KEY_BYTES, 32);

	+ uint8_t out_shared_key[IPDWING_KEY_BYTES],
	+ uint8_t ss[IPDWING_KEY_BYTES],

	+ const struct IPDWING_public_key *in) {
	+ const struct IPDWING_public_key *in_pub) {

	4. Supportfor IPDWing under codepoint 0xfe41. This key agreement
	4. Support for IPDWing under codepoint 0xfe41. This key agreement

PQ: fix timing sidechannels and add IPDWing #243

PQ: fix timing sidechannels and add IPDWing #243

Conversation

bwesterb commented Jun 24, 2024

cjpatton 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

cjpatton 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

Choose a reason for hiding this comment

bwesterb commented Jun 28, 2024 • edited Loading

cjpatton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwesterb Jun 30, 2024 • edited Loading

Choose a reason for hiding this comment

bwesterb commented Jun 30, 2024 • edited Loading

Choose a reason for hiding this comment

bwesterb commented Jun 28, 2024 •

edited

Loading

bwesterb Jun 30, 2024 •

edited

Loading

bwesterb commented Jun 30, 2024 •

edited

Loading