- 
                Notifications
    You must be signed in to change notification settings 
- Fork 5.2k
Add cryptographic operation counts to prevent process crashes #100371
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
Conversation
| Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones | 
        
          
                src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/Shake128.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/Shake128.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...ibraries/System.Security.Cryptography/src/System/Security/Cryptography/ConcurrentSafeKmac.cs
          
            Show resolved
            Hide resolved
        
      | } | ||
|  | ||
| _hHash = hHash; | ||
| SafeBCryptHashHandle? previousHash = Interlocked.Exchange(ref _hHash, hHash); | 
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.
Assuming this made it past the _running guard, this
- Disposed the current handle in DestroyHash
- Nulled out _hHash
- Created a new hash
- Assigned _hHashto 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
- Create a new hash.
- Exchange old and new
- Dispose of old.
This ensures we don't have a period of time where _hHash can be null and trip asserts during test runs.
| 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. | 
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.