Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update EVP cipher APIs to gracefully handle null EVP_CIPHER_CTX #1398

Merged
merged 4 commits into from
Apr 16, 2024
Merged

Update EVP cipher APIs to gracefully handle null EVP_CIPHER_CTX #1398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bb8f301
Update EVP cipher APIs to gracefully handle null EVP_CIPHER_CTX. Add …
andrewhop Jan 11, 2024
40658d7
Staging pr comments
andrewhop Jan 12, 2024
4c0a39b
Poison more spots
andrewhop Apr 5, 2024
b95d0d5
Add more checks in other functions
andrewhop Apr 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions crypto/cipher_extra/cipher_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<int>(iv.size()), nullptr));
ASSERT_TRUE(EVP_EncryptInit_ex(ref.get(), /*cipher=*/nullptr,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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<uint8_t> in_vec(in_len);
int out_len = in_len + 256;
std::vector<uint8_t> 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<EVP_CIPHER_CTX> 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);
}
21 changes: 19 additions & 2 deletions crypto/fipsmodule/cipher/cipher.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,24 +70,28 @@

void EVP_CIPHER_CTX_init(EVP_CIPHER_CTX *ctx) {
OPENSSL_memset(ctx, 0, sizeof(EVP_CIPHER_CTX));
ctx->poisoned = 1;
justsmth marked this conversation as resolved.
Show resolved Hide resolved
}

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);
}
return ctx;
}

int EVP_CIPHER_CTX_cleanup(EVP_CIPHER_CTX *c) {
GUARD_PTR(c);
if (c->cipher != NULL && c->cipher->cleanup) {
c->cipher->cleanup(c);
}
OPENSSL_free(c->cipher_data);

OPENSSL_memset(c, 0, sizeof(EVP_CIPHER_CTX));
c->poisoned = 1;
return 1;
}

Expand All @@ -108,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));
Expand Down Expand Up @@ -139,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 {
Expand Down Expand Up @@ -255,6 +261,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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why there are no guards for CipherInit_ex, CipherUpdate and CipherFinal_ex? ctx is accessed there right away.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still see CipherInit_ex not having a GUARD_PTR, is there a reason for that?

aws-lc/crypto/fipsmodule/cipher/cipher.c

Line 142 in 4c0a39b

int EVP_CipherInit_ex(EVP_CIPHER_CTX *ctx, const EVP_CIPHER *cipher,

if (ctx->poisoned) {
OPENSSL_PUT_ERROR(CIPHER, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
return 0;
Expand All @@ -266,6 +273,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);
Expand Down Expand Up @@ -347,12 +355,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
Expand Down Expand Up @@ -398,13 +407,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);
Expand Down Expand Up @@ -464,6 +476,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
Expand All @@ -473,7 +486,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) {
Expand Down Expand Up @@ -532,6 +545,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
Expand All @@ -554,6 +569,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 {
Expand All @@ -562,6 +578,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 {
Expand Down
23 changes: 23 additions & 0 deletions crypto/internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1155,6 +1155,29 @@ OPENSSL_EXPORT int OPENSSL_vasprintf_internal(char **str, const char *format,
va_list args, int system_malloc)
OPENSSL_PRINTF_FORMAT_FUNC(2, 0);


// Experimental 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) \
andrewhop marked this conversation as resolved.
Show resolved Hide resolved
do { \
if (!(cond)) { \
action; \
} \
} while (0)

#define AWS_LC_ERROR 0
#define AWS_LC_SUCCESS 1

// 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)

#if defined(__cplusplus)
} // extern C
#endif
Expand Down
Loading