-
Notifications
You must be signed in to change notification settings - Fork 5.2k
HMAC Verify #119652
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.
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
base: main
Are you sure you want to change the base?
HMAC Verify #119652
Conversation
Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds HMAC "Verify" APIs for validating HMAC signatures across the cryptographic library. The changes introduce new verification methods that perform fixed-time comparisons to prevent timing attacks.
Key changes include:
- Addition of new
Verify
andVerifyAsync
methods to all HMAC classes (HMACSHA1, HMACSHA256, HMACSHA384, HMACSHA512, HMACSHA3_256, HMACSHA3_384, HMACSHA3_512, HMACMD5) - New verification APIs in
CryptographicOperations
class for general HMAC verification - Addition of
VerifyCurrentHash
andVerifyHashAndReset
methods toIncrementalHash
class - Creation of shared implementation infrastructure through
HMACShared
class
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
IncrementalHashTests.cs |
Adds comprehensive test coverage for new verify methods in IncrementalHash |
HmacTests.cs |
Adds base test infrastructure and validation tests for HMAC verification APIs |
HmacSha*.cs (test files) |
Implements abstract verify methods for each HMAC algorithm test class |
IncrementalHash.cs |
Implements VerifyCurrentHash and VerifyHashAndReset methods |
HMACShared.cs |
New shared implementation for HMAC verification logic across all algorithms |
HMAC*.cs (implementation files) |
Adds Verify and VerifyAsync static methods to each HMAC class |
CryptographicOperations.cs |
Adds general-purpose HMAC verification APIs |
Project and reference files | Updates to include new HMACShared.cs file and API surface |
src/libraries/System.Security.Cryptography/tests/IncrementalHashTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography/tests/IncrementalHashTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography/tests/IncrementalHashTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography/tests/IncrementalHashTests.cs
Outdated
Show resolved
Hide resolved
...ies/System.Security.Cryptography/src/System/Security/Cryptography/CryptographicOperations.cs
Show resolved
Hide resolved
...ies/System.Security.Cryptography/src/System/Security/Cryptography/CryptographicOperations.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/HMACShared.cs
Outdated
Show resolved
Hide resolved
...ies/System.Security.Cryptography/src/System/Security/Cryptography/CryptographicOperations.cs
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography/tests/IncrementalHashTests.cs
Show resolved
Hide resolved
...ies/System.Security.Cryptography/src/System/Security/Cryptography/CryptographicOperations.cs
Outdated
Show resolved
Hide resolved
...ies/System.Security.Cryptography/src/System/Security/Cryptography/CryptographicOperations.cs
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/HMACSHA1.cs
Show resolved
Hide resolved
|
||
if (ih.HashLengthInBytes > MaxStackAlloc) | ||
{ | ||
computedBuffer = new byte[ih.HashLengthInBytes]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unlike CryptographicOperations, I think IncrementalHash supports asking the provider (at least, CNG) for an algorithm we don't know about. So the defensive up-sizing makes sense here.
Wish we could test it, though 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to ideas. We could do something like
#if DEBUG
const int MaxStackAlloc = 32;
#else
const int MaxStackAlloc = 64;
#endif
So that a debug run always puts SHA-X-512 through the allocating path. But I really also dislike different behaviors between debug vs. release. Up to you though.
This adds HMAC "Verify" APIs for validating an HMAC signature.
KMAC will be a followup PR.
Contributes to #116028