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.

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

Add cryptographic operation counts to prevent process crashes #100371

Merged
merged 8 commits into from
Apr 11, 2024

Conversation

vcsjones
Copy link
Member

@vcsjones vcsjones commented Mar 27, 2024

OpenSSL, CNG, and CommonCrypto are not thread safe, and some improper uses result in hard process crashes. This is easiest to hit in paths that use initialize and reset primitives because the internal init routine frees and re-creates structures that might be in use by other operations. This has been observed in all three of our desktop platforms.

This pull request adds counts to in-flight operations for Hash and HMAC primitives so that we can throw managed exceptions in this scenario instead of process crashes.

Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

@vcsjones vcsjones changed the title Add hash operation counts to OpenSSL to prevent process crashes Add cryptographic operation counts to prevent process crashes Mar 29, 2024

_hHash = hHash;
SafeBCryptHashHandle? previousHash = Interlocked.Exchange(ref _hHash, hHash);
Copy link
Member Author

Choose a reason for hiding this comment

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

Assuming this made it past the _running guard, this

  1. Disposed the current handle in DestroyHash
  2. Nulled out _hHash
  3. Created a new hash
  4. Assigned _hHash to the new one.

If another thread attempted to use that hash during 2 or 3, then some asserts would trip because we never expected _hHash to be null.

Now, we

  1. Create a new hash.
  2. Exchange old and new
  3. Dispose of old.

This ensures we don't have a period of time where _hHash can be null and trip asserts during test runs.

@vcsjones vcsjones marked this pull request as ready for review April 1, 2024 15:07
@vcsjones
Copy link
Member Author

vcsjones commented Apr 4, 2024

Benchmarks basically showed no major change when the interlocked value is not under contention. I didn't both to benchmark when it is under contention since that is an error path.

@vcsjones vcsjones merged commit db8a764 into dotnet:main Apr 11, 2024
84 of 87 checks passed
@vcsjones vcsjones deleted the hash-no-crash branch April 11, 2024 15:23
@vcsjones vcsjones added this to the 9.0.0 milestone Apr 11, 2024
@github-actions github-actions bot locked and limited conversation to collaborators May 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants