From 40b7cbca10c4fb044670afa1b3f079903417cfdf Mon Sep 17 00:00:00 2001 From: Viktor Dukhovni Date: Thu, 22 May 2025 01:33:03 +1000 Subject: [PATCH] Note finished state in cipher BIO EOF When the cipher BIO encounters a non-retriable EOF (or error), mark the state as "finished", else a subsequent BIO_flush() or attempted read may attempt to finalise the crypto state again, and fail, leading, for example, to users seeing erroneous apparent decryption failure. This is not a new problem, the fix should be backported to all supported releases. It was made more apparent by recent changes to the base64 BIO, which returns a non-retriable EOF when padding is seen at the end of the base64 data, even if the underlying next BIO is "retriable". Reviewed-by: Matt Caswell Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/27680) (cherry picked from commit 005fa3e00e1ccfd83aa99d28e2eff55597dd5fc2) --- crypto/evp/bio_enc.c | 1 + test/bio_enc_test.c | 71 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+) diff --git a/crypto/evp/bio_enc.c b/crypto/evp/bio_enc.c index ffe4b5bb02e..b0889fe0bb1 100644 --- a/crypto/evp/bio_enc.c +++ b/crypto/evp/bio_enc.c @@ -161,6 +161,7 @@ static int enc_read(BIO *b, char *out, int outl) /* Should be continue next time we are called? */ if (!BIO_should_retry(next)) { ctx->cont = i; + ctx->finished = 1; i = EVP_CipherFinal_ex(ctx->cipher, ctx->buf, &(ctx->buf_len)); ctx->ok = i; diff --git a/test/bio_enc_test.c b/test/bio_enc_test.c index b12cf9c38bf..df16341fcb5 100644 --- a/test/bio_enc_test.c +++ b/test/bio_enc_test.c @@ -139,7 +139,17 @@ static int do_bio_cipher(const EVP_CIPHER* cipher, const unsigned char* key, if (!TEST_ptr(mem)) goto err; BIO_push(b, mem); +#if 0 + /* + * This is wrong to do, it always fails, and incorrectly ends up + * calling `EVP_CipherFinal()` and setting ctx->finished = 1, ... + * all of which are unwanted. But just deleting this is less + * instructive to future readers of the code. Don't call BIO_flush + * until you're done either reading or writing and want to finalise + * the state. + */ (void)BIO_flush(b); +#endif memset(out, 0, sizeof(out)); len = BIO_read(b, out, sizeof(out)); BIO_free_all(b); @@ -256,6 +266,66 @@ static int test_bio_enc_chacha20_poly1305(int idx) # endif # endif +static int test_bio_enc_eof_read_flush(void) +{ + /* Length chosen to ensure base64 encoding employs padding */ + const unsigned char pbuf[] = "Attack at dawn"; + unsigned char cbuf[16]; /* At least as long as pbuf */ + const EVP_CIPHER *cipher = EVP_aes_256_gcm(); + EVP_CIPHER_CTX *ctx = NULL; + BIO *mem = NULL, *b64 = NULL, *cbio = NULL; + unsigned char tag[16]; + size_t key_size, iv_size; + int n, ret = 0; + + memset(tag, 0, sizeof(tag)); + if (!TEST_ptr(cipher) + || !TEST_int_gt((key_size = EVP_CIPHER_key_length(cipher)), 0) + || !TEST_int_gt((iv_size = EVP_CIPHER_iv_length(cipher)), 0) + || !TEST_ptr(mem = BIO_new(BIO_s_mem())) + || !TEST_ptr(b64 = BIO_new(BIO_f_base64())) + || !TEST_ptr(cbio = BIO_new(BIO_f_cipher())) + || !TEST_ptr(BIO_push(b64, mem)) + || !TEST_ptr(BIO_push(cbio, b64)) + || !TEST_int_gt(BIO_get_cipher_ctx(cbio, &ctx), 0) + || !TEST_true(EVP_CipherInit_ex(ctx, cipher, NULL, KEY, IV, ENCRYPT)) + || !TEST_int_gt(BIO_write(cbio, pbuf, sizeof(pbuf) - 1), 0) + || !TEST_int_gt(BIO_flush(cbio), 0) + || !TEST_int_gt(EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_GCM_GET_TAG, + sizeof(tag), tag), 0)) + goto end; + BIO_free(cbio); + BIO_free(b64); + b64 = cbio = NULL; + + BIO_set_mem_eof_return(mem, 0); + BIO_set_flags(mem, BIO_FLAGS_NONCLEAR_RST); + if (!TEST_int_gt(BIO_reset(mem), 0) + || !TEST_ptr(b64 = BIO_new(BIO_f_base64())) + || !TEST_ptr(cbio = BIO_new(BIO_f_cipher())) + || !TEST_ptr(BIO_push(b64, mem)) + || !TEST_ptr(BIO_push(cbio, b64)) + || !TEST_int_gt(BIO_get_cipher_ctx(cbio, &ctx), 0) + || !TEST_true(EVP_CipherInit_ex(ctx, cipher, NULL, KEY, IV, DECRYPT)) + || !TEST_int_gt(EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_GCM_SET_TAG, + sizeof(tag), tag), 0) + || !TEST_int_gt((n = BIO_read(cbio, cbuf, sizeof(cbuf))), 0) + || !TEST_true(BIO_get_cipher_status(cbio)) + /* Evaluate both and report whether either or both failed */ + || (!TEST_int_gt(BIO_flush(cbio), 0) + + !TEST_true(BIO_get_cipher_status(cbio))) + || !TEST_mem_eq(cbuf, n, pbuf, sizeof(pbuf) - 1)) + goto end; + + ret = 1; + + end: + BIO_free(cbio); + BIO_free(b64); + BIO_free(mem); + return ret; +} + int setup_tests(void) { ADD_ALL_TESTS(test_bio_enc_aes_128_cbc, 2); @@ -268,5 +338,6 @@ int setup_tests(void) ADD_ALL_TESTS(test_bio_enc_chacha20_poly1305, 2); # endif # endif + ADD_TEST(test_bio_enc_eof_read_flush); return 1; } -- 2.47.2