Skip to content
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

Jump to bottom

Refine hashing usages #15621

Merged
merged 4 commits into from
Mar 29, 2024
Merged

Refine hashing usages #15621

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
feb177b
Refine hashing usages
sebastienros Mar 28, 2024
c3ac7f3
Remove Sha1
sebastienros Mar 28, 2024
444c980
Remove MD5 usage
sebastienros Mar 28, 2024
da775f2
Recover SHA1 for Twitter auth
sebastienros Mar 28, 2024
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
1 change: 1 addition & 0 deletions src/OrchardCore.Build/Dependencies.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@
<PackageManagement Include="StackExchange.Redis" Version="2.7.33" />
<PackageManagement Include="StyleCop.Analyzers" Version="1.1.118" />
<PackageManagement Include="System.Linq.Async" Version="6.0.1" />
<PackageManagement Include="System.IO.Hashing" Version="8.0.0" />
Copy link
Member Author

Choose a reason for hiding this comment

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

Contains new hashing algorithm, official Ms library.

<PackageManagement Include="xunit" Version="2.7.0" />
<PackageManagement Include="xunit.analyzers" Version="1.11.0" />
<PackageManagement Include="xunit.runner.visualstudio" Version="2.5.7" />
Expand Down
1 change: 1 addition & 0 deletions src/OrchardCore.Modules/OrchardCore.Media/OrchardCore.Media.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
<PackageReference Include="Shortcodes" />
<PackageReference Include="SixLabors.ImageSharp.Web" />
<PackageReference Include="System.Linq.Async" />
<PackageReference Include="System.IO.Hashing" />
</ItemGroup>

</Project>
6 changes: 2 additions & 4 deletions src/OrchardCore.Modules/OrchardCore.Media/Processing/MediaTokenService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,6 @@ private string GetHash(string queryStringTokenKey)

entry.SlidingExpiration = TimeSpan.FromHours(5);

using var hmac = new HMACSHA256(_hashKey);
Copy link
Member Author

Choose a reason for hiding this comment

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

Removing the custom instantiation, it's recommended to use the static methods.


// 'queryStringTokenKey' also contains prefix.
var chars = queryStringTokenKey.AsSpan(TokenCacheKeyPrefix.Length);

Expand All @@ -177,11 +175,11 @@ private string GetHash(string queryStringTokenKey)
: new byte[requiredLength];

// 256 for SHA-256, fits in stack nicely.
Span<byte> hashBytes = stackalloc byte[hmac.HashSize];
Span<byte> hashBytes = stackalloc byte[HMACSHA256.HashSizeInBytes];

var stringBytesLength = Encoding.UTF8.GetBytes(chars, stringBytes);

hmac.TryComputeHash(stringBytes[..stringBytesLength], hashBytes, out var hashBytesLength);
HMACSHA256.TryHashData(_hashKey, stringBytes[..stringBytesLength], hashBytes, out var hashBytesLength);

entry.Value = result = Convert.ToBase64String(hashBytes[..hashBytesLength]);
}
Expand Down
12 changes: 4 additions & 8 deletions src/OrchardCore.Modules/OrchardCore.Media/Processing/MediaTokenSettingsUpdater.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ namespace OrchardCore.Media.Processing
/// </summary>
public class MediaTokenSettingsUpdater : FeatureEventHandler, IModularTenantEvents
{
private const int DefaultMediaTokenKeySize = 64;

private readonly ISiteService _siteService;
private readonly ShellSettings _shellSettings;

Expand All @@ -38,10 +40,7 @@ public async Task ActivatedAsync()
{
var siteSettings = await _siteService.LoadSiteSettingsAsync();

var rng = RandomNumberGenerator.Create();

mediaTokenSettings.HashKey = new byte[64];
rng.GetBytes(mediaTokenSettings.HashKey);
mediaTokenSettings.HashKey = RandomNumberGenerator.GetBytes(DefaultMediaTokenKeySize);
siteSettings.Put(mediaTokenSettings);

await _siteService.UpdateSiteSettingsAsync(siteSettings);
Expand All @@ -65,10 +64,7 @@ private async Task SetMediaTokenSettingsAsync(IFeatureInfo feature)
var siteSettings = await _siteService.LoadSiteSettingsAsync();
var mediaTokenSettings = siteSettings.As<MediaTokenSettings>();

var rng = RandomNumberGenerator.Create();

mediaTokenSettings.HashKey = new byte[64];
rng.GetBytes(mediaTokenSettings.HashKey);
mediaTokenSettings.HashKey = RandomNumberGenerator.GetBytes(DefaultMediaTokenKeySize);
Copy link
Member Author

Choose a reason for hiding this comment

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

Use of static method

siteSettings.Put(mediaTokenSettings);

await _siteService.UpdateSiteSettingsAsync(siteSettings);
Expand Down
20 changes: 4 additions & 16 deletions src/OrchardCore.Modules/OrchardCore.Media/Services/AttachedMediaFieldFileService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
using System;
using System.Collections.Generic;
using System.IO.Hashing;
using System.Linq;
using System.Security.Cryptography;
using System.Text;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.Logging;
Expand Down Expand Up @@ -126,20 +125,9 @@ private string GetContentItemFolder(ContentItem contentItem)
private async Task<string> GetFileHashAsync(string filePath)
{
using var fs = await _fileStore.GetFileStreamAsync(filePath);
using HashAlgorithm hashAlgorithm = MD5.Create();
var hash = hashAlgorithm.ComputeHash(fs);
return ByteArrayToHexString(hash);
}

public static string ByteArrayToHexString(byte[] bytes)
{
var sb = new StringBuilder();
foreach (var b in bytes)
{
sb.Append(b.ToString("x2").ToLower());
}

return sb.ToString();
var hash = new XxHash32();
Copy link
Member Author

Choose a reason for hiding this comment

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

MD5/SHA1 should not be used for security reasons (I have been told by experts). xxHash32/64/128 are also much faster so the ones to use for non-cryptographic hashes.

await hash.AppendAsync(fs);
return Convert.ToHexString(hash.GetCurrentHash()).ToLowerInvariant();
}

private static string GetFileExtension(string path)
Expand Down
11 changes: 8 additions & 3 deletions src/OrchardCore.Modules/OrchardCore.Media/Services/ChunkFileUploadService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
using System;
using System.Collections.Generic;
using System.IO;
using System.IO.Hashing;
using System.Linq;
using System.Net.Http.Headers;
using System.Security.Cryptography;
using System.Text;
using System.Threading;
using System.Threading.Tasks;
Expand Down Expand Up @@ -200,9 +200,14 @@ private static FileStream CreateTemporaryFile(string tempPath, long size)

private static string CalculateHash(params string[] parts)
{
var hash = SHA256.HashData(Encoding.UTF8.GetBytes(string.Join(string.Empty, parts)));
var hash = new XxHash64();

return Convert.ToHexString(hash);
foreach (var part in parts)
{
hash.Append(Encoding.UTF8.GetBytes(part));
Copy link
Member Author

Choose a reason for hiding this comment

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

No string concatenation necessary with this one.

Copy link
Member

Choose a reason for hiding this comment

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

Note: since you're changing the algorithm, I presume the result is only used internally and can change at any time.
If so, you may want to use the UTF-16 representation here, to avoid an intermediate string allocation.

Something like hash.Append(MemoryMarshal.AsBytes<char>(part));

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is a benchmark with UTF8 vs Marshal.AsBytes, with and without the HEX encoding. Was wondering is the encoding would cancel out the benefits, still good. A little slower however, probably because of the extra hashing work.

| Method       | Mean      | Error     | StdDev   | Gen0   | Allocated |
|------------- |----------:|----------:|---------:|-------:|----------:|
| UTF8         |  77.37 ns | 39.491 ns | 2.165 ns | 0.0348 |     328 B |
| UTF8ToHex    |  82.21 ns | 51.756 ns | 2.837 ns | 0.0408 |     384 B |
| Marshal      | 104.60 ns | 25.270 ns | 1.385 ns | 0.0196 |     184 B |
| MarshalToHex |  91.35 ns |  7.423 ns | 0.407 ns | 0.0254 |     240 B |

}

return Convert.ToHexString(hash.GetCurrentHash());
}

private sealed class ChunkedFormFile : IFormFile, IDisposable
Expand Down
8 changes: 2 additions & 6 deletions src/OrchardCore.Modules/OrchardCore.Twitter/Services/TwitterClientMessageHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage

public virtual string GetNonce()
{
return Convert.ToBase64String(new ASCIIEncoding().GetBytes(_clock.UtcNow.Ticks.ToString()));
return Convert.ToBase64String(Encoding.ASCII.GetBytes(_clock.UtcNow.Ticks.ToString()));
}

public async Task ConfigureOAuthAsync(HttpRequestMessage request)
Expand Down Expand Up @@ -100,11 +100,7 @@ public async Task ConfigureOAuthAsync(HttpRequestMessage request)

var secret = string.Concat(_twitterSettings.ConsumerSecret, "&", _twitterSettings.AccessTokenSecret);

string signature;
using (var hasher = new HMACSHA1(Encoding.ASCII.GetBytes(secret)))
{
signature = Convert.ToBase64String(hasher.ComputeHash(Encoding.ASCII.GetBytes(baseString)));
}
var signature = Convert.ToBase64String(HMACSHA1.HashData(key: Encoding.UTF8.GetBytes(secret), source: Encoding.UTF8.GetBytes(baseString)));

var sb = new StringBuilder();
sb.Append("oauth_consumer_key=\"").Append(Uri.EscapeDataString(_twitterSettings.ConsumerKey)).Append("\", ");
Expand Down
Loading