From 28be5302b592d69425488b3bddef7c685e91de68 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 16 Jan 2024 15:48:17 -0500 Subject: [PATCH 1/4] Start making asserts constant-time too We've historically settled on treating asserts as not in scope for our constant-time goals. Production binaries are expected to be optimized builds, with debug assertions turned off. (We have a handful of assertions in perf-sensitive code that you definitely do not want to run with.) Secret data has invariants too, so it is useful to be able to write debug assertions on them. However, combined with our default CMake build being a debug build, this seems to cause some confusion with researchers sometimes. Also, if we ever get language-level constant-time support, we would need to resolve this mismatch anyway. (I assume any language support would put enough into the type system to force us to declassify any intentional branches on secret-by-data-flow bools, notably those we assert on.) So I'm inclined to just make our asserts constant-time. There are two issues around asserts, at least with our valgrind-based validation: The first is that a couple of asserts over secret data compute their condition leakily. We can just fix these. The only such ones I found were in bn_reduce_once and bn_gcd_consttime. The second is that almost every assert over secret data will be flagged as an invalid branch by valgrind. However, presuming the condition itself was computed in constant time, this branch is actually safe. If we were willing to abort the process when false, the assert is clearly publicly true. We just need to declassify the boolean to assert on it. assert(constant_time_declassify_int(expr)) is really long, so I made an internal wrapper macro declassify_assert(expr). Not sure if that's the best name. constant_time_declassify_assert(expr) is kinda long. constant_time_assert(expr) fits with the rest of that namespace, but reads as if we're somehow running an assert without branching, when the whole point is that we *are* branching and need to explicitly say it's okay to. Fixed: 339 Change-Id: Ie33b99bf9a269b11d2c48d246cc4934be7e239ff Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65467 Reviewed-by: Bob Beck Commit-Queue: David Benjamin (cherry picked from commit 9b8b483276da2b3d36ea21e97743e310314a8de0) --- CMakeLists.txt | 2 -- crypto/cipher_extra/tls_cbc.c | 4 ++-- crypto/fipsmodule/bn/bytes.c | 2 +- crypto/fipsmodule/bn/div.c | 6 ++--- crypto/fipsmodule/bn/div_extra.c | 2 +- crypto/fipsmodule/bn/exponentiation.c | 2 +- crypto/fipsmodule/bn/gcd_extra.c | 8 +++---- crypto/fipsmodule/bn/montgomery_inv.c | 2 +- crypto/fipsmodule/bn/mul.c | 4 ++-- crypto/fipsmodule/bn/prime.c | 2 +- crypto/fipsmodule/bn/random.c | 3 ++- .../fipsmodule/curve25519/curve25519_nohw.c | 14 ++++++------ crypto/fipsmodule/rsa/rsa_impl.c | 4 ++-- crypto/internal.h | 22 ++++++++++++------- 14 files changed, 41 insertions(+), 36 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 61b026ead0..eef2dc003b 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -773,8 +773,6 @@ endif() if(CONSTANT_TIME_VALIDATION) add_definitions(-DBORINGSSL_CONSTANT_TIME_VALIDATION) - # Asserts will often test secret data. - add_definitions(-DNDEBUG) endif() # CMake's iOS support uses Apple's multiple-architecture toolchain. It takes an diff --git a/crypto/cipher_extra/tls_cbc.c b/crypto/cipher_extra/tls_cbc.c index 51e137e73c..d60f4b857f 100644 --- a/crypto/cipher_extra/tls_cbc.c +++ b/crypto/cipher_extra/tls_cbc.c @@ -123,8 +123,8 @@ void EVP_tls_cbc_copy_mac(uint8_t *out, size_t md_size, const uint8_t *in, size_t mac_end = in_len; size_t mac_start = mac_end - md_size; - assert(orig_len >= in_len); - assert(in_len >= md_size); + declassify_assert(orig_len >= in_len); + declassify_assert(in_len >= md_size); assert(md_size <= EVP_MAX_MD_SIZE); assert(md_size > 0); diff --git a/crypto/fipsmodule/bn/bytes.c b/crypto/fipsmodule/bn/bytes.c index e222f2f685..c8b3a15fe6 100644 --- a/crypto/fipsmodule/bn/bytes.c +++ b/crypto/fipsmodule/bn/bytes.c @@ -246,7 +246,7 @@ void bn_assert_fits_in_bytes(const BIGNUM *bn, size_t num) { void bn_words_to_big_endian(uint8_t *out, size_t out_len, const BN_ULONG *in, size_t in_len) { // The caller should have selected an output length without truncation. - assert(fits_in_bytes(in, in_len, out_len)); + declassify_assert(fits_in_bytes(in, in_len, out_len)); size_t num_bytes = in_len * sizeof(BN_ULONG); if (out_len < num_bytes) { num_bytes = out_len; diff --git a/crypto/fipsmodule/bn/div.c b/crypto/fipsmodule/bn/div.c index 6c13716920..f524f8939a 100644 --- a/crypto/fipsmodule/bn/div.c +++ b/crypto/fipsmodule/bn/div.c @@ -425,7 +425,7 @@ BN_ULONG bn_reduce_once(BN_ULONG *r, const BN_ULONG *a, BN_ULONG carry, // // Although |carry| may be one if it was one on input and |bn_sub_words| // returns zero, this would give |r| > |m|, violating our input assumptions. - assert(carry == 0 || carry == (BN_ULONG)-1); + declassify_assert(carry + 1 <= 1); bn_select_words(r, carry, a /* r < 0 */, r /* r >= 0 */, num); return carry; } @@ -434,7 +434,7 @@ BN_ULONG bn_reduce_once_in_place(BN_ULONG *r, BN_ULONG carry, const BN_ULONG *m, BN_ULONG *tmp, size_t num) { // See |bn_reduce_once| for why this logic works. carry -= bn_sub_words(tmp, r, m, num); - assert(carry == 0 || carry == (BN_ULONG)-1); + declassify_assert(carry + 1 <= 1); bn_select_words(r, carry, r /* tmp < 0 */, tmp /* tmp >= 0 */, num); return carry; } @@ -504,7 +504,7 @@ int bn_div_consttime(BIGNUM *quotient, BIGNUM *remainder, // |divisor_min_bits| bits, the top |divisor_min_bits - 1| can be incorporated // without reductions. This significantly speeds up |RSA_check_key|. For // simplicity, we round down to a whole number of words. - assert(divisor_min_bits <= BN_num_bits(divisor)); + declassify_assert(divisor_min_bits <= BN_num_bits(divisor)); int initial_words = 0; if (divisor_min_bits > 0) { initial_words = (divisor_min_bits - 1) / BN_BITS2; diff --git a/crypto/fipsmodule/bn/div_extra.c b/crypto/fipsmodule/bn/div_extra.c index 141f7da67a..f75c9146c2 100644 --- a/crypto/fipsmodule/bn/div_extra.c +++ b/crypto/fipsmodule/bn/div_extra.c @@ -39,7 +39,7 @@ static uint16_t mod_u16(uint32_t n, uint16_t d, uint32_t p, uint32_t m) { // Multiply and subtract to get the remainder. n -= d * t; - assert(n < d); + declassify_assert(n < d); return n; } diff --git a/crypto/fipsmodule/bn/exponentiation.c b/crypto/fipsmodule/bn/exponentiation.c index eb404fbbf2..a35658223b 100644 --- a/crypto/fipsmodule/bn/exponentiation.c +++ b/crypto/fipsmodule/bn/exponentiation.c @@ -1058,7 +1058,7 @@ int BN_mod_exp_mont_consttime(BIGNUM *rr, const BIGNUM *a, const BIGNUM *p, // Prepare a^1 in the Montgomery domain. assert(!a->neg); - assert(BN_ucmp(a, m) < 0); + declassify_assert(BN_ucmp(a, m) < 0); if (!BN_to_montgomery(&am, a, mont, ctx) || !bn_resize_words(&am, top)) { goto err; diff --git a/crypto/fipsmodule/bn/gcd_extra.c b/crypto/fipsmodule/bn/gcd_extra.c index 76f337cbca..531ff59f39 100644 --- a/crypto/fipsmodule/bn/gcd_extra.c +++ b/crypto/fipsmodule/bn/gcd_extra.c @@ -93,7 +93,7 @@ static int bn_gcd_consttime(BIGNUM *r, unsigned *out_shift, const BIGNUM *x, // At least one of |u| and |v| is now even. BN_ULONG u_is_odd = word_is_odd_mask(u->d[0]); BN_ULONG v_is_odd = word_is_odd_mask(v->d[0]); - assert(!(u_is_odd & v_is_odd)); + declassify_assert(!(u_is_odd & v_is_odd)); // If both are even, the final GCD gains a factor of two. shift += 1 & (~u_is_odd & ~v_is_odd); @@ -106,7 +106,7 @@ static int bn_gcd_consttime(BIGNUM *r, unsigned *out_shift, const BIGNUM *x, // One of |u| or |v| is zero at this point. The algorithm usually makes |u| // zero, unless |y| was already zero on input. Fix this by combining the // values. - assert(BN_is_zero(u) || BN_is_zero(v)); + declassify_assert(BN_is_zero(u) | BN_is_zero(v)); for (size_t i = 0; i < width; i++) { v->d[i] |= u->d[i]; } @@ -289,7 +289,7 @@ int bn_mod_inverse_consttime(BIGNUM *r, int *out_no_inverse, const BIGNUM *a, // and |v| is now even. BN_ULONG u_is_even = ~word_is_odd_mask(u->d[0]); BN_ULONG v_is_even = ~word_is_odd_mask(v->d[0]); - assert(u_is_even != v_is_even); + declassify_assert(u_is_even != v_is_even); // Halve the even one and adjust the corresponding coefficient. maybe_rshift1_words(u->d, u_is_even, tmp->d, n_width); @@ -313,7 +313,7 @@ int bn_mod_inverse_consttime(BIGNUM *r, int *out_no_inverse, const BIGNUM *a, maybe_rshift1_words_carry(D->d, D_carry, v_is_even, tmp->d, a_width); } - assert(BN_is_zero(v)); + declassify_assert(BN_is_zero(v)); // While the inputs and output are secret, this function considers whether the // input was invertible to be public. It is used as part of RSA key // generation, where inputs are chosen to already be invertible. diff --git a/crypto/fipsmodule/bn/montgomery_inv.c b/crypto/fipsmodule/bn/montgomery_inv.c index bd80310c6f..98089f84c8 100644 --- a/crypto/fipsmodule/bn/montgomery_inv.c +++ b/crypto/fipsmodule/bn/montgomery_inv.c @@ -154,7 +154,7 @@ static uint64_t bn_neg_inv_mod_r_u64(uint64_t n) { // The invariant now shows that u*r - v*n == 1 since r == 2 * alpha. #if BN_BITS2 == 64 && defined(BN_ULLONG) - assert(1 == ((BN_ULLONG)u * 2 * alpha) - ((BN_ULLONG)v * beta)); + declassify_assert(1 == ((BN_ULLONG)u * 2 * alpha) - ((BN_ULLONG)v * beta)); #endif return v; diff --git a/crypto/fipsmodule/bn/mul.c b/crypto/fipsmodule/bn/mul.c index df529c66ce..09d92380c4 100644 --- a/crypto/fipsmodule/bn/mul.c +++ b/crypto/fipsmodule/bn/mul.c @@ -293,7 +293,7 @@ static void bn_mul_recursive(BN_ULONG *r, const BN_ULONG *a, const BN_ULONG *b, } // The product should fit without carries. - assert(c == 0); + declassify_assert(c == 0); } // bn_mul_part_recursive sets |r| to |a| * |b|, using |t| as scratch space. |r| @@ -407,7 +407,7 @@ static void bn_mul_part_recursive(BN_ULONG *r, const BN_ULONG *a, } // The product should fit without carries. - assert(c == 0); + declassify_assert(c == 0); } // bn_mul_impl implements |BN_mul| and |bn_mul_consttime|. Note this function diff --git a/crypto/fipsmodule/bn/prime.c b/crypto/fipsmodule/bn/prime.c index b31456470b..3704414973 100644 --- a/crypto/fipsmodule/bn/prime.c +++ b/crypto/fipsmodule/bn/prime.c @@ -788,7 +788,7 @@ int BN_primality_test(int *out_is_probably_prime, const BIGNUM *w, int checks, } } - assert(uniform_iterations >= (crypto_word_t)checks); + declassify_assert(uniform_iterations >= (crypto_word_t)checks); *out_is_probably_prime = 1; ret = 1; diff --git a/crypto/fipsmodule/bn/random.c b/crypto/fipsmodule/bn/random.c index c9738d57f7..04e737900d 100644 --- a/crypto/fipsmodule/bn/random.c +++ b/crypto/fipsmodule/bn/random.c @@ -351,7 +351,8 @@ int bn_rand_secret_range(BIGNUM *r, int *out_is_uniform, BN_ULONG min_inclusive, // If the value is not in range, force it to be in range. r->d[0] |= constant_time_select_w(in_range, 0, min_inclusive); r->d[words - 1] &= constant_time_select_w(in_range, BN_MASK2, mask >> 1); - assert(bn_in_range_words(r->d, min_inclusive, max_exclusive->d, words)); + declassify_assert( + bn_in_range_words(r->d, min_inclusive, max_exclusive->d, words)); r->neg = 0; r->width = (int)words; diff --git a/crypto/fipsmodule/curve25519/curve25519_nohw.c b/crypto/fipsmodule/curve25519/curve25519_nohw.c index d51c94c261..d163ada6c0 100644 --- a/crypto/fipsmodule/curve25519/curve25519_nohw.c +++ b/crypto/fipsmodule/curve25519/curve25519_nohw.c @@ -77,7 +77,7 @@ typedef uint64_t fe_limb_t; #define assert_fe(f) \ do { \ for (unsigned _assert_fe_i = 0; _assert_fe_i < 5; _assert_fe_i++) { \ - assert(f[_assert_fe_i] <= UINT64_C(0x8cccccccccccc)); \ + declassify_assert(f[_assert_fe_i] <= UINT64_C(0x8cccccccccccc)); \ } \ } while (0) @@ -94,7 +94,7 @@ typedef uint64_t fe_limb_t; #define assert_fe_loose(f) \ do { \ for (unsigned _assert_fe_i = 0; _assert_fe_i < 5; _assert_fe_i++) { \ - assert(f[_assert_fe_i] <= UINT64_C(0x1a666666666664)); \ + declassify_assert(f[_assert_fe_i] <= UINT64_C(0x1a666666666664)); \ } \ } while (0) @@ -116,8 +116,8 @@ typedef uint32_t fe_limb_t; #define assert_fe(f) \ do { \ for (unsigned _assert_fe_i = 0; _assert_fe_i < 10; _assert_fe_i++) { \ - assert(f[_assert_fe_i] <= \ - ((_assert_fe_i & 1) ? 0x2333333u : 0x4666666u)); \ + declassify_assert(f[_assert_fe_i] <= \ + ((_assert_fe_i & 1) ? 0x2333333u : 0x4666666u)); \ } \ } while (0) @@ -134,8 +134,8 @@ typedef uint32_t fe_limb_t; #define assert_fe_loose(f) \ do { \ for (unsigned _assert_fe_i = 0; _assert_fe_i < 10; _assert_fe_i++) { \ - assert(f[_assert_fe_i] <= \ - ((_assert_fe_i & 1) ? 0x6999999u : 0xd333332u)); \ + declassify_assert(f[_assert_fe_i] <= \ + ((_assert_fe_i & 1) ? 0x6999999u : 0xd333332u)); \ } \ } while (0) @@ -146,7 +146,7 @@ OPENSSL_STATIC_ASSERT(sizeof(fe) == sizeof(fe_limb_t) * FE_NUM_LIMBS, static void fe_frombytes_strict(fe *h, const uint8_t s[32]) { // |fiat_25519_from_bytes| requires the top-most bit be clear. - assert((s[31] & 0x80) == 0); + declassify_assert((s[31] & 0x80) == 0); fiat_25519_from_bytes(h->v, s); assert_fe(h->v); } diff --git a/crypto/fipsmodule/rsa/rsa_impl.c b/crypto/fipsmodule/rsa/rsa_impl.c index 4839bad482..d5d6660fed 100644 --- a/crypto/fipsmodule/rsa/rsa_impl.c +++ b/crypto/fipsmodule/rsa/rsa_impl.c @@ -738,7 +738,7 @@ static int mod_exp(BIGNUM *r0, const BIGNUM *I, RSA *rsa, BN_CTX *ctx) { // This is a pre-condition for |mod_montgomery|. It was already checked by the // caller. - assert(BN_ucmp(I, n) < 0); + declassify_assert(BN_ucmp(I, n) < 0); if (!mod_montgomery(r1, I, q, rsa->mont_q, p, ctx) || !mod_montgomery(r2, I, p, rsa->mont_p, q, ctx) || @@ -773,7 +773,7 @@ static int mod_exp(BIGNUM *r0, const BIGNUM *I, RSA *rsa, BN_CTX *ctx) { // bound the width slightly higher, so fix it. This trips constant-time checks // because a naive data flow analysis does not realize the excess words are // publicly zero. - assert(BN_cmp(r0, n) < 0); + declassify_assert(BN_cmp(r0, n) < 0); bn_assert_fits_in_bytes(r0, BN_num_bytes(n)); if (!bn_resize_words(r0, n->width)) { goto err; diff --git a/crypto/internal.h b/crypto/internal.h index 891da2931d..6658284788 100644 --- a/crypto/internal.h +++ b/crypto/internal.h @@ -552,6 +552,12 @@ static inline int constant_time_declassify_int(int v) { return value_barrier_u32(v); } +// declassify_assert behaves like |assert| but declassifies the result of +// evaluating |expr|. This allows the assertion to branch on the (presumably +// public) result, but still ensures that values leading up to the computation +// were secret. +#define declassify_assert(expr) assert(constant_time_declassify_int(expr)) + // Thread-safe initialisation. @@ -1165,13 +1171,13 @@ static inline uint64_t CRYPTO_rotr_u64(uint64_t value, int shift) { static inline uint32_t CRYPTO_addc_u32(uint32_t x, uint32_t y, uint32_t carry, uint32_t *out_carry) { - assert(carry <= 1); + declassify_assert(carry <= 1); return CRYPTO_GENERIC_ADDC(x, y, carry, out_carry); } static inline uint64_t CRYPTO_addc_u64(uint64_t x, uint64_t y, uint64_t carry, uint64_t *out_carry) { - assert(carry <= 1); + declassify_assert(carry <= 1); return CRYPTO_GENERIC_ADDC(x, y, carry, out_carry); } @@ -1179,7 +1185,7 @@ static inline uint64_t CRYPTO_addc_u64(uint64_t x, uint64_t y, uint64_t carry, static inline uint32_t CRYPTO_addc_u32(uint32_t x, uint32_t y, uint32_t carry, uint32_t *out_carry) { - assert(carry <= 1); + declassify_assert(carry <= 1); uint64_t ret = carry; ret += (uint64_t)x + y; *out_carry = (uint32_t)(ret >> 32); @@ -1188,7 +1194,7 @@ static inline uint32_t CRYPTO_addc_u32(uint32_t x, uint32_t y, uint32_t carry, static inline uint64_t CRYPTO_addc_u64(uint64_t x, uint64_t y, uint64_t carry, uint64_t *out_carry) { - assert(carry <= 1); + declassify_assert(carry <= 1); #if defined(BORINGSSL_HAS_UINT128) uint128_t ret = carry; ret += (uint128_t)x + y; @@ -1217,13 +1223,13 @@ static inline uint64_t CRYPTO_addc_u64(uint64_t x, uint64_t y, uint64_t carry, static inline uint32_t CRYPTO_subc_u32(uint32_t x, uint32_t y, uint32_t borrow, uint32_t *out_borrow) { - assert(borrow <= 1); + declassify_assert(borrow <= 1); return CRYPTO_GENERIC_SUBC(x, y, borrow, out_borrow); } static inline uint64_t CRYPTO_subc_u64(uint64_t x, uint64_t y, uint64_t borrow, uint64_t *out_borrow) { - assert(borrow <= 1); + declassify_assert(borrow <= 1); return CRYPTO_GENERIC_SUBC(x, y, borrow, out_borrow); } @@ -1231,7 +1237,7 @@ static inline uint64_t CRYPTO_subc_u64(uint64_t x, uint64_t y, uint64_t borrow, static inline uint32_t CRYPTO_subc_u32(uint32_t x, uint32_t y, uint32_t borrow, uint32_t *out_borrow) { - assert(borrow <= 1); + declassify_assert(borrow <= 1); uint32_t ret = x - y - borrow; *out_borrow = (x < y) | ((x == y) & borrow); return ret; @@ -1239,7 +1245,7 @@ static inline uint32_t CRYPTO_subc_u32(uint32_t x, uint32_t y, uint32_t borrow, static inline uint64_t CRYPTO_subc_u64(uint64_t x, uint64_t y, uint64_t borrow, uint64_t *out_borrow) { - assert(borrow <= 1); + declassify_assert(borrow <= 1); uint64_t ret = x - y - borrow; *out_borrow = (x < y) | ((x == y) & borrow); return ret; From ca17ac7517ec647d71112994e6d884425cfe2b36 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 17 Mar 2024 17:17:59 +1000 Subject: [PATCH 2/4] Fix EVP_PKEY_CTX_dup with EC generation gen_group wasn't copied over. Change-Id: If5341dce69fe0297b6bd9a5fb7ed34d546201604 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67167 Reviewed-by: Bob Beck Commit-Queue: David Benjamin (cherry picked from commit 8248baaf3e14895cc85255c009aace5fb92d0c95) --- crypto/evp_extra/evp_extra_test.cc | 84 ++++++++++++++++++------------ crypto/fipsmodule/evp/p_ec.c | 7 ++- 2 files changed, 55 insertions(+), 36 deletions(-) diff --git a/crypto/evp_extra/evp_extra_test.cc b/crypto/evp_extra/evp_extra_test.cc index b8c42c60f1..6bd3460dc2 100644 --- a/crypto/evp_extra/evp_extra_test.cc +++ b/crypto/evp_extra/evp_extra_test.cc @@ -1687,40 +1687,60 @@ static void ExpectECGroupAndKey(const EVP_PKEY *pkey, int nid) { } TEST(EVPExtraTest, ECKeygen) { - // |EVP_PKEY_paramgen| may be used as an extremely roundabout way to get an - // |EC_GROUP|. - bssl::UniquePtr ctx(EVP_PKEY_CTX_new_id(EVP_PKEY_EC, nullptr)); - ASSERT_TRUE(ctx); - ASSERT_TRUE(EVP_PKEY_paramgen_init(ctx.get())); - ASSERT_TRUE( - EVP_PKEY_CTX_set_ec_paramgen_curve_nid(ctx.get(), NID_X9_62_prime256v1)); - EVP_PKEY *raw = nullptr; - ASSERT_TRUE(EVP_PKEY_paramgen(ctx.get(), &raw)); - bssl::UniquePtr pkey(raw); - raw = nullptr; - ExpectECGroupOnly(pkey.get(), NID_X9_62_prime256v1); + for (bool copy : {false, true}) { + SCOPED_TRACE(copy); - // That resulting |EVP_PKEY| may be used as a template for key generation. - ctx.reset(EVP_PKEY_CTX_new(pkey.get(), nullptr)); - ASSERT_TRUE(ctx); - ASSERT_TRUE(EVP_PKEY_keygen_init(ctx.get())); - raw = nullptr; - ASSERT_TRUE(EVP_PKEY_keygen(ctx.get(), &raw)); - pkey.reset(raw); - raw = nullptr; - ExpectECGroupAndKey(pkey.get(), NID_X9_62_prime256v1); + auto maybe_copy = [&](bssl::UniquePtr *ctx) -> bool { + if (copy) { + ctx->reset(EVP_PKEY_CTX_dup(ctx->get())); + } + return *ctx != nullptr; + }; - // |EVP_PKEY_paramgen| may also be skipped. - ctx.reset(EVP_PKEY_CTX_new_id(EVP_PKEY_EC, nullptr)); - ASSERT_TRUE(ctx); - ASSERT_TRUE(EVP_PKEY_keygen_init(ctx.get())); - ASSERT_TRUE( - EVP_PKEY_CTX_set_ec_paramgen_curve_nid(ctx.get(), NID_X9_62_prime256v1)); - raw = nullptr; - ASSERT_TRUE(EVP_PKEY_keygen(ctx.get(), &raw)); - pkey.reset(raw); - raw = nullptr; - ExpectECGroupAndKey(pkey.get(), NID_X9_62_prime256v1); + // |EVP_PKEY_paramgen| may be used as an extremely roundabout way to get an + // |EC_GROUP|. + bssl::UniquePtr ctx( + EVP_PKEY_CTX_new_id(EVP_PKEY_EC, nullptr)); + ASSERT_TRUE(ctx); + ASSERT_TRUE(maybe_copy(&ctx)); + ASSERT_TRUE(EVP_PKEY_paramgen_init(ctx.get())); + ASSERT_TRUE(maybe_copy(&ctx)); + ASSERT_TRUE(EVP_PKEY_CTX_set_ec_paramgen_curve_nid(ctx.get(), + NID_X9_62_prime256v1)); + ASSERT_TRUE(maybe_copy(&ctx)); + EVP_PKEY *raw = nullptr; + ASSERT_TRUE(EVP_PKEY_paramgen(ctx.get(), &raw)); + bssl::UniquePtr pkey(raw); + raw = nullptr; + ExpectECGroupOnly(pkey.get(), NID_X9_62_prime256v1); + + // That resulting |EVP_PKEY| may be used as a template for key generation. + ctx.reset(EVP_PKEY_CTX_new(pkey.get(), nullptr)); + ASSERT_TRUE(ctx); + ASSERT_TRUE(maybe_copy(&ctx)); + ASSERT_TRUE(EVP_PKEY_keygen_init(ctx.get())); + ASSERT_TRUE(maybe_copy(&ctx)); + raw = nullptr; + ASSERT_TRUE(EVP_PKEY_keygen(ctx.get(), &raw)); + pkey.reset(raw); + raw = nullptr; + ExpectECGroupAndKey(pkey.get(), NID_X9_62_prime256v1); + + // |EVP_PKEY_paramgen| may also be skipped. + ctx.reset(EVP_PKEY_CTX_new_id(EVP_PKEY_EC, nullptr)); + ASSERT_TRUE(ctx); + ASSERT_TRUE(maybe_copy(&ctx)); + ASSERT_TRUE(EVP_PKEY_keygen_init(ctx.get())); + ASSERT_TRUE(maybe_copy(&ctx)); + ASSERT_TRUE(EVP_PKEY_CTX_set_ec_paramgen_curve_nid(ctx.get(), + NID_X9_62_prime256v1)); + ASSERT_TRUE(maybe_copy(&ctx)); + raw = nullptr; + ASSERT_TRUE(EVP_PKEY_keygen(ctx.get(), &raw)); + pkey.reset(raw); + raw = nullptr; + ExpectECGroupAndKey(pkey.get(), NID_X9_62_prime256v1); + } } TEST(EVPExtraTest, DHKeygen) { diff --git a/crypto/fipsmodule/evp/p_ec.c b/crypto/fipsmodule/evp/p_ec.c index 0138a3d5ac..d48bbaf3c4 100644 --- a/crypto/fipsmodule/evp/p_ec.c +++ b/crypto/fipsmodule/evp/p_ec.c @@ -92,15 +92,14 @@ static int pkey_ec_init(EVP_PKEY_CTX *ctx) { } static int pkey_ec_copy(EVP_PKEY_CTX *dst, EVP_PKEY_CTX *src) { - EC_PKEY_CTX *dctx, *sctx; if (!pkey_ec_init(dst)) { return 0; } - sctx = src->data; - dctx = dst->data; + const EC_PKEY_CTX *sctx = src->data; + EC_PKEY_CTX *dctx = dst->data; dctx->md = sctx->md; - + dctx->gen_group = sctx->gen_group; return 1; } From e9f12ec14e14df6e76d5c1b89a2bac68154735ec Mon Sep 17 00:00:00 2001 From: Theo Buehler Date: Thu, 21 Mar 2024 22:16:06 +1000 Subject: [PATCH 3/4] Remove unused flags argument from trust handlers AWS-LC: - X509_TRUST is in include/openssl/x509.h; the check_trust function pointer was changed there. Change-Id: Ie16e9ab0897305089672720efa4530d43074f692 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67387 Auto-Submit: Theo Buehler Reviewed-by: Bob Beck Reviewed-by: David Benjamin Commit-Queue: Bob Beck (cherry picked from commit 4ac76f07a401b9b11d6ff305049721cfe3f6a777) --- crypto/x509/x509_trs.c | 24 ++++++++++++------------ include/openssl/x509.h | 2 +- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/crypto/x509/x509_trs.c b/crypto/x509/x509_trs.c index deb8930119..530daf2145 100644 --- a/crypto/x509/x509_trs.c +++ b/crypto/x509/x509_trs.c @@ -66,10 +66,10 @@ #include "internal.h" -static int trust_1oidany(const X509_TRUST *trust, X509 *x, int flags); -static int trust_compat(const X509_TRUST *trust, X509 *x, int flags); +static int trust_1oidany(const X509_TRUST *trust, X509 *x); +static int trust_compat(const X509_TRUST *trust, X509 *x); -static int obj_trust(int id, X509 *x, int flags); +static int obj_trust(int id, X509 *x); static const X509_TRUST trstandard[] = { {X509_TRUST_COMPAT, 0, trust_compat, (char *)"compatible", 0, NULL}, @@ -90,18 +90,18 @@ int X509_check_trust(X509 *x, int id, int flags) { } // We get this as a default value if (id == 0) { - int rv = obj_trust(NID_anyExtendedKeyUsage, x, 0); + int rv = obj_trust(NID_anyExtendedKeyUsage, x); if (rv != X509_TRUST_UNTRUSTED) { return rv; } - return trust_compat(NULL, x, 0); + return trust_compat(NULL, x); } int idx = X509_TRUST_get_by_id(id); if (idx == -1) { - return obj_trust(id, x, flags); + return obj_trust(id, x); } const X509_TRUST *pt = X509_TRUST_get0(idx); - return pt->check_trust(pt, x, flags); + return pt->check_trust(pt, x); } int X509_TRUST_get_count(void) { return OPENSSL_ARRAY_SIZE(trstandard); } @@ -139,16 +139,16 @@ char *X509_TRUST_get0_name(const X509_TRUST *xp) { return xp->name; } int X509_TRUST_get_trust(const X509_TRUST *xp) { return xp->trust; } -static int trust_1oidany(const X509_TRUST *trust, X509 *x, int flags) { +static int trust_1oidany(const X509_TRUST *trust, X509 *x) { if (x->aux && (x->aux->trust || x->aux->reject)) { - return obj_trust(trust->arg1, x, flags); + return obj_trust(trust->arg1, x); } // we don't have any trust settings: for compatibility we return trusted // if it is self signed - return trust_compat(trust, x, flags); + return trust_compat(trust, x); } -static int trust_compat(const X509_TRUST *trust, X509 *x, int flags) { +static int trust_compat(const X509_TRUST *trust, X509 *x) { if (!x509v3_cache_extensions(x)) { return X509_TRUST_UNTRUSTED; } @@ -159,7 +159,7 @@ static int trust_compat(const X509_TRUST *trust, X509 *x, int flags) { } } -static int obj_trust(int id, X509 *x, int flags) { +static int obj_trust(int id, X509 *x) { ASN1_OBJECT *obj; size_t i; X509_CERT_AUX *ax; diff --git a/include/openssl/x509.h b/include/openssl/x509.h index d757d8493c..688be12e8e 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -5070,7 +5070,7 @@ DECLARE_STACK_OF(DIST_POINT) struct x509_trust_st { int trust; int flags; -int (*check_trust)(const X509_TRUST *, X509 *, int); +int (*check_trust)(const X509_TRUST *, X509 *); char *name; int arg1; void *arg2; From 89a486957429741cf14d5c23707877e37a13da84 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 22 Mar 2024 13:16:03 +1000 Subject: [PATCH 4/4] Document that null STACK_OF(T) can be used with several functions Lots of code relies on this, so we ought to document it. A null STACK_OF(T) is treated as an immutable empty list. AWS-LC: - sk_TEST_INT_find takes 2 arguments only due to https://github.com/aws/aws-lc/commit/98604464dc5b8589bd1a19acdfb0a34254ddc15f - Based on changes to stack.h L-513-515 in the same commit, in compatibility with OpenSSL, -1 is returned in the last check in NullIsEmpty test. Change-Id: I10d0ba8f7b33c7f3febaf92c2cd3da25a0eb0f80 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67407 Reviewed-by: Theo Buehler Auto-Submit: David Benjamin Commit-Queue: Bob Beck Reviewed-by: Bob Beck (cherry picked from commit 821fe3380cce646fa3557b882d91fba318981b9b) --- crypto/stack/stack_test.cc | 14 ++++++++++++++ include/openssl/stack.h | 8 ++++++-- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/crypto/stack/stack_test.cc b/crypto/stack/stack_test.cc index 6f15a74ca7..05b7a99cf0 100644 --- a/crypto/stack/stack_test.cc +++ b/crypto/stack/stack_test.cc @@ -508,3 +508,17 @@ TEST(StackTest, IsSorted) { sk_TEST_INT_set_cmp_func(sk.get(), nullptr); EXPECT_FALSE(sk_TEST_INT_is_sorted(sk.get())); } + +TEST(StackTest, NullIsEmpty) { + EXPECT_EQ(0u, sk_TEST_INT_num(nullptr)); + + EXPECT_EQ(nullptr, sk_TEST_INT_value(nullptr, 0)); + EXPECT_EQ(nullptr, sk_TEST_INT_value(nullptr, 1)); + + bssl::UniquePtr value = TEST_INT_new(6); + ASSERT_TRUE(value); + // The return value of -1 is compatible with OpenSSL + // BoringSSL's function takes 3 arguments in this case + // and returns False. + EXPECT_EQ(-1, sk_TEST_INT_find(nullptr, value.get())); +} diff --git a/include/openssl/stack.h b/include/openssl/stack.h index bb82e7eb86..7ff03fe48b 100644 --- a/include/openssl/stack.h +++ b/include/openssl/stack.h @@ -143,7 +143,8 @@ STACK_OF(SAMPLE) *sk_SAMPLE_new(sk_SAMPLE_cmp_func comp); STACK_OF(SAMPLE) *sk_SAMPLE_new_null(void); // sk_SAMPLE_num returns the number of elements in |sk|. It is safe to cast this -// value to |int|. |sk| is guaranteed to have at most |INT_MAX| elements. +// value to |int|. |sk| is guaranteed to have at most |INT_MAX| elements. If +// |sk| is NULL, it is treated as the empty list and this function returns zero. size_t sk_SAMPLE_num(const STACK_OF(SAMPLE) *sk); // sk_SAMPLE_zero resets |sk| to the empty state but does nothing to free the @@ -151,7 +152,8 @@ size_t sk_SAMPLE_num(const STACK_OF(SAMPLE) *sk); void sk_SAMPLE_zero(STACK_OF(SAMPLE) *sk); // sk_SAMPLE_value returns the |i|th pointer in |sk|, or NULL if |i| is out of -// range. +// range. If |sk| is NULL, it is treated as an empty list and the function +// returns NULL. SAMPLE *sk_SAMPLE_value(const STACK_OF(SAMPLE) *sk, size_t i); // sk_SAMPLE_set sets the |i|th pointer in |sk| to |p| and returns |p|. If |i| @@ -199,6 +201,8 @@ void sk_SAMPLE_delete_if(STACK_OF(SAMPLE) *sk, sk_SAMPLE_delete_if_func func, // If the stack is sorted (see |sk_SAMPLE_sort|), this function uses a binary // search. Otherwise it performs a linear search. If it finds a matching // element, it returns the index of that element. Otherwise, it returns -1. +// If |sk| is NULL, it is treated as the empty list and the function returns +// zero. // // Note this differs from OpenSSL in that OpenSSL's version will implicitly // sort |sk| if it has a comparison function defined.