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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Jul 26, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
97bba2e
Add test for thread racing certificate usage and disposal
vcsjones Jul 24, 2021
c5e4fae
Implement changes to keep the X509* alive while in use.
vcsjones Jul 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
201 changes: 114 additions & 87 deletions ...raphy.X509Certificates/src/Internal/Cryptography/Pal.Unix/OpenSslX509CertificateReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,31 +267,25 @@ public string Issuer
{
get
{
if (_issuer == null)
{
// IssuerName is mutable to callers in X509Certificate. We want to be
// able to get the issuer even if IssuerName has been mutated, so we
// don't use it here.
_issuer = Interop.Crypto.LoadX500Name(Interop.Crypto.X509GetIssuerName(_cert)).Name;
}

return _issuer;
// IssuerName is mutable to callers in X509Certificate. We want to be
// able to get the issuer even if IssuerName has been mutated, so we
// don't use it here.
return _issuer ??= UseCertInteriorData(static cert => {
return Interop.Crypto.LoadX500Name(Interop.Crypto.X509GetIssuerName(cert)).Name;
});
}
}

public string Subject
{
get
{
if (_subject == null)
{
// SubjectName is mutable to callers in X509Certificate. We want to be
// able to get the subject even if SubjectName has been mutated, so we
// don't use it here.
_subject = Interop.Crypto.LoadX500Name(Interop.Crypto.X509GetSubjectName(_cert)).Name;
}

return _subject;
// SubjectName is mutable to callers in X509Certificate. We want to be
// able to get the subject even if SubjectName has been mutated, so we
// don't use it here.
return _subject ??= UseCertInteriorData(static cert => {
return Interop.Crypto.LoadX500Name(Interop.Crypto.X509GetSubjectName(cert)).Name;
});
}
}

Expand All @@ -311,8 +305,10 @@ public string KeyAlgorithm
{
get
{
IntPtr oidPtr = Interop.Crypto.GetX509PublicKeyAlgorithm(_cert);
return Interop.Crypto.GetOidValue(oidPtr);
return UseCertInteriorData(static cert => {
IntPtr oidPtr = Interop.Crypto.GetX509PublicKeyAlgorithm(cert);
return Interop.Crypto.GetOidValue(oidPtr);
});
}
}

Expand All @@ -328,8 +324,10 @@ public byte[] PublicKeyValue
{
get
{
IntPtr keyBytesPtr = Interop.Crypto.GetX509PublicKeyBytes(_cert);
return Interop.Crypto.GetAsn1StringBytes(keyBytesPtr);
return UseCertInteriorData(static cert => {
IntPtr keyBytesPtr = Interop.Crypto.GetX509PublicKeyBytes(cert);
return Interop.Crypto.GetAsn1StringBytes(keyBytesPtr);
});
}
}

Expand All @@ -348,24 +346,31 @@ public string SignatureAlgorithm
{
get
{
IntPtr oidPtr = Interop.Crypto.GetX509SignatureAlgorithm(_cert);
return Interop.Crypto.GetOidValue(oidPtr);
return UseCertInteriorData(static cert => {
IntPtr oidPtr = Interop.Crypto.GetX509SignatureAlgorithm(cert);
return Interop.Crypto.GetOidValue(oidPtr);
});
}
}

public DateTime NotAfter
{
get
{
return ExtractValidityDateTime(Interop.Crypto.GetX509NotAfter(_cert));

return UseCertInteriorData(static cert => {
return ExtractValidityDateTime(Interop.Crypto.GetX509NotAfter(cert));
});
}
}

public DateTime NotBefore
{
get
{
return ExtractValidityDateTime(Interop.Crypto.GetX509NotBefore(_cert));
return UseCertInteriorData(static cert => {
return ExtractValidityDateTime(Interop.Crypto.GetX509NotBefore(cert));
});
}
}

Expand Down Expand Up @@ -421,103 +426,101 @@ public X500DistinguishedName SubjectName
{
get
{
if (_subjectName == null)
{
_subjectName = Interop.Crypto.LoadX500Name(Interop.Crypto.X509GetSubjectName(_cert));
}

return _subjectName;
return _subjectName ??= UseCertInteriorData(static cert => {
return Interop.Crypto.LoadX500Name(Interop.Crypto.X509GetSubjectName(cert));
});
}
}

