From 75bc76033bbdbee1784dd24024f4a6e5f3a1e763 Mon Sep 17 00:00:00 2001 From: Jeremy Barton Date: Sat, 14 Mar 2020 17:03:05 -0700 Subject: [PATCH 1/2] Don't delete private keys detected from SerializedCert imports --- .../Pal.Windows/CertificatePal.Import.cs | 11 +++++-- .../tests/CertTests.cs | 22 ++++++++++++++ .../tests/CollectionTests.cs | 29 +++++++++++++++++++ 3 files changed, 60 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/CertificatePal.Import.cs b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/CertificatePal.Import.cs index 3ad280b3eddd6b..95c1cb76cf1f97 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/CertificatePal.Import.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/CertificatePal.Import.cs @@ -35,7 +35,7 @@ private static ICertificatePal FromBlobOrFile(byte[]? rawData, string? fileName, bool loadFromFile = (fileName != null); PfxCertStoreFlags pfxCertStoreFlags = MapKeyStorageFlags(keyStorageFlags); - bool persistKeySet = (0 != (keyStorageFlags & X509KeyStorageFlags.PersistKeySet)); + bool deleteKeySet = false; CertEncodingType msgAndCertEncodingType; ContentType contentType; @@ -87,9 +87,16 @@ out pCertContext if (loadFromFile) rawData = File.ReadAllBytes(fileName!); pCertContext = FilterPFXStore(rawData!, password, pfxCertStoreFlags); + + // If PersistKeySet is set we don't delete the key, so it persists. + // If EphemeralKeySet is set we don't delete the key, because there's no file, so it's a wasteful call. + const X509KeyStorageFlags DeleteUnless = + X509KeyStorageFlags.PersistKeySet | X509KeyStorageFlags.EphemeralKeySet; + + deleteKeySet = (0 == (keyStorageFlags & DeleteUnless)); } - CertificatePal pal = new CertificatePal(pCertContext, deleteKeyContainer: !persistKeySet); + CertificatePal pal = new CertificatePal(pCertContext, deleteKeySet); pCertContext = null; return pal; } diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/CertTests.cs b/src/libraries/System.Security.Cryptography.X509Certificates/tests/CertTests.cs index 95a9d7499d39c2..f53fa2e31f3aef 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/tests/CertTests.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/tests/CertTests.cs @@ -372,6 +372,28 @@ public static void X509Certificate2WithT61String() } } + [Fact] + [PlatformSpecific(TestPlatforms.Windows)] + public static void SerializedCertDisposeDoesNotRemoveKeyFile() + { + using (X509Certificate2 fromPfx = new X509Certificate2(TestData.PfxData, TestData.PfxDataPassword)) + { + Assert.True(fromPfx.HasPrivateKey, "fromPfx.HasPrivateKey - before"); + + byte[] serializedCert = fromPfx.Export(X509ContentType.SerializedCert); + + using (X509Certificate2 fromSerialized = new X509Certificate2(serializedCert)) + { + Assert.True(fromSerialized.HasPrivateKey, "fromSerialized.HasPrivateKey"); + } + + using (RSA key = fromPfx.GetRSAPrivateKey()) + { + key.SignData(serializedCert, HashAlgorithmName.SHA256, RSASignaturePadding.Pkcs1); + } + } + } + public static IEnumerable StorageFlags => CollectionImportTests.StorageFlags; } } diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/CollectionTests.cs b/src/libraries/System.Security.Cryptography.X509Certificates/tests/CollectionTests.cs index f3db79aa9a200c..d280c9b53c941d 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/tests/CollectionTests.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/tests/CollectionTests.cs @@ -1397,6 +1397,35 @@ public static void X509ExtensionCollection_OidIndexer_NoMatchByFriendlyName() } } + [Fact] + [PlatformSpecific(TestPlatforms.Windows)] + public static void SerializedCertDisposeDoesNotRemoveKeyFile() + { + using (X509Certificate2 fromPfx = new X509Certificate2(TestData.PfxData, TestData.PfxDataPassword)) + { + Assert.True(fromPfx.HasPrivateKey, "fromPfx.HasPrivateKey - before"); + + byte[] serializedCert = fromPfx.Export(X509ContentType.SerializedCert); + + X509Certificate2 fromSerialized; + + using (ImportedCollection imported = Cert.Import(serializedCert)) + { + fromSerialized = imported.Collection[0]; + Assert.True(fromSerialized.HasPrivateKey, "fromSerialized.HasPrivateKey"); + Assert.NotEqual(IntPtr.Zero, fromSerialized.Handle); + } + + // The certificate got disposed by the collection holder. + Assert.Equal(IntPtr.Zero, fromSerialized.Handle); + + using (RSA key = fromPfx.GetRSAPrivateKey()) + { + key.SignData(serializedCert, HashAlgorithmName.SHA256, RSASignaturePadding.Pkcs1); + } + } + } + private static void TestExportSingleCert_SecureStringPassword(X509ContentType ct) { using (var pfxCer = new X509Certificate2(TestData.PfxData, TestData.CreatePfxDataPasswordSecureString(), Cert.EphemeralIfPossible)) From f3ce8249ab42f4e42ca6c6a1b93472f08d4dbf04 Mon Sep 17 00:00:00 2001 From: Jeremy Barton Date: Sun, 15 Mar 2020 15:46:58 -0700 Subject: [PATCH 2/2] Apply feedback --- .../Cryptography/Pal.Windows/CertificatePal.Import.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/CertificatePal.Import.cs b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/CertificatePal.Import.cs index 95c1cb76cf1f97..8c24813b3302dc 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/CertificatePal.Import.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/CertificatePal.Import.cs @@ -35,7 +35,7 @@ private static ICertificatePal FromBlobOrFile(byte[]? rawData, string? fileName, bool loadFromFile = (fileName != null); PfxCertStoreFlags pfxCertStoreFlags = MapKeyStorageFlags(keyStorageFlags); - bool deleteKeySet = false; + bool deleteKeyContainer = false; CertEncodingType msgAndCertEncodingType; ContentType contentType; @@ -88,15 +88,15 @@ out pCertContext rawData = File.ReadAllBytes(fileName!); pCertContext = FilterPFXStore(rawData!, password, pfxCertStoreFlags); - // If PersistKeySet is set we don't delete the key, so it persists. + // If PersistKeySet is set we don't delete the key, so that it persists. // If EphemeralKeySet is set we don't delete the key, because there's no file, so it's a wasteful call. const X509KeyStorageFlags DeleteUnless = X509KeyStorageFlags.PersistKeySet | X509KeyStorageFlags.EphemeralKeySet; - deleteKeySet = (0 == (keyStorageFlags & DeleteUnless)); + deleteKeyContainer = ((keyStorageFlags & DeleteUnless) == 0); } - CertificatePal pal = new CertificatePal(pCertContext, deleteKeySet); + CertificatePal pal = new CertificatePal(pCertContext, deleteKeyContainer); pCertContext = null; return pal; }