From 31ef16ac2d2f450a9816c861ce3d9d6f270e1be0 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 10 Apr 2019 17:03:42 -0500 Subject: [PATCH] Add missing nonce_len check to aead_aes_gcm_siv_asm_open. Test invalid nonce lengths more thoroughly to cover this case on all our AEADs. Thanks to Guido Vranken for catching this! In doing so, this also reveals we have a ton of redundant error codes (https://crbug.com/boringssl/269). I'll tidy that up in a separate change as it may require some changes to code in Android. For now, this change uses CIPHER_R_UNSUPPORTED_NONCE_SIZE just to be consistent with the rest of that file. Bug: 268 Change-Id: I0a479000ec3005ee55c828eaa92c8302b4625847 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/35545 Reviewed-by: Adam Langley --- crypto/cipher_extra/aead_test.cc | 133 ++++++++++++++++-------------- crypto/cipher_extra/e_aesgcmsiv.c | 5 ++ 2 files changed, 77 insertions(+), 61 deletions(-) diff --git a/crypto/cipher_extra/aead_test.cc b/crypto/cipher_extra/aead_test.cc index 25d4a8ee..25924bd5 100644 --- a/crypto/cipher_extra/aead_test.cc +++ b/crypto/cipher_extra/aead_test.cc @@ -42,56 +42,58 @@ struct KnownAEAD { // truncated_tags is true if the AEAD supports truncating tags to arbitrary // lengths. bool truncated_tags; + // variable_nonce is true if the AEAD supports a variable nonce length. + bool variable_nonce; // ad_len, if non-zero, is the required length of the AD. size_t ad_len; }; static const struct KnownAEAD kAEADs[] = { {"AES_128_GCM", EVP_aead_aes_128_gcm, "aes_128_gcm_tests.txt", false, true, - 0}, + true, 0}, {"AES_128_GCM_NIST", EVP_aead_aes_128_gcm, "nist_cavp/aes_128_gcm.txt", - false, true, 0}, + false, true, true, 0}, {"AES_256_GCM", EVP_aead_aes_256_gcm, "aes_256_gcm_tests.txt", false, true, - 0}, + true, 0}, {"AES_256_GCM_NIST", EVP_aead_aes_256_gcm, "nist_cavp/aes_256_gcm.txt", - false, true, 0}, + false, true, true, 0}, {"AES_128_GCM_SIV", EVP_aead_aes_128_gcm_siv, "aes_128_gcm_siv_tests.txt", - false, false, 0}, + false, false, false, 0}, {"AES_256_GCM_SIV", EVP_aead_aes_256_gcm_siv, "aes_256_gcm_siv_tests.txt", - false, false, 0}, + false, false, false, 0}, {"ChaCha20Poly1305", EVP_aead_chacha20_poly1305, - "chacha20_poly1305_tests.txt", false, true, 0}, + "chacha20_poly1305_tests.txt", false, true, false, 0}, {"XChaCha20Poly1305", EVP_aead_xchacha20_poly1305, - "xchacha20_poly1305_tests.txt", false, true, 0}, + "xchacha20_poly1305_tests.txt", false, true, false, 0}, {"AES_128_CBC_SHA1_TLS", EVP_aead_aes_128_cbc_sha1_tls, - "aes_128_cbc_sha1_tls_tests.txt", true, false, 11}, + "aes_128_cbc_sha1_tls_tests.txt", true, false, false, 11}, {"AES_128_CBC_SHA1_TLSImplicitIV", EVP_aead_aes_128_cbc_sha1_tls_implicit_iv, - "aes_128_cbc_sha1_tls_implicit_iv_tests.txt", true, false, 11}, + "aes_128_cbc_sha1_tls_implicit_iv_tests.txt", true, false, false, 11}, {"AES_128_CBC_SHA256_TLS", EVP_aead_aes_128_cbc_sha256_tls, - "aes_128_cbc_sha256_tls_tests.txt", true, false, 11}, + "aes_128_cbc_sha256_tls_tests.txt", true, false, false, 11}, {"AES_256_CBC_SHA1_TLS", EVP_aead_aes_256_cbc_sha1_tls, - "aes_256_cbc_sha1_tls_tests.txt", true, false, 11}, + "aes_256_cbc_sha1_tls_tests.txt", true, false, false, 11}, {"AES_256_CBC_SHA1_TLSImplicitIV", EVP_aead_aes_256_cbc_sha1_tls_implicit_iv, - "aes_256_cbc_sha1_tls_implicit_iv_tests.txt", true, false, 11}, + "aes_256_cbc_sha1_tls_implicit_iv_tests.txt", true, false, false, 11}, {"AES_256_CBC_SHA256_TLS", EVP_aead_aes_256_cbc_sha256_tls, - "aes_256_cbc_sha256_tls_tests.txt", true, false, 11}, + "aes_256_cbc_sha256_tls_tests.txt", true, false, false, 11}, {"AES_256_CBC_SHA384_TLS", EVP_aead_aes_256_cbc_sha384_tls, - "aes_256_cbc_sha384_tls_tests.txt", true, false, 11}, + "aes_256_cbc_sha384_tls_tests.txt", true, false, false, 11}, {"DES_EDE3_CBC_SHA1_TLS", EVP_aead_des_ede3_cbc_sha1_tls, - "des_ede3_cbc_sha1_tls_tests.txt", true, false, 11}, + "des_ede3_cbc_sha1_tls_tests.txt", true, false, false, 11}, {"DES_EDE3_CBC_SHA1_TLSImplicitIV", EVP_aead_des_ede3_cbc_sha1_tls_implicit_iv, - "des_ede3_cbc_sha1_tls_implicit_iv_tests.txt", true, false, 11}, + "des_ede3_cbc_sha1_tls_implicit_iv_tests.txt", true, false, false, 11}, {"AES_128_CTR_HMAC_SHA256", EVP_aead_aes_128_ctr_hmac_sha256, - "aes_128_ctr_hmac_sha256.txt", false, true, 0}, + "aes_128_ctr_hmac_sha256.txt", false, true, false, 0}, {"AES_256_CTR_HMAC_SHA256", EVP_aead_aes_256_ctr_hmac_sha256, - "aes_256_ctr_hmac_sha256.txt", false, true, 0}, + "aes_256_ctr_hmac_sha256.txt", false, true, false, 0}, {"AES_128_CCM_BLUETOOTH", EVP_aead_aes_128_ccm_bluetooth, - "aes_128_ccm_bluetooth_tests.txt", false, false, 0}, + "aes_128_ccm_bluetooth_tests.txt", false, false, false, 0}, {"AES_128_CCM_BLUETOOTH_8", EVP_aead_aes_128_ccm_bluetooth_8, - "aes_128_ccm_bluetooth_8_tests.txt", false, false, 0}, + "aes_128_ccm_bluetooth_8_tests.txt", false, false, false, 0}, }; class PerAEADTest : public testing::TestWithParam { @@ -605,50 +607,59 @@ TEST_P(PerAEADTest, Overflow) { // as the input.) } -// Test that EVP_aead_aes_128_gcm and EVP_aead_aes_256_gcm reject empty nonces. -// AES-GCM is not defined for those. -TEST(AEADTest, AESGCMEmptyNonce) { - static const uint8_t kZeros[32] = {0}; +TEST_P(PerAEADTest, InvalidNonceLength) { + size_t valid_nonce_len = EVP_AEAD_nonce_length(aead()); + std::vector nonce_lens; + if (valid_nonce_len != 0) { + // Other than the implicit IV TLS "AEAD"s, none of our AEADs allow empty + // nonces. In particular, although AES-GCM was incorrectly specified with + // variable-length nonces, it does not allow the empty nonce. + nonce_lens.push_back(0); + } + if (!GetParam().variable_nonce) { + nonce_lens.push_back(valid_nonce_len + 1); + if (valid_nonce_len != 0) { + nonce_lens.push_back(valid_nonce_len - 1); + } + } - // Test AES-128-GCM. - uint8_t buf[16]; - size_t len; - bssl::ScopedEVP_AEAD_CTX ctx; - ASSERT_TRUE(EVP_AEAD_CTX_init(ctx.get(), EVP_aead_aes_128_gcm(), kZeros, 16, - EVP_AEAD_DEFAULT_TAG_LENGTH, nullptr)); + static const uint8_t kZeros[EVP_AEAD_MAX_KEY_LENGTH] = {0}; + const size_t ad_len = GetParam().ad_len != 0 ? GetParam().ad_len : 16; + ASSERT_LE(ad_len, sizeof(kZeros)); - EXPECT_FALSE(EVP_AEAD_CTX_seal(ctx.get(), buf, &len, sizeof(buf), - nullptr /* nonce */, 0, nullptr /* in */, 0, - nullptr /* ad */, 0)); - uint32_t err = ERR_get_error(); - EXPECT_EQ(ERR_LIB_CIPHER, ERR_GET_LIB(err)); - EXPECT_EQ(CIPHER_R_INVALID_NONCE_SIZE, ERR_GET_REASON(err)); + for (size_t nonce_len : nonce_lens) { + SCOPED_TRACE(nonce_len); + uint8_t buf[256]; + size_t len; + std::vector nonce(nonce_len); + bssl::ScopedEVP_AEAD_CTX ctx; + ASSERT_TRUE(EVP_AEAD_CTX_init_with_direction( + ctx.get(), aead(), kZeros, EVP_AEAD_key_length(aead()), + EVP_AEAD_DEFAULT_TAG_LENGTH, evp_aead_seal)); - EXPECT_FALSE(EVP_AEAD_CTX_open(ctx.get(), buf, &len, sizeof(buf), - nullptr /* nonce */, 0, kZeros /* in */, - sizeof(kZeros), nullptr /* ad */, 0)); - err = ERR_get_error(); - EXPECT_EQ(ERR_LIB_CIPHER, ERR_GET_LIB(err)); - EXPECT_EQ(CIPHER_R_INVALID_NONCE_SIZE, ERR_GET_REASON(err)); + EXPECT_FALSE(EVP_AEAD_CTX_seal(ctx.get(), buf, &len, sizeof(buf), + nonce.data(), nonce.size(), nullptr /* in */, + 0, kZeros /* ad */, ad_len)); + uint32_t err = ERR_get_error(); + EXPECT_EQ(ERR_LIB_CIPHER, ERR_GET_LIB(err)); + // TODO(davidben): Merge these errors. https://crbug.com/boringssl/129. + if (ERR_GET_REASON(err) != CIPHER_R_UNSUPPORTED_NONCE_SIZE) { + EXPECT_EQ(CIPHER_R_INVALID_NONCE_SIZE, ERR_GET_REASON(err)); + } - // Test AES-256-GCM. - ctx.Reset(); - ASSERT_TRUE(EVP_AEAD_CTX_init(ctx.get(), EVP_aead_aes_256_gcm(), kZeros, 32, - EVP_AEAD_DEFAULT_TAG_LENGTH, nullptr)); - - EXPECT_FALSE(EVP_AEAD_CTX_seal(ctx.get(), buf, &len, sizeof(buf), - nullptr /* nonce */, 0, nullptr /* in */, 0, - nullptr /* ad */, 0)); - err = ERR_get_error(); - EXPECT_EQ(ERR_LIB_CIPHER, ERR_GET_LIB(err)); - EXPECT_EQ(CIPHER_R_INVALID_NONCE_SIZE, ERR_GET_REASON(err)); - - EXPECT_FALSE(EVP_AEAD_CTX_open(ctx.get(), buf, &len, sizeof(buf), - nullptr /* nonce */, 0, kZeros /* in */, - sizeof(kZeros), nullptr /* ad */, 0)); - err = ERR_get_error(); - EXPECT_EQ(ERR_LIB_CIPHER, ERR_GET_LIB(err)); - EXPECT_EQ(CIPHER_R_INVALID_NONCE_SIZE, ERR_GET_REASON(err)); + ctx.Reset(); + ASSERT_TRUE(EVP_AEAD_CTX_init_with_direction( + ctx.get(), aead(), kZeros, EVP_AEAD_key_length(aead()), + EVP_AEAD_DEFAULT_TAG_LENGTH, evp_aead_open)); + EXPECT_FALSE(EVP_AEAD_CTX_open(ctx.get(), buf, &len, sizeof(buf), + nonce.data(), nonce.size(), kZeros /* in */, + sizeof(kZeros), kZeros /* ad */, ad_len)); + err = ERR_get_error(); + EXPECT_EQ(ERR_LIB_CIPHER, ERR_GET_LIB(err)); + if (ERR_GET_REASON(err) != CIPHER_R_UNSUPPORTED_NONCE_SIZE) { + EXPECT_EQ(CIPHER_R_INVALID_NONCE_SIZE, ERR_GET_REASON(err)); + } + } } TEST(AEADTest, AESCCMLargeAD) { diff --git a/crypto/cipher_extra/e_aesgcmsiv.c b/crypto/cipher_extra/e_aesgcmsiv.c index 71a71fac..64febae4 100644 --- a/crypto/cipher_extra/e_aesgcmsiv.c +++ b/crypto/cipher_extra/e_aesgcmsiv.c @@ -426,6 +426,11 @@ static int aead_aes_gcm_siv_asm_open(const EVP_AEAD_CTX *ctx, uint8_t *out, return 0; } + if (nonce_len != EVP_AEAD_AES_GCM_SIV_NONCE_LEN) { + OPENSSL_PUT_ERROR(CIPHER, CIPHER_R_UNSUPPORTED_NONCE_SIZE); + return 0; + } + const struct aead_aes_gcm_siv_asm_ctx *gcm_siv_ctx = asm_ctx_from_ctx(ctx); const size_t plaintext_len = in_len - EVP_AEAD_AES_GCM_SIV_TAG_LEN; const uint8_t *const given_tag = in + plaintext_len;