Clean up some usage of SHA1 and SHA256 in the code base by GrabYourPitchforks · Pull Request #24696 · dotnet/aspnetcore

GrabYourPitchforks · 2020-08-08T01:53:40Z

Two main changes:

Prefer static SHA256.HashData method where applicable, as it's going to use the most optimal implementation available on the current platform. It also avoids allocating temporary SHA256 instances in cases where you're going to hash a single buffer.
Remove references to *CryptoServiceProvider and friends. Use the standard factories like SHA256.Create. This also reduces slightly the total number of types referenced by the application, which could help with linker trimming scenarios.

I did not change any projects which targeted netstandard2.0 or any other pre-5.0 target framework.

GrabYourPitchforks · 2020-08-08T01:55:14Z

Note to reviewers: I recommend hiding whitespace changes. Removing some using blocks affected indentation.

davidfowl · 2020-08-08T06:50:44Z

src/Antiforgery/src/Internal/AntiforgeryOptionsSetup.cs

-                var subHash = hash.Take(8).ToArray();
-                return WebEncoders.Base64UrlEncode(subHash);
-            }
+            byte[] fullHash = SHA256.HashData(Encoding.UTF8.GetBytes(applicationId));


Since you're here, may as well improve the allocations here.

var source = Encoding.UTF8.GetBytes(applicationId); Span<byte> target = stackalloc byte[32]; SHA256.HashData(source, target); WebEncoders.Base64UrlEncode(target.Slice(0, 8));

This code is called exactly once at app start. Wasn't worth complicating the code for so little gain.

that's fair

davidfowl

LGTM, small nits.

davidfowl · 2020-08-08T06:53:18Z

src/Antiforgery/src/Internal/DefaultClaimUidExtractor.cs

+                    throw new InvalidOperationException();
+                }
+
+                var bytes = SHA256.HashData(buffer);


Implicit span conversion?

Yes. ArraySegment<byte> is implicitly convertible to ROS<byte>.

davidfowl · 2020-08-08T06:54:55Z

src/Middleware/WebSockets/src/HandshakeHelpers.cs


-                return Convert.ToBase64String(hashedBytes);
+            Span<byte> hashedBytes = stackalloc byte[20];
+            var written = SHA1.HashData(mergedBytes, hashedBytes);


Does SHA1.HashData allocate any objects or is it truly static?

I believe it only allocates on Win7 & Win8. But it's still faster than alternatives: see dotnet/runtime#40510.

I care more about linux? Is it allocating? This change had a measurable impact on websocket upgrades when it was made. I want to make sure this isn't regressing that.

@BrennanConroy did you commit those performance tests?

Of course 😄
https://github.com/dotnet/aspnetcore/blob/master/src/Middleware/perf/Microbenchmarks/HandshakeBenchmark.cs

Last I checked Linux was neither Win7 nor Win8, so it should be non-allocating. :)

Here's the Linux implementation, for reference:
https://github.com/dotnet/runtime/blob/46acbaa5f13b501dc2e750bc2023d4a30eded9c7/src/libraries/System.Security.Cryptography.Algorithms/src/Internal/Cryptography/HashProviderDispenser.Unix.cs#L43-L62

I don't have a suitable environment for running the perf tests, sorry. If you have a shared environment where I can do so, along with clear instructions for it (copy + paste these exact commands), I can give it a spin.

Unrelated to this PR, if IsRequestKeyValid is a hot method I think you can get significant perf gains in it from rolling your own logic. The framework method you're calling performs base64 decoding, but you don't actually need to decode it. You only care that it is valid base64 and that it decodes to 16 bytes. So a rough outline of the logic would be:

Validate that the incoming string length is exactly 24 chars.

Perform vectorized logic to validate that the first 22 chars of the string are within [a-zA-Z0-9+/]. (Your vectorized logic will consume 8 or 16 chars at a time, depending on the current OS.)

Validate that the last 2 chars are both '='.

If any of these checks fail, you can fall back to the framework's Convert.TryFromBsae64String string for extra validation if you need to handle things like whitespace chars.

Linux

Before:

Method Mean Error StdDev Op/s Gen 0 Gen 1 Gen 2 Allocated

CreateResponseKey 2,292.6 ns 22.81 ns 21.33 ns 436,191.8 0.0031 - - 200 B

IsRequestKeyValid 143.3 ns 1.13 ns 1.00 ns 6,979,003.0 - - - -

After:

Method Mean Error StdDev Op/s Gen 0 Gen 1 Gen 2 Allocated

CreateResponseKey 1,406.8 ns 12.49 ns 11.07 ns 710,842.7 0.0015 - - 80 B

IsRequestKeyValid 147.2 ns 1.12 ns 0.99 ns 6,792,335.8 - - - -

if IsRequestKeyValid is a hot method I think you can get significant perf gains in it from rolling your own logic

We had a try of this in #13196

BrennanConroy · 2020-08-09T04:11:32Z

src/Mvc/Mvc.Razor/src/Infrastructure/DefaultFileVersionProvider.cs

        private static string GetHashForFile(IFileInfo fileInfo)
        {
-            using (var sha256 = CryptographyAlgorithms.CreateSHA256())
+            using (var sha256 = SHA256.Create())


Why isn't this one using the static method?

It's hashing a stream and the one-shot doesn't have a stream overload, only byte[] and ReadOnlySpan<byte>.

pranavkm · 2020-08-10T21:26:52Z

Thanks @GrabYourPitchforks

GrabYourPitchforks added 5 commits August 7, 2020 18:01

AntiForgery: Use SHA256 one-shot

97ebdfe

AuthN: Use SHA256 one-shot APIs

cb97e54

MVC/Razor: Cleanup SHA256 references

fd1f41f

WebSockets: Use SHA1 one-shots

fac3aaf

dotnet-openapi: Use preferred SHA256 factory

b896a75

GrabYourPitchforks requested a review from Tratcher as a code owner August 8, 2020 01:53

SignalR: Prefer RNG.Fill over RNG.GetBytes

76d0095

GrabYourPitchforks requested review from BrennanConroy and halter73 as code owners August 8, 2020 02:02

GrabYourPitchforks requested a review from pranavkm August 8, 2020 02:02

davidfowl reviewed Aug 8, 2020

View reviewed changes

davidfowl approved these changes Aug 8, 2020

View reviewed changes

davidfowl reviewed Aug 8, 2020

View reviewed changes

GrabYourPitchforks mentioned this pull request Aug 8, 2020

Analyzer / fixer proposal: Prefer static HashData methods over ComputeHash dotnet/runtime#40579

Closed

BrennanConroy reviewed Aug 9, 2020

View reviewed changes

BrennanConroy approved these changes Aug 9, 2020

View reviewed changes

Pilchie added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Aug 10, 2020

BrennanConroy added this to the 5.0.0-rc1 milestone Aug 10, 2020

pranavkm merged commit bbf7c87 into dotnet:master Aug 10, 2020

GrabYourPitchforks deleted the crypto_upd_dev branch August 10, 2020 21:48

martincostello mentioned this pull request Aug 11, 2020

Support ASP.NET 5 using RC1 aspnet-contrib/AspNet.Security.OAuth.Providers#438

Merged

Method	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
CreateResponseKey	2,292.6 ns	22.81 ns	21.33 ns	436,191.8	0.0031	-	-	200 B
IsRequestKeyValid	143.3 ns	1.13 ns	1.00 ns	6,979,003.0	-	-	-	-

Method	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
CreateResponseKey	1,406.8 ns	12.49 ns	11.07 ns	710,842.7	0.0015	-	-	80 B
IsRequestKeyValid	147.2 ns	1.12 ns	0.99 ns	6,792,335.8	-	-	-	-

Conversation

GrabYourPitchforks commented Aug 8, 2020

Uh oh!

GrabYourPitchforks commented Aug 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranavkm commented Aug 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants