From 97bba2e894a3f15834ba57d0ae4592f51ea6176f Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Sat, 24 Jul 2021 21:04:37 +0000 Subject: [PATCH 1/2] Add test for thread racing certificate usage and disposal --- .../tests/CertTests.cs | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/CertTests.cs b/src/libraries/System.Security.Cryptography.X509Certificates/tests/CertTests.cs index 24363f1ab4e42..98cd5c47cbdc7 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/tests/CertTests.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/tests/CertTests.cs @@ -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; @@ -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() { From c5e4faeb50a7a5b30125bce1628c09004eab6f62 Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Sun, 25 Jul 2021 14:19:09 -0400 Subject: [PATCH 2/2] Implement changes to keep the X509* alive while in use. --- .../Pal.Unix/OpenSslX509CertificateReader.cs | 201 ++++++++++-------- 1 file changed, 114 insertions(+), 87 deletions(-) diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/OpenSslX509CertificateReader.cs b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/OpenSslX509CertificateReader.cs index 942c6251f6cba..27f1df388e7f1 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/OpenSslX509CertificateReader.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/OpenSslX509CertificateReader.cs @@ -267,15 +267,12 @@ 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; + }); } } @@ -283,15 +280,12 @@ 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; + }); } } @@ -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); + }); } } @@ -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); + }); } } @@ -348,8 +346,10 @@ 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); + }); } } @@ -357,7 +357,10 @@ public DateTime NotAfter { get { - return ExtractValidityDateTime(Interop.Crypto.GetX509NotAfter(_cert)); + + return UseCertInteriorData(static cert => { + return ExtractValidityDateTime(Interop.Crypto.GetX509NotAfter(cert)); + }); } } @@ -365,7 +368,9 @@ public DateTime NotBefore { get { - return ExtractValidityDateTime(Interop.Crypto.GetX509NotBefore(_cert)); + return UseCertInteriorData(static cert => { + return ExtractValidityDateTime(Interop.Crypto.GetX509NotBefore(cert)); + }); } } @@ -421,12 +426,9 @@ 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)); + }); } } @@ -434,90 +436,91 @@ 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 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; + }); } } @@ -850,5 +853,29 @@ public byte[] Export(X509ContentType contentType, SafePasswordHandle password) return exported; } } + + private T UseCertInteriorData(Func 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(); + } + } + } } }