Skip to content

Commit

Permalink
Update EVP cipher APIs to gracefully handle null EVP_CIPHER_CTX (#1398)
Browse files Browse the repository at this point in the history 
### Issues:
Resolves V1187459157

### Description of changes: 
This change does 3 things:
1. All EVP_CIPHER_CTX now start off as poisoned and require calling
EVP_CipherInit_ex to cure it
2. All EVP Encrypt/Decrypt Update/Final now check that the
EVP_CIPHER_CTX and EVP_CIPHER_CTX->cipher are not null
3. Add the start of reusable safety macros inspired by
[s2n-tls](https://github.com/aws/s2n-tls/blob/main/docs/SAFETY-MACROS.md)

### Call-outs:
This is an alternative approach to
#1420.

The `__AWS_LC_ENSURE` macro uses the `do {} while (0)` trick to ensure
the action is run once, anything passed into the macro doesn't
accidentally expand and change the scope, and the compiler enforces you
add a `;` after the macro. We use this trick in other macros and all
compilers are smart enough to optimize out the jump.

### Testing:
`GUARD_PTR(ctx);` expands to:
```
  do {
    if (!((ctx) != ((void *)0))) {
      ERR_put_error(ERR_LIB_CRYPTO, 0, (3 | 64), "_file_name_", 259);
      return 0;
    }
  } while (0);
```

[Here is an example](https://godbolt.org/z/z99roroKq) with the macro,
and here it is implemented as a [traditional
if/else](https://godbolt.org/z/E56EYnnW9). Both result in the same code.

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.
  • Loading branch information
@andrewhop
andrewhop authored Apr 16, 2024
1 parent e91524c commit be51025
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 5 deletions.
26 changes: 23 additions & 3 deletions crypto/cipher_extra/cipher_test.cc
View file
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
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;
}

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);
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
Original file line number Diff line number Diff line change
Expand Up @@ -1189,6 +1189,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) \
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

0 comments on commit be51025

Please sign in to comment.