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

[release/8.0-staging]: Add cryptographic operation counts to prevent process crashes #101737

Merged

Conversation

vcsjones
Copy link
Member

@vcsjones vcsjones commented Apr 30, 2024

Partial backport of #100371. Only the changes for Unix were back-ported as that is where all customer feedback has come from.

Customer Impact

  • Customer reported
  • Found internally

There have been a number of reports from the community where upon upgrading to .NET 8 docker images they report process crashes (#101722, #93205). This is due to two things happening.

  1. The first is that the dotnet docker images for 8.0 move to Debian Bookworm, which changes OpenSSL to version 3.0, from 1.1 in previous Debian images.
  2. Developers improperly using instances of IncrementalHash or HashAlgorithm and its derived types in a way that results in concurrent use (multithreaded).

Instances of HashAlgorithm never were thread-safe, however misuse acted in such a way that did not result in process crashes. We have observed that this concurrent use, when combined with OpenSSL 3.0, results in more frequent process crashes. These crashes are difficult for customers to diagnose because they occur in native code.

This change detects the misuse of concurrent operations and throws a managed CryptographicException with a message informing of the incorrect concurrent use.

Regression

  • Yes
  • No
  • Sort of

Developers perceive this as a regression in .NET 8 because the docker image for the .NET 8 SDK and runtime change the behavior of a dependency.

Testing

There are unit tests for this in the main branch of the runtime. The tests were not back ported because they were written with the assumption of all platforms.

Risk

Medium-Low.

Non-concurrent use is verified with the plethora of tests already present for the types. The new concurrency guard has an advantage, and a disadvantage. The advantage is that when the race condition would have lined up for a process termination it now results in an exception (so something with a call stack to give a hint as to what went wrong). The disadvantage is that our concurrency guard is wider than the native race, so what would have been "near misses" will also now throw (however, it would be quite difficult for a near miss to be reliable, as it is dependent upon the thread scheduler).

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.

@bartonjs bartonjs added the Servicing-consider Issue for next servicing release review label Apr 30, 2024
@vcsjones vcsjones force-pushed the linux-concurrent-hash branch from ec2d805 to d4f91aa Compare April 30, 2024 21:46
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels May 2, 2024
@rbhanda rbhanda modified the milestones: 8.0.x, 8.0.6 May 2, 2024
@bartonjs bartonjs merged commit 70f5112 into dotnet:release/8.0-staging May 2, 2024
109 of 110 checks passed
@vcsjones vcsjones deleted the linux-concurrent-hash branch May 2, 2024 21:40
@github-actions github-actions bot locked and limited conversation to collaborators Jun 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants