EVP_CIPHER: only poison the object on failure if it was mutated by the call. This makes span size errors non-poisoning, as well as EVP_DecryptFinal padding failures (as that function does not even change state of the EVP_CIPHER object at all unless the cipher is a custom cipher). Bug: 520121641 Change-Id: I80bfcd77fc8c842d13901deaef90aca26a6a6964 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/99648 Presubmit-BoringSSL-Verified: boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> Commit-Queue: Rudolf Polzer <rpolzer@google.com> Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/cipher/cipher_test.cc b/crypto/cipher/cipher_test.cc index 57aee67..a55b502 100644 --- a/crypto/cipher/cipher_test.cc +++ b/crypto/cipher/cipher_test.cc
@@ -31,6 +31,7 @@ #include <openssl/sha.h> #include <openssl/span.h> +#include "../fipsmodule/cipher/internal.h" #include "../internal.h" #include "../test/file_test.h" #include "../test/test_util.h" @@ -1416,5 +1417,219 @@ } } +TEST(CipherTest, NonPoisoningErrors) { + const uint8_t kKey[16] = {1, 2, 3, 4, 5, 6, 7, 8, + 9, 10, 11, 12, 13, 14, 15, 16}; + const uint8_t kIV[16] = {0}; + uint8_t in[32] = {0}; + uint8_t out[64]; + size_t out_len; + + // EncryptUpdate output buffer size error does not poison the context. + { + bssl::UniquePtr<EVP_CIPHER_CTX> ctx(EVP_CIPHER_CTX_new()); + ASSERT_TRUE(ctx); + ASSERT_TRUE( + EVP_EncryptInit_ex(ctx.get(), EVP_aes_128_cbc(), nullptr, kKey, kIV)); + // Calling EVP_EncryptUpdate_ex with max_out_len = 0 fails with + // CIPHER_R_BUFFER_TOO_SMALL. + EXPECT_FALSE( + EVP_EncryptUpdate_ex(ctx.get(), out, &out_len, 0, in, sizeof(in))); + EXPECT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_CIPHER, + CIPHER_R_BUFFER_TOO_SMALL)); + // The context is not poisoned; a subsequent call with sufficient output + // buffer succeeds. + EXPECT_TRUE(EVP_EncryptUpdate_ex(ctx.get(), out, &out_len, sizeof(out), in, + sizeof(in))); + } + + // EncryptFinal output buffer size error does not poison the context. + { + bssl::UniquePtr<EVP_CIPHER_CTX> ctx(EVP_CIPHER_CTX_new()); + ASSERT_TRUE(ctx); + ASSERT_TRUE( + EVP_EncryptInit_ex(ctx.get(), EVP_aes_128_cbc(), nullptr, kKey, kIV)); + ASSERT_TRUE(EVP_EncryptUpdate_ex(ctx.get(), out, &out_len, sizeof(out), in, + sizeof(in))); + // Calling EVP_EncryptFinal_ex2 with max_out_len = 0 fails with + // CIPHER_R_BUFFER_TOO_SMALL. + EXPECT_FALSE(EVP_EncryptFinal_ex2(ctx.get(), out, &out_len, 0)); + EXPECT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_CIPHER, + CIPHER_R_BUFFER_TOO_SMALL)); + // The context is not poisoned; a subsequent call with sufficient output + // buffer succeeds. + EXPECT_TRUE(EVP_EncryptFinal_ex2(ctx.get(), out, &out_len, sizeof(out))); + } + + // DecryptUpdate output buffer size error does not poison the context. + { + bssl::UniquePtr<EVP_CIPHER_CTX> ctx(EVP_CIPHER_CTX_new()); + ASSERT_TRUE(ctx); + ASSERT_TRUE( + EVP_DecryptInit_ex(ctx.get(), EVP_aes_128_cbc(), nullptr, kKey, kIV)); + // Calling EVP_DecryptUpdate_ex with max_out_len = 0 fails with + // CIPHER_R_BUFFER_TOO_SMALL. + EXPECT_FALSE( + EVP_DecryptUpdate_ex(ctx.get(), out, &out_len, 0, in, sizeof(in))); + EXPECT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_CIPHER, + CIPHER_R_BUFFER_TOO_SMALL)); + // The context is not poisoned; a subsequent call with sufficient output + // buffer succeeds. + EXPECT_TRUE(EVP_DecryptUpdate_ex(ctx.get(), out, &out_len, sizeof(out), in, + sizeof(in))); + } + + // DecryptFinal output buffer size error does not poison the context. + { + // Generate valid ciphertext for 10 bytes of input (so payload length is 10 + // > 0). + uint8_t ciphertext[32]; + size_t ciphertext_len = 0, final_len = 0; + bssl::UniquePtr<EVP_CIPHER_CTX> enc_ctx(EVP_CIPHER_CTX_new()); + ASSERT_TRUE(enc_ctx); + ASSERT_TRUE(EVP_EncryptInit_ex(enc_ctx.get(), EVP_aes_128_cbc(), nullptr, + kKey, kIV)); + ASSERT_TRUE(EVP_EncryptUpdate_ex(enc_ctx.get(), ciphertext, &ciphertext_len, + sizeof(ciphertext), in, 10)); + ASSERT_TRUE(EVP_EncryptFinal_ex2(enc_ctx.get(), ciphertext + ciphertext_len, + &final_len, + sizeof(ciphertext) - ciphertext_len)); + ciphertext_len += final_len; + + bssl::UniquePtr<EVP_CIPHER_CTX> ctx(EVP_CIPHER_CTX_new()); + ASSERT_TRUE(ctx); + ASSERT_TRUE( + EVP_DecryptInit_ex(ctx.get(), EVP_aes_128_cbc(), nullptr, kKey, kIV)); + ASSERT_TRUE(EVP_DecryptUpdate_ex(ctx.get(), out, &out_len, sizeof(out), + ciphertext, ciphertext_len)); + // Calling EVP_DecryptFinal_ex2 with max_out_len = 5 (< payload size 10) + // fails with CIPHER_R_BUFFER_TOO_SMALL. + EXPECT_FALSE(EVP_DecryptFinal_ex2(ctx.get(), out, &out_len, 5)); + EXPECT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_CIPHER, + CIPHER_R_BUFFER_TOO_SMALL)); + // The context is not poisoned; a subsequent call with sufficient output + // buffer succeeds. + EXPECT_TRUE(EVP_DecryptFinal_ex2(ctx.get(), out, &out_len, sizeof(out))); + EXPECT_EQ(out_len, 10u); + } + + // DecryptFinal padding failure does not poison the context. + // + // google/play-licensing, sadly, relies on it by only ever calling `doFinal` + // on a cipher object. (Which is known broken as the CBC IV will be wrong for + // next call, but as next call's padding will pass if the input string has + // correct padding, the IV for the call after will be correct again, + // therefore reading bad data once in their `unobfuscate` method will "fix + // itself" two calls later). + { + uint8_t bad_ciphertext[16] = {0}; + int len; + + bssl::UniquePtr<EVP_CIPHER_CTX> ctx(EVP_CIPHER_CTX_new()); + ASSERT_TRUE(ctx); + ASSERT_TRUE( + EVP_DecryptInit_ex(ctx.get(), EVP_aes_128_cbc(), nullptr, kKey, kIV)); + ASSERT_TRUE(EVP_DecryptUpdate(ctx.get(), out, &len, bad_ciphertext, + sizeof(bad_ciphertext))); + // EVP_DecryptFinal_ex fails due to bad padding. + EXPECT_FALSE(EVP_DecryptFinal_ex(ctx.get(), out, &len)); + EXPECT_TRUE( + ErrorEquals(ERR_get_error(), ERR_LIB_CIPHER, CIPHER_R_BAD_DECRYPT)); + + // The context is not poisoned: a subsequent call returns + // CIPHER_R_BAD_DECRYPT rather than ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED. + EXPECT_FALSE(EVP_DecryptFinal_ex(ctx.get(), out, &len)); + EXPECT_TRUE( + ErrorEquals(ERR_get_error(), ERR_LIB_CIPHER, CIPHER_R_BAD_DECRYPT)); + } +} + +TEST(CipherTest, PoisoningErrors) { + const uint8_t kKey[16] = {1, 2, 3, 4, 5, 6, 7, 8, + 9, 10, 11, 12, 13, 14, 15, 16}; + const uint8_t kIV[16] = {0}; + uint8_t in[32] = {0}; + uint8_t out[64]; + size_t out_len; + + // A cipher whose `cipher_update` callback fails mid-operation. + EVP_CIPHER failing_update_cipher = {}; + failing_update_cipher.block_size = 16; + failing_update_cipher.key_len = 16; + failing_update_cipher.iv_len = 16; + failing_update_cipher.init = [](EVP_CIPHER_CTX *ctx, const uint8_t *key, + const uint8_t *iv, int enc) { return 1; }; + failing_update_cipher.cipher_update = [](EVP_CIPHER_CTX *ctx, + uint8_t *out_arg, + const uint8_t *in_arg, size_t len) { + OPENSSL_PUT_ERROR(CIPHER, CIPHER_R_AES_KEY_SETUP_FAILED); + return 0; + }; + + { + bssl::UniquePtr<EVP_CIPHER_CTX> ctx(EVP_CIPHER_CTX_new()); + ASSERT_TRUE(ctx); + ASSERT_TRUE(EVP_EncryptInit_ex(ctx.get(), &failing_update_cipher, nullptr, + kKey, kIV)); + // EVP_EncryptUpdate_ex fails because the underlying cipher_update fails. + EXPECT_FALSE(EVP_EncryptUpdate_ex(ctx.get(), out, &out_len, sizeof(out), in, + sizeof(in))); + EXPECT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_CIPHER, + CIPHER_R_AES_KEY_SETUP_FAILED)); + + // The context is poisoned: subsequent operations fail with + // ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED. + EXPECT_FALSE(EVP_EncryptUpdate_ex(ctx.get(), out, &out_len, sizeof(out), in, + sizeof(in))); + EXPECT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_CIPHER, + ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED)); + + EXPECT_FALSE(EVP_EncryptFinal_ex2(ctx.get(), out, &out_len, sizeof(out))); + EXPECT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_CIPHER, + ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED)); + + // Re-initializing the context clears the poison flag. + ASSERT_TRUE(EVP_EncryptInit_ex(ctx.get(), &failing_update_cipher, nullptr, + kKey, kIV)); + EXPECT_FALSE(EVP_EncryptUpdate_ex(ctx.get(), out, &out_len, sizeof(out), in, + sizeof(in))); + EXPECT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_CIPHER, + CIPHER_R_AES_KEY_SETUP_FAILED)); + } + + // A custom cipher whose `cipher_final` callback fails. + EVP_CIPHER failing_final_cipher = {}; + failing_final_cipher.block_size = 1; + failing_final_cipher.key_len = 16; + failing_final_cipher.iv_len = 16; + failing_final_cipher.flags = EVP_CIPH_FLAG_CUSTOM_CIPHER; + failing_final_cipher.init = [](EVP_CIPHER_CTX *ctx, const uint8_t *key, + const uint8_t *iv, int enc) { return 1; }; + failing_final_cipher.cipher_update = [](EVP_CIPHER_CTX *ctx, uint8_t *out_arg, + const uint8_t *in_arg, + size_t len) { return 1; }; + failing_final_cipher.cipher_final = [](EVP_CIPHER_CTX *ctx) { + OPENSSL_PUT_ERROR(CIPHER, CIPHER_R_AES_KEY_SETUP_FAILED); + return 0; + }; + + { + bssl::UniquePtr<EVP_CIPHER_CTX> ctx(EVP_CIPHER_CTX_new()); + ASSERT_TRUE(ctx); + ASSERT_TRUE(EVP_EncryptInit_ex(ctx.get(), &failing_final_cipher, nullptr, + kKey, kIV)); + // EVP_EncryptFinal_ex2 fails because cipher_final fails. + EXPECT_FALSE(EVP_EncryptFinal_ex2(ctx.get(), out, &out_len, sizeof(out))); + EXPECT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_CIPHER, + CIPHER_R_AES_KEY_SETUP_FAILED)); + + // The context is poisoned: subsequent operations fail with + // ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED. + EXPECT_FALSE(EVP_EncryptFinal_ex2(ctx.get(), out, &out_len, sizeof(out))); + EXPECT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_CIPHER, + ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED)); + } +} + } // namespace BSSL_NAMESPACE_END
diff --git a/crypto/fipsmodule/cipher/cipher.cc.inc b/crypto/fipsmodule/cipher/cipher.cc.inc index 49fb979..bda33dc 100644 --- a/crypto/fipsmodule/cipher/cipher.cc.inc +++ b/crypto/fipsmodule/cipher/cipher.cc.inc
@@ -192,7 +192,7 @@ ctx->final_used = 0; // Clear the poisoned flag to permit reuse of a CTX that previously had a // failed operation. - ctx->poisoned = 0; + ctx->poisoned = false; return 1; } @@ -250,10 +250,14 @@ OPENSSL_PUT_ERROR(CIPHER, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); return 0; } + // Invariant fulfilled: `mutated` is only cleared if `!poisoned`. + ctx->mutated = false; if (!f()) { // Functions using `WrapWithPoison` may leave `ctx` in an indeterminate - // state. Mark the object as poisoned. - ctx->poisoned = 1; + // state. Mark the object as poisoned if it's been mutated. + // Invariant fulfilled: `poisoned` is only set if `mutated`. + // This never unsets `poisoned` because `poisoned` implies `mutated`. + ctx->poisoned = ctx->mutated; return 0; } return 1; @@ -264,6 +268,11 @@ const uint8_t *in, size_t in_len) { *out_len = 0; + if (max_out_len < EVP_CIPHER_CTX_max_next_update(ctx, in_len)) { + OPENSSL_PUT_ERROR(CIPHER, CIPHER_R_BUFFER_TOO_SMALL); + return 0; + } + // Ciphers that use blocks may write up to `block_size` extra bytes. Ensure // the output does not overflow `*out_len`. Span<const uint8_t> in_span(in, in_len); @@ -279,6 +288,7 @@ if (buf_len != 0) { if (block_size - buf_len > in_span.size()) { CopyToPrefix(in_span, Span(ctx->buf).subspan(buf_len)); + ctx->mutated = true; ctx->buf_len += in_span.size(); return 1; } else { @@ -288,6 +298,7 @@ OPENSSL_PUT_ERROR(CIPHER, CIPHER_R_BUFFER_TOO_SMALL); return 0; } + ctx->mutated = true; if (!ctx->cipher->cipher_update(ctx, out_span.data(), ctx->buf, block_size)) { return 0; @@ -303,6 +314,7 @@ OPENSSL_PUT_ERROR(CIPHER, CIPHER_R_BUFFER_TOO_SMALL); return 0; } + ctx->mutated = true; if (!ctx->cipher->cipher_update(ctx, out_span.data(), in_span.data(), whole_blocks)) { return 0; @@ -313,6 +325,7 @@ assert(in_span.size() < block_size); CopyToPrefix(in_span, ctx->buf); + ctx->mutated = true; ctx->buf_len = in_span.size(); *out_len = max_out_len - out_span.size(); @@ -341,10 +354,16 @@ size_t *out_len, size_t max_out_len) { *out_len = 0; + if (max_out_len < EVP_CIPHER_CTX_max_final(ctx)) { + OPENSSL_PUT_ERROR(CIPHER, CIPHER_R_BUFFER_TOO_SMALL); + return 0; + } + size_t block_size = ctx->cipher->block_size; assert(block_size <= sizeof(ctx->buf)); if (block_size == 1) { if (ctx->cipher->flags & EVP_CIPH_FLAG_CUSTOM_CIPHER) { + ctx->mutated = true; return ctx->cipher->cipher_final(ctx); } return 1; @@ -369,6 +388,7 @@ OPENSSL_PUT_ERROR(CIPHER, CIPHER_R_BUFFER_TOO_SMALL); return 0; } + ctx->mutated = true; if (!ctx->cipher->cipher_update(ctx, out_span.data(), ctx->buf, block_size)) { return 0; } @@ -422,6 +442,11 @@ const uint8_t *in, size_t in_len) { *out_len = 0; + if (max_out_len < EVP_CIPHER_CTX_max_next_update(ctx, in_len)) { + OPENSSL_PUT_ERROR(CIPHER, CIPHER_R_BUFFER_TOO_SMALL); + return 0; + } + // Ciphers that use blocks may write up to `block_size` extra bytes. Ensure // the output does not overflow `*out_len`. Span<const uint8_t> in_span(in, in_len); @@ -446,6 +471,7 @@ return 0; } CopyToPrefix(Span(ctx->final).first(block_size), out_span); + ctx->mutated = true; ctx->final_used = 0; out_span = out_span.subspan(block_size); } @@ -478,6 +504,7 @@ in_span.size())) { return 0; } + ctx->mutated = true; ctx->final_used = 1; assert(final_size == block_size); assert(ctx->buf_len == 0); @@ -519,10 +546,16 @@ size_t max_out_len) { *out_len = 0; + // NOT checking `max_out_len < EVP_CIPHER_CTX_max_final(ctx)` here. This + // method doesn't mutate the context, and as such, the resulting status is + // always well-defined. That way, we get `CIPHER_R_BUFFER_TOO_SMALL` only if + // the output buffer was actually too small. + size_t block_size = ctx->cipher->block_size; assert(block_size <= sizeof(ctx->buf)); if (block_size == 1) { if (ctx->cipher->flags & EVP_CIPH_FLAG_CUSTOM_CIPHER) { + ctx->mutated = true; return ctx->cipher->cipher_final(ctx); } return 1; @@ -659,6 +692,7 @@ OPENSSL_PUT_ERROR(CIPHER, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); return 0; } + ctx->mutated = true; return ctx->cipher->update_aad(ctx, in, in_len); }); }
diff --git a/include/openssl/cipher.h b/include/openssl/cipher.h index 324033d..5f3c592 100644 --- a/include/openssl/cipher.h +++ b/include/openssl/cipher.h
@@ -758,6 +758,12 @@ // Has this structure been rendered unusable by a failure. int poisoned; + + // INTERNAL: Has this structure been mutated by the current operation? + // To be set whenever anything meaningful has been modified. Modification + // to `buf` beyond `buf_len` needs not be indicated. + // Invariant: `poisoned` implies `mutated`. + int mutated; } /* EVP_CIPHER_CTX */; typedef struct evp_cipher_info_st {