From bb8f301ea65e31357092f38d5c497863a09f8bc7 Mon Sep 17 00:00:00 2001 From: Andrew Hopkins Date: Thu, 11 Jan 2024 15:30:54 -0800 Subject: [PATCH 1/4] Update EVP cipher APIs to gracefully handle null EVP_CIPHER_CTX. Add new safety macro GUARD_PTR --- crypto/cipher_extra/cipher_test.cc | 26 +++++++++++++++++++++++--- crypto/fipsmodule/cipher/cipher.c | 12 ++++++++++-- crypto/internal.h | 22 ++++++++++++++++++++++ 3 files changed, 55 insertions(+), 5 deletions(-) diff --git a/crypto/cipher_extra/cipher_test.cc b/crypto/cipher_extra/cipher_test.cc index efa79096b5..aaac48d53b 100644 --- a/crypto/cipher_extra/cipher_test.cc +++ b/crypto/cipher_extra/cipher_test.cc @@ -1095,8 +1095,8 @@ TEST(CipherTest, GCMIncrementingIV) { bool enc) { // Make a reference ciphertext. bssl::ScopedEVP_CIPHER_CTX ref; - ASSERT_TRUE(EVP_EncryptInit_ex(ref.get(), kCipher, /*impl=*/nullptr, - kKey, /*iv=*/nullptr)); + ASSERT_TRUE(EVP_EncryptInit_ex(ref.get(), kCipher, /*impl=*/nullptr, kKey, + /*iv=*/nullptr)); ASSERT_TRUE(EVP_CIPHER_CTX_ctrl(ref.get(), EVP_CTRL_AEAD_SET_IVLEN, static_cast(iv.size()), nullptr)); ASSERT_TRUE(EVP_EncryptInit_ex(ref.get(), /*cipher=*/nullptr, @@ -1376,7 +1376,7 @@ TEST(CipherTest, GCMIncrementingIV) { ASSERT_NO_FATAL_FAILURE(expect_iv(ctx.get(), iv, /*enc=*/true)); } - { + { // Same as above, but with a larger IV. const uint8_t kFixedIV[8] = {1, 2, 3, 4, 5, 6, 7, 8}; bssl::ScopedEVP_CIPHER_CTX ctx; @@ -1405,3 +1405,23 @@ TEST(CipherTest, GCMIncrementingIV) { ASSERT_NO_FATAL_FAILURE(expect_iv(ctx.get(), iv, /*enc=*/true)); } } + +#define CHECK_ERROR(function, err) \ + ERR_clear_error(); \ + EXPECT_FALSE(function); \ + EXPECT_EQ(err, ERR_GET_REASON(ERR_peek_last_error())); + +TEST(CipherTest, Empty_EVP_CIPHER_CTX_V1187459157) { + int in_len = 10; + std::vector in_vec(in_len); + int out_len = in_len + 256; + std::vector out_vec(out_len); + + CHECK_ERROR(EVP_EncryptUpdate(nullptr, out_vec.data(), &out_len, in_vec.data(), in_len), ERR_R_PASSED_NULL_PARAMETER); + + bssl::UniquePtr ctx(EVP_CIPHER_CTX_new()); + CHECK_ERROR(EVP_EncryptUpdate(ctx.get(), out_vec.data(), &out_len, in_vec.data(), in_len), ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); + CHECK_ERROR(EVP_EncryptFinal(ctx.get(), out_vec.data(), &out_len), ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); + CHECK_ERROR(EVP_DecryptUpdate(ctx.get(), out_vec.data(), &out_len, in_vec.data(), in_len), ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); + CHECK_ERROR(EVP_DecryptFinal(ctx.get(), out_vec.data(), &out_len), ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); +} diff --git a/crypto/fipsmodule/cipher/cipher.c b/crypto/fipsmodule/cipher/cipher.c index 0184f5e553..a96df09f97 100644 --- a/crypto/fipsmodule/cipher/cipher.c +++ b/crypto/fipsmodule/cipher/cipher.c @@ -70,6 +70,7 @@ void EVP_CIPHER_CTX_init(EVP_CIPHER_CTX *ctx) { OPENSSL_memset(ctx, 0, sizeof(EVP_CIPHER_CTX)); + ctx->poisoned = 1; } EVP_CIPHER_CTX *EVP_CIPHER_CTX_new(void) { @@ -255,6 +256,7 @@ static int block_remainder(const EVP_CIPHER_CTX *ctx, int len) { int EVP_EncryptUpdate(EVP_CIPHER_CTX *ctx, uint8_t *out, int *out_len, const uint8_t *in, int in_len) { + GUARD_PTR(ctx); if (ctx->poisoned) { OPENSSL_PUT_ERROR(CIPHER, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); return 0; @@ -266,6 +268,7 @@ int EVP_EncryptUpdate(EVP_CIPHER_CTX *ctx, uint8_t *out, int *out_len, // Ciphers that use blocks may write up to |bl| extra bytes. Ensure the output // does not overflow |*out_len|. + GUARD_PTR(ctx->cipher); int bl = ctx->cipher->block_size; if (bl > 1 && in_len > INT_MAX - bl) { OPENSSL_PUT_ERROR(CIPHER, ERR_R_OVERFLOW); @@ -347,12 +350,13 @@ int EVP_EncryptUpdate(EVP_CIPHER_CTX *ctx, uint8_t *out, int *out_len, int EVP_EncryptFinal_ex(EVP_CIPHER_CTX *ctx, uint8_t *out, int *out_len) { int n; unsigned int i, b, bl; + GUARD_PTR(ctx); if (ctx->poisoned) { OPENSSL_PUT_ERROR(CIPHER, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); return 0; } - + GUARD_PTR(ctx->cipher); if (ctx->cipher->flags & EVP_CIPH_FLAG_CUSTOM_CIPHER) { // When EVP_CIPH_FLAG_CUSTOM_CIPHER is set, the return value of |cipher| is // the number of bytes written, or -1 on error. Otherwise the return value @@ -398,13 +402,16 @@ int EVP_EncryptFinal_ex(EVP_CIPHER_CTX *ctx, uint8_t *out, int *out_len) { int EVP_DecryptUpdate(EVP_CIPHER_CTX *ctx, uint8_t *out, int *out_len, const uint8_t *in, int in_len) { + GUARD_PTR(ctx); if (ctx->poisoned) { OPENSSL_PUT_ERROR(CIPHER, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); return 0; } + // Ciphers that use blocks may write up to |bl| extra bytes. Ensure the output // does not overflow |*out_len|. + GUARD_PTR(ctx->cipher); unsigned int b = ctx->cipher->block_size; if (b > 1 && in_len > INT_MAX - (int)b) { OPENSSL_PUT_ERROR(CIPHER, ERR_R_OVERFLOW); @@ -464,6 +471,7 @@ int EVP_DecryptFinal_ex(EVP_CIPHER_CTX *ctx, unsigned char *out, int *out_len) { int i, n; unsigned int b; *out_len = 0; + GUARD_PTR(ctx); // |ctx->cipher->cipher| calls the static aes encryption function way under // the hood instead of |EVP_Cipher|, so the service indicator does not need @@ -473,7 +481,7 @@ int EVP_DecryptFinal_ex(EVP_CIPHER_CTX *ctx, unsigned char *out, int *out_len) { OPENSSL_PUT_ERROR(CIPHER, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); return 0; } - + GUARD_PTR(ctx->cipher); if (ctx->cipher->flags & EVP_CIPH_FLAG_CUSTOM_CIPHER) { i = ctx->cipher->cipher(ctx, out, NULL, 0); if (i < 0) { diff --git a/crypto/internal.h b/crypto/internal.h index 6791f4a25e..82e846425a 100644 --- a/crypto/internal.h +++ b/crypto/internal.h @@ -1155,6 +1155,28 @@ OPENSSL_EXPORT int OPENSSL_vasprintf_internal(char **str, const char *format, va_list args, int system_malloc) OPENSSL_PRINTF_FORMAT_FUNC(2, 0); + +// Safety Macros + +// Inspired by s2n-tls + +// __AWS_LC_ENSURE checks if |cond| is true nothing happens, else |action| is called +#define __AWS_LC_ENSURE(cond, action) \ + do { \ + if (!(cond)) { \ + action; \ + } \ + } while (0) + +#define AWS_LC_ERROR 0 +#define AWS_LC_SUCCESS 1 + +// RESULT_GUARD_PTR checks if |ptr|, if it is null it adds ERR_R_PASSED_NULL_PARAMETER +// to the error queue and returns 0. NOTE: this macro should only be used with +// functions that return 0 (for error) and 1 (for success). +#define GUARD_PTR(ptr) __AWS_LC_ENSURE((ptr) != NULL, OPENSSL_PUT_ERROR(CRYPTO, ERR_R_PASSED_NULL_PARAMETER); \ + return AWS_LC_ERROR) + #if defined(__cplusplus) } // extern C #endif From 40658d7273d3e39f62e6cd2f48f9c268a3be2e01 Mon Sep 17 00:00:00 2001 From: Andrew Hopkins Date: Fri, 12 Jan 2024 11:28:02 -0800 Subject: [PATCH 2/4] Staging pr comments --- crypto/fipsmodule/cipher/cipher.c | 4 ++++ crypto/internal.h | 9 +++++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/crypto/fipsmodule/cipher/cipher.c b/crypto/fipsmodule/cipher/cipher.c index a96df09f97..c05fdf3bc7 100644 --- a/crypto/fipsmodule/cipher/cipher.c +++ b/crypto/fipsmodule/cipher/cipher.c @@ -540,6 +540,8 @@ int EVP_DecryptFinal_ex(EVP_CIPHER_CTX *ctx, unsigned char *out, int *out_len) { int EVP_Cipher(EVP_CIPHER_CTX *ctx, uint8_t *out, const uint8_t *in, size_t in_len) { + GUARD_PTR(ctx); + GUARD_PTR(ctx->cipher); const int ret = ctx->cipher->cipher(ctx, out, in, in_len); // |EVP_CIPH_FLAG_CUSTOM_CIPHER| never sets the FIPS indicator via @@ -562,6 +564,7 @@ int EVP_Cipher(EVP_CIPHER_CTX *ctx, uint8_t *out, const uint8_t *in, int EVP_CipherUpdate(EVP_CIPHER_CTX *ctx, uint8_t *out, int *out_len, const uint8_t *in, int in_len) { + GUARD_PTR(ctx); if (ctx->encrypt) { return EVP_EncryptUpdate(ctx, out, out_len, in, in_len); } else { @@ -570,6 +573,7 @@ int EVP_CipherUpdate(EVP_CIPHER_CTX *ctx, uint8_t *out, int *out_len, } int EVP_CipherFinal_ex(EVP_CIPHER_CTX *ctx, uint8_t *out, int *out_len) { + GUARD_PTR(ctx); if (ctx->encrypt) { return EVP_EncryptFinal_ex(ctx, out, out_len); } else { diff --git a/crypto/internal.h b/crypto/internal.h index 82e846425a..3cc967f1f1 100644 --- a/crypto/internal.h +++ b/crypto/internal.h @@ -1156,7 +1156,7 @@ OPENSSL_EXPORT int OPENSSL_vasprintf_internal(char **str, const char *format, OPENSSL_PRINTF_FORMAT_FUNC(2, 0); -// Safety Macros +// Experimental Safety Macros // Inspired by s2n-tls @@ -1171,9 +1171,10 @@ OPENSSL_EXPORT int OPENSSL_vasprintf_internal(char **str, const char *format, #define AWS_LC_ERROR 0 #define AWS_LC_SUCCESS 1 -// RESULT_GUARD_PTR checks if |ptr|, if it is null it adds ERR_R_PASSED_NULL_PARAMETER -// to the error queue and returns 0. NOTE: this macro should only be used with -// functions that return 0 (for error) and 1 (for success). +// RESULT_GUARD_PTR checks |ptr|: if it is null it adds ERR_R_PASSED_NULL_PARAMETER +// to the error queue and returns 0, if it is not null nothing happens. +// NOTE: this macro should only be used with functions that return 0 (for error) +// and 1 (for success). #define GUARD_PTR(ptr) __AWS_LC_ENSURE((ptr) != NULL, OPENSSL_PUT_ERROR(CRYPTO, ERR_R_PASSED_NULL_PARAMETER); \ return AWS_LC_ERROR) From 4c0a39b273302d9ab2ded5733fa9a010045ae87d Mon Sep 17 00:00:00 2001 From: Andrew Hopkins Date: Thu, 4 Apr 2024 18:40:20 -0700 Subject: [PATCH 3/4] Poison more spots --- crypto/fipsmodule/cipher/cipher.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crypto/fipsmodule/cipher/cipher.c b/crypto/fipsmodule/cipher/cipher.c index c05fdf3bc7..3017da52cd 100644 --- a/crypto/fipsmodule/cipher/cipher.c +++ b/crypto/fipsmodule/cipher/cipher.c @@ -76,6 +76,7 @@ void EVP_CIPHER_CTX_init(EVP_CIPHER_CTX *ctx) { EVP_CIPHER_CTX *EVP_CIPHER_CTX_new(void) { EVP_CIPHER_CTX *ctx = OPENSSL_zalloc(sizeof(EVP_CIPHER_CTX)); if (ctx) { + ctx->poisoned = 1; // NO-OP: struct already zeroed // EVP_CIPHER_CTX_init(ctx); } @@ -89,6 +90,7 @@ int EVP_CIPHER_CTX_cleanup(EVP_CIPHER_CTX *c) { OPENSSL_free(c->cipher_data); OPENSSL_memset(c, 0, sizeof(EVP_CIPHER_CTX)); + c->poisoned = 1; return 1; } From b95d0d53a48533e245bc83eb3528ef2f2b934fd8 Mon Sep 17 00:00:00 2001 From: Andrew Hopkins Date: Tue, 9 Apr 2024 14:12:57 -0700 Subject: [PATCH 4/4] Add more checks in other functions --- crypto/fipsmodule/cipher/cipher.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crypto/fipsmodule/cipher/cipher.c b/crypto/fipsmodule/cipher/cipher.c index 3017da52cd..579a82d466 100644 --- a/crypto/fipsmodule/cipher/cipher.c +++ b/crypto/fipsmodule/cipher/cipher.c @@ -84,6 +84,7 @@ EVP_CIPHER_CTX *EVP_CIPHER_CTX_new(void) { } int EVP_CIPHER_CTX_cleanup(EVP_CIPHER_CTX *c) { + GUARD_PTR(c); if (c->cipher != NULL && c->cipher->cleanup) { c->cipher->cleanup(c); } @@ -111,6 +112,7 @@ int EVP_CIPHER_CTX_copy(EVP_CIPHER_CTX *out, const EVP_CIPHER_CTX *in) { OPENSSL_PUT_ERROR(CIPHER, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); return 0; } + GUARD_PTR(out); EVP_CIPHER_CTX_cleanup(out); OPENSSL_memcpy(out, in, sizeof(EVP_CIPHER_CTX)); @@ -142,6 +144,7 @@ int EVP_CIPHER_CTX_reset(EVP_CIPHER_CTX *ctx) { int EVP_CipherInit_ex(EVP_CIPHER_CTX *ctx, const EVP_CIPHER *cipher, ENGINE *engine, const uint8_t *key, const uint8_t *iv, int enc) { + GUARD_PTR(ctx); if (enc == -1) { enc = ctx->encrypt; } else {