public X500DistinguishedName IssuerName
{
get
{
if (_issuerName == null)
{
_issuerName = Interop.Crypto.LoadX500Name(Interop.Crypto.X509GetIssuerName(_cert));
}

return _issuerName;
return _issuerName ??= UseCertInteriorData(static cert => {
return Interop.Crypto.LoadX500Name(Interop.Crypto.X509GetIssuerName(cert));
});
}
}

public PolicyData GetPolicyData()
{
PolicyData policyData = default;
return UseCertInteriorData(static cert => {
PolicyData policyData = default;

int extensionCount = Interop.Crypto.X509GetExtCount(_cert);
int extensionCount = Interop.Crypto.X509GetExtCount(cert);

for (int i = 0; i < extensionCount; i++)
{
IntPtr ext = Interop.Crypto.X509GetExt(_cert, i);
Interop.Crypto.CheckValidOpenSslHandle(ext);
for (int i = 0; i < extensionCount; i++)
{
IntPtr ext = Interop.Crypto.X509GetExt(cert, i);
Interop.Crypto.CheckValidOpenSslHandle(ext);

IntPtr oidPtr = Interop.Crypto.X509ExtensionGetOid(ext);
Interop.Crypto.CheckValidOpenSslHandle(oidPtr);
string oidValue = Interop.Crypto.GetOidValue(oidPtr);
IntPtr oidPtr = Interop.Crypto.X509ExtensionGetOid(ext);
Interop.Crypto.CheckValidOpenSslHandle(oidPtr);
string oidValue = Interop.Crypto.GetOidValue(oidPtr);

IntPtr dataPtr = Interop.Crypto.X509ExtensionGetData(ext);
Interop.Crypto.CheckValidOpenSslHandle(dataPtr);
IntPtr dataPtr = Interop.Crypto.X509ExtensionGetData(ext);
Interop.Crypto.CheckValidOpenSslHandle(dataPtr);

switch (oidValue)
{
case Oids.ApplicationCertPolicies:
policyData.ApplicationCertPolicies = Interop.Crypto.GetAsn1StringBytes(dataPtr);
break;
case Oids.CertPolicies:
policyData.CertPolicies = Interop.Crypto.GetAsn1StringBytes(dataPtr);
break;
case Oids.CertPolicyMappings:
policyData.CertPolicyMappings = Interop.Crypto.GetAsn1StringBytes(dataPtr);
break;
case Oids.CertPolicyConstraints:
policyData.CertPolicyConstraints = Interop.Crypto.GetAsn1StringBytes(dataPtr);
break;
case Oids.EnhancedKeyUsage:
policyData.EnhancedKeyUsage = Interop.Crypto.GetAsn1StringBytes(dataPtr);
break;
case Oids.InhibitAnyPolicyExtension:
policyData.InhibitAnyPolicyExtension = Interop.Crypto.GetAsn1StringBytes(dataPtr);
switch (oidValue)
{
case Oids.ApplicationCertPolicies:
policyData.ApplicationCertPolicies = Interop.Crypto.GetAsn1StringBytes(dataPtr);
break;
case Oids.CertPolicies:
policyData.CertPolicies = Interop.Crypto.GetAsn1StringBytes(dataPtr);
break;
case Oids.CertPolicyMappings:
policyData.CertPolicyMappings = Interop.Crypto.GetAsn1StringBytes(dataPtr);
break;
case Oids.CertPolicyConstraints:
policyData.CertPolicyConstraints = Interop.Crypto.GetAsn1StringBytes(dataPtr);
break;
case Oids.EnhancedKeyUsage:
policyData.EnhancedKeyUsage = Interop.Crypto.GetAsn1StringBytes(dataPtr);
break;
case Oids.InhibitAnyPolicyExtension:
policyData.InhibitAnyPolicyExtension = Interop.Crypto.GetAsn1StringBytes(dataPtr);
break;
}
}
}

return policyData;
return policyData;
});
}

public IEnumerable<X509Extension> Extensions
{
get
{
int extensionCount = Interop.Crypto.X509GetExtCount(_cert);
X509Extension[] extensions = new X509Extension[extensionCount];
return UseCertInteriorData(static cert => {
int extensionCount = Interop.Crypto.X509GetExtCount(cert);
X509Extension[] extensions = new X509Extension[extensionCount];

for (int i = 0; i < extensionCount; i++)
{
IntPtr ext = Interop.Crypto.X509GetExt(_cert, i);
for (int i = 0; i < extensionCount; i++)
{
IntPtr ext = Interop.Crypto.X509GetExt(cert, i);

Interop.Crypto.CheckValidOpenSslHandle(ext);
Interop.Crypto.CheckValidOpenSslHandle(ext);

IntPtr oidPtr = Interop.Crypto.X509ExtensionGetOid(ext);
IntPtr oidPtr = Interop.Crypto.X509ExtensionGetOid(ext);

Interop.Crypto.CheckValidOpenSslHandle(oidPtr);
Interop.Crypto.CheckValidOpenSslHandle(oidPtr);

string oidValue = Interop.Crypto.GetOidValue(oidPtr);
Oid oid = new Oid(oidValue);
string oidValue = Interop.Crypto.GetOidValue(oidPtr);
Oid oid = new Oid(oidValue);

IntPtr dataPtr = Interop.Crypto.X509ExtensionGetData(ext);
IntPtr dataPtr = Interop.Crypto.X509ExtensionGetData(ext);

Interop.Crypto.CheckValidOpenSslHandle(dataPtr);
Interop.Crypto.CheckValidOpenSslHandle(dataPtr);

byte[] extData = Interop.Crypto.GetAsn1StringBytes(dataPtr);
bool critical = Interop.Crypto.X509ExtensionGetCritical(ext);
byte[] extData = Interop.Crypto.GetAsn1StringBytes(dataPtr);
bool critical = Interop.Crypto.X509ExtensionGetCritical(ext);

extensions[i] = new X509Extension(oid, extData, critical);
}
extensions[i] = new X509Extension(oid, extData, critical);
}

return extensions;
return extensions;
});
}
}

Expand Down Expand Up @@ -850,5 +853,29 @@ public byte[] Export(X509ContentType contentType, SafePasswordHandle password)
return exported;
}
}

private T UseCertInteriorData<T>(Func<SafeX509Handle, T> callback)
{
// Many of the reader's APIs perform two steps of getting an IntPtr to
// interior data of the X509* object, then passing that IntPtr to some
// other API that interprets the data in the pointer. If the SafeX509Handle
// is disposed in between the two calls, then the data in the IntPtr no longer
// points to valid data.
// To keep the X509 object alive, manually increment the reference to it.
bool addedRef = false;

try
{
_cert.DangerousAddRef(ref addedRef);
return callback(_cert);
}
finally
{
if (addedRef)
{
_cert.DangerousRelease();
}
}
}
}
}
29 changes: 29 additions & 0 deletions src/libraries/System.Security.Cryptography.X509Certificates/tests/CertTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Collections.Generic;
using System.IO;
using System.Runtime.InteropServices;
using System.Threading;
using Microsoft.DotNet.XUnitExtensions;
using Test.Cryptography;
using Xunit;
Expand All @@ -23,6 +24,34 @@ public CertTests(ITestOutputHelper output)
_log = output;
}

[Fact]
public static void RaceUseAndDisposeDoesNotCrash()
{
X509Certificate2 cert = new X509Certificate2(TestFiles.MicrosoftRootCertFile);

Thread subjThread = new Thread(static state => {
X509Certificate2 c = (X509Certificate2)state;

try
{
_ = c.Subject;
}
catch
{
// managed exceptions are okay, we are looking for runtime crashes.
}
});

Thread disposeThread = new Thread(static state => {
((X509Certificate2)state).Dispose();
});

subjThread.Start(cert);
disposeThread.Start(cert);
disposeThread.Join();
subjThread.Join();
}

[Fact]
public static void X509CertTest()
{
Expand Down