From 4b07805bddc55f68e5ce8c42f215da51c7a4e099 Mon Sep 17 00:00:00 2001 From: Gerardo Ravago Date: Thu, 6 Jun 2024 13:43:24 -0400 Subject: [PATCH] Prevent non-constant-time code in Kyber-R3 and ML-KEM implementation (#1619) ### Issues: Resolves V1399146249 ### Description of changes: A branch can be introduced by the compiler from the `poly_frommsg` code. This has been reported for Clang versions 15-18, x86_64, for the following compiler flags: - `-Os` - `-O1` - `-O2 -fno-vectorize` - `-O3 -fno-vectorize` An upstream fix has been published [here](https://github.com/pq-crystals/kyber/commit/9b8d30698a3e7449aeb34e62339d4176f11e3c6c) which we are deciding not to take directly. Due to the way AWS-LC compiles the Kyber reference code, it may not have the desired effect. We instead opt to use our own constant-time implementations for performing conditional bit-masks which have much stronger guarantees around portability. ### Call-outs: Point out areas that need special attention or support during the review process. Discuss architecture or design changes. - An interesting observation is that the old code would *always* produce SIMD instructions despite passing in `-fno-vectorize` and technically never produced the non-constant-time branching described. - This new code for performing the conditional bit-masks seems to completely avoid SIMD now. The fact that this is more controlled/predicted is an improvement in my book. --- crypto/kyber/README.md | 1 + crypto/kyber/pqcrystals_kyber_ref_common/poly.c | 16 +++++++++++++--- crypto/ml_kem/README.md | 1 + crypto/ml_kem/ml_kem_ipd_ref_common/poly.c | 16 +++++++++++++--- 4 files changed, 28 insertions(+), 6 deletions(-) diff --git a/crypto/kyber/README.md b/crypto/kyber/README.md index aa56a1ab5c..d47e4850fd 100644 --- a/crypto/kyber/README.md +++ b/crypto/kyber/README.md @@ -24,5 +24,6 @@ The following changes were made to the source code in `pqcrystals_kyber_ref_comm * `symmetric-shake.c`: unnecessary include of `fips202.h` is removed. * `api.h`, `fips202.h`, `params.h`: modified [in this PR](https://github.com/aws/aws-lc/pull/655) to support our [prefixed symbols build](https://github.com/aws/aws-lc/blob/main/BUILDING.md#building-with-prefixed-symbols). * `poly.c` and 'polyvec.c' were modified to remove 6 lines of comment from these two reference commits ([dda29cc](https://github.com/pq-crystals/kyber/commit/dda29cc63af721981ee2c831cf00822e69be3220), [272125f](https://github.com/pq-crystals/kyber/commit/272125f6acc8e8b6850fd68ceb901a660ff48196)) +* `poly.c`: the `poly_frommsg` function was modified to address the constant-time issue described [here](https://github.com/pq-crystals/kyber/commit/9b8d30698a3e7449aeb34e62339d4176f11e3c6c). **Usage.** The KEM API is defined and documented in `include/openssl/evp.h`. To see examples of how to use any KEM, including Kyber, see `crypto/kem/README.md`. diff --git a/crypto/kyber/pqcrystals_kyber_ref_common/poly.c b/crypto/kyber/pqcrystals_kyber_ref_common/poly.c index 0aac83a5d8..eee8508392 100644 --- a/crypto/kyber/pqcrystals_kyber_ref_common/poly.c +++ b/crypto/kyber/pqcrystals_kyber_ref_common/poly.c @@ -1,4 +1,7 @@ #include + +#include + #include "params.h" #include "poly.h" #include "ntt.h" @@ -6,6 +9,8 @@ #include "cbd.h" #include "symmetric.h" +#include "../../internal.h" + /************************************************* * Name: poly_compress * @@ -164,7 +169,7 @@ void poly_frombytes(poly *r, const uint8_t a[KYBER_POLYBYTES]) void poly_frommsg(poly *r, const uint8_t msg[KYBER_INDCPA_MSGBYTES]) { unsigned int i,j; - int16_t mask; + crypto_word_t mask; #if (KYBER_INDCPA_MSGBYTES != KYBER_N/8) #error "KYBER_INDCPA_MSGBYTES must be equal to KYBER_N/8 bytes!" @@ -172,8 +177,13 @@ void poly_frommsg(poly *r, const uint8_t msg[KYBER_INDCPA_MSGBYTES]) for(i=0;i> j)&1); - r->coeffs[8*i+j] = mask & ((KYBER_Q+1)/2); + mask = constant_time_is_zero_w((msg[i] >> j) & 1); + // We cast the result of constant_time_select_w, which is a crypto_word_t, + // to int16_t. The constants must be within the range of int16_t. + OPENSSL_STATIC_ASSERT(((KYBER_Q+1)/2) <= INT16_MAX, + value_exceeds_int16_max); + r->coeffs[8*i+j] = (int16_t) constant_time_select_w(mask, + 0, ((KYBER_Q+1)/2)); } } } diff --git a/crypto/ml_kem/README.md b/crypto/ml_kem/README.md index 1353d24464..83d11dff7d 100644 --- a/crypto/ml_kem/README.md +++ b/crypto/ml_kem/README.md @@ -12,5 +12,6 @@ The following changes were made to the source code in `ml_kem_ipd_ref_common` di - `fips202.{h|c}` are deleted and the ones from `crypto/kyber/pqcrystals_kyber_ref_common` directory are used. - `symmetric-shake.c`: unnecessary include of fips202.h is removed. - `api.h`: `pqcrystals` prefix substituted with `ml_kem` (to be able build alongside `crypto/kyber`). +* `poly.c`: the `poly_frommsg` function was modified to address the constant-time issue described [here](https://github.com/pq-crystals/kyber/commit/9b8d30698a3e7449aeb34e62339d4176f11e3c6c). The KATs were generated by compiling and running the KAT generator tests from the official repository. Specifically, running `make` in the `ref` folder produces `nistkat/PQCgenKAT_kem512` binary that can generates the test vectors. diff --git a/crypto/ml_kem/ml_kem_ipd_ref_common/poly.c b/crypto/ml_kem/ml_kem_ipd_ref_common/poly.c index 0fe5a20f63..0869324a92 100644 --- a/crypto/ml_kem/ml_kem_ipd_ref_common/poly.c +++ b/crypto/ml_kem/ml_kem_ipd_ref_common/poly.c @@ -1,4 +1,7 @@ #include + +#include + #include "params.h" #include "poly.h" #include "ntt.h" @@ -6,6 +9,8 @@ #include "cbd.h" #include "symmetric.h" +#include "../../internal.h" + /************************************************* * Name: poly_compress * @@ -167,7 +172,7 @@ void poly_frombytes(poly *r, const uint8_t a[KYBER_POLYBYTES]) void poly_frommsg(poly *r, const uint8_t msg[KYBER_INDCPA_MSGBYTES]) { unsigned int i,j; - int16_t mask; + crypto_word_t mask; #if (KYBER_INDCPA_MSGBYTES != KYBER_N/8) #error "KYBER_INDCPA_MSGBYTES must be equal to KYBER_N/8 bytes!" @@ -175,8 +180,13 @@ void poly_frommsg(poly *r, const uint8_t msg[KYBER_INDCPA_MSGBYTES]) for(i=0;i> j)&1); - r->coeffs[8*i+j] = mask & ((KYBER_Q+1)/2); + mask = constant_time_is_zero_w((msg[i] >> j) & 1); + // We cast the result of constant_time_select_w, which is a crypto_word_t, + // to int16_t. The constants must be within the range of int16_t. + OPENSSL_STATIC_ASSERT(((KYBER_Q+1)/2) <= INT16_MAX, + value_exceeds_int16_max); + r->coeffs[8*i+j] = (int16_t) constant_time_select_w(mask, + 0, ((KYBER_Q+1)/2)); } } }