Skip to content

Clean up some usage of SHA1 and SHA256 in the code base #24696

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Aug 10, 2020
Merged

Clean up some usage of SHA1 and SHA256 in the code base #24696

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
97ebdfe
AntiForgery: Use SHA256 one-shot
GrabYourPitchforks Aug 8, 2020
cb97e54
AuthN: Use SHA256 one-shot APIs
GrabYourPitchforks Aug 8, 2020
fd1f41f
MVC/Razor: Cleanup SHA256 references
GrabYourPitchforks Aug 8, 2020
fac3aaf
WebSockets: Use SHA1 one-shots
GrabYourPitchforks Aug 8, 2020
b896a75
dotnet-openapi: Use preferred SHA256 factory
GrabYourPitchforks Aug 8, 2020
76d0095
SignalR: Prefer RNG.Fill over RNG.GetBytes
GrabYourPitchforks Aug 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 3 additions & 7 deletions src/Antiforgery/src/Internal/AntiforgeryOptionsSetup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Linq;
using System.Security.Cryptography;
using System.Text;
using Microsoft.AspNetCore.DataProtection;
using Microsoft.AspNetCore.WebUtilities;
Expand Down Expand Up @@ -35,12 +35,8 @@ public void Configure(AntiforgeryOptions options)

private static string ComputeCookieName(string applicationId)
{
using (var sha256 = CryptographyAlgorithms.CreateSHA256())
{
var hash = sha256.ComputeHash(Encoding.UTF8.GetBytes(applicationId));
var subHash = hash.Take(8).ToArray();
return WebEncoders.Base64UrlEncode(subHash);
}
byte[] fullHash = SHA256.HashData(Encoding.UTF8.GetBytes(applicationId));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's fair

return WebEncoders.Base64UrlEncode(fullHash, 0, 8);
}
}
}
18 changes: 0 additions & 18 deletions src/Antiforgery/src/Internal/AntiforgerySerializationContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ internal class AntiforgerySerializationContext
private MemoryStream? _stream;
private BinaryReader? _reader;
private BinaryWriter? _writer;
private SHA256? _sha256;

public MemoryStream Stream
{
Expand Down Expand Up @@ -82,23 +81,6 @@ private set
}
}

public SHA256 Sha256
{
get
{
if (_sha256 == null)
{
_sha256 = CryptographyAlgorithms.CreateSHA256();
}

return _sha256;
}
private set
{
_sha256 = value;
}
}

public char[] GetChars(int count)
{
if (_chars == null || _chars.Length < count)
Expand Down
25 changes: 0 additions & 25 deletions src/Antiforgery/src/Internal/CryptographyAlgorithms.cs
View file
Open in desktop

This file was deleted.

11 changes: 8 additions & 3 deletions src/Antiforgery/src/Internal/DefaultClaimUidExtractor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Collections.Generic;
using System.Diagnostics;
using System.Security.Claims;
using System.Security.Cryptography;
using Microsoft.Extensions.ObjectPool;

namespace Microsoft.AspNetCore.Antiforgery
Expand Down Expand Up @@ -134,9 +135,13 @@ private byte[] ComputeSha256(IEnumerable<string> parameters)

writer.Flush();

var sha256 = serializationContext.Sha256;
var stream = serializationContext.Stream;
var bytes = sha256.ComputeHash(stream.ToArray(), 0, checked((int)stream.Length));
bool success = serializationContext.Stream.TryGetBuffer(out ArraySegment<byte> buffer);
if (!success)
{
throw new InvalidOperationException();
}

var bytes = SHA256.HashData(buffer);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implicit span conversion?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return bytes;
}
Expand Down
27 changes: 12 additions & 15 deletions src/Middleware/WebSockets/src/HandshakeHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,23 +110,20 @@ public static string CreateResponseKey(string requestKey)
// this concatenated value to obtain a 20-byte value and base64-encoding"
// https://tools.ietf.org/html/rfc6455#section-4.2.2

using (var algorithm = SHA1.Create())
{
// requestKey is already verified to be small (24 bytes) by 'IsRequestKeyValid()' and everything is 1:1 mapping to UTF8 bytes
// so this can be hardcoded to 60 bytes for the requestKey + static websocket string
Span<byte> mergedBytes = stackalloc byte[60];
Encoding.UTF8.GetBytes(requestKey, mergedBytes);
EncodedWebSocketKey.CopyTo(mergedBytes.Slice(24));

Span<byte> hashedBytes = stackalloc byte[20];
var success = algorithm.TryComputeHash(mergedBytes, hashedBytes, out var written);
if (!success || written != 20)
{
throw new InvalidOperationException("Could not compute the hash for the 'Sec-WebSocket-Accept' header.");
}
// requestKey is already verified to be small (24 bytes) by 'IsRequestKeyValid()' and everything is 1:1 mapping to UTF8 bytes
// so this can be hardcoded to 60 bytes for the requestKey + static websocket string
Span<byte> mergedBytes = stackalloc byte[60];
Encoding.UTF8.GetBytes(requestKey, mergedBytes);
EncodedWebSocketKey.CopyTo(mergedBytes.Slice(24));

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BrennanConroy did you commit those performance tests?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Validate that the incoming string length is exactly 24 chars.
  2. 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.)
  3. 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 - - - -

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (written != 20)
{
throw new InvalidOperationException("Could not compute the hash for the 'Sec-WebSocket-Accept' header.");
}

return Convert.ToBase64String(hashedBytes);
}
}
}
25 changes: 0 additions & 25 deletions src/Mvc/Mvc.Razor/src/Infrastructure/CryptographyAlgorithms.cs
View file
Open in desktop

This file was deleted.

5 changes: 3 additions & 2 deletions src/Mvc/Mvc.Razor/src/Infrastructure/DefaultFileVersionProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Security.Cryptography;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc.ViewFeatures;
Expand Down Expand Up @@ -97,7 +98,7 @@ public string AddFileVersionToPath(PathString requestPathBase, string path)

private static string GetHashForFile(IFileInfo fileInfo)
{
using (var sha256 = CryptographyAlgorithms.CreateSHA256())
using (var sha256 = SHA256.Create())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't this one using the static method?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
using (var readStream = fileInfo.CreateReadStream())
{
Expand All @@ -107,4 +108,4 @@ private static string GetHashForFile(IFileInfo fileInfo)
}
}
}
}
}
12 changes: 4 additions & 8 deletions src/Mvc/Mvc.TagHelpers/src/Cache/CacheTagKey.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,11 @@
using System;
using System.Collections.Generic;
using System.Globalization;
using System.Security.Cryptography;
using System.Text;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc.Razor.Infrastructure;
using Microsoft.AspNetCore.Razor.TagHelpers;
using Microsoft.AspNetCore.Routing;
using Microsoft.Extensions.Internal;
using Microsoft.Extensions.Primitives;

namespace Microsoft.AspNetCore.Mvc.TagHelpers.Cache
Expand Down Expand Up @@ -180,12 +179,9 @@ public string GenerateHashedKey()
// The key is typically too long to be useful, so we use a cryptographic hash
// as the actual key (better randomization and key distribution, so small vary
// values will generate dramatically different keys).
using (var sha256 = CryptographyAlgorithms.CreateSHA256())
{
var contentBytes = Encoding.UTF8.GetBytes(key);
var hashedBytes = sha256.ComputeHash(contentBytes);
return Convert.ToBase64String(hashedBytes);
}
var contentBytes = Encoding.UTF8.GetBytes(key);
var hashedBytes = SHA256.HashData(contentBytes);
return Convert.ToBase64String(hashedBytes);
}

/// <inheritdoc />
Expand Down
3 changes: 1 addition & 2 deletions src/Security/Authentication/MicrosoftAccount/src/MicrosoftAccountHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,7 @@ protected override string BuildChallengeUrl(AuthenticationProperties properties,
// Store this for use during the code redemption.
properties.Items.Add(OAuthConstants.CodeVerifierKey, codeVerifier);

using var sha256 = SHA256.Create();
var challengeBytes = sha256.ComputeHash(Encoding.UTF8.GetBytes(codeVerifier));
var challengeBytes = SHA256.HashData(Encoding.UTF8.GetBytes(codeVerifier));
var codeChallenge = WebEncoders.Base64UrlEncode(challengeBytes);

queryStrings[OAuthConstants.CodeChallengeKey] = codeChallenge;
Expand Down
3 changes: 1 addition & 2 deletions src/Security/Authentication/OAuth/src/OAuthHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,8 +280,7 @@ protected virtual string BuildChallengeUrl(AuthenticationProperties properties,
// Store this for use during the code redemption.
properties.Items.Add(OAuthConstants.CodeVerifierKey, codeVerifier);

using var sha256 = SHA256.Create();
var challengeBytes = sha256.ComputeHash(Encoding.UTF8.GetBytes(codeVerifier));
var challengeBytes = SHA256.HashData(Encoding.UTF8.GetBytes(codeVerifier));
var codeChallenge = WebEncoders.Base64UrlEncode(challengeBytes);

parameters[OAuthConstants.CodeChallengeKey] = codeChallenge;
Expand Down
3 changes: 1 addition & 2 deletions src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,8 +375,7 @@ private async Task HandleChallengeAsyncInternal(AuthenticationProperties propert
// Store this for use during the code redemption. See RunAuthorizationCodeReceivedEventAsync.
properties.Items.Add(OAuthConstants.CodeVerifierKey, codeVerifier);

using var sha256 = SHA256.Create();
var challengeBytes = sha256.ComputeHash(Encoding.UTF8.GetBytes(codeVerifier));
var challengeBytes = SHA256.HashData(Encoding.UTF8.GetBytes(codeVerifier));
var codeChallenge = WebEncoders.Base64UrlEncode(challengeBytes);

message.Parameters.Add(OAuthConstants.CodeChallengeKey, codeChallenge);
Expand Down
4 changes: 1 addition & 3 deletions src/SignalR/common/Http.Connections/src/Internal/HttpConnectionManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ internal partial class HttpConnectionManager
// TODO: Consider making this configurable? At least for testing?
private static readonly TimeSpan _heartbeatTickRate = TimeSpan.FromSeconds(1);

private static readonly RNGCryptoServiceProvider _keyGenerator = new RNGCryptoServiceProvider();

private readonly ConcurrentDictionary<string, (HttpConnectionContext Connection, ValueStopwatch Timer)> _connections =
new ConcurrentDictionary<string, (HttpConnectionContext Connection, ValueStopwatch Timer)>(StringComparer.Ordinal);
private readonly TimerAwaitable _nextHeartbeat;
Expand Down Expand Up @@ -113,7 +111,7 @@ private static string MakeNewConnectionId()
// 128 bit buffer / 8 bits per byte = 16 bytes
Span<byte> buffer = stackalloc byte[16];
// Generate the id with RNGCrypto because we want a cryptographically random id, which GUID is not
_keyGenerator.GetBytes(buffer);
RandomNumberGenerator.Fill(buffer);
return WebEncoders.Base64UrlEncode(buffer);
}

Expand Down
14 changes: 1 addition & 13 deletions src/Tools/Microsoft.dotnet-openapi/src/Commands/BaseCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,19 +513,7 @@ private static IDictionary<string, string> GetServicePackages(CodeGenerator? typ

private static byte[] GetHash(Stream stream)
{
SHA256 algorithm;
try
{
algorithm = SHA256.Create();
}
catch (TargetInvocationException)
{
// SHA256.Create is documented to throw this exception on FIPS-compliant machines. See
// https://msdn.microsoft.com/en-us/library/z08hz7ad Fall back to a FIPS-compliant SHA256 algorithm.
algorithm = new SHA256CryptoServiceProvider();
}

using (algorithm)
using (var algorithm = SHA256.Create())
{
return algorithm.ComputeHash(stream);
}
Expand Down