From 1b2b7b2e70ce5ff50df917ee7745403d824155c5 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 22 Jun 2022 09:25:59 -0400 Subject: [PATCH] Various -Wshorten-64-to-32 fixes. This is far from all of it, but finishes a good chunk of bcm.c. Bug: 516 Change-Id: If764e5af1c6b62e8342554502ecc4d563e44bc50 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54207 Reviewed-by: Bob Beck Auto-Submit: David Benjamin Commit-Queue: David Benjamin --- crypto/fipsmodule/aes/aes_nohw.c | 2 +- crypto/fipsmodule/bn/bn.c | 16 +++++++++---- crypto/fipsmodule/bn/bytes.c | 2 +- crypto/fipsmodule/bn/div.c | 2 +- crypto/fipsmodule/bn/div_extra.c | 2 +- crypto/fipsmodule/bn/gcd_extra.c | 8 ++++--- crypto/fipsmodule/bn/internal.h | 2 +- crypto/fipsmodule/bn/random.c | 2 +- crypto/fipsmodule/bn/shift.c | 6 ++--- crypto/fipsmodule/cipher/e_aes.c | 19 +++++++++++---- crypto/fipsmodule/cmac/cmac.c | 6 ++++- crypto/fipsmodule/dh/dh.c | 3 ++- crypto/fipsmodule/rand/ctrdrbg.c | 2 +- crypto/fipsmodule/rsa/rsa.c | 41 ++++++++++++++++---------------- crypto/fipsmodule/tls/kdf.c | 7 +++--- crypto/test/file_test.cc | 4 +++- crypto/test/wycheproof_util.cc | 9 ++++--- include/openssl/bn.h | 4 ++++ 18 files changed, 85 insertions(+), 52 deletions(-) diff --git a/crypto/fipsmodule/aes/aes_nohw.c b/crypto/fipsmodule/aes/aes_nohw.c index b5990b84d2..c5dec0e5c1 100644 --- a/crypto/fipsmodule/aes/aes_nohw.c +++ b/crypto/fipsmodule/aes/aes_nohw.c @@ -1192,7 +1192,7 @@ void aes_nohw_ctr32_encrypt_blocks(const uint8_t *in, uint8_t *out, for (;;) { // Update counters. for (size_t i = 0; i < AES_NOHW_BATCH_SIZE; i++) { - CRYPTO_store_u32_be(ivs + 16 * i + 12, ctr + i); + CRYPTO_store_u32_be(ivs + 16 * i + 12, ctr + (uint32_t)i); } size_t todo = blocks >= AES_NOHW_BATCH_SIZE ? AES_NOHW_BATCH_SIZE : blocks; diff --git a/crypto/fipsmodule/bn/bn.c b/crypto/fipsmodule/bn/bn.c index f2e3b7b131..f3fbb7a680 100644 --- a/crypto/fipsmodule/bn/bn.c +++ b/crypto/fipsmodule/bn/bn.c @@ -67,6 +67,11 @@ #include "../delocate.h" +// BN_MAX_WORDS is the maximum number of words allowed in a |BIGNUM|. It is +// sized so byte and bit counts of a |BIGNUM| always fit in |int|, with room to +// spare. +#define BN_MAX_WORDS (INT_MAX / (4 * BN_BITS2)) + BIGNUM *BN_new(void) { BIGNUM *bn = OPENSSL_malloc(sizeof(BIGNUM)); @@ -292,8 +297,9 @@ void bn_set_static_words(BIGNUM *bn, const BN_ULONG *words, size_t num) { } bn->d = (BN_ULONG *)words; - bn->width = num; - bn->dmax = num; + assert(num <= BN_MAX_WORDS); + bn->width = (int)num; + bn->dmax = (int)num; bn->neg = 0; bn->flags |= BN_FLG_STATIC_DATA; } @@ -346,7 +352,7 @@ int bn_wexpand(BIGNUM *bn, size_t words) { return 1; } - if (words > (INT_MAX / (4 * BN_BITS2))) { + if (words > BN_MAX_WORDS) { OPENSSL_PUT_ERROR(BN, BN_R_BIGNUM_TOO_LONG); return 0; } @@ -403,7 +409,7 @@ int bn_resize_words(BIGNUM *bn, size_t words) { } OPENSSL_memset(bn->d + bn->width, 0, (words - bn->width) * sizeof(BN_ULONG)); - bn->width = words; + bn->width = (int)words; return 1; } @@ -412,7 +418,7 @@ int bn_resize_words(BIGNUM *bn, size_t words) { OPENSSL_PUT_ERROR(BN, BN_R_BIGNUM_TOO_LONG); return 0; } - bn->width = words; + bn->width = (int)words; return 1; } diff --git a/crypto/fipsmodule/bn/bytes.c b/crypto/fipsmodule/bn/bytes.c index 38d71a3c45..da27426c86 100644 --- a/crypto/fipsmodule/bn/bytes.c +++ b/crypto/fipsmodule/bn/bytes.c @@ -138,7 +138,7 @@ BIGNUM *BN_le2bn(const uint8_t *in, size_t len, BIGNUM *ret) { BN_free(bn); return NULL; } - ret->width = num_words; + ret->width = (int)num_words; // Make sure the top bytes will be zeroed. ret->d[num_words - 1] = 0; diff --git a/crypto/fipsmodule/bn/div.c b/crypto/fipsmodule/bn/div.c index 02b9931cd2..6de5a4303a 100644 --- a/crypto/fipsmodule/bn/div.c +++ b/crypto/fipsmodule/bn/div.c @@ -552,7 +552,7 @@ static BIGNUM *bn_scratch_space_from_ctx(size_t width, BN_CTX *ctx) { return NULL; } ret->neg = 0; - ret->width = width; + ret->width = (int)width; return ret; } diff --git a/crypto/fipsmodule/bn/div_extra.c b/crypto/fipsmodule/bn/div_extra.c index 7f03f28d09..141f7da67a 100644 --- a/crypto/fipsmodule/bn/div_extra.c +++ b/crypto/fipsmodule/bn/div_extra.c @@ -70,7 +70,7 @@ uint16_t bn_mod_u16_consttime(const BIGNUM *bn, uint16_t d) { // This operation is not constant-time, but |p| and |d| are public values. // Note that |p| is at most 16, so the computation fits in |uint64_t|. assert(p <= 16); - uint32_t m = ((UINT64_C(1) << (32 + p)) + d - 1) / d; + uint32_t m = (uint32_t)(((UINT64_C(1) << (32 + p)) + d - 1) / d); uint16_t ret = 0; for (int i = bn->width - 1; i >= 0; i--) { diff --git a/crypto/fipsmodule/bn/gcd_extra.c b/crypto/fipsmodule/bn/gcd_extra.c index 53ab1705cb..b4fe00e3e4 100644 --- a/crypto/fipsmodule/bn/gcd_extra.c +++ b/crypto/fipsmodule/bn/gcd_extra.c @@ -240,8 +240,10 @@ int bn_mod_inverse_consttime(BIGNUM *r, int *out_no_inverse, const BIGNUM *a, // Each loop iteration halves at least one of |u| and |v|. Thus we need at // most the combined bit width of inputs for at least one value to be zero. - unsigned a_bits = a_width * BN_BITS2, n_bits = n_width * BN_BITS2; - unsigned num_iters = a_bits + n_bits; + // |a_bits| and |n_bits| cannot overflow because |bn_wexpand| ensures bit + // counts fit in even |int|. + size_t a_bits = a_width * BN_BITS2, n_bits = n_width * BN_BITS2; + size_t num_iters = a_bits + n_bits; if (num_iters < a_bits) { OPENSSL_PUT_ERROR(BN, BN_R_BIGNUM_TOO_LONG); goto err; @@ -260,7 +262,7 @@ int bn_mod_inverse_consttime(BIGNUM *r, int *out_no_inverse, const BIGNUM *a, // // After each loop iteration, u and v only get smaller, and at least one of // them shrinks by at least a factor of two. - for (unsigned i = 0; i < num_iters; i++) { + for (size_t i = 0; i < num_iters; i++) { BN_ULONG both_odd = word_is_odd_mask(u->d[0]) & word_is_odd_mask(v->d[0]); // If both |u| and |v| are odd, subtract the smaller from the larger. diff --git a/crypto/fipsmodule/bn/internal.h b/crypto/fipsmodule/bn/internal.h index 50fd362d3f..f7c181e012 100644 --- a/crypto/fipsmodule/bn/internal.h +++ b/crypto/fipsmodule/bn/internal.h @@ -439,7 +439,7 @@ int bn_jacobi(const BIGNUM *a, const BIGNUM *b, BN_CTX *ctx); // bn_is_bit_set_words returns one if bit |bit| is set in |a| and zero // otherwise. -int bn_is_bit_set_words(const BN_ULONG *a, size_t num, unsigned bit); +int bn_is_bit_set_words(const BN_ULONG *a, size_t num, size_t bit); // bn_one_to_montgomery sets |r| to one in Montgomery form. It returns one on // success and zero on error. This function treats the bit width of the modulus diff --git a/crypto/fipsmodule/bn/random.c b/crypto/fipsmodule/bn/random.c index 4966778efb..2500cc1ad4 100644 --- a/crypto/fipsmodule/bn/random.c +++ b/crypto/fipsmodule/bn/random.c @@ -336,7 +336,7 @@ int bn_rand_secret_range(BIGNUM *r, int *out_is_uniform, BN_ULONG min_inclusive, assert(bn_in_range_words(r->d, min_inclusive, max_exclusive->d, words)); r->neg = 0; - r->width = words; + r->width = (int)words; return 1; } diff --git a/crypto/fipsmodule/bn/shift.c b/crypto/fipsmodule/bn/shift.c index 55f864ea3c..6960e57438 100644 --- a/crypto/fipsmodule/bn/shift.c +++ b/crypto/fipsmodule/bn/shift.c @@ -258,9 +258,9 @@ int BN_clear_bit(BIGNUM *a, int n) { return 1; } -int bn_is_bit_set_words(const BN_ULONG *a, size_t num, unsigned bit) { - unsigned i = bit / BN_BITS2; - unsigned j = bit % BN_BITS2; +int bn_is_bit_set_words(const BN_ULONG *a, size_t num, size_t bit) { + size_t i = bit / BN_BITS2; + size_t j = bit % BN_BITS2; if (i >= num) { return 0; } diff --git a/crypto/fipsmodule/cipher/e_aes.c b/crypto/fipsmodule/cipher/e_aes.c index 8d5ed4cac1..5f23793420 100644 --- a/crypto/fipsmodule/cipher/e_aes.c +++ b/crypto/fipsmodule/cipher/e_aes.c @@ -47,6 +47,7 @@ * ==================================================================== */ #include +#include #include #include @@ -289,8 +290,10 @@ static int aes_ofb_cipher(EVP_CIPHER_CTX *ctx, uint8_t *out, const uint8_t *in, ctr128_f aes_ctr_set_key(AES_KEY *aes_key, GCM128_KEY *gcm_key, block128_f *out_block, const uint8_t *key, size_t key_bytes) { + // This function assumes the key length was previously validated. + assert(key_bytes == 128 / 8 || key_bytes == 192 / 8 || key_bytes == 256 / 8); if (hwaes_capable()) { - aes_hw_set_encrypt_key(key, key_bytes * 8, aes_key); + aes_hw_set_encrypt_key(key, (int)key_bytes * 8, aes_key); if (gcm_key != NULL) { CRYPTO_gcm128_init_key(gcm_key, aes_key, aes_hw_encrypt, 1); } @@ -301,7 +304,7 @@ ctr128_f aes_ctr_set_key(AES_KEY *aes_key, GCM128_KEY *gcm_key, } if (vpaes_capable()) { - vpaes_set_encrypt_key(key, key_bytes * 8, aes_key); + vpaes_set_encrypt_key(key, (int)key_bytes * 8, aes_key); if (out_block) { *out_block = vpaes_encrypt; } @@ -318,7 +321,7 @@ ctr128_f aes_ctr_set_key(AES_KEY *aes_key, GCM128_KEY *gcm_key, #endif } - aes_nohw_set_encrypt_key(key, key_bytes * 8, aes_key); + aes_nohw_set_encrypt_key(key, (int)key_bytes * 8, aes_key); if (gcm_key != NULL) { CRYPTO_gcm128_init_key(gcm_key, aes_key, aes_nohw_encrypt, 0); } @@ -552,6 +555,14 @@ static int aes_gcm_cipher(EVP_CIPHER_CTX *ctx, uint8_t *out, const uint8_t *in, return -1; } + if (len > INT_MAX) { + // This function signature can only express up to |INT_MAX| bytes encrypted. + // + // TODO(https://crbug.com/boringssl/494): Make the internal |EVP_CIPHER| + // calling convention |size_t|-clean. + return -1; + } + if (in) { if (out == NULL) { if (!CRYPTO_gcm128_aad(&gctx->gcm, in, len)) { @@ -580,7 +591,7 @@ static int aes_gcm_cipher(EVP_CIPHER_CTX *ctx, uint8_t *out, const uint8_t *in, } } } - return len; + return (int)len; } else { if (!ctx->encrypt) { if (gctx->taglen < 0 || diff --git a/crypto/fipsmodule/cmac/cmac.c b/crypto/fipsmodule/cmac/cmac.c index f5c805dce4..efffe5f6be 100644 --- a/crypto/fipsmodule/cmac/cmac.c +++ b/crypto/fipsmodule/cmac/cmac.c @@ -49,6 +49,7 @@ #include #include +#include #include #include @@ -269,7 +270,10 @@ int CMAC_Update(CMAC_CTX *ctx, const uint8_t *in, size_t in_len) { } OPENSSL_memcpy(ctx->block, in, in_len); - ctx->block_used = in_len; + // |in_len| is bounded by |block_size|, which fits in |unsigned|. + static_assert(EVP_MAX_BLOCK_LENGTH < UINT_MAX, + "EVP_MAX_BLOCK_LENGTH is too large"); + ctx->block_used = (unsigned)in_len; ret = 1; out: diff --git a/crypto/fipsmodule/dh/dh.c b/crypto/fipsmodule/dh/dh.c index 31eaff4fce..1a3c9745a0 100644 --- a/crypto/fipsmodule/dh/dh.c +++ b/crypto/fipsmodule/dh/dh.c @@ -368,7 +368,8 @@ int DH_compute_key(unsigned char *out, const BIGNUM *peers_key, DH *dh) { int ret = -1; BIGNUM *shared_key = BN_CTX_get(ctx); if (shared_key && dh_compute_key(dh, shared_key, peers_key, ctx)) { - ret = BN_bn2bin(shared_key, out); + // A |BIGNUM|'s byte count fits in |int|. + ret = (int)BN_bn2bin(shared_key, out); } BN_CTX_end(ctx); diff --git a/crypto/fipsmodule/rand/ctrdrbg.c b/crypto/fipsmodule/rand/ctrdrbg.c index 0e8995f4bd..805372fd28 100644 --- a/crypto/fipsmodule/rand/ctrdrbg.c +++ b/crypto/fipsmodule/rand/ctrdrbg.c @@ -184,7 +184,7 @@ int CTR_DRBG_generate(CTR_DRBG_STATE *drbg, uint8_t *out, size_t out_len, OPENSSL_memset(out, 0, todo); ctr32_add(drbg, 1); drbg->ctr(out, out, num_blocks, &drbg->ks, drbg->counter); - ctr32_add(drbg, num_blocks - 1); + ctr32_add(drbg, (uint32_t)(num_blocks - 1)); } else { for (size_t i = 0; i < todo; i += AES_BLOCK_SIZE) { ctr32_add(drbg, 1); diff --git a/crypto/fipsmodule/rsa/rsa.c b/crypto/fipsmodule/rsa/rsa.c index 733e7faa76..14cdae59b1 100644 --- a/crypto/fipsmodule/rsa/rsa.c +++ b/crypto/fipsmodule/rsa/rsa.c @@ -300,7 +300,7 @@ int RSA_public_encrypt(size_t flen, const uint8_t *from, uint8_t *to, RSA *rsa, OPENSSL_PUT_ERROR(RSA, ERR_R_OVERFLOW); return -1; } - return out_len; + return (int)out_len; } static int rsa_sign_raw_no_self_test(RSA *rsa, size_t *out_len, uint8_t *out, @@ -332,7 +332,7 @@ int RSA_private_encrypt(size_t flen, const uint8_t *from, uint8_t *to, RSA *rsa, OPENSSL_PUT_ERROR(RSA, ERR_R_OVERFLOW); return -1; } - return out_len; + return (int)out_len; } int RSA_decrypt(RSA *rsa, size_t *out_len, uint8_t *out, size_t max_out, @@ -347,7 +347,6 @@ int RSA_decrypt(RSA *rsa, size_t *out_len, uint8_t *out, size_t max_out, int RSA_private_decrypt(size_t flen, const uint8_t *from, uint8_t *to, RSA *rsa, int padding) { size_t out_len; - if (!RSA_decrypt(rsa, &out_len, to, RSA_size(rsa), from, flen, padding)) { return -1; } @@ -356,13 +355,12 @@ int RSA_private_decrypt(size_t flen, const uint8_t *from, uint8_t *to, RSA *rsa, OPENSSL_PUT_ERROR(RSA, ERR_R_OVERFLOW); return -1; } - return out_len; + return (int)out_len; } int RSA_public_decrypt(size_t flen, const uint8_t *from, uint8_t *to, RSA *rsa, int padding) { size_t out_len; - if (!RSA_verify_raw(rsa, &out_len, to, RSA_size(rsa), from, flen, padding)) { return -1; } @@ -371,15 +369,16 @@ int RSA_public_decrypt(size_t flen, const uint8_t *from, uint8_t *to, RSA *rsa, OPENSSL_PUT_ERROR(RSA, ERR_R_OVERFLOW); return -1; } - return out_len; + return (int)out_len; } unsigned RSA_size(const RSA *rsa) { - if (rsa->meth->size) { - return rsa->meth->size(rsa); - } - - return rsa_default_size(rsa); + size_t ret = rsa->meth->size ? rsa->meth->size(rsa) : rsa_default_size(rsa); + // RSA modulus sizes are bounded by |BIGNUM|, which must fit in |unsigned|. + // + // TODO(https://crbug.com/boringssl/516): Should we make this return |size_t|? + assert(ret < UINT_MAX); + return (unsigned)ret; } int RSA_is_opaque(const RSA *rsa) { @@ -474,8 +473,6 @@ static const struct pkcs1_sig_prefix kPKCS1SigPrefixes[] = { int RSA_add_pkcs1_prefix(uint8_t **out_msg, size_t *out_msg_len, int *is_alloced, int hash_nid, const uint8_t *digest, size_t digest_len) { - unsigned i; - if (hash_nid == NID_md5_sha1) { // Special case: SSL signature, just check the length. if (digest_len != SSL_SIG_LENGTH) { @@ -489,7 +486,7 @@ int RSA_add_pkcs1_prefix(uint8_t **out_msg, size_t *out_msg_len, return 1; } - for (i = 0; kPKCS1SigPrefixes[i].nid != NID_undef; i++) { + for (size_t i = 0; kPKCS1SigPrefixes[i].nid != NID_undef; i++) { const struct pkcs1_sig_prefix *sig_prefix = &kPKCS1SigPrefixes[i]; if (sig_prefix->nid != hash_nid) { continue; @@ -501,17 +498,14 @@ int RSA_add_pkcs1_prefix(uint8_t **out_msg, size_t *out_msg_len, } const uint8_t* prefix = sig_prefix->bytes; - unsigned prefix_len = sig_prefix->len; - unsigned signed_msg_len; - uint8_t *signed_msg; - - signed_msg_len = prefix_len + digest_len; + size_t prefix_len = sig_prefix->len; + size_t signed_msg_len = prefix_len + digest_len; if (signed_msg_len < prefix_len) { OPENSSL_PUT_ERROR(RSA, RSA_R_TOO_LONG); return 0; } - signed_msg = OPENSSL_malloc(signed_msg_len); + uint8_t *signed_msg = OPENSSL_malloc(signed_msg_len); if (!signed_msg) { OPENSSL_PUT_ERROR(RSA, ERR_R_MALLOC_FAILURE); return 0; @@ -554,7 +548,12 @@ int rsa_sign_no_self_test(int hash_nid, const uint8_t *digest, goto err; } - *out_len = size_t_out_len; + if (size_t_out_len > UINT_MAX) { + OPENSSL_PUT_ERROR(RSA, ERR_R_OVERFLOW); + goto err; + } + + *out_len = (unsigned)size_t_out_len; ret = 1; err: diff --git a/crypto/fipsmodule/tls/kdf.c b/crypto/fipsmodule/tls/kdf.c index 6f2b68bdc9..046cb526aa 100644 --- a/crypto/fipsmodule/tls/kdf.c +++ b/crypto/fipsmodule/tls/kdf.c @@ -91,7 +91,7 @@ static int tls1_P_hash(uint8_t *out, size_t out_len, } for (;;) { - unsigned len; + unsigned len_u; uint8_t hmac[EVP_MAX_MD_SIZE]; if (!HMAC_CTX_copy_ex(&ctx, &ctx_init) || !HMAC_Update(&ctx, A1, A1_len) || @@ -100,16 +100,17 @@ static int tls1_P_hash(uint8_t *out, size_t out_len, !HMAC_Update(&ctx, (const uint8_t *) label, label_len) || !HMAC_Update(&ctx, seed1, seed1_len) || !HMAC_Update(&ctx, seed2, seed2_len) || - !HMAC_Final(&ctx, hmac, &len)) { + !HMAC_Final(&ctx, hmac, &len_u)) { goto err; } + size_t len = len_u; assert(len == chunk); // XOR the result into |out|. if (len > out_len) { len = out_len; } - for (unsigned i = 0; i < len; i++) { + for (size_t i = 0; i < len; i++) { out[i] ^= hmac[i]; } out += len; diff --git a/crypto/test/file_test.cc b/crypto/test/file_test.cc index 47a6f4c8b3..e1ab2aa7c8 100644 --- a/crypto/test/file_test.cc +++ b/crypto/test/file_test.cc @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -377,7 +378,8 @@ class FileLineReader : public FileTest::LineReader { return FileTest::kReadError; } - if (fgets(out, len, file_) == nullptr) { + len = std::min(len, size_t{INT_MAX}); + if (fgets(out, static_cast(len), file_) == nullptr) { return feof(file_) ? FileTest::kReadEOF : FileTest::kReadError; } diff --git a/crypto/test/wycheproof_util.cc b/crypto/test/wycheproof_util.cc index cd870ddf17..9d31706099 100644 --- a/crypto/test/wycheproof_util.cc +++ b/crypto/test/wycheproof_util.cc @@ -14,6 +14,7 @@ #include "./wycheproof_util.h" +#include #include #include @@ -138,7 +139,8 @@ bssl::UniquePtr GetWycheproofBIGNUM(FileTest *t, const char *key, return nullptr; } BIGNUM *bn = nullptr; - if (BN_hex2bn(&bn, value.c_str()) != static_cast(value.size())) { + if (value.size() > INT_MAX || + BN_hex2bn(&bn, value.c_str()) != static_cast(value.size())) { BN_free(bn); t->PrintLine("Could not decode value '%s'", value.c_str()); return nullptr; @@ -151,8 +153,9 @@ bssl::UniquePtr GetWycheproofBIGNUM(FileTest *t, const char *key, // https://github.com/google/wycheproof/blob/0329f5b751ef102bd6b7b7181b6e049522a887f5/java/com/google/security/wycheproof/JsonUtil.java#L62. if ('0' > value[0] || value[0] > '7') { bssl::UniquePtr tmp(BN_new()); - if (!tmp || - !BN_set_bit(tmp.get(), value.size() * 4) || + if (!tmp || // + value.size() > INT_MAX / 4 || + !BN_set_bit(tmp.get(), static_cast(value.size() * 4)) || !BN_sub(ret.get(), ret.get(), tmp.get())) { return nullptr; } diff --git a/include/openssl/bn.h b/include/openssl/bn.h index 3a721fe736..0f381af85a 100644 --- a/include/openssl/bn.h +++ b/include/openssl/bn.h @@ -205,6 +205,10 @@ OPENSSL_EXPORT unsigned BN_num_bits(const BIGNUM *bn); // BN_num_bytes returns the minimum number of bytes needed to represent the // absolute value of |bn|. +// +// While |size_t| is the preferred type for byte counts, callers can assume that +// |BIGNUM|s are bounded such that this value, and its corresponding bit count, +// will always fit in |int|. OPENSSL_EXPORT unsigned BN_num_bytes(const BIGNUM *bn); // BN_zero sets |bn| to zero.