From a0670f3ee466d9f3860ba0fb68e7274bd53b2d4d Mon Sep 17 00:00:00 2001 From: Andrew Hopkins Date: Thu, 13 Apr 2023 15:36:22 -0700 Subject: [PATCH] Update RAND_bytes to return return 0 on failure (#950) Update RAND_bytes to return return 0 on failure, update all libcrypto callers to handle this new failure. --- crypto/curve25519/curve25519.c | 14 ++++++++++++-- crypto/curve25519/spake25519.c | 4 +++- crypto/fips_callback_test.cc | 22 ++++++++++++++++++++++ crypto/fipsmodule/bn/random.c | 9 +++++++-- crypto/fipsmodule/cipher/e_aes.c | 5 ++++- crypto/fipsmodule/rand/internal.h | 2 +- crypto/fipsmodule/rand/rand.c | 29 ++++++++++++++++------------- crypto/fipsmodule/rsa/padding.c | 16 ++++++++++++---- crypto/hpke/hpke.c | 4 +++- crypto/hrss/hrss.c | 9 ++++++--- crypto/pool/pool.c | 6 +++++- crypto/trust_token/pmbtoken.c | 8 ++++++-- crypto/trust_token/voprf.c | 4 +++- tests/ci/run_fips_callback_tests.sh | 11 +++++++++++ 14 files changed, 111 insertions(+), 32 deletions(-) diff --git a/crypto/curve25519/curve25519.c b/crypto/curve25519/curve25519.c index 72cbe9350b..e943b00350 100644 --- a/crypto/curve25519/curve25519.c +++ b/crypto/curve25519/curve25519.c @@ -1872,7 +1872,12 @@ static void sc_muladd(uint8_t *s, const uint8_t *a, const uint8_t *b, void ED25519_keypair(uint8_t out_public_key[32], uint8_t out_private_key[64]) { uint8_t seed[ED25519_SEED_LEN]; - RAND_bytes(seed, ED25519_SEED_LEN); + if (!RAND_bytes(seed, ED25519_SEED_LEN)) { + // This is a public void function and can't be updated + OPENSSL_cleanse(out_public_key, 32); + OPENSSL_cleanse(out_private_key, 64); + return; + } ED25519_keypair_from_seed(out_public_key, out_private_key, seed); OPENSSL_cleanse(seed, ED25519_SEED_LEN); } @@ -2091,7 +2096,12 @@ static void x25519_scalar_mult(uint8_t out[32], const uint8_t scalar[32], } void X25519_keypair(uint8_t out_public_value[32], uint8_t out_private_key[32]) { - RAND_bytes(out_private_key, 32); + if (!RAND_bytes(out_private_key, 32)) { + // This is a public void function and can't be updated + OPENSSL_cleanse(out_public_value, 32); + OPENSSL_cleanse(out_private_key, 32); + return; + } // All X25519 implementations should decode scalars correctly (see // https://tools.ietf.org/html/rfc7748#section-5). However, if an diff --git a/crypto/curve25519/spake25519.c b/crypto/curve25519/spake25519.c index c50e96d210..c373cd72f3 100644 --- a/crypto/curve25519/spake25519.c +++ b/crypto/curve25519/spake25519.c @@ -353,7 +353,9 @@ int SPAKE2_generate_msg(SPAKE2_CTX *ctx, uint8_t *out, size_t *out_len, } uint8_t private_tmp[64]; - RAND_bytes(private_tmp, sizeof(private_tmp)); + if (!RAND_bytes(private_tmp, sizeof(private_tmp))) { + return 0; + } x25519_sc_reduce(private_tmp); // Multiply by the cofactor (eight) so that we'll clear it when operating on // the peer's point later in the protocol. diff --git a/crypto/fips_callback_test.cc b/crypto/fips_callback_test.cc index b7b79c302d..811642ee6f 100644 --- a/crypto/fips_callback_test.cc +++ b/crypto/fips_callback_test.cc @@ -8,6 +8,7 @@ #include #include +#include #include #include #include @@ -95,4 +96,25 @@ TEST(FIPSCallback, PowerOnTests) { } } +TEST(FIPSCallback, DRBGRuntime) { + // At this point the library has loaded, if a self test was broken + // AWS_LC_FIPS_Callback would have already been called. If this test + // wasn't broken the call count should be zero + char*broken_runtime_test = getenv("BORINGSSL_FIPS_BREAK_TEST"); + ASSERT_EQ(0, failure_count); + ASSERT_EQ(1, FIPS_mode()); + uint8_t buf[10]; + if (broken_runtime_test != nullptr && strcmp(broken_runtime_test, "CRNG" ) == 0) { + ASSERT_FALSE(RAND_bytes(buf, sizeof(buf))); + // TODO: make AWS_LC_FIPS_error update a new global state so FIPS_mode returns 0 + ASSERT_EQ(1, FIPS_mode()); + ASSERT_EQ(1, failure_count); + } else { + // BORINGSSL_FIPS_BREAK_TEST has not been set and everything should work + ASSERT_TRUE(RAND_bytes(buf, sizeof(buf))); + ASSERT_EQ(1, FIPS_mode()); + ASSERT_EQ(0, failure_count); + } +} + #endif diff --git a/crypto/fipsmodule/bn/random.c b/crypto/fipsmodule/bn/random.c index cf003dfaa2..6cbe0bf7b9 100644 --- a/crypto/fipsmodule/bn/random.c +++ b/crypto/fipsmodule/bn/random.c @@ -158,8 +158,11 @@ int BN_rand(BIGNUM *rnd, int bits, int top, int bottom) { // |RAND_bytes| calls within the fipsmodule should be wrapped with state lock // functions to avoid updating the service indicator with the DRBG functions. FIPS_service_indicator_lock_state(); - RAND_bytes((uint8_t *)rnd->d, words * sizeof(BN_ULONG)); + int rand_success = RAND_bytes((uint8_t *)rnd->d, words * sizeof(BN_ULONG)); FIPS_service_indicator_unlock_state(); + if (!rand_success) { + return 0; + } rnd->d[words - 1] &= mask; if (top != BN_RAND_TOP_ANY) { @@ -333,7 +336,9 @@ int bn_rand_secret_range(BIGNUM *r, int *out_is_uniform, BN_ULONG min_inclusive, } // Select a uniform random number with num_bits(max_exclusive) bits. - RAND_bytes((uint8_t *)r->d, words * sizeof(BN_ULONG)); + if (!RAND_bytes((uint8_t *)r->d, words * sizeof(BN_ULONG))) { + goto end; + } r->d[words - 1] &= mask; // Check, in constant-time, if the value is in range. diff --git a/crypto/fipsmodule/cipher/e_aes.c b/crypto/fipsmodule/cipher/e_aes.c index 4cea97376c..e7fda8e60b 100644 --- a/crypto/fipsmodule/cipher/e_aes.c +++ b/crypto/fipsmodule/cipher/e_aes.c @@ -1295,8 +1295,11 @@ static int aead_aes_gcm_seal_scatter_randnonce( // |RAND_bytes| calls within the fipsmodule should be wrapped with state lock // functions to avoid updating the service indicator with the DRBG functions. FIPS_service_indicator_lock_state(); - RAND_bytes(nonce, sizeof(nonce)); + int rand_result = RAND_bytes(nonce, sizeof(nonce)); FIPS_service_indicator_unlock_state(); + if (!rand_result) { + return 0; + } const struct aead_aes_gcm_ctx *gcm_ctx = (const struct aead_aes_gcm_ctx *)&ctx->state; if (!aead_aes_gcm_seal_scatter_impl(gcm_ctx, out, out_tag, out_tag_len, diff --git a/crypto/fipsmodule/rand/internal.h b/crypto/fipsmodule/rand/internal.h index eea2be1eca..3ac5ea83fe 100644 --- a/crypto/fipsmodule/rand/internal.h +++ b/crypto/fipsmodule/rand/internal.h @@ -33,7 +33,7 @@ extern "C" { // RAND_bytes_with_additional_data samples from the RNG after mixing 32 bytes // from |user_additional_data| in. -void RAND_bytes_with_additional_data(uint8_t *out, size_t out_len, +int RAND_bytes_with_additional_data(uint8_t *out, size_t out_len, const uint8_t user_additional_data[32]); // CRYPTO_sysrand fills |len| bytes at |buf| with entropy from the operating diff --git a/crypto/fipsmodule/rand/rand.c b/crypto/fipsmodule/rand/rand.c index e1d7cae486..4ea1a0a669 100644 --- a/crypto/fipsmodule/rand/rand.c +++ b/crypto/fipsmodule/rand/rand.c @@ -301,7 +301,7 @@ static void CRYPTO_get_fips_seed(uint8_t *out_entropy, size_t out_entropy_len, // rand_get_seed fills |seed| with entropy and sets |*out_want_additional_input| // to one if that entropy came directly from the CPU and zero otherwise. -static void rand_get_seed(struct rand_thread_state *state, +static int rand_get_seed(struct rand_thread_state *state, uint8_t seed[CTR_DRBG_ENTROPY_LEN], int *out_want_additional_input) { if (!state->last_block_valid) { @@ -317,9 +317,7 @@ static void rand_get_seed(struct rand_thread_state *state, // generator test” which causes the program to randomly abort. Hopefully the // rate of failure is small enough not to be a problem in practice. if (CRYPTO_memcmp(state->last_block, entropy, CRNGT_BLOCK_SIZE) == 0) { - fprintf(stderr, "CRNGT failed.\n"); - // TODO return the result from AWS_LC_FIPS_error and update rand_get_seed to return an int - AWS_LC_FIPS_error("CRNGT failed.", ERR_R_FIPS_TEST_FAILURE); + return AWS_LC_FIPS_error("CRNGT failed.", ERR_R_FIPS_TEST_FAILURE); } OPENSSL_STATIC_ASSERT(sizeof(entropy) % CRNGT_BLOCK_SIZE == 0, _) @@ -327,8 +325,7 @@ static void rand_get_seed(struct rand_thread_state *state, i += CRNGT_BLOCK_SIZE) { if (CRYPTO_memcmp(entropy + i - CRNGT_BLOCK_SIZE, entropy + i, CRNGT_BLOCK_SIZE) == 0) { - // TODO return the result from AWS_LC_FIPS_error and update rand_get_seed to return an int - AWS_LC_FIPS_error("CRNGT failed.", ERR_R_FIPS_TEST_FAILURE); + return AWS_LC_FIPS_error("CRNGT failed.", ERR_R_FIPS_TEST_FAILURE); } } OPENSSL_memcpy(state->last_block, @@ -336,27 +333,29 @@ static void rand_get_seed(struct rand_thread_state *state, CRNGT_BLOCK_SIZE); OPENSSL_memcpy(seed, entropy, CTR_DRBG_ENTROPY_LEN); + return 1; } #else // BORINGSSL_FIPS // rand_get_seed fills |seed| with entropy and sets |*out_want_additional_input| // to one if that entropy came directly from the CPU and zero otherwise. -static void rand_get_seed(struct rand_thread_state *state, +static int rand_get_seed(struct rand_thread_state *state, uint8_t seed[CTR_DRBG_ENTROPY_LEN], int *out_want_additional_input) { // If not in FIPS mode, we don't overread from the system entropy source and // we don't depend only on the hardware RDRAND. CRYPTO_sysrand_for_seed(seed, CTR_DRBG_ENTROPY_LEN); *out_want_additional_input = 0; + return 1; } #endif // BORINGSSL_FIPS -void RAND_bytes_with_additional_data(uint8_t *out, size_t out_len, +int RAND_bytes_with_additional_data(uint8_t *out, size_t out_len, const uint8_t user_additional_data[32]) { if (out_len == 0) { - return; + return 1; } const uint64_t fork_generation = CRYPTO_get_fork_generation(); @@ -422,7 +421,9 @@ void RAND_bytes_with_additional_data(uint8_t *out, size_t out_len, state->last_block_valid = 0; uint8_t seed[CTR_DRBG_ENTROPY_LEN]; int want_additional_input; - rand_get_seed(state, seed, &want_additional_input); + if (!rand_get_seed(state, seed, &want_additional_input)) { + return 0; + } uint8_t personalization[CTR_DRBG_ENTROPY_LEN] = {0}; size_t personalization_len = 0; @@ -462,7 +463,9 @@ void RAND_bytes_with_additional_data(uint8_t *out, size_t out_len, state->fork_generation != fork_generation) { uint8_t seed[CTR_DRBG_ENTROPY_LEN]; int want_additional_input; - rand_get_seed(state, seed, &want_additional_input); + if (!rand_get_seed(state, seed, &want_additional_input)){ + return 0; + } uint8_t add_data_for_reseed[CTR_DRBG_ENTROPY_LEN]; size_t add_data_for_reseed_len = 0; @@ -526,12 +529,12 @@ void RAND_bytes_with_additional_data(uint8_t *out, size_t out_len, #if defined(BORINGSSL_FIPS) CRYPTO_STATIC_MUTEX_unlock_read(state_clear_all_lock_bss_get()); #endif + return 1; } int RAND_bytes(uint8_t *out, size_t out_len) { static const uint8_t kZeroAdditionalData[32] = {0}; - RAND_bytes_with_additional_data(out, out_len, kZeroAdditionalData); - return 1; + return RAND_bytes_with_additional_data(out, out_len, kZeroAdditionalData); } int RAND_pseudo_bytes(uint8_t *buf, size_t len) { diff --git a/crypto/fipsmodule/rsa/padding.c b/crypto/fipsmodule/rsa/padding.c index 3f33e9e09f..19e7dc42a9 100644 --- a/crypto/fipsmodule/rsa/padding.c +++ b/crypto/fipsmodule/rsa/padding.c @@ -145,19 +145,27 @@ int RSA_padding_check_PKCS1_type_1(uint8_t *out, size_t *out_len, return 1; } -static void rand_nonzero(uint8_t *out, size_t len) { +static int rand_nonzero(uint8_t *out, size_t len) { + int result = 0; // |RAND_bytes| calls within the fipsmodule should be wrapped with state lock // functions to avoid updating the service indicator with the DRBG functions. FIPS_service_indicator_lock_state(); - RAND_bytes(out, len); + if (!RAND_bytes(out, len)) { + goto out; + } for (size_t i = 0; i < len; i++) { while (out[i] == 0) { - RAND_bytes(out + i, 1); + if (!RAND_bytes(out + i, 1)){ + goto out; + } } } - + result = 1; + +out: FIPS_service_indicator_unlock_state(); + return result; } int RSA_padding_add_PKCS1_type_2(uint8_t *to, size_t to_len, diff --git a/crypto/hpke/hpke.c b/crypto/hpke/hpke.c index faea2eea06..22faf3d6b7 100644 --- a/crypto/hpke/hpke.c +++ b/crypto/hpke/hpke.c @@ -487,7 +487,9 @@ int EVP_HPKE_CTX_setup_sender(EVP_HPKE_CTX *ctx, uint8_t *out_enc, size_t peer_public_key_len, const uint8_t *info, size_t info_len) { uint8_t seed[MAX_SEED_LEN]; - RAND_bytes(seed, kem->seed_len); + if (!RAND_bytes(seed, kem->seed_len)) { + return 0; + } return EVP_HPKE_CTX_setup_sender_with_seed_for_testing( ctx, out_enc, out_enc_len, max_enc, kem, kdf, aead, peer_public_key, peer_public_key_len, info, info_len, seed, kem->seed_len); diff --git a/crypto/hrss/hrss.c b/crypto/hrss/hrss.c index e05fef4851..e2cef886b6 100644 --- a/crypto/hrss/hrss.c +++ b/crypto/hrss/hrss.c @@ -1999,7 +1999,8 @@ int HRSS_generate_key( if (!vars) { // If the caller ignores the return value the output will still be safe. // The private key output is randomised in case it's later passed to - // |HRSS_encap|. + // |HRSS_encap|. As this is the error case we can't error out harder + // hence the result of RAND_bytes is not checked. memset(out_pub, 0, sizeof(struct HRSS_public_key)); RAND_bytes((uint8_t*) out_priv, sizeof(struct HRSS_private_key)); return 0; @@ -2062,7 +2063,8 @@ int HRSS_encap(uint8_t out_ciphertext[POLY_BYTES], uint8_t out_shared_key[32], if (!vars) { // If the caller ignores the return value the output will still be safe. // The private key output is randomised in case it's used to encrypt and - // transmit something. + // transmit something. As this is the error case we can't error out harder + // hence the result of RAND_bytes is not checked. memset(out_ciphertext, 0, POLY_BYTES); RAND_bytes(out_shared_key, 32); return 0; @@ -2124,7 +2126,8 @@ int HRSS_decap(uint8_t out_shared_key[HRSS_KEY_BYTES], if (!vars) { // If the caller ignores the return value the output will still be safe. // The private key output is randomised in case it's used to encrypt and - // transmit something. + // transmit something. As this is the error case we can't error out harder + // hence the result of RAND_bytes is not checked. RAND_bytes(out_shared_key, HRSS_KEY_BYTES); return 0; } diff --git a/crypto/pool/pool.c b/crypto/pool/pool.c index e889f521da..a7382eb1b4 100644 --- a/crypto/pool/pool.c +++ b/crypto/pool/pool.c @@ -55,7 +55,11 @@ CRYPTO_BUFFER_POOL* CRYPTO_BUFFER_POOL_new(void) { } CRYPTO_MUTEX_init(&pool->lock); - RAND_bytes((uint8_t *)&pool->hash_key, sizeof(pool->hash_key)); + if (!RAND_bytes((uint8_t *)&pool->hash_key, sizeof(pool->hash_key))) { + OPENSSL_free(pool); + return NULL; + + } return pool; } diff --git a/crypto/trust_token/pmbtoken.c b/crypto/trust_token/pmbtoken.c index 7a95a7d142..09556e4996 100644 --- a/crypto/trust_token/pmbtoken.c +++ b/crypto/trust_token/pmbtoken.c @@ -346,7 +346,9 @@ static STACK_OF(TRUST_TOKEN_PRETOKEN) *pmbtoken_blind( goto err; } - RAND_bytes(pretoken->salt, sizeof(pretoken->salt)); + if (!RAND_bytes(pretoken->salt, sizeof(pretoken->salt))) { + goto err; + } if (include_message) { assert(SHA512_DIGEST_LENGTH == TRUST_TOKEN_NONCE_SIZE); SHA512_Init(&hash_ctx); @@ -854,7 +856,9 @@ static int pmbtoken_sign(const PMBTOKEN_METHOD *method, ec_scalar_select(group, &yb, mask, &key->y1, &key->y0); uint8_t s[TRUST_TOKEN_NONCE_SIZE]; - RAND_bytes(s, TRUST_TOKEN_NONCE_SIZE); + if(!RAND_bytes(s, TRUST_TOKEN_NONCE_SIZE)) { + goto err; + } // The |jacobians| and |affines| contain Sp, Wp, and Wsp. EC_RAW_POINT jacobians[3]; EC_AFFINE affines[3]; diff --git a/crypto/trust_token/voprf.c b/crypto/trust_token/voprf.c index da29c85641..945687bafd 100644 --- a/crypto/trust_token/voprf.c +++ b/crypto/trust_token/voprf.c @@ -227,7 +227,9 @@ static STACK_OF(TRUST_TOKEN_PRETOKEN) *voprf_blind(const VOPRF_METHOD *method, goto err; } - RAND_bytes(pretoken->salt, sizeof(pretoken->salt)); + if(!RAND_bytes(pretoken->salt, sizeof(pretoken->salt))) { + goto err; + } if (include_message) { assert(SHA512_DIGEST_LENGTH == TRUST_TOKEN_NONCE_SIZE); SHA512_Init(&hash_ctx); diff --git a/tests/ci/run_fips_callback_tests.sh b/tests/ci/run_fips_callback_tests.sh index e33b461bf4..df0966a40f 100755 --- a/tests/ci/run_fips_callback_tests.sh +++ b/tests/ci/run_fips_callback_tests.sh @@ -28,3 +28,14 @@ for kat in $KATS; do $broken_test --gtest_filter=FIPSCallback.PowerOnTests unset FIPS_CALLBACK_TEST_POWER_ON_TEST_FAILURE done + +runtime_tests=("CRNG") +for runtime_test in "${runtime_tests[@]}"; do + # Tell our test what test is expected to fail + export FIPS_CALLBACK_TEST_RUNTIME_TEST_FAILURE="$runtime_test" + # Tell bcm which test to break + export BORINGSSL_FIPS_BREAK_TEST="$runtime_test" + # These tests will have side affects in the future (modifying the global FIPS state) and must be run in separate process + $original_test --gtest_filter=FIPSCallback.PowerOnTests + $original_test --gtest_filter=FIPSCallback.DRBGRuntime +done