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

Keep X509 handle alive while in use when reading certificate data #56277

Merged
merged 2 commits into from
Jul 26, 2021

Conversation

vcsjones
Copy link
Member

@vcsjones vcsjones commented Jul 25, 2021

The X509 certificate reader for OpenSSL does several operations in two steps. First, get a pointer to some interior data of the X509* object, and then pass that pointer off to some other API that knows how to interpret the data that is in the pointer.

If the X509SafeHandle is freed between these two steps, then the interior data pointer no longer points to valid data. This change keeps the SafeX509Handle and the X509 OpenSSL object alive while it is in use. This prevents crashes if a X509Certificate2 object is being used while it is in the middle of being disposed.

This also adds a specific test that tries to reproduce the issue. It will rarely get hit, but often enough that it would show up as a flaky test very soon if the behavior regresses.

PR is best reviewed ignoring white space.

Closes #49732

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 25, 2021
@ghost
Copy link

ghost commented Jul 25, 2021

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

The X509 certificate reader in for OpenSSL does several operations in two steps. First, get a pointer to some interior data of the X509* object, and then pass that pointer off to some other API that knows how to interpret the data that is in the pointer.

If the X509SafeHandle is freed between these two steps, then the interior data pointer no longer points to valid data. This change keeps the SafeX509Handle and the X509 OpenSSL object alive while it is in use. This prevents crashes if a X509Certificate2 object is being used while it is in the middle of being disposed.

This also adds a specific test that tries to reproduce the issue. It will rarely get hit, but often enough that it would show up as a flaky test very soon if the behavior regresses.

PR is best reviewed ignoring white space.

Closes #49732

Author: vcsjones
Assignees: -
Labels:

area-System.Security, community-contribution

Milestone: -

@vcsjones
Copy link
Member Author

That test failed about 1/2000 runs for me. I let it run several million times after making the associated changes and was no longer able to reproduce the issue.

@stephentoub stephentoub merged commit 10c2383 into dotnet:main Jul 26, 2021
@vcsjones vcsjones deleted the x509-race branch July 26, 2021 12:25
@ghost ghost locked as resolved and limited conversation to collaborators Aug 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System.Security.Cryptography.X509Certificates.Tests: Assertion `0 && "Huge length X509_NAME"' failed.
3 participants