Add back support for EVP_PKEY_HMAC #1324

samuel40791765 · 2023-11-27T22:48:14Z

Issues:

Resolves CryptoAlg-1704

Description of changes:

Some legacy libraries still depend on EVP_PKEY_HMAC unfortunately, so we're adding back support. OpenSSL 3.0 hasn't deprecated support for this either, so we still need to support this for the SHIM layer long term. Much of logic was reapplied from the original commit removal, but I've cleaned some unneeded branching logic to makes things more straight forward.

The original EVP_PKEY_HMAC code from OpenSSL 1.1.1 exists in these two files:

The code has remained relatively untouched since the removal in BoringSSL.

We've also made modifications to remove the need for additional EVP_PKEY function pointers and ctrl logic. Since HMAC needs to be specially handled in many of the EVP_DigestSign* APIs, the additional function pointers make much less sense. We can just directly make calls to HMAC_CTX without the EVP_PKEY_METHOD redirections.

Call-outs:

Reapply the original removal
Reapply tests from the original removal.
Clean up code
Add tests against HMAC wycheproof test vectors
Update documentation that mentions the deprecation of EVP_PKEY_HMAC and add follow up comments for service indicator adjustments
Migrate ASN1 usage to use CBB/CBS
Add test using EVP_PKEY_CTX_dup with EVP_PKEY_HMAC

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.

codecov-commenter · 2023-11-27T23:45:46Z

Codecov Report

Attention: 31 lines in your changes are missing coverage. Please review.

Comparison is base (b201aea) 76.78% compared to head (52ea5f9) 76.79%.

Files	Patch %	Lines
crypto/fipsmodule/evp/evp.c	47.82%	12 Missing ⚠️
crypto/fipsmodule/evp/p_hmac.c	86.44%	8 Missing ⚠️
crypto/fipsmodule/digest/digest.c	82.14%	5 Missing ⚠️
crypto/fipsmodule/evp/digestsign.c	91.11%	4 Missing ⚠️
crypto/evp_extra/p_hmac_asn1.c	66.66%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #1324    +/-   ##
========================================
  Coverage   76.78%   76.79%            
========================================
  Files         422      424     +2     
  Lines       71267    71392   +125     
========================================
+ Hits        54725    54826   +101     
- Misses      16542    16566    +24

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

samuel40791765 · 2023-11-29T23:13:35Z

Bind9 consumes EVP_PKEY_HMAC from the EVP_PKEY_new_raw_private_key function, which this PR doesn't quite resolve yet. There are some subsequent ASN1 functions (hmac_set_priv_key and hmac_get_priv_key) here that we'll need to import to support this.
This change is pretty large already, so I'm leaning towards opening up a follow up commit to support this. We can let this PR focus on backporting what's in the original upstream commit removal.

crypto/evp_extra/p_hmac_asn1.c

crypto/fipsmodule/digest/digest.c

include/openssl/digest.h

crypto/fipsmodule/digest/internal.h

crypto/fipsmodule/evp/p_hmac.c

crypto/hmac_extra/hmac_test.cc

include/openssl/evp.h

PORTING.md

crypto/fipsmodule/evp/p_hmac.c

include/openssl/digest.h

crypto/fipsmodule/evp/evp.c

crypto/fipsmodule/evp/digestsign.c

nebeid · 2023-12-12T14:36:35Z

crypto/fipsmodule/evp/digestsign.c

-      return 0;
+    if (pkey->type == EVP_PKEY_HMAC) {
+      // |ctx->update| gets repurposed as a hook to call |HMAC_Update|.
+      // |ctx->update| is normally copied from |mctx->digest->update|, but


What is mctx? or was it supposed to be pctx?

I forgot to update this, it's ctx->digest->update from the same structure.

nebeid · 2023-12-12T14:42:42Z

crypto/fipsmodule/digest/digest.c

+    }
+    return 1;
+  }
+
  ctx->digest->init(ctx);


Is this still required in the case of used_for_hmac(ctx)?

No, it's the original EVP_Digest digest code path. HMAC specific logic is contained within used_for_hmac

nebeid · 2023-12-12T14:44:01Z

crypto/fipsmodule/evp/evp.c

+  // HMAC keys aren't absolutely required for HMAC operations, but the
+  // combination below isn't allowed.


Suggested change

// HMAC keys aren't absolutely required for HMAC operations, but the

// combination below isn't allowed.

// HMAC keys aren't absolutely required for HMAC operations (can be NULL),

// but in that case, their length must be zero.

Also added this to the API documentation

nebeid · 2023-12-12T14:45:21Z

Should the PR description reflect that the difference in design from the original brought-back commit?

crypto/fipsmodule/evp/internal.h

crypto/fipsmodule/digest/digest.c

include/openssl/evp.h

crypto/hmac_extra/hmac_test.cc

crypto/fipsmodule/evp/digestsign.c

crypto/fipsmodule/digest/digest.c

crypto/evp_extra/p_hmac_asn1.c

samuel40791765

Should the PR description reflect that the difference in design from the original brought-back commit?

Sure, I'll update it :)

samuel40791765 · 2023-12-12T17:37:42Z

crypto/fipsmodule/digest/digest.c

+    }
+    return 1;
+  }
+
  ctx->digest->init(ctx);


No, it's the original EVP_Digest digest code path. HMAC specific logic is contained within used_for_hmac

crypto/fipsmodule/evp/evp.c

include/openssl/evp.h

crypto/fipsmodule/evp/p_hmac.c

crypto/fipsmodule/digest/digest.c

samuel40791765 · 2023-12-12T18:27:50Z

crypto/fipsmodule/evp/digestsign.c

-      return 0;
+    if (pkey->type == EVP_PKEY_HMAC) {
+      // |ctx->update| gets repurposed as a hook to call |HMAC_Update|.
+      // |ctx->update| is normally copied from |mctx->digest->update|, but


I forgot to update this, it's ctx->digest->update from the same structure.

samuel40791765 · 2023-12-12T18:58:11Z

crypto/fipsmodule/evp/evp.c

+  // HMAC keys aren't absolutely required for HMAC operations, but the
+  // combination below isn't allowed.


Also added this to the API documentation

crypto/fipsmodule/digest/digest.c

crypto/fipsmodule/evp/digestsign.c

crypto/fipsmodule/digest/digest.c

torben-hansen · 2023-12-14T17:32:27Z

crypto/fipsmodule/evp/digestsign.c

@@ -130,7 +144,7 @@ int EVP_DigestVerifyInit(EVP_MD_CTX *ctx, EVP_PKEY_CTX **pctx,
 }



Need to bail early for these functions if called with an HMAC type pkey?

EVP_DigestVerifyFinal

EVP_DigestVerify

Nice detail, I've added an additional HMACTest.EVP_DigestVerify test to verify that the emitted error codes are what we want.
I was about to add some checks to bail early, but I noticed that the consistent behavior of the APIs was to bail if either verify or verify_message wasn't defined as a EVP_PKEY_METHOD (which is the case for HMAC here). In that case, should we still add the additional HMAC specific logic here? But, no strong opinions either way.

Added 984fccc per our discussion :)

crypto/hmac_extra/hmac_test.cc

pennyannn · 2023-12-21T22:47:28Z

include/openssl/digest.h

+  // 1. Set flag |EVP_MD_CTX_FLAG_KEEP_PKEY_CTX|, so as to let |*pctx| refrain
+  //    from being freed when |*pctx| was set externally with
+  //    |EVP_MD_CTX_set_pkey_ctx|.
+  // 2. Set flag |EVP_MD_CTX_FLAG_NO_INIT| for |EVP_PKEY_HMAC|.


The comment mentions constant EVP_MD_CTX_FLAG_NO_INIT, but the code checks for EVP_MD_CTX_HMAC. Could it be that EVP_MD_CTX_FLAG_NO_INIT is deprecated? Maybe the comment needs to be updated?

Yes it should be EVP_MD_CTX_HMAC I'll change this rn

include/openssl/digest.h

Some legacy libraries still depend on `EVP_PKEY_HMAC` unfortunately, so we're adding back support. OpenSSL 3.0 hasn't deprecated support for this either, so we still need to support this for the SHIM layer long term. Much of logic was reapplied from the [original commit removal] (https://github.com/google/boringssl/commit/ 65ee9b7), but I've cleaned up some unneeded branching logic to makes things more straight forward. We've also made modifications to remove the need for additional `EVP_PKEY` function pointers and `ctrl` logic. Since HMAC needs to be specially handled in many of the `EVP_DigestSign*` APIs, the additional function pointers make much less sense. We can just directly make calls to `HMAC_CTX` without the `EVP_PKEY_METHOD` redirections.

samuel40791765 force-pushed the evp-pkey-hmac branch 2 times, most recently from d63dd27 to 99c9474 Compare November 27, 2023 23:22

samuel40791765 marked this pull request as ready for review November 29, 2023 00:47

samuel40791765 requested a review from a team as a code owner November 29, 2023 00:47

samuel40791765 force-pushed the evp-pkey-hmac branch from 500d369 to a8f1c87 Compare November 29, 2023 00:50

torben-hansen self-requested a review November 29, 2023 15:23

samuel40791765 requested a review from andrewhop November 30, 2023 21:45

samuel40791765 force-pushed the evp-pkey-hmac branch 4 times, most recently from b460815 to 3f92262 Compare December 5, 2023 00:21

samuel40791765 mentioned this pull request Dec 5, 2023

allow HMAC via EVP_PKEY raw privkey functions #1338

Merged

andrewhop reviewed Dec 6, 2023

View reviewed changes

torben-hansen reviewed Dec 8, 2023

View reviewed changes

samuel40791765 force-pushed the evp-pkey-hmac branch 3 times, most recently from 3486667 to 75caba2 Compare December 12, 2023 01:35

nebeid reviewed Dec 12, 2023

View reviewed changes

torben-hansen reviewed Dec 12, 2023

View reviewed changes

samuel40791765 commented Dec 12, 2023

View reviewed changes

samuel40791765 force-pushed the evp-pkey-hmac branch from f1cfc5f to a32e47c Compare December 12, 2023 19:34

torben-hansen reviewed Dec 14, 2023

View reviewed changes

samuel40791765 force-pushed the evp-pkey-hmac branch from e49b791 to eb28388 Compare December 14, 2023 21:25

torben-hansen previously approved these changes Dec 15, 2023

View reviewed changes

andrewhop reviewed Dec 19, 2023

View reviewed changes

crypto/hmac_extra/hmac_test.cc Show resolved Hide resolved

samuel40791765 dismissed torben-hansen’s stale review via 899d830 December 19, 2023 21:35

andrewhop previously approved these changes Dec 20, 2023

View reviewed changes

This was referenced Dec 20, 2023

Add support for EVP_PKEY_HMAC awslabs/aws-lc-verification#136

Merged

Add proofs for top-level EVP function of HMAC awslabs/aws-lc-verification#137

Open

torben-hansen previously approved these changes Dec 21, 2023

View reviewed changes

pennyannn reviewed Dec 21, 2023

View reviewed changes

samuel40791765 dismissed stale reviews from torben-hansen and andrewhop via 6df1d67 December 22, 2023 00:15

samuel40791765 force-pushed the evp-pkey-hmac branch from 6df1d67 to cbb2d32 Compare December 22, 2023 00:15

nebeid reviewed Dec 22, 2023

View reviewed changes

include/openssl/digest.h Outdated Show resolved Hide resolved

samuel40791765 force-pushed the evp-pkey-hmac branch 2 times, most recently from 73f3740 to 0ffe09c Compare December 22, 2023 18:19

samuel40791765 added 12 commits December 22, 2023 13:25

add back support for EVP_PKEY_HMAC

7362b11

slight enhancements and move tests to hmac_tests.cc

67c9e20

Add follow ups and remove deprecation mentions

b4079fa

change internal callback function names

e4941d4

use CBS instead of ASN1 and more evp tests

4e1697b

use CBB when storing HMAC key

d3e1550

restructure and remove unneeded p_hmac functions

e593ac4

PR comments; memory edge cases and general improvements

0c49703

PR comments; stronger null check and verify test

65a8857

add used_for_hmac check for verify operations

1f5be51

add byte by byte incremental test

d7553ba

clear up comments

52ea5f9

samuel40791765 force-pushed the evp-pkey-hmac branch from 0ffe09c to 52ea5f9 Compare December 22, 2023 21:25

torben-hansen approved these changes Dec 22, 2023

View reviewed changes

andrewhop approved these changes Dec 22, 2023

View reviewed changes

samuel40791765 enabled auto-merge (squash) December 26, 2023 20:57

samuel40791765 merged commit 7226be1 into aws:main Dec 26, 2023

samuel40791765 deleted the evp-pkey-hmac branch December 26, 2023 21:14

pennyannn mentioned this pull request Dec 27, 2023

Flip back src submodule awslabs/aws-lc-verification#138

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add back support for EVP_PKEY_HMAC #1324

Add back support for EVP_PKEY_HMAC #1324

samuel40791765 commented Nov 27, 2023 •

edited

Loading

codecov-commenter commented Nov 27, 2023 •

edited

Loading

samuel40791765 commented Nov 29, 2023 •

edited

Loading

nebeid Dec 12, 2023

samuel40791765 Dec 12, 2023

nebeid Dec 12, 2023

samuel40791765 Dec 12, 2023

nebeid Dec 12, 2023

samuel40791765 Dec 12, 2023

nebeid commented Dec 12, 2023

samuel40791765 left a comment

samuel40791765 Dec 12, 2023

samuel40791765 Dec 12, 2023

samuel40791765 Dec 12, 2023

torben-hansen Dec 14, 2023

samuel40791765 Dec 14, 2023

samuel40791765 Dec 14, 2023

pennyannn Dec 21, 2023

samuel40791765 Dec 22, 2023 •

edited

Loading

		// HMAC keys aren't absolutely required for HMAC operations, but the
		// combination below isn't allowed.

		@@ -130,7 +144,7 @@ int EVP_DigestVerifyInit(EVP_MD_CTX ctx, EVP_PKEY_CTX *pctx,
		}

Add back support for EVP_PKEY_HMAC #1324

Add back support for EVP_PKEY_HMAC #1324

Conversation

samuel40791765 commented Nov 27, 2023 • edited Loading

Issues:

Description of changes:

Call-outs:

Testing:

codecov-commenter commented Nov 27, 2023 • edited Loading

Codecov Report

samuel40791765 commented Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nebeid commented Dec 12, 2023

samuel40791765 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuel40791765 Dec 22, 2023 • edited Loading

Choose a reason for hiding this comment

samuel40791765 commented Nov 27, 2023 •

edited

Loading

codecov-commenter commented Nov 27, 2023 •

edited

Loading

samuel40791765 commented Nov 29, 2023 •

edited

Loading

samuel40791765 Dec 22, 2023 •

edited

Loading