Commit 4789e83b authored by eroman@chromium.org's avatar eroman@chromium.org

[refactor] Use base::CheckedNumeric<> in place of manual checks.

BUG=394821

Review URL: https://codereview.chromium.org/405693002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@284546 0039d316-1c4b-4281-b951-d872f2087c98
parent 8cb570c2
...@@ -4,6 +4,7 @@ ...@@ -4,6 +4,7 @@
#include <cryptohi.h> #include <cryptohi.h>
#include "base/numerics/safe_math.h"
#include "content/child/webcrypto/crypto_data.h" #include "content/child/webcrypto/crypto_data.h"
#include "content/child/webcrypto/nss/aes_key_nss.h" #include "content/child/webcrypto/nss/aes_key_nss.h"
#include "content/child/webcrypto/nss/key_nss.h" #include "content/child/webcrypto/nss/key_nss.h"
...@@ -50,7 +51,9 @@ Status AesCbcEncryptDecrypt(EncryptOrDecrypt mode, ...@@ -50,7 +51,9 @@ Status AesCbcEncryptDecrypt(EncryptOrDecrypt mode,
// Oddly PK11_CipherOp takes input and output lengths as "int" rather than // Oddly PK11_CipherOp takes input and output lengths as "int" rather than
// "unsigned int". Do some checks now to avoid integer overflowing. // "unsigned int". Do some checks now to avoid integer overflowing.
if (data.byte_length() >= INT_MAX - AES_BLOCK_SIZE) { base::CheckedNumeric<int> output_max_len = data.byte_length();
output_max_len += AES_BLOCK_SIZE;
if (!output_max_len.IsValid()) {
// TODO(eroman): Handle this by chunking the input fed into NSS. Right now // TODO(eroman): Handle this by chunking the input fed into NSS. Right now
// it doesn't make much difference since the one-shot API would end up // it doesn't make much difference since the one-shot API would end up
// blowing out the memory and crashing anyway. // blowing out the memory and crashing anyway.
...@@ -67,10 +70,7 @@ Status AesCbcEncryptDecrypt(EncryptOrDecrypt mode, ...@@ -67,10 +70,7 @@ Status AesCbcEncryptDecrypt(EncryptOrDecrypt mode,
// TODO(eroman): Refine the output buffer size. It can be computed exactly for // TODO(eroman): Refine the output buffer size. It can be computed exactly for
// encryption, and can be smaller for decryption. // encryption, and can be smaller for decryption.
unsigned int output_max_len = data.byte_length() + AES_BLOCK_SIZE; buffer->resize(output_max_len.ValueOrDie());
CHECK_GT(output_max_len, data.byte_length());
buffer->resize(output_max_len);
unsigned char* buffer_data = Uint8VectorStart(buffer); unsigned char* buffer_data = Uint8VectorStart(buffer);
...@@ -85,10 +85,11 @@ Status AesCbcEncryptDecrypt(EncryptOrDecrypt mode, ...@@ -85,10 +85,11 @@ Status AesCbcEncryptDecrypt(EncryptOrDecrypt mode,
} }
unsigned int final_output_chunk_len; unsigned int final_output_chunk_len;
if (SECSuccess != PK11_DigestFinal(context.get(), if (SECSuccess !=
buffer_data + output_len, PK11_DigestFinal(context.get(),
&final_output_chunk_len, buffer_data + output_len,
output_max_len - output_len)) { &final_output_chunk_len,
(output_max_len - output_len).ValueOrDie())) {
return Status::OperationError(); return Status::OperationError();
} }
......
...@@ -2,6 +2,7 @@ ...@@ -2,6 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be // Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file. // found in the LICENSE file.
#include "base/numerics/safe_math.h"
#include "content/child/webcrypto/crypto_data.h" #include "content/child/webcrypto/crypto_data.h"
#include "content/child/webcrypto/nss/aes_key_nss.h" #include "content/child/webcrypto/nss/aes_key_nss.h"
#include "content/child/webcrypto/nss/key_nss.h" #include "content/child/webcrypto/nss/key_nss.h"
...@@ -89,30 +90,28 @@ Status AesGcmEncryptDecrypt(EncryptOrDecrypt mode, ...@@ -89,30 +90,28 @@ Status AesGcmEncryptDecrypt(EncryptOrDecrypt mode,
param.data = reinterpret_cast<unsigned char*>(&gcm_params); param.data = reinterpret_cast<unsigned char*>(&gcm_params);
param.len = sizeof(gcm_params); param.len = sizeof(gcm_params);
unsigned int buffer_size = 0; base::CheckedNumeric<unsigned int> buffer_size(data.byte_length());
// Calculate the output buffer size. // Calculate the output buffer size.
if (mode == ENCRYPT) { if (mode == ENCRYPT) {
// TODO(eroman): This is ugly, abstract away the safe integer arithmetic. buffer_size += tag_length_bytes;
if (data.byte_length() > (UINT_MAX - tag_length_bytes)) if (!buffer_size.IsValid())
return Status::ErrorDataTooLarge(); return Status::ErrorDataTooLarge();
buffer_size = data.byte_length() + tag_length_bytes;
} else {
// TODO(eroman): In theory the buffer allocated for the plain text should be
// sized as |data.byte_length() - tag_length_bytes|.
//
// However NSS has a bug whereby it will fail if the output buffer size is
// not at least as large as the ciphertext:
//
// https://bugzilla.mozilla.org/show_bug.cgi?id=%20853674
//
// From the analysis of that bug it looks like it might be safe to pass a
// correctly sized buffer but lie about its size. Since resizing the
// WebCryptoArrayBuffer is expensive that hack may be worth looking into.
buffer_size = data.byte_length();
} }
buffer->resize(buffer_size); // TODO(eroman): In theory the buffer allocated for the plain text should be
// sized as |data.byte_length() - tag_length_bytes|.
//
// However NSS has a bug whereby it will fail if the output buffer size is
// not at least as large as the ciphertext:
//
// https://bugzilla.mozilla.org/show_bug.cgi?id=%20853674
//
// From the analysis of that bug it looks like it might be safe to pass a
// correctly sized buffer but lie about its size. Since resizing the
// WebCryptoArrayBuffer is expensive that hack may be worth looking into.
buffer->resize(buffer_size.ValueOrDie());
unsigned char* buffer_data = Uint8VectorStart(buffer); unsigned char* buffer_data = Uint8VectorStart(buffer);
PK11_EncryptDecryptFunction encrypt_or_decrypt_func = PK11_EncryptDecryptFunction encrypt_or_decrypt_func =
......
...@@ -4,6 +4,7 @@ ...@@ -4,6 +4,7 @@
#include <secerr.h> #include <secerr.h>
#include "base/numerics/safe_math.h"
#include "content/child/webcrypto/crypto_data.h" #include "content/child/webcrypto/crypto_data.h"
#include "content/child/webcrypto/nss/aes_key_nss.h" #include "content/child/webcrypto/nss/aes_key_nss.h"
#include "content/child/webcrypto/nss/key_nss.h" #include "content/child/webcrypto/nss/key_nss.h"
...@@ -97,8 +98,6 @@ Status WrapSymKeyAesKw(PK11SymKey* key, ...@@ -97,8 +98,6 @@ Status WrapSymKeyAesKw(PK11SymKey* key,
const unsigned int input_length = PK11_GetKeyLength(key); const unsigned int input_length = PK11_GetKeyLength(key);
DCHECK_GE(input_length, 16u); DCHECK_GE(input_length, 16u);
DCHECK((input_length % 8) == 0); DCHECK((input_length % 8) == 0);
if (input_length > UINT_MAX - 8)
return Status::ErrorDataTooLarge();
SECItem iv_item = MakeSECItemForBuffer(CryptoData(kAesIv, sizeof(kAesIv))); SECItem iv_item = MakeSECItemForBuffer(CryptoData(kAesIv, sizeof(kAesIv)));
crypto::ScopedSECItem param_item( crypto::ScopedSECItem param_item(
...@@ -106,8 +105,12 @@ Status WrapSymKeyAesKw(PK11SymKey* key, ...@@ -106,8 +105,12 @@ Status WrapSymKeyAesKw(PK11SymKey* key,
if (!param_item) if (!param_item)
return Status::ErrorUnexpected(); return Status::ErrorUnexpected();
const unsigned int output_length = input_length + 8; base::CheckedNumeric<unsigned int> output_length = input_length;
buffer->resize(output_length); output_length += 8;
if (!output_length.IsValid())
return Status::ErrorDataTooLarge();
buffer->resize(output_length.ValueOrDie());
SECItem wrapped_key_item = MakeSECItemForBuffer(CryptoData(*buffer)); SECItem wrapped_key_item = MakeSECItemForBuffer(CryptoData(*buffer));
if (SECSuccess != PK11_WrapSymKey(CKM_NSS_AES_KEY_WRAP, if (SECSuccess != PK11_WrapSymKey(CKM_NSS_AES_KEY_WRAP,
...@@ -117,7 +120,7 @@ Status WrapSymKeyAesKw(PK11SymKey* key, ...@@ -117,7 +120,7 @@ Status WrapSymKeyAesKw(PK11SymKey* key,
&wrapped_key_item)) { &wrapped_key_item)) {
return Status::OperationError(); return Status::OperationError();
} }
if (output_length != wrapped_key_item.len) if (output_length.ValueOrDie() != wrapped_key_item.len)
return Status::ErrorUnexpected(); return Status::ErrorUnexpected();
return Status::Success(); return Status::Success();
......
...@@ -5,6 +5,7 @@ ...@@ -5,6 +5,7 @@
#include "content/child/webcrypto/nss/rsa_key_nss.h" #include "content/child/webcrypto/nss/rsa_key_nss.h"
#include "base/logging.h" #include "base/logging.h"
#include "base/numerics/safe_math.h"
#include "content/child/webcrypto/crypto_data.h" #include "content/child/webcrypto/crypto_data.h"
#include "content/child/webcrypto/jwk.h" #include "content/child/webcrypto/jwk.h"
#include "content/child/webcrypto/nss/key_nss.h" #include "content/child/webcrypto/nss/key_nss.h"
...@@ -598,9 +599,10 @@ Status RsaHashedAlgorithm::GenerateKeyPair( ...@@ -598,9 +599,10 @@ Status RsaHashedAlgorithm::GenerateKeyPair(
PK11RSAGenParams rsa_gen_params; PK11RSAGenParams rsa_gen_params;
// keySizeInBits is a signed type, don't pass in a negative value. // keySizeInBits is a signed type, don't pass in a negative value.
if (params->modulusLengthBits() > INT_MAX) base::CheckedNumeric<int> signed_modulus(params->modulusLengthBits());
if (!signed_modulus.IsValid())
return Status::OperationError(); return Status::OperationError();
rsa_gen_params.keySizeInBits = params->modulusLengthBits(); rsa_gen_params.keySizeInBits = signed_modulus.ValueOrDie();
rsa_gen_params.pe = public_exponent; rsa_gen_params.pe = public_exponent;
const CK_FLAGS operation_flags_mask = const CK_FLAGS operation_flags_mask =
......
...@@ -6,6 +6,7 @@ ...@@ -6,6 +6,7 @@
#include <openssl/evp.h> #include <openssl/evp.h>
#include "base/logging.h" #include "base/logging.h"
#include "base/numerics/safe_math.h"
#include "content/child/webcrypto/crypto_data.h" #include "content/child/webcrypto/crypto_data.h"
#include "content/child/webcrypto/openssl/aes_key_openssl.h" #include "content/child/webcrypto/openssl/aes_key_openssl.h"
#include "content/child/webcrypto/openssl/key_openssl.h" #include "content/child/webcrypto/openssl/key_openssl.h"
...@@ -47,12 +48,19 @@ Status AesCbcEncryptDecrypt(CipherOperation cipher_operation, ...@@ -47,12 +48,19 @@ Status AesCbcEncryptDecrypt(CipherOperation cipher_operation,
if (params->iv().size() != 16) if (params->iv().size() != 16)
return Status::ErrorIncorrectSizeAesCbcIv(); return Status::ErrorIncorrectSizeAesCbcIv();
if (data.byte_length() >= INT_MAX - AES_BLOCK_SIZE) { // According to the openssl docs, the amount of data written may be as large
// TODO(padolph): Handle this by chunking the input fed into OpenSSL. Right // as (data_size + cipher_block_size - 1), constrained to a multiple of
// now it doesn't make much difference since the one-shot API would end up // cipher_block_size.
// blowing out the memory and crashing anyway. base::CheckedNumeric<int> output_max_len = data.byte_length();
output_max_len += AES_BLOCK_SIZE - 1;
if (!output_max_len.IsValid())
return Status::ErrorDataTooLarge();
const unsigned remainder = output_max_len.ValueOrDie() % AES_BLOCK_SIZE;
if (remainder != 0)
output_max_len += AES_BLOCK_SIZE - remainder;
if (!output_max_len.IsValid())
return Status::ErrorDataTooLarge(); return Status::ErrorDataTooLarge();
}
// Note: PKCS padding is enabled by default // Note: PKCS padding is enabled by default
crypto::ScopedOpenSSL<EVP_CIPHER_CTX, EVP_CIPHER_CTX_free>::Type context( crypto::ScopedOpenSSL<EVP_CIPHER_CTX, EVP_CIPHER_CTX_free>::Type context(
...@@ -73,16 +81,7 @@ Status AesCbcEncryptDecrypt(CipherOperation cipher_operation, ...@@ -73,16 +81,7 @@ Status AesCbcEncryptDecrypt(CipherOperation cipher_operation,
return Status::OperationError(); return Status::OperationError();
} }
// According to the openssl docs, the amount of data written may be as large buffer->resize(output_max_len.ValueOrDie());
// as (data_size + cipher_block_size - 1), constrained to a multiple of
// cipher_block_size.
unsigned int output_max_len = data.byte_length() + AES_BLOCK_SIZE - 1;
const unsigned remainder = output_max_len % AES_BLOCK_SIZE;
if (remainder != 0)
output_max_len += AES_BLOCK_SIZE - remainder;
DCHECK_GT(output_max_len, data.byte_length());
buffer->resize(output_max_len);
unsigned char* const buffer_data = Uint8VectorStart(buffer); unsigned char* const buffer_data = Uint8VectorStart(buffer);
...@@ -102,7 +101,6 @@ Status AesCbcEncryptDecrypt(CipherOperation cipher_operation, ...@@ -102,7 +101,6 @@ Status AesCbcEncryptDecrypt(CipherOperation cipher_operation,
const unsigned int final_output_len = const unsigned int final_output_len =
static_cast<unsigned int>(output_len) + static_cast<unsigned int>(output_len) +
static_cast<unsigned int>(final_output_chunk_len); static_cast<unsigned int>(final_output_chunk_len);
DCHECK_LE(final_output_len, output_max_len);
buffer->resize(final_output_len); buffer->resize(final_output_len);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment