-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Use Windows BCrypt for Sha1ForNonSecretPurposes instead of managed implementation #120633
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?
Conversation
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
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 replaces the managed SHA-1 implementation in Sha1ForNonSecretPurposes
with Windows BCrypt API calls on Windows platforms while preserving the managed implementation for non-Windows platforms. The change is motivated by compliance requirements (easier security exemptions for OS-provided implementations), performance improvements (20-58% faster), and reduced binary size on Windows.
Key changes:
- Added conditional compilation to use BCrypt on Windows (
TARGET_WINDOWS
) and managed implementation elsewhere - Windows implementation buffers input data and uses BCrypt's one-shot hash API in
Finish()
- Added required BCrypt interop declarations to the project file
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
System.Private.CoreLib.Shared.projitems | Adds BCrypt interop file references for Windows builds |
Sha1ForNonSecretPurposes.cs | Implements Windows-specific BCrypt implementation with conditional compilation directives |
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
src/libraries/System.Security.Cryptography/src/System.Security.Cryptography.csproj
Outdated
Show resolved
Hide resolved
….Cryptography.csproj
src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
Outdated
Show resolved
Hide resolved
…dows file alphabetically Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
@copilot, what happened to the original runtime/src/libraries/Common/src/System/Sha1ForNonSecretPurposes.cs file? I no longer see that being modified or deleted. Shouldn't it have just been renamed with the .Unix.cs suffix, rather than duplicating it and leaving the original? |
|
||
input.CopyTo(_buffer.AsSpan(_bufferPos)); | ||
_bufferPos += input.Length; | ||
} |
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.
@copilot, this implementation looks problematic from a performance perspective. It's buffering up all input and then only hashing it when done. In contrast, the existing implementation was streaming; it didn't need to buffer the whole input, it just kept updating the hash. Is there no way to do the same incremental hashing with BCrypt?
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.
Fixed in commit 4bcdc5f. Changed to use BCrypt's streaming hash APIs (BCryptCreateHash, BCryptHashData, BCryptFinishHash) instead of buffering all input. The implementation now maintains a SafeBCryptHashHandle and incrementally hashes data as it's appended, matching the performance characteristics of the original managed implementation.
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.
Outside browser, Sha1ForNonSecretPurposes is used for very small amount of data (100s bytes). The savings were demonstrated on one-shot bcrypt hash and they are <1 microsecond. If we switch to incremental hashing, I do not expect that we will see any savings anymore (more like regressions).
- Sha1ForNonSecretPurposes should expose new one-shot static hashing method
- The two uses of Sha1ForNonSecretPurposes in CoreLib should be switched to use this one-shot static hashing method
- BCrypt-based implementation of Sha1ForNonSecretPurposes should expose the one-shot static hashing method only. It should not expose the incremental hashing at all.
@copilot Could you please implement these changes?
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.
Done in commit 7e52c0b. Implemented the requested changes:
- Added static
HashData(ReadOnlySpan<byte>, Span<byte>)
method to both implementations - Updated the two CoreLib usages (EventSource and AssemblyNameHelpers) to use the one-shot API
- Windows implementation now only exposes the one-shot static method using BCryptHash with pseudo-handle
- Removed incremental hashing support, BCrypt streaming APIs, and SafeHandle types from Windows implementation
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
You're right - the original file should have been deleted. Fixed in commit 8f9c17a. The original Sha1ForNonSecretPurposes.cs has been removed, leaving only the platform-specific .Windows.cs and .Unix.cs versions. |
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
@copilot, there are build failures:
|
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Fixed in commit e8f0e1b. Added SafeBCryptAlgorithmHandle to System.Private.CoreLib.Shared.projitems. The BCryptCreateHash interop file has two overloads - one uses SafeBCryptAlgorithmHandle (for regular algorithm handles) and one uses nuint (for pseudo-handles). Even though we only use the pseudo-handle version, the first overload still needs SafeBCryptAlgorithmHandle to be available for compilation. |
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
@copilot, there are build failures:
|
Span<byte> source = totalLength <= 256 ? stackalloc byte[totalLength] : new byte[totalLength]; | ||
|
||
namespaceBytes.CopyTo(source); | ||
nameBytes.CopyTo(source.Slice(namespaceBytes.Length)); |
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.
@copilot, currently this always allocates a byte[] for the name, and then copies that again into another buffer, which itself might be stack or heap allocated. You could instead compute the output length of the encoding with GetByteCount, then get the single buffer to use to store the whole output, either stack or heap allocated, copying in the namespace bytes and encoding directly into the relevant sliced portion of the destination. This will cost the extra length computation, but still save the extra allocation and copy.
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.
Done in commit df3a9c3. Changed to use GetByteCount to compute the length upfront, then encode directly into the destination buffer slice. This eliminates the extra nameBytes array allocation and copy while trading off an additional length computation.
... Fixed in commit fc5be0a. Added Sha1ForNonSecretPurposes.Unix.cs to the Browser ItemGroup in System.Security.Cryptography.csproj. The Browser build uses SHAHashProvider.Browser.Managed.cs which needs the Unix implementation with its incremental hashing support (Start/Append/Finish). |
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Summary
Per reviewer feedback, implemented one-shot hashing API for better performance:
Sha1ForNonSecretPurposes.Windows.cs
- Uses BCrypt one-shot hash API (BCryptHash) with pseudo-handleSha1ForNonSecretPurposes.Unix.cs
- Managed implementation with static HashData methodBoth implementations expose a static
HashData(ReadOnlySpan<byte>, Span<byte>)
method. The Windows implementation uses the BCrypt one-shot API for optimal performance on small inputs (typical usage is ~100s of bytes). The Unix implementation wraps the existing incremental implementation and is also used for Browser builds.The EventSource GUID generation has been optimized to reduce allocations by encoding directly into the destination buffer.
Original prompt
This section details on the original issue you should resolve
<issue_title>Consider ifdefing away Sha1ForNonSecretPurposes on Windows</issue_title>
<issue_description>We have a managed implementation of SHA-1 hanging around for use in
EventSource
and related APIs. We should consider ifdefing it away and relying on the OS's underlying implementation on runtimes where we know an OS implementation exists. This can help compliance, as getting an exemption for SHA-1 for compat purposes is far easier than getting an exemption for carrying our own implementation. There are also some small perf wins.Benchmark code below the fold