Repository navigation
[release/8.0-staging] Fix Duplicate ownership state machine under concurrency for NCrypt handles - #135454
Merged
ViveliDuCh merged 2 commits intoOct 9, 2026
Conversation
…ndles Duplicating NCrypt safe handles is not currently thread safe. This is observable through `CngKey.Handle` which calls `Duplicate` on a `SafeNCryptKeyHandle`. The crux of the issue is that two threads can both observe `_ownershipState == Owner` and enter `DuplicateOwnerHandle()`. Each creates a different holder for the same native handle. The first transitions the original handle to `Duplicate`, the second then hits the assertion when it attempts the same transition. This assert was observed in #130760. In a release build, this can result in an invalid `NCryptKeyHandle` getting passed to native functions. 1. Both threads create separate holders for the same raw NCrypt handle. 2. The later thread overwrites the original handle's _holder . 3. Managed reference counts no longer represent the native handle's actual users. 4. The winning holder can call `NCryptFreeObject` while duplicates associated with an overwritten holder remain open. This simplest solution is to take a lock, which will unfortunately ding performance a little bit when accessing `Handle`. A unit test was added to attempt racing the handles. Before the fix, this test would fail quickly with ``` Error Message: Assert.All() Failure: 7 out of 8 items in the collection did not pass. [0]: Item: Microsoft.VisualStudio.TestPlatform.TestHost.DebugAssertException: Method Debug.Fail failed with '_ownershipState == OwnershipState.Owner ', and was translated to Microsoft.VisualStudio.TestPlatform.TestHost.DebugAssertException to avoid terminating the process hosting the test. at Microsoft.Win32.SafeHandles.SafeNCryptHandle.DuplicateOwnerHandle[T]() in E:\code\runtime\src\libraries\System.Security.Cryptography\src\Microsoft\Win32\SafeHandles\NCryptSafeHandles.cs:line 263 at Microsoft.Win32.SafeHandles.SafeNCryptHandle.Duplicate[T]() in E:\code\runtime\src\libraries\System.Security.Cryptography\src\Microsoft\Win32\SafeHandles\NCryptSafeHandles.cs:line 219 at Microsoft.Win32.SafeHandles.SafeNCryptKeyHandle.Duplicate() in E:\code\runtime\src\libraries\System.Security.Cryptography\src\Microsoft\Win32\SafeHandles\NCryptSafeHandles.cs:line 364 at System.Security.Cryptography.CngKey.get_Handle() in E:\code\runtime\src\libraries\System.Security.Cryptography\src\System\Security\Cryptography\CngKey.StandardProperties.cs:line 93 at System.Security.Cryptography.Tests.CngKeyTests.<>c__DisplayClass3_1.<Handle_ConcurrentFirstAccess>b__0() in E:\code\runtime\src\libraries\System.Security.Cryptography\tests\CngKeyTests.cs:line 86 at System.Threading.Thread.StartCallback(Thread* pThread) in E:\code\runtime\src\coreclr\System.Private.CoreLib\src\System\Threading\Thread.CoreCLR.cs:line 117 ``` After the lock the test consistently passes. Doing this lock-free is non-trivial. There are multiple fields we need to ensure get published atomically, namely `_ownershipState`, `_holder`, and `_parentHandle`. However we avoid the lock entirely unless the current state is `OwnershipState.Owner`.
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @bartonjs, @vcsjones, @dotnet/area-system-security |
Member
Author
|
Servicing-approved via email |
vcsjones
approved these changes
Oct 9, 2026
This was referenced Oct 9, 2026
Member
|
Hi, Friendly reminder that: The code complete date for 8.0.33 (the November 2026 release) is Monday 12 October. Make sure to merge this PR on that date at the latest, or it won't make it into that release. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #130804 to release/8.0-staging
/cc @bartonjs @vcsjones
Customer Impact
The invalid state was detected by an assert in a CI run, in #130760, but customer reports of ObjectDisposedException being thrown from these classes during finalization appear to be the same problem.
In the worst case, the customer impact is the aforementioned exception during finalization, which tears down the process.
The invalid state is entered into by a race condition that can only be encountered when two threads simultaneously use a key object for the first time. Parallel access once the first use completes is/was not subject to the race.
Regression
The logic hasn't changed since .NET Core 1.0.
Testing
The change includes a test that does aggressive parallelization.
Risk
Low. It is a simple lock addition around the code susceptible to the race.