Test, re-document, and deprecate EVP_Cipher.
It would be nice to have a single-shot EVP_CIPHER_CTX API. This function
is not it.
EVP_Cipher is absurd. It's actually just exposing the internal
EVP_CIPHER 'cipher' callback, whose calling convention is extremely
complex. We've currently documented it as a "single-shot" API, but it's
not single-shot either, as it does update cipher state. It just can't
update across block boundaries.
It is particularly bizarre for "custom ciphers", which include AEADs,
which completely changes the return value convention from
bytes_written/-1 to 1/0, but also adds a bunch of magic NULL behaviors:
- out == NULL, in != NULL: supply AAD
- out != NULL, in != NULL: bulk encrypt/decrypt
- out == NULL, in == NULL: compute/check the tag
Moreover, existing code, like OpenSSH, relies on this behavior. To
ensure we don't break it when refactoring EVP_CIPHER internals, capture
the current behavior in tests. But also, no one should be using this in
new code, so deprecate it.
Upstream hasn't quite deprecated it, they now say "Due to the
constraints of the API contract of this function it shouldn't be used in
applications, please consider using EVP_CipherUpdate() and
EVP_CipherFinal_ex() instead."
Bug: 494
Change-Id: Icfe39a8fbbc860b03c9861f4164b7ee8da340216
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55391
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
diff --git a/crypto/cipher_extra/cipher_test.cc b/crypto/cipher_extra/cipher_test.cc
index 8f99b5e..80146fe 100644
--- a/crypto/cipher_extra/cipher_test.cc
+++ b/crypto/cipher_extra/cipher_test.cc
@@ -170,14 +170,16 @@
}
static void TestCipherAPI(const EVP_CIPHER *cipher, Operation op, bool padding,
- bool copy, bool in_place, size_t chunk_size,
- bssl::Span<const uint8_t> key,
+ bool copy, bool in_place, bool use_evp_cipher,
+ size_t chunk_size, bssl::Span<const uint8_t> key,
bssl::Span<const uint8_t> iv,
bssl::Span<const uint8_t> plaintext,
bssl::Span<const uint8_t> ciphertext,
bssl::Span<const uint8_t> aad,
bssl::Span<const uint8_t> tag) {
bool encrypt = op == Operation::kEncrypt;
+ bool is_custom_cipher =
+ EVP_CIPHER_flags(cipher) & EVP_CIPH_FLAG_CUSTOM_CIPHER;
bssl::Span<const uint8_t> in = encrypt ? plaintext : ciphertext;
bssl::Span<const uint8_t> expected = encrypt ? ciphertext : plaintext;
bool is_aead = EVP_CIPHER_mode(cipher) == EVP_CIPH_GCM_MODE;
@@ -227,12 +229,19 @@
while (!aad.empty()) {
size_t todo =
chunk_size == 0 ? aad.size() : std::min(aad.size(), chunk_size);
- int len;
- ASSERT_TRUE(
- EVP_CipherUpdate(ctx.get(), nullptr, &len, aad.data(), todo));
- // Although it doesn't output anything, |EVP_CipherUpdate| should claim to
- // output the input length.
- EXPECT_EQ(len, static_cast<int>(todo));
+ if (use_evp_cipher) {
+ // AEADs always use the "custom cipher" return value convention. Passing a
+ // null output pointer triggers the AAD logic.
+ ASSERT_TRUE(is_custom_cipher);
+ ASSERT_EQ(static_cast<int>(todo),
+ EVP_Cipher(ctx.get(), nullptr, aad.data(), todo));
+ } else {
+ int len;
+ ASSERT_TRUE(EVP_CipherUpdate(ctx.get(), nullptr, &len, aad.data(), todo));
+ // Although it doesn't output anything, |EVP_CipherUpdate| should claim to
+ // output the input length.
+ EXPECT_EQ(len, static_cast<int>(todo));
+ }
aad = aad.subspan(todo);
}
@@ -256,18 +265,47 @@
size_t todo = chunk_size == 0 ? in.size() : std::min(in.size(), chunk_size);
EXPECT_LE(todo, static_cast<size_t>(INT_MAX));
ASSERT_TRUE(MaybeCopyCipherContext(copy, &ctx));
- ASSERT_TRUE(EVP_CipherUpdate(ctx.get(), result.data() + total, &len,
- in.data(), static_cast<int>(todo)));
+ if (use_evp_cipher) {
+ // |EVP_Cipher| sometimes returns the number of bytes written, or -1 on
+ // error, and sometimes 1 or 0, implicitly writing |in_len| bytes.
+ if (is_custom_cipher) {
+ len = EVP_Cipher(ctx.get(), result.data() + total, in.data(), todo);
+ } else {
+ ASSERT_EQ(
+ 1, EVP_Cipher(ctx.get(), result.data() + total, in.data(), todo));
+ len = static_cast<int>(todo);
+ }
+ } else {
+ ASSERT_TRUE(EVP_CipherUpdate(ctx.get(), result.data() + total, &len,
+ in.data(), static_cast<int>(todo)));
+ }
ASSERT_GE(len, 0);
total += static_cast<size_t>(len);
in = in.subspan(todo);
}
if (op == Operation::kInvalidDecrypt) {
- // Invalid padding and invalid tags all appear as a failed
- // |EVP_CipherFinal_ex|.
- EXPECT_FALSE(EVP_CipherFinal_ex(ctx.get(), result.data() + total, &len));
+ if (use_evp_cipher) {
+ // Only the "custom cipher" return value convention can report failures.
+ // Passing all nulls should act like |EVP_CipherFinal_ex|.
+ ASSERT_TRUE(is_custom_cipher);
+ EXPECT_EQ(-1, EVP_Cipher(ctx.get(), nullptr, nullptr, 0));
+ } else {
+ // Invalid padding and invalid tags all appear as a failed
+ // |EVP_CipherFinal_ex|.
+ EXPECT_FALSE(EVP_CipherFinal_ex(ctx.get(), result.data() + total, &len));
+ }
} else {
- ASSERT_TRUE(EVP_CipherFinal_ex(ctx.get(), result.data() + total, &len));
+ if (use_evp_cipher) {
+ if (is_custom_cipher) {
+ // Only the "custom cipher" convention has an |EVP_CipherFinal_ex|
+ // equivalent.
+ len = EVP_Cipher(ctx.get(), nullptr, nullptr, 0);
+ } else {
+ len = 0;
+ }
+ } else {
+ ASSERT_TRUE(EVP_CipherFinal_ex(ctx.get(), result.data() + total, &len));
+ }
ASSERT_GE(len, 0);
total += static_cast<size_t>(len);
result.resize(total);
@@ -379,6 +417,7 @@
bssl::Span<const uint8_t> ciphertext,
bssl::Span<const uint8_t> aad,
bssl::Span<const uint8_t> tag) {
+ size_t block_size = EVP_CIPHER_block_size(cipher);
std::vector<Operation> ops;
if (input_op == Operation::kBoth) {
ops = {Operation::kEncrypt, Operation::kDecrypt};
@@ -400,8 +439,14 @@
SCOPED_TRACE(in_place);
for (bool copy : {false, true}) {
SCOPED_TRACE(copy);
- TestCipherAPI(cipher, op, padding, copy, in_place, chunk_size, key,
- iv, plaintext, ciphertext, aad, tag);
+ TestCipherAPI(cipher, op, padding, copy, in_place,
+ /*use_evp_cipher=*/false, chunk_size, key, iv,
+ plaintext, ciphertext, aad, tag);
+ if (!padding && chunk_size % block_size == 0) {
+ TestCipherAPI(cipher, op, padding, copy, in_place,
+ /*use_evp_cipher=*/true, chunk_size, key, iv,
+ plaintext, ciphertext, aad, tag);
+ }
}
if (!padding) {
TestLowLevelAPI(cipher, op, in_place, chunk_size, key, iv, plaintext,
diff --git a/include/openssl/cipher.h b/include/openssl/cipher.h
index ba4b698..b1876e0 100644
--- a/include/openssl/cipher.h
+++ b/include/openssl/cipher.h
@@ -214,26 +214,6 @@
OPENSSL_EXPORT int EVP_DecryptFinal_ex(EVP_CIPHER_CTX *ctx, uint8_t *out,
int *out_len);
-// EVP_Cipher performs a one-shot encryption/decryption operation for non-AEAD
-// ciphers. No partial blocks are maintained between calls. However, any
-// internal cipher state is still updated. For CBC-mode ciphers, the IV is
-// updated to the final ciphertext block. For stream ciphers, the stream is
-// advanced past the bytes used. It returns one on success and zero otherwise.
-//
-// WARNING: This function behaves completely differently on AEAD ciphers, such
-// as |EVP_aes_128_gcm|. Rather than being a one-shot operation, it behaves like
-// |EVP_CipherUpdate| or |EVP_CipherFinal_ex|, depending on whether |in| is
-// NULL. It also instead returns the number of bytes written or -1 on error.
-// This behavior is deprecated. Use |EVP_CipherUpdate| or |EVP_CipherFinal_ex|
-// instead.
-//
-// TODO(davidben): The normal ciphers currently never fail, even if, e.g.,
-// |in_len| is not a multiple of the block size for CBC-mode decryption. The
-// input just gets rounded up while the output gets truncated. This should
-// either be officially documented or fail.
-OPENSSL_EXPORT int EVP_Cipher(EVP_CIPHER_CTX *ctx, uint8_t *out,
- const uint8_t *in, size_t in_len);
-
// EVP_CipherUpdate calls either |EVP_EncryptUpdate| or |EVP_DecryptUpdate|
// depending on how |ctx| has been setup.
OPENSSL_EXPORT int EVP_CipherUpdate(EVP_CIPHER_CTX *ctx, uint8_t *out,
@@ -432,6 +412,30 @@
OPENSSL_EXPORT int EVP_DecryptFinal(EVP_CIPHER_CTX *ctx, uint8_t *out,
int *out_len);
+// EVP_Cipher historically exposed an internal implementation detail of |ctx|
+// and should not be used. Use |EVP_CipherUpdate| and |EVP_CipherFinal_ex|
+// instead.
+//
+// If |ctx|'s cipher does not have the |EVP_CIPH_FLAG_CUSTOM_CIPHER| flag, it
+// encrypts or decrypts |in_len| bytes from |in| and writes the resulting
+// |in_len| bytes to |out|. It returns one on success and zero on error.
+// |in_len| must be a multiple of the cipher's block size, or the behavior is
+// undefined.
+//
+// TODO(davidben): Rather than being undefined (it'll often round the length up
+// and likely read past the buffer), just fail the operation.
+//
+// If |ctx|'s cipher has the |EVP_CIPH_FLAG_CUSTOM_CIPHER| flag, it runs in one
+// of two modes: If |in| is non-NULL, it behaves like |EVP_CipherUpdate|. If
+// |in| is NULL, it behaves like |EVP_CipherFinal_ex|. In both cases, it returns
+// |*out_len| on success and -1 on error.
+//
+// WARNING: The two possible calling conventions of this function signal errors
+// incompatibly. In the first, zero indicates an error. In the second, zero
+// indicates success with zero bytes of output.
+OPENSSL_EXPORT int EVP_Cipher(EVP_CIPHER_CTX *ctx, uint8_t *out,
+ const uint8_t *in, size_t in_len);
+
// EVP_add_cipher_alias does nothing and returns one.
OPENSSL_EXPORT int EVP_add_cipher_alias(const char *a, const char *b);
