Skip to content

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.

@ghost ghost added the area-System.Security label Mar 27, 2024
@dotnet-policy-service
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.

@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