From 0964879501b67f07a1f9b060d9bf1cd8570d8535 Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Tue, 7 Jul 2020 13:29:16 -0400 Subject: [PATCH 1/6] Fix mutability of exported X509 certificate on macOS. Export(Cert) returned the original byte array from the PAL. If a caller mutated the result of the export, they would be mutating the underlying representation of RawData in the PAL. To be consistent with Windows and Linux, we return a copy in the PAL. --- .../Cryptography/Pal.OSX/AppleCertificatePal.cs | 2 +- .../tests/CertTests.cs | 11 +++++++++++ .../tests/ExportTests.cs | 11 +++++++++++ 3 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/AppleCertificatePal.cs b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/AppleCertificatePal.cs index c21beabe1e577f..134875a4527638 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/AppleCertificatePal.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/AppleCertificatePal.cs @@ -323,7 +323,7 @@ public byte[] RawData get { EnsureCertData(); - return _certData.RawData; + return _certData.RawData.CloneByteArray(); } } diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/CertTests.cs b/src/libraries/System.Security.Cryptography.X509Certificates/tests/CertTests.cs index 76dd7a64d9d8fc..9e44c9f07f74f0 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/tests/CertTests.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/tests/CertTests.cs @@ -434,6 +434,17 @@ public static void SerializedCertDisposeDoesNotRemoveKeyFile() } } + [Fact] + public static void CopyResult_RawData() + { + using (X509Certificate2 cert = new X509Certificate2(TestData.MsCertificate)) + { + byte[] first = cert.RawData; + byte[] second = cert.RawData; + Assert.NotSame(first, second); + } + } + public static IEnumerable StorageFlags => CollectionImportTests.StorageFlags; } } diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/ExportTests.cs b/src/libraries/System.Security.Cryptography.X509Certificates/tests/ExportTests.cs index 30a1b19bb6893b..9cbd31aed11495 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/tests/ExportTests.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/tests/ExportTests.cs @@ -8,6 +8,17 @@ namespace System.Security.Cryptography.X509Certificates.Tests { public static class ExportTests { + [Fact] + public static void ExportAsCert_CreatesCopy() + { + using (X509Certificate2 cert = new X509Certificate2(TestData.MsCertificate)) + { + byte[] first = cert.Export(X509ContentType.Cert); + byte[] second = cert.Export(X509ContentType.Cert); + Assert.NotSame(first, second); + } + } + [Fact] public static void ExportAsCert() { From e96726d36e9ac688e68f762e52505c92cb578a09 Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Tue, 7 Jul 2020 14:05:20 -0400 Subject: [PATCH 2/6] Create copies of SubjectName and IssuerName. --- .../X509Certificates/X509Certificate2.cs | 4 ++-- .../tests/CertTests.cs | 24 +++++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509Certificate2.cs b/src/libraries/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509Certificate2.cs index f1cf8f86904339..957ab2661b8e23 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509Certificate2.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509Certificate2.cs @@ -275,7 +275,7 @@ public X500DistinguishedName IssuerName X500DistinguishedName? issuerName = _lazyIssuerName; if (issuerName == null) issuerName = _lazyIssuerName = Pal.IssuerName; - return issuerName; + return new X500DistinguishedName(issuerName); } } @@ -356,7 +356,7 @@ public X500DistinguishedName SubjectName X500DistinguishedName? subjectName = _lazySubjectName; if (subjectName == null) subjectName = _lazySubjectName = Pal.SubjectName; - return subjectName; + return new X500DistinguishedName(subjectName); } } diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/CertTests.cs b/src/libraries/System.Security.Cryptography.X509Certificates/tests/CertTests.cs index 9e44c9f07f74f0..53ef0807f23b60 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/tests/CertTests.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/tests/CertTests.cs @@ -445,6 +445,30 @@ public static void CopyResult_RawData() } } + [Fact] + public static void CopyResult_IssuerName() + { + using (X509Certificate2 cert = new X509Certificate2(TestData.MsCertificate)) + { + X500DistinguishedName first = cert.IssuerName; + X500DistinguishedName second = cert.IssuerName; + Assert.NotSame(first, second); + Assert.NotSame(first.RawData, second.RawData); + } + } + + [Fact] + public static void CopyResult_SubjectName() + { + using (X509Certificate2 cert = new X509Certificate2(TestData.MsCertificate)) + { + X500DistinguishedName first = cert.SubjectName; + X500DistinguishedName second = cert.SubjectName; + Assert.NotSame(first, second); + Assert.NotSame(first.RawData, second.RawData); + } + } + public static IEnumerable StorageFlags => CollectionImportTests.StorageFlags; } } From c88ac64d496ad5fa7695a1053633c5427c44a04e Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Tue, 7 Jul 2020 15:19:53 -0400 Subject: [PATCH 3/6] Allow mutation of X500DN properties but split from string properties. --- .../Pal.OSX/AppleCertificatePal.cs | 19 ++++++++++++++++--- .../Cryptography/Pal.OSX/CertificateData.cs | 4 ++++ .../X509Certificates/X509Certificate2.cs | 4 ++-- .../tests/CertTests.cs | 18 ++++++++---------- 4 files changed, 30 insertions(+), 15 deletions(-) diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/AppleCertificatePal.cs b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/AppleCertificatePal.cs index 134875a4527638..5c72917edb960a 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/AppleCertificatePal.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/AppleCertificatePal.cs @@ -186,10 +186,23 @@ public IntPtr Handle } } + public string Issuer + { + get + { + EnsureCertData(); + return _certData.IssuerName; + } + } - public string Issuer => IssuerName.Name; - - public string Subject => SubjectName.Name; + public string Subject + { + get + { + EnsureCertData(); + return _certData.SubjectName; + } + } public string LegacyIssuer => IssuerName.Decode(X500DistinguishedNameFlags.None); diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/CertificateData.cs b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/CertificateData.cs index dded057cc62e89..613976dc7c8854 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/CertificateData.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/CertificateData.cs @@ -50,6 +50,8 @@ public AlgorithmIdentifier(AlgorithmIdentifierAsn algorithmIdentifier) internal X500DistinguishedName Issuer; internal X500DistinguishedName Subject; internal List Extensions; + internal string IssuerName; + internal string SubjectName; internal int Version => certificate.TbsCertificate.Version; @@ -82,6 +84,8 @@ internal CertificateData(byte[] rawData) certificate.TbsCertificate.ValidateVersion(); Issuer = new X500DistinguishedName(certificate.TbsCertificate.Issuer.ToArray()); Subject = new X500DistinguishedName(certificate.TbsCertificate.Subject.ToArray()); + IssuerName = Issuer.Name; + SubjectName = Subject.Name; AsnWriter writer = new AsnWriter(AsnEncodingRules.DER); certificate.TbsCertificate.SubjectPublicKeyInfo.Encode(writer); diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509Certificate2.cs b/src/libraries/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509Certificate2.cs index 957ab2661b8e23..f1cf8f86904339 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509Certificate2.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509Certificate2.cs @@ -275,7 +275,7 @@ public X500DistinguishedName IssuerName X500DistinguishedName? issuerName = _lazyIssuerName; if (issuerName == null) issuerName = _lazyIssuerName = Pal.IssuerName; - return new X500DistinguishedName(issuerName); + return issuerName; } } @@ -356,7 +356,7 @@ public X500DistinguishedName SubjectName X500DistinguishedName? subjectName = _lazySubjectName; if (subjectName == null) subjectName = _lazySubjectName = Pal.SubjectName; - return new X500DistinguishedName(subjectName); + return subjectName; } } diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/CertTests.cs b/src/libraries/System.Security.Cryptography.X509Certificates/tests/CertTests.cs index 53ef0807f23b60..59c8b93b919dae 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/tests/CertTests.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/tests/CertTests.cs @@ -446,26 +446,24 @@ public static void CopyResult_RawData() } [Fact] - public static void CopyResult_IssuerName() + public static void MutateDistinguishedName_IssuerName_DoesNotImpactIssuer() { using (X509Certificate2 cert = new X509Certificate2(TestData.MsCertificate)) { - X500DistinguishedName first = cert.IssuerName; - X500DistinguishedName second = cert.IssuerName; - Assert.NotSame(first, second); - Assert.NotSame(first.RawData, second.RawData); + byte[] issuerBytes = cert.IssuerName.RawData; + Array.Clear(issuerBytes, 0, issuerBytes.Length); + Assert.Equal("CN=Microsoft Code Signing PCA, O=Microsoft Corporation, L=Redmond, S=Washington, C=US", cert.Issuer); } } [Fact] - public static void CopyResult_SubjectName() + public static void MutateDistinguishedName_SubjectName_DoesNotImpactSubject() { using (X509Certificate2 cert = new X509Certificate2(TestData.MsCertificate)) { - X500DistinguishedName first = cert.SubjectName; - X500DistinguishedName second = cert.SubjectName; - Assert.NotSame(first, second); - Assert.NotSame(first.RawData, second.RawData); + byte[] subjectBytes = cert.SubjectName.RawData; + Array.Clear(subjectBytes, 0, subjectBytes.Length); + Assert.Equal("CN=Microsoft Corporation, OU=MOPR, O=Microsoft Corporation, L=Redmond, S=Washington, C=US", cert.Subject); } } From 1e554e12ab22c570043ec318e514b855970e89bb Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Tue, 7 Jul 2020 15:34:06 -0400 Subject: [PATCH 4/6] Fix OpenSSL PAL to prevent side effects between properties. --- .../Pal.Unix/OpenSslX509CertificateReader.cs | 34 +++++++++++++++++-- 1 file changed, 32 insertions(+), 2 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 b5c6809a8c2fb6..15e85b2606c106 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 @@ -24,6 +24,8 @@ internal sealed class OpenSslX509CertificateReader : ICertificatePal private SafeEvpPKeyHandle? _privateKey; private X500DistinguishedName? _subjectName; private X500DistinguishedName? _issuerName; + private string? _subject; + private string? _issuer; public static ICertificatePal FromHandle(IntPtr handle) { @@ -259,9 +261,37 @@ internal SafeX509Handle SafeHandle get { return _cert; } } - public string Issuer => IssuerName.Name; + 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; + } + } - public string Subject => SubjectName.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; + } + } public string LegacyIssuer => IssuerName.Decode(X500DistinguishedNameFlags.None); From 9222336a21427fba242c4efd18fb02fadcc3e183 Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Wed, 8 Jul 2020 13:00:03 -0400 Subject: [PATCH 5/6] CI trigger From 3d57f6bb09d07953b96ea7977818d77fc9ca50f1 Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Thu, 9 Jul 2020 09:19:08 -0400 Subject: [PATCH 6/6] CI trigger