Skip to content

Commit

Permalink
Update HMAC to fail when null value is passed to out parameter (#1662)
Browse files Browse the repository at this point in the history
### Issues:
CryptoAlg-2522

### Description of changes: 

Currently, `NULL` passed as the value of `out` can result in a
segmentation fault. This change adds checks to the HMAC one-shot API and
HMAC_final function to handle scenarios where `NULL` is passed as a
value to the `out` parameter to return from the functions and prevent
further computation.

### Testing:

Added additional test case in `crypto/hmac_extra/hmac_test.cc` to verify
the behavior when `NULL` is passed as a value to `out` in both
functions.

### Call-outs:

- OpenSSL supports this differently by allowing the computation to occur
but allocating a throw-away array [OpenSSL
implementation](https://github.com/openssl/openssl/blob/1977c00f00ad0546421a5ec0b40c1326aee4cddb/crypto/hmac/hmac.c#L233)

- We should also evaluate if this change should be back-ported to the
FIPS branches

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.

Co-authored-by: Sean McGrail <[email protected]>
  • Loading branch information
kexgaber and skmcgrail authored Jul 1, 2024
1 parent 05d3bfd commit 21c5e48
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 0 deletions.
9 changes: 9 additions & 0 deletions crypto/fipsmodule/hmac/hmac.c
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,11 @@ OPENSSL_STATIC_ASSERT(HMAC_STATE_UNINITIALIZED == 0, HMAC_STATE_UNINITIALIZED_is
uint8_t *HMAC(const EVP_MD *evp_md, const void *key, size_t key_len,
const uint8_t *data, size_t data_len, uint8_t *out,
unsigned int *out_len) {

if (out == NULL) {
// Prevent further work from being done
return NULL;
}

HMAC_CTX ctx;
OPENSSL_memset(&ctx, 0, sizeof(HMAC_CTX));
Expand Down Expand Up @@ -341,6 +346,10 @@ int HMAC_Update(HMAC_CTX *ctx, const uint8_t *data, size_t data_len) {
}

int HMAC_Final(HMAC_CTX *ctx, uint8_t *out, unsigned int *out_len) {
if (out == NULL) {
return 0;
}

const HmacMethods *methods = ctx->methods;
if (!hmac_ctx_is_initialized(ctx)) {
return 0;
Expand Down
25 changes: 25 additions & 0 deletions crypto/hmac_extra/hmac_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -351,3 +351,28 @@ TEST(HMACTest, EVP_DigestVerify) {
EXPECT_EQ(EVP_R_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE,
ERR_GET_REASON(ERR_get_error()));
}

TEST(HMACTest, HandlesNullOutputParameters) {
bssl::ScopedHMAC_CTX ctx;
const EVP_MD *digest = EVP_sha256();
// make key and input valid
const uint8_t key[32] = {0};
const uint8_t input[16] = {0};

// Test one-shot API with out and out_len as NULL
ASSERT_FALSE(HMAC(digest, &key[0], sizeof(key), &input[0], sizeof(input),
nullptr,nullptr));
unsigned mac_len;
// Test one-shot API with only out as NULL
ASSERT_FALSE(HMAC(digest, &key[0], sizeof(key), &input[0], sizeof(input),
nullptr, &mac_len));

// Test HMAC_ctx
ASSERT_TRUE(HMAC_Init_ex(ctx.get(), &key[0], sizeof(key), digest, nullptr));
ASSERT_TRUE(HMAC_Update(ctx.get(), &input[0], sizeof(input)));
ASSERT_FALSE(HMAC_Final(ctx.get(), nullptr, nullptr));

ASSERT_TRUE(HMAC_Init_ex(ctx.get(), &key[0], sizeof(key), digest, nullptr));
ASSERT_TRUE(HMAC_Update(ctx.get(), &input[0], sizeof(input)));
ASSERT_FALSE(HMAC_Final(ctx.get(), nullptr, &mac_len));
}

0 comments on commit 21c5e48

Please sign in to comment.