Skip to content

Commit f16a20d

Browse files
Ensure HMAC_Init_ex reinitializes data properly (#2806)
### Description of changes: I discovered this edge case when working with an older Ruby version. This discrepancy only exists in older versions since Ruby openssl migrated from using the `HMAC_CTX` APIs to use the `EVP` layer in 3.1: ruby/ruby@b91f62f. [`test_reset_keep_key`](https://github.com/ruby/ruby/blame/cf4a034d59913fb71a7dd1b052164984be4a3d14/test/openssl/test_hmac.rb#L37-L43) was failing since we were MACing the data twice. It turns out the call to `h1.reset` wasn't working properly and this was due to our implementation of `HMAC_Init_ex` not reinitializing the data input when only `HMAC_Update` had been called. According to the original function contract, `HMAC_Init_ex` should reinitialize the inputted data, but the computed key should still be preserved. This is a minor edge case due to how older versions of Ruby were consuming `HMAC_CTX`. [Their call](https://github.com/ruby/ruby/blob/ruby_2_7/ext/openssl/ossl_hmac.c#L167-L174) to `HMAC_Final` was called upon a copy of the original `HMAC_CTX`. The original `HMAC_CTX` was always within a `HMAC_Update` state and AWS-LC was not properly reinitializing these cases. ### Call-outs: N/A ### Testing: New tests 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.
1 parent a3c40ea commit f16a20d

File tree

3 files changed

+59
-5
lines changed

3 files changed

+59
-5
lines changed

crypto/fipsmodule/hmac/hmac.c

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -371,17 +371,24 @@ int HMAC_Init_ex(HMAC_CTX *ctx, const void *key, size_t key_len,
371371
const EVP_MD *md, ENGINE *impl) {
372372
assert(impl == NULL);
373373

374+
GUARD_PTR(ctx);
375+
374376
if (HMAC_STATE_READY_NEEDS_INIT == ctx->state ||
375377
HMAC_STATE_PRECOMPUTED_KEY_EXPORT_READY == ctx->state) {
376378
ctx->state = HMAC_STATE_INIT_NO_DATA; // Mark that init has been called
377379
}
378380

379-
if (HMAC_STATE_INIT_NO_DATA == ctx->state) {
381+
if (hmac_ctx_is_initialized(ctx)) {
380382
// TODO(davidben,eroman): Passing the previous |md| with a NULL |key| is
381383
// ambiguous between using the empty key and reusing the previous key. There
382384
// exist callers which intend the latter, but the former is an awkward edge
383385
// case. Fix to API to avoid this.
384386
if (key == NULL && (md == NULL || md == ctx->md)) {
387+
if(HMAC_STATE_IN_PROGRESS == ctx->state) {
388+
// Reinitialize |md_ctx| from |i_ctx| to start fresh with the same key. This
389+
// is the same behavior as Openssl.
390+
OPENSSL_memcpy(&ctx->md_ctx, &ctx->i_ctx, sizeof(ctx->i_ctx));
391+
}
385392
// If nothing is changing then we can return without doing any further work.
386393
return 1;
387394
}

crypto/hmac_extra/hmac_test.cc

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -635,3 +635,48 @@ TEST(HMACTest, HandlesNullOutputParameters) {
635635
ASSERT_TRUE(HMAC_Update(ctx.get(), &input[0], sizeof(input)));
636636
ASSERT_FALSE(HMAC_Final(ctx.get(), nullptr, &mac_len));
637637
}
638+
639+
TEST(HMACTest, InitExResetsContext) {
640+
bssl::ScopedHMAC_CTX ctx;
641+
bssl::ScopedHMAC_CTX ctx_copy;
642+
643+
const EVP_MD *digest = EVP_sha256();
644+
645+
const uint8_t key1[] = "test_key_1";
646+
const uint8_t data1[] = "first_message";
647+
648+
uint8_t mac1[EVP_MAX_MD_SIZE];
649+
uint8_t mac2[EVP_MAX_MD_SIZE];
650+
uint8_t mac3[EVP_MAX_MD_SIZE];
651+
unsigned mac_len;
652+
653+
// First HMAC computation with |key1| and |data1|.
654+
ASSERT_TRUE(HMAC_Init_ex(ctx.get(), key1, sizeof(key1), digest, nullptr));
655+
ASSERT_TRUE(HMAC_Update(ctx.get(), data1, sizeof(data1)));
656+
// Copy to |ctx_copy| and finalize that to preserve |HMAC_STATE_IN_PROGRESS|
657+
// in |ctx|.
658+
ASSERT_TRUE(HMAC_CTX_copy(ctx_copy.get(), ctx.get()));
659+
ASSERT_TRUE(HMAC_Final(ctx_copy.get(), mac1, &mac_len));
660+
661+
// Reset context with NULL |key|/|digest| - should reuse previous key and
662+
// reset state. This tests that |HMAC_Init_ex| properly resets even after
663+
// Update/Final.
664+
ASSERT_TRUE(HMAC_Init_ex(ctx.get(), nullptr, 0, nullptr, nullptr));
665+
ASSERT_TRUE(HMAC_Update(ctx.get(), data1, sizeof(data1)));
666+
ASSERT_TRUE(HMAC_Final(ctx.get(), mac2, &mac_len));
667+
668+
// |mac1| and |mac2| should be the same (same key reused, context reset).
669+
EXPECT_EQ(Bytes(mac1, mac_len), Bytes(mac2, mac_len));
670+
671+
// Reset |ctx| with NULL |key| but explicit digest - should reuse previous
672+
// key and reset state.
673+
ASSERT_TRUE(HMAC_Init_ex(ctx.get(), nullptr, 0, digest, nullptr));
674+
ASSERT_TRUE(HMAC_Update(ctx.get(), data1, sizeof(data1)));
675+
// Copy to |ctx_copy| and finalize that to preserve |HMAC_STATE_IN_PROGRESS|
676+
// in |ctx|.
677+
ASSERT_TRUE(HMAC_CTX_copy(ctx_copy.get(), ctx.get()));
678+
ASSERT_TRUE(HMAC_Final(ctx_copy.get(), mac3, &mac_len));
679+
680+
// |mac1| and |mac3| should be the same (same key reused, context reset).
681+
EXPECT_EQ(Bytes(mac1, mac_len), Bytes(mac3, mac_len));
682+
}

include/openssl/hmac.h

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -108,10 +108,12 @@ OPENSSL_EXPORT void HMAC_CTX_cleanse(HMAC_CTX *ctx);
108108
OPENSSL_EXPORT void HMAC_CTX_free(HMAC_CTX *ctx);
109109

110110
// HMAC_Init_ex sets up an initialised |HMAC_CTX| to use |md| as the hash
111-
// function and |key| as the key. For a non-initial call, |md| may be NULL, in
112-
// which case the previous hash function will be used. If the hash function has
113-
// not changed and |key| is NULL, |ctx| reuses the previous key. It returns one
114-
// on success or zero on allocation failure.
111+
// function and |key| as the key. This function resets |HMAC_CTX| to a
112+
// fresh state, even if |HMAC_Update| or |HMAC_Final| have been called
113+
// previously. For a non-initial call, |md| may be NULL, in which case the
114+
// previous hash function will be used. If the hash function has not changed and
115+
// |key| is NULL, |ctx| reuses the previous key and resets to a clean state
116+
// ready for new data. It returns one on success or zero on allocation failure.
115117
//
116118
// WARNING: NULL and empty keys are ambiguous on non-initial calls. Passing NULL
117119
// |key| but repeating the previous |md| reuses the previous key rather than the

0 commit comments

Comments
 (0)