Unwind EVP_CIPHER_CTX poisoning The original motivation here was that CBC caused the wrapper layer to call into the EVP_CIPHER twice. An error in the second call would put the EVP_CIPHER_CTX in an unpredictable state. And so https://boringssl-review.googlesource.com/c/boringssl/+/54185 added some basic "poison" tracking to try to catch egregious caller mistakes. This then got extended significantly in https://boringssl-review.googlesource.com/c/boringssl/+/83447 to catch all failures. Extending it caused some problems, both in Conscrypt: 1. This changed public API at the Conscrypt level if a Java Cipher object was called after an error. See b/504728350. While somewhat questionable caller behavior, it was an unexpected behavior change. 2. The Java Cipher APIs promise that you can retry after a ShortBufferException. See [0]. Conscrypt currently does all the bounds checks itself, but this is itself problematic, so we'd rather shift that into BoringSSL, which is better suited to calculate this. https://boringssl-review.googlesource.com/c/boringssl/+/99648 patched this with further complexity to the 'poisoned' bit, but it turns out this was all unnecessary. If we look at where cipher_update is *actually* fallible, it turns out: - Almost every cipher_update is infallible, notably all the CBC ones. - AES-GCM and AES-XTS check if their keys have been initialized. (The others should but don't. We should probably have a better notion of when the EVP_CIPHER_CTX has been initialized. Either way, this is not a pattern of failure that can trigger our problem.) - AES-XTS checks if its input is long enough. - AES-GCM checks if the input is too long. Our XTS implementation doesn't support streaming anyway, and GCM is not padded or block-buffered, so the motivation for poisoning doesn't apply. THat means this was all moot. Indeed it's telling that the only positive test for poisoning had to make a custom EVP_CIPHER, and supporting custom EVP_CIPHERs is explicitly out of scope. Unwind the whole poisoning machinery entirely. Also fix some of the tests added in 99648 to actually test what [0] needs: that retrying the operation gives the same thing. Also fix a bug, caught by asserts, in the EVP_EncryptUpdate_ex length check: EVP_CIPHER_CTX_max_next_update checks ctx->encrypt, but decrypt calls into encrypt. [0] https://docs.oracle.com/en/java/javase/21/docs/api/java.base/javax/crypto/Cipher.html#:~:text=at%20outputOffset%20inclusive.-,If%20the%20output%20buffer%20is%20too%20small%20to%20hold%20the%20result%2C%20a%20ShortBufferException%20is%20thrown.%20In%20this%20case%2C%20repeat%20this%20call%20with%20a%20larger%20output%20buffer.%20Use%20getOutputSize%20to%20determine%20how%20big%20the%20output%20buffer%20should%20be.,-If%20inputLen%20is Fixed: 520121641 Change-Id: I7c1c4138d2553fbd9ea72013962f60fa9a3e628c Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/99990 Commit-Queue: David Benjamin <davidben@google.com> Auto-Submit: David Benjamin <davidben@google.com> Reviewed-by: Rudolf Polzer <rpolzer@google.com> Presubmit-BoringSSL-Verified: boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com>
diff --git a/crypto/cipher/cipher_test.cc b/crypto/cipher/cipher_test.cc index a55b502..692902e 100644 --- a/crypto/cipher/cipher_test.cc +++ b/crypto/cipher/cipher_test.cc
@@ -18,6 +18,7 @@ #include <string.h> #include <algorithm> +#include <cstdint> #include <string> #include <vector> @@ -1417,13 +1418,31 @@ } } -TEST(CipherTest, NonPoisoningErrors) { +TEST(CipherTest, RetryAfterError) { 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; + + // Compute a sample plaintext/ciphertext pair. + uint8_t plaintext[40]; + for (size_t i = 0; i < sizeof(plaintext); i++) { + plaintext[i] = static_cast<uint8_t>(i); + } + uint8_t ciphertext[48]; + { + 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)); + size_t len1; + ASSERT_TRUE(EVP_EncryptUpdate_ex(ctx.get(), ciphertext, &len1, + sizeof(ciphertext), plaintext, + sizeof(plaintext))); + size_t len2; + ASSERT_TRUE(EVP_EncryptFinal_ex2(ctx.get(), ciphertext + len1, &len2, + sizeof(ciphertext) - len1)); + ASSERT_EQ(sizeof(ciphertext), len1 + len2); + } // EncryptUpdate output buffer size error does not poison the context. { @@ -1431,16 +1450,50 @@ 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))); + // Call EVP_EncryptUpdate_ex with too small of a buffer. + uint8_t out[sizeof(ciphertext)]; + size_t out_len; + EXPECT_FALSE(EVP_EncryptUpdate_ex(ctx.get(), out, &out_len, 0, plaintext, + sizeof(plaintext))); + ASSERT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_CIPHER, + CIPHER_R_BUFFER_TOO_SMALL)); + EXPECT_FALSE(EVP_EncryptUpdate_ex(ctx.get(), out, &out_len, + 31 /* one byte too short */, plaintext, + sizeof(plaintext))); + ASSERT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_CIPHER, + CIPHER_R_BUFFER_TOO_SMALL)); + // A subsequent call with sufficient output buffer succeeds. + ASSERT_TRUE(EVP_EncryptUpdate_ex(ctx.get(), out, &out_len, sizeof(out), + plaintext, sizeof(plaintext))); + EXPECT_EQ(Bytes(out, out_len), Bytes(ciphertext, out_len)); + } + + // Same as above, but with some buffered plaintext. + { + 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)); + // Call EVP_EncryptUpdate_ex with too small of a buffer. + uint8_t out[sizeof(ciphertext)]; + size_t out_len; + ASSERT_TRUE( + EVP_EncryptUpdate_ex(ctx.get(), out, &out_len, 0, plaintext, 5)); + EXPECT_EQ(out_len, 0u); + auto rest = Span(plaintext).subspan(5); + EXPECT_FALSE(EVP_EncryptUpdate_ex(ctx.get(), out, &out_len, 0, rest.data(), + rest.size())); 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))); + EXPECT_FALSE(EVP_EncryptUpdate_ex(ctx.get(), out, &out_len, + 31 /* one byte too short */, rest.data(), + rest.size())); + EXPECT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_CIPHER, + CIPHER_R_BUFFER_TOO_SMALL)); + // A subsequent call with sufficient output buffer succeeds. + ASSERT_TRUE(EVP_EncryptUpdate_ex(ctx.get(), out, &out_len, sizeof(out), + rest.data(), rest.size())); + EXPECT_EQ(Bytes(out, out_len), Bytes(ciphertext, out_len)); } // EncryptFinal output buffer size error does not poison the context. @@ -1449,16 +1502,22 @@ 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)); + uint8_t out[sizeof(ciphertext)]; + size_t len1, len2; + ASSERT_TRUE(EVP_EncryptUpdate_ex(ctx.get(), out, &len1, sizeof(out), + plaintext, sizeof(plaintext))); + // Call EVP_EncryptFinal_ex2 with too small of a buffer. + EXPECT_FALSE(EVP_EncryptFinal_ex2(ctx.get(), out + len1, &len2, 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))); + EXPECT_FALSE(EVP_EncryptFinal_ex2(ctx.get(), out + len1, &len2, + sizeof(out) - len1 - 1)); + EXPECT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_CIPHER, + CIPHER_R_BUFFER_TOO_SMALL)); + // A subsequent call with sufficient output buffer succeeds. + ASSERT_TRUE( + EVP_EncryptFinal_ex2(ctx.get(), out + len1, &len2, sizeof(out) - len1)); + EXPECT_EQ(Bytes(out + len1, len2), Bytes(Span(ciphertext).subspan(len1))); } // DecryptUpdate output buffer size error does not poison the context. @@ -1467,50 +1526,46 @@ 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))); + uint8_t out[sizeof(plaintext)]; + size_t out_len; + // Call EVP_DecryptUpdate_ex with too small of a buffer. + EXPECT_FALSE(EVP_DecryptUpdate_ex(ctx.get(), out, &out_len, 0, ciphertext, + sizeof(ciphertext))); 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))); + EXPECT_FALSE(EVP_DecryptUpdate_ex(ctx.get(), out, &out_len, + 31 /* one byte too short */, ciphertext, + sizeof(ciphertext))); + EXPECT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_CIPHER, + CIPHER_R_BUFFER_TOO_SMALL)); + // A subsequent call with sufficient output buffer succeeds. + ASSERT_TRUE(EVP_DecryptUpdate_ex(ctx.get(), out, &out_len, sizeof(out), + ciphertext, sizeof(ciphertext))); + EXPECT_EQ(Bytes(out, out_len), Bytes(plaintext, out_len)); } // 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)); + uint8_t out[sizeof(plaintext)]; + size_t len1, len2; + ASSERT_TRUE(EVP_DecryptUpdate_ex(ctx.get(), out, &len1, sizeof(out), + ciphertext, sizeof(ciphertext))); + // Calling EVP_DecryptFinal_ex2 with too small of a buffer. + EXPECT_FALSE(EVP_DecryptFinal_ex2(ctx.get(), out + len1, &len2, 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_DecryptFinal_ex2(ctx.get(), out, &out_len, sizeof(out))); - EXPECT_EQ(out_len, 10u); + EXPECT_FALSE(EVP_DecryptFinal_ex2(ctx.get(), out + len1, &len2, + sizeof(out) - len1 - 1)); + EXPECT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_CIPHER, + CIPHER_R_BUFFER_TOO_SMALL)); + // A subsequent call with sufficient output buffer succeeds. + ASSERT_TRUE( + EVP_DecryptFinal_ex2(ctx.get(), out + len1, &len2, sizeof(out) - len1)); + EXPECT_EQ(Bytes(out + len1, len2), Bytes(Span(plaintext).subspan(len1))); } // DecryptFinal padding failure does not poison the context. @@ -1520,9 +1575,10 @@ // 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). + // itself" two calls later). See b/504728350. { uint8_t bad_ciphertext[16] = {0}; + uint8_t out[16]; int len; bssl::UniquePtr<EVP_CIPHER_CTX> ctx(EVP_CIPHER_CTX_new()); @@ -1536,100 +1592,12 @@ 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. + // Subsequent calls continue to return CIPHER_R_BAD_DECRYPT. 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 bda33dc..56d9501 100644 --- a/crypto/fipsmodule/cipher/cipher.cc.inc +++ b/crypto/fipsmodule/cipher/cipher.cc.inc
@@ -66,11 +66,6 @@ return 0; } - if (in->poisoned) { - OPENSSL_PUT_ERROR(CIPHER, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); - return 0; - } - EVP_CIPHER_CTX_cleanup(out); OPENSSL_memcpy(out, in, sizeof(EVP_CIPHER_CTX)); @@ -190,9 +185,6 @@ ctx->buf_len = 0; ctx->final_used = 0; - // Clear the poisoned flag to permit reuse of a CTX that previously had a - // failed operation. - ctx->poisoned = false; return 1; } @@ -215,6 +207,47 @@ return len & (ctx->cipher->block_size - 1); } +static size_t evp_cipher_ctx_max_encrypt_update(const EVP_CIPHER_CTX *ctx, + size_t in_len) { + if (in_len == 0) { + return 0; + } + + size_t block_size = ctx->cipher->block_size; + // `block_size` must be a power of 2. + assert(block_size != 0 && (block_size & (block_size - 1)) == 0); + size_t buf_len = ctx->buf_len; + + // Any buffered input is combined with `in_len`, then we round down to a + // multiple of the block size. + return (in_len + buf_len) & ~(block_size - 1); +} + +size_t EVP_CIPHER_CTX_max_next_update(const EVP_CIPHER_CTX *ctx, + size_t in_len) { + if (in_len == 0) { + return 0; + } + + size_t ret = evp_cipher_ctx_max_encrypt_update(ctx, in_len); + size_t block_size = ctx->cipher->block_size; + size_t buf_len = ctx->buf_len; + if (!ctx->encrypt && block_size > 1 && !(ctx->flags & EVP_CIPH_NO_PADDING)) { + if (ctx->final_used) { + // There was a buffered decrypted block. Now that it is known to not have + // padding, DecryptUpdate will output it. + ret += block_size; + } + if (block_remainder(ctx, in_len + buf_len) == 0) { + // This call ends on a block boundary. The last block will be buffered in + // `ctx->final` until it is known to have padding. + assert(ret >= block_size); + ret -= block_size; + } + } + return ret; +} + int EVP_EncryptUpdate(EVP_CIPHER_CTX *ctx, uint8_t *out, int *out_len, const uint8_t *in, int in_len) { *out_len = 0; @@ -242,33 +275,16 @@ return 1; } -template <typename F> -static int WrapWithPoison(EVP_CIPHER_CTX *ctx, F f) { - if (ctx->poisoned) { - // `ctx` has been left in an indeterminate state by a previous failed - // operation. Do not allow proceeding. - 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 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; -} - -static int EVP_EncryptUpdate_ex_internal(EVP_CIPHER_CTX *ctx, uint8_t *out, - size_t *out_len, size_t max_out_len, - const uint8_t *in, size_t in_len) { +int EVP_EncryptUpdate_ex(EVP_CIPHER_CTX *ctx, uint8_t *out, size_t *out_len, + size_t max_out_len, const uint8_t *in, size_t in_len) { *out_len = 0; - if (max_out_len < EVP_CIPHER_CTX_max_next_update(ctx, in_len)) { + // `evp_cipher_ctx_max_encrypt_update` is currently exact. Check bounds before + // performing any operations so that, if the output is too small, retrying the + // operation with a larger buffer is well-defined. This only matters when it + // takes two steps to process `in`. + size_t expected = evp_cipher_ctx_max_encrypt_update(ctx, in_len); + if (max_out_len < expected) { OPENSSL_PUT_ERROR(CIPHER, CIPHER_R_BUFFER_TOO_SMALL); return 0; } @@ -288,7 +304,6 @@ 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 { @@ -298,7 +313,6 @@ 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; @@ -314,7 +328,6 @@ 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; @@ -325,22 +338,13 @@ 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(); + assert(*out_len == expected); // See `EVP_CIPHER_CTX_max_next_update` check. return 1; } -int EVP_EncryptUpdate_ex(EVP_CIPHER_CTX *ctx, uint8_t *out, size_t *out_len, - size_t max_out_len, const uint8_t *in, size_t in_len) { - *out_len = 0; - return WrapWithPoison(ctx, [&] { - return EVP_EncryptUpdate_ex_internal(ctx, out, out_len, max_out_len, in, - in_len); - }); -} - int EVP_EncryptFinal_ex(EVP_CIPHER_CTX *ctx, uint8_t *out, int *out_len) { size_t out_len_sz; int ret = @@ -350,22 +354,17 @@ return ret; } -static int EVP_EncryptFinal_ex2_internal(EVP_CIPHER_CTX *ctx, uint8_t *out, - size_t *out_len, size_t max_out_len) { +int EVP_EncryptFinal_ex2(EVP_CIPHER_CTX *ctx, uint8_t *out, 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); } + EVP_Cipher_verify_service_indicator(ctx); return 1; } @@ -375,41 +374,30 @@ OPENSSL_PUT_ERROR(CIPHER, CIPHER_R_DATA_NOT_MULTIPLE_OF_BLOCK_LENGTH); return 0; } + EVP_Cipher_verify_service_indicator(ctx); return 1; } - size_t padding = block_size - buf_len; - for (size_t i = buf_len; i < block_size; i++) { - ctx->buf[i] = padding; - } - Span<uint8_t> out_span(out, max_out_len); if (out_span.size() < block_size) { OPENSSL_PUT_ERROR(CIPHER, CIPHER_R_BUFFER_TOO_SMALL); return 0; } - ctx->mutated = true; + + size_t padding = block_size - buf_len; + for (size_t i = buf_len; i < block_size; i++) { + ctx->buf[i] = padding; + } if (!ctx->cipher->cipher_update(ctx, out_span.data(), ctx->buf, block_size)) { return 0; } out_span = out_span.subspan(block_size); *out_len = max_out_len - out_span.size(); + EVP_Cipher_verify_service_indicator(ctx); return 1; } -int EVP_EncryptFinal_ex2(EVP_CIPHER_CTX *ctx, uint8_t *out, size_t *out_len, - size_t max_out_len) { - *out_len = 0; - return WrapWithPoison(ctx, [&] { - if (!EVP_EncryptFinal_ex2_internal(ctx, out, out_len, max_out_len)) { - return 0; - } - EVP_Cipher_verify_service_indicator(ctx); - return 1; - }); -} - int EVP_DecryptUpdate(EVP_CIPHER_CTX *ctx, uint8_t *out, int *out_len, const uint8_t *in, int in_len) { *out_len = 0; @@ -437,21 +425,14 @@ return 1; } -static int EVP_DecryptUpdate_ex_internal(EVP_CIPHER_CTX *ctx, uint8_t *out, - size_t *out_len, size_t max_out_len, - const uint8_t *in, size_t in_len) { +int EVP_DecryptUpdate_ex(EVP_CIPHER_CTX *ctx, uint8_t *out, size_t *out_len, + size_t max_out_len, 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); size_t block_size = ctx->cipher->block_size; - if (in_span.empty()) { return 1; } @@ -459,9 +440,18 @@ Span<uint8_t> out_span(out, max_out_len); if (ctx->flags & EVP_CIPH_NO_PADDING) { // Use the shared block handling logic from encryption. - return EVP_EncryptUpdate_ex_internal(ctx, out_span.data(), out_len, - out_span.size(), in_span.data(), - in_span.size()); + return EVP_EncryptUpdate_ex(ctx, out_span.data(), out_len, out_span.size(), + in_span.data(), in_span.size()); + } + + // `EVP_CIPHER_CTX_max_next_update` is currently exact. Check bounds before + // performing any operations so that, if the output is too small, retrying the + // operation with a larger buffer is well-defined. This only matters when it + // takes two steps to process `in`. + size_t expected = EVP_CIPHER_CTX_max_next_update(ctx, in_len); + if (max_out_len < expected) { + OPENSSL_PUT_ERROR(CIPHER, CIPHER_R_BUFFER_TOO_SMALL); + return 0; } assert(block_size <= sizeof(ctx->final)); @@ -471,7 +461,6 @@ 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); } @@ -492,28 +481,26 @@ // less. size_t head = in_span.size() > block_size ? in_span.size() - block_size : 0; size_t head_out_len; - if (!EVP_EncryptUpdate_ex_internal(ctx, out_span.data(), &head_out_len, - out_span.size(), in_span.data(), head)) { + if (!EVP_EncryptUpdate_ex(ctx, out_span.data(), &head_out_len, + out_span.size(), in_span.data(), head)) { return 0; } in_span = in_span.subspan(head); out_span = out_span.subspan(head_out_len); size_t final_size; - if (!EVP_EncryptUpdate_ex_internal(ctx, ctx->final, &final_size, - sizeof(ctx->final), in_span.data(), - in_span.size())) { + if (!EVP_EncryptUpdate_ex(ctx, ctx->final, &final_size, sizeof(ctx->final), + in_span.data(), in_span.size())) { return 0; } - ctx->mutated = true; ctx->final_used = 1; assert(final_size == block_size); assert(ctx->buf_len == 0); } else { // Buffer will be non-empty. size_t written_out_len; - if (!EVP_EncryptUpdate_ex_internal(ctx, out_span.data(), &written_out_len, - out_span.size(), in_span.data(), - in_span.size())) { + if (!EVP_EncryptUpdate_ex(ctx, out_span.data(), &written_out_len, + out_span.size(), in_span.data(), + in_span.size())) { return 0; } assert(block_size == 1 || ctx->buf_len != 0); @@ -521,17 +508,10 @@ } *out_len = max_out_len - out_span.size(); + assert(*out_len == expected); // See `EVP_CIPHER_CTX_max_next_update` check. return 1; } -int EVP_DecryptUpdate_ex(EVP_CIPHER_CTX *ctx, uint8_t *out, size_t *out_len, - size_t max_out_len, const uint8_t *in, size_t in_len) { - return WrapWithPoison(ctx, [&] { - return EVP_DecryptUpdate_ex_internal(ctx, out, out_len, max_out_len, in, - in_len); - }); -} - int EVP_DecryptFinal_ex(EVP_CIPHER_CTX *ctx, uint8_t *out, int *out_len) { size_t out_len_sz; int ret = @@ -541,23 +521,17 @@ return ret; } -static int EVP_DecryptFinal_ex2_internal(EVP_CIPHER_CTX *ctx, - unsigned char *out, size_t *out_len, - size_t max_out_len) { +int EVP_DecryptFinal_ex2(EVP_CIPHER_CTX *ctx, uint8_t *out, size_t *out_len, + 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); } + EVP_Cipher_verify_service_indicator(ctx); return 1; } @@ -567,6 +541,7 @@ OPENSSL_PUT_ERROR(CIPHER, CIPHER_R_DATA_NOT_MULTIPLE_OF_BLOCK_LENGTH); return 0; } + EVP_Cipher_verify_service_indicator(ctx); return 1; } @@ -576,8 +551,8 @@ } assert(block_size <= sizeof(ctx->final)); - // The following assumes that the ciphertext has been authenticated. - // Otherwise it provides a padding oracle. + // The following assumes that the ciphertext has been authenticated. Otherwise + // it provides a padding oracle. size_t padding = ctx->final[block_size - 1]; if (padding == 0 || padding > block_size) { OPENSSL_PUT_ERROR(CIPHER, CIPHER_R_BAD_DECRYPT); @@ -601,21 +576,10 @@ out_span = out_span.subspan(payload); *out_len = max_out_len - out_span.size(); + EVP_Cipher_verify_service_indicator(ctx); return 1; } -int EVP_DecryptFinal_ex2(EVP_CIPHER_CTX *ctx, unsigned char *out, - size_t *out_len, size_t max_out_len) { - *out_len = 0; - return WrapWithPoison(ctx, [&] { - if (!EVP_DecryptFinal_ex2_internal(ctx, out, out_len, max_out_len)) { - return 0; - } - EVP_Cipher_verify_service_indicator(ctx); - return 1; - }); -} - int EVP_Cipher(EVP_CIPHER_CTX *ctx, uint8_t *out, const uint8_t *in, size_t in_len) { const int kError = @@ -687,14 +651,11 @@ } int EVP_CipherUpdateAAD(EVP_CIPHER_CTX *ctx, const uint8_t *in, size_t in_len) { - return WrapWithPoison(ctx, [&] { - if (!(ctx->cipher->flags & EVP_CIPH_FLAG_CUSTOM_CIPHER)) { - 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); - }); + if (!(ctx->cipher->flags & EVP_CIPH_FLAG_CUSTOM_CIPHER)) { + OPENSSL_PUT_ERROR(CIPHER, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); + return 0; + } + return ctx->cipher->update_aad(ctx, in, in_len); } int EVP_CipherFinal_ex(EVP_CIPHER_CTX *ctx, uint8_t *out, int *out_len) { @@ -714,36 +675,6 @@ } } -size_t EVP_CIPHER_CTX_max_next_update(const EVP_CIPHER_CTX *ctx, - size_t in_len) { - if (in_len == 0) { - return 0; - } - - size_t block_size = ctx->cipher->block_size; - // |block_size| must be a power of 2. - assert(block_size != 0 && (block_size & (block_size - 1)) == 0); - size_t buf_len = ctx->buf_len; - - // Any buffered input is combined with |in_len|, then we round down to a - // multiple of the block size. - size_t ret = (in_len + buf_len) & ~(block_size - 1); - if (!ctx->encrypt && block_size > 1 && !(ctx->flags & EVP_CIPH_NO_PADDING)) { - if (ctx->final_used) { - // There was a buffered decrypted block. Now that it is known to not have - // padding, DecryptUpdate will output it. - ret += block_size; - } - if (block_remainder(ctx, in_len + buf_len) == 0) { - // This call ends on a block boundary. The last block will be buffered in - // |ctx->final| until it is known to have padding. - assert(ret >= block_size); - ret -= block_size; - } - } - return ret; -} - size_t EVP_CIPHER_CTX_max_final(const EVP_CIPHER_CTX *ctx) { size_t block_size = ctx->cipher->block_size; if (block_size == 1 || (ctx->flags & EVP_CIPH_NO_PADDING)) {
diff --git a/include/openssl/cipher.h b/include/openssl/cipher.h index 74738f8..1410f33 100644 --- a/include/openssl/cipher.h +++ b/include/openssl/cipher.h
@@ -760,15 +760,6 @@ int final_used; uint8_t final[EVP_MAX_BLOCK_LENGTH]; // possible final block - - // 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 {