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 <bbe@google.com> Auto-Submit: David Benjamin <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/fipsmodule/aes/aes_nohw.c b/crypto/fipsmodule/aes/aes_nohw.c index b5990b8..c5dec0e 100644 --- a/crypto/fipsmodule/aes/aes_nohw.c +++ b/crypto/fipsmodule/aes/aes_nohw.c
@@ -1192,7 +1192,7 @@ 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 f2e3b7b..f3fbb7a 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 @@ } 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 @@ 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 @@ } 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 @@ 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 38d71a3..da27426 100644 --- a/crypto/fipsmodule/bn/bytes.c +++ b/crypto/fipsmodule/bn/bytes.c
@@ -138,7 +138,7 @@ 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 02b9931..6de5a43 100644 --- a/crypto/fipsmodule/bn/div.c +++ b/crypto/fipsmodule/bn/div.c
@@ -552,7 +552,7 @@ 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 7f03f28..141f7da 100644 --- a/crypto/fipsmodule/bn/div_extra.c +++ b/crypto/fipsmodule/bn/div_extra.c
@@ -70,7 +70,7 @@ // 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 53ab170..b4fe00e 100644 --- a/crypto/fipsmodule/bn/gcd_extra.c +++ b/crypto/fipsmodule/bn/gcd_extra.c
@@ -240,8 +240,10 @@ // 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 @@ // // 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 50fd362..f7c181e 100644 --- a/crypto/fipsmodule/bn/internal.h +++ b/crypto/fipsmodule/bn/internal.h
@@ -439,7 +439,7 @@ // 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 4966778..2500cc1 100644 --- a/crypto/fipsmodule/bn/random.c +++ b/crypto/fipsmodule/bn/random.c
@@ -336,7 +336,7 @@ 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 55f864e..6960e57 100644 --- a/crypto/fipsmodule/bn/shift.c +++ b/crypto/fipsmodule/bn/shift.c
@@ -258,9 +258,9 @@ 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 8d5ed4c..5f23793 100644 --- a/crypto/fipsmodule/cipher/e_aes.c +++ b/crypto/fipsmodule/cipher/e_aes.c
@@ -47,6 +47,7 @@ * ==================================================================== */ #include <assert.h> +#include <limits.h> #include <string.h> #include <openssl/aead.h> @@ -289,8 +290,10 @@ 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 @@ } 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 @@ #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 @@ 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 @@ } } } - 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 f5c805d..efffe5f 100644 --- a/crypto/fipsmodule/cmac/cmac.c +++ b/crypto/fipsmodule/cmac/cmac.c
@@ -49,6 +49,7 @@ #include <openssl/cmac.h> #include <assert.h> +#include <limits.h> #include <string.h> #include <openssl/aes.h> @@ -269,7 +270,10 @@ } 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 31eaff4..1a3c974 100644 --- a/crypto/fipsmodule/dh/dh.c +++ b/crypto/fipsmodule/dh/dh.c
@@ -368,7 +368,8 @@ 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 0e8995f..805372f 100644 --- a/crypto/fipsmodule/rand/ctrdrbg.c +++ b/crypto/fipsmodule/rand/ctrdrbg.c
@@ -184,7 +184,7 @@ 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 733e7fa..14cdae5 100644 --- a/crypto/fipsmodule/rsa/rsa.c +++ b/crypto/fipsmodule/rsa/rsa.c
@@ -300,7 +300,7 @@ 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 @@ 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_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 @@ 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 @@ 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 @@ 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 @@ 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 @@ } 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 @@ 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 6f2b68b..046cb52 100644 --- a/crypto/fipsmodule/tls/kdf.c +++ b/crypto/fipsmodule/tls/kdf.c
@@ -91,7 +91,7 @@ } 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 @@ !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 47a6f4c..e1ab2aa 100644 --- a/crypto/test/file_test.cc +++ b/crypto/test/file_test.cc
@@ -20,6 +20,7 @@ #include <assert.h> #include <ctype.h> #include <errno.h> +#include <limits.h> #include <stdarg.h> #include <stdio.h> #include <stdlib.h> @@ -377,7 +378,8 @@ return FileTest::kReadError; } - if (fgets(out, len, file_) == nullptr) { + len = std::min(len, size_t{INT_MAX}); + if (fgets(out, static_cast<int>(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 cd870dd..9d31706 100644 --- a/crypto/test/wycheproof_util.cc +++ b/crypto/test/wycheproof_util.cc
@@ -14,6 +14,7 @@ #include "./wycheproof_util.h" +#include <limits.h> #include <stdlib.h> #include <algorithm> @@ -138,7 +139,8 @@ return nullptr; } BIGNUM *bn = nullptr; - if (BN_hex2bn(&bn, value.c_str()) != static_cast<int>(value.size())) { + if (value.size() > INT_MAX || + BN_hex2bn(&bn, value.c_str()) != static_cast<int>(value.size())) { BN_free(bn); t->PrintLine("Could not decode value '%s'", value.c_str()); return nullptr; @@ -151,8 +153,9 @@ // https://github.com/google/wycheproof/blob/0329f5b751ef102bd6b7b7181b6e049522a887f5/java/com/google/security/wycheproof/JsonUtil.java#L62. if ('0' > value[0] || value[0] > '7') { bssl::UniquePtr<BIGNUM> 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<int>(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 3a721fe..0f381af 100644 --- a/include/openssl/bn.h +++ b/include/openssl/bn.h
@@ -205,6 +205,10 @@ // 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.