Skip to content

Commit

Permalink
Fix more SafeHandle cleanup in Windows crypto PAL (#72116)
Browse files Browse the repository at this point in the history 
* Fix more SafeHandle cleanup in Windows crypto PAL

Most of these are actually on the success path.  These were highlighted via the System.Net.WebSockets.Client tests with the new checked SafeHandle finalization flag.

* Apply suggestions from code review

Co-authored-by: Jeremy Barton <jbarton@microsoft.com>

Co-authored-by: Jeremy Barton <jbarton@microsoft.com>
  • Loading branch information
@stephentoub @bartonjs
stephentoub and bartonjs committed Jul 14, 2022
1 parent 6247106 commit 125b73e
Show file tree
Hide file tree
Showing 7 changed files with 69 additions and 47 deletions.
7 changes: 7 additions & 0 deletions ...graphy/src/System/Security/Cryptography/X509Certificates/CertificatePal.Windows.Import.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,12 +74,17 @@ out pCertContext

if (contentType == Interop.Crypt32.ContentType.CERT_QUERY_CONTENT_PKCS7_SIGNED || contentType == Interop.Crypt32.ContentType.CERT_QUERY_CONTENT_PKCS7_SIGNED_EMBED)
{
pCertContext?.Dispose();
pCertContext = GetSignerInPKCS7Store(hCertStore, hCryptMsg);
}
else if (contentType == Interop.Crypt32.ContentType.CERT_QUERY_CONTENT_PFX)
{
if (loadFromFile)
{
rawData = File.ReadAllBytes(fileName!);
}

pCertContext?.Dispose();
pCertContext = FilterPFXStore(rawData, password, pfxCertStoreFlags);

// If PersistKeySet is set we don't delete the key, so that it persists.
Expand Down Expand Up @@ -198,13 +203,15 @@ private static SafeCertContextHandle FilterPFXStore(
if (pCertContext.IsInvalid)
{
// Doesn't have a private key but hang on to it anyway in case we don't find any certs with a private key.
pCertContext.Dispose();
pCertContext = pEnumContext.Duplicate();
}
}
}

if (pCertContext.IsInvalid)
{
pCertContext.Dispose();
throw new CryptographicException(SR.Cryptography_Pfx_NoCertificates);
}

Expand Down
11 changes: 7 additions & 4 deletions ...hy/src/System/Security/Cryptography/X509Certificates/CertificatePal.Windows.PrivateKey.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,11 +202,14 @@ public ICertificatePal CopyWithPrivateKey(RSA rsa)
private T? GetPrivateKey<T>(Func<CspParameters, T> createCsp, Func<CngKey, T> createCng) where T : AsymmetricAlgorithm
{
CngKeyHandleOpenOptions cngHandleOptions;
SafeNCryptKeyHandle? ncryptKey = TryAcquireCngPrivateKey(CertContext, out cngHandleOptions);
if (ncryptKey != null)
using (SafeCertContextHandle certContext = GetCertContext())
{
CngKey cngKey = CngKey.Open(ncryptKey, cngHandleOptions);
return createCng(cngKey);
SafeNCryptKeyHandle? ncryptKey = TryAcquireCngPrivateKey(certContext, out cngHandleOptions);
if (ncryptKey != null)
{
CngKey cngKey = CngKey.Open(ncryptKey, cngHandleOptions);
return createCng(cngKey);
}
}

CspParameters? cspParameters = GetPrivateKeyCsp();
Expand Down
18 changes: 8 additions & 10 deletions ....Cryptography/src/System/Security/Cryptography/X509Certificates/CertificatePal.Windows.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -475,14 +475,11 @@ public void Dispose()
}
}

internal SafeCertContextHandle CertContext
internal SafeCertContextHandle GetCertContext()
{
get
{
SafeCertContextHandle certContext = Interop.Crypt32.CertDuplicateCertificateContext(_certContext.DangerousGetHandle());
GC.KeepAlive(_certContext);
return certContext;
}
SafeCertContextHandle certContext = Interop.Crypt32.CertDuplicateCertificateContext(_certContext.DangerousGetHandle());
GC.KeepAlive(_certContext);
return certContext;
}

private static Interop.Crypt32.CertNameType MapNameType(X509NameType nameType)
Expand Down Expand Up @@ -530,9 +527,10 @@ private CertificatePal(SafeCertContextHandle certContext, bool deleteKeyContaine
{
// We need to delete any associated key container upon disposition. Thus, replace the safehandle we got with a safehandle whose
// Release() method performs the key container deletion.
SafeCertContextHandle oldCertContext = certContext;
certContext = Interop.Crypt32.CertDuplicateCertificateContextWithKeyContainerDeletion(oldCertContext.DangerousGetHandle());
GC.KeepAlive(oldCertContext);
using (SafeCertContextHandle oldCertContext = certContext)
{
certContext = Interop.Crypt32.CertDuplicateCertificateContextWithKeyContainerDeletion(oldCertContext.DangerousGetHandle());
}
}
_certContext = certContext;
}
Expand Down
8 changes: 6 additions & 2 deletions ...tography/src/System/Security/Cryptography/X509Certificates/ChainPal.Windows.BuildChain.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,13 @@ internal sealed partial class ChainPal : IDisposable, IChainPal
Interop.Crypt32.FILETIME ft = Interop.Crypt32.FILETIME.FromDateTime(verificationTime);
Interop.Crypt32.CertChainFlags flags = MapRevocationFlags(revocationMode, revocationFlag, disableAia);
SafeX509ChainHandle chain;
if (!Interop.Crypt32.CertGetCertificateChain(storeHandle.DangerousGetHandle(), certificatePal.CertContext, &ft, extraStoreHandle, ref chainPara, flags, IntPtr.Zero, out chain))
using (SafeCertContextHandle certContext = certificatePal.GetCertContext())
{
return null;
if (!Interop.Crypt32.CertGetCertificateChain(storeHandle.DangerousGetHandle(), certContext, &ft, extraStoreHandle, ref chainPara, flags, IntPtr.Zero, out chain))
{
chain.Dispose();
return null;
}
}

return new ChainPal(chain);
Expand Down
55 changes: 32 additions & 23 deletions ...Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.Windows.Import.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,12 +118,15 @@ internal static partial IExportPal FromCertificate(ICertificatePalCore cert)
Interop.Crypt32.CertStoreFlags.CERT_STORE_ENUM_ARCHIVED_FLAG | Interop.Crypt32.CertStoreFlags.CERT_STORE_CREATE_NEW_FLAG | Interop.Crypt32.CertStoreFlags.CERT_STORE_DEFER_CLOSE_UNTIL_LAST_FREE_FLAG,
null);

if (certStore.IsInvalid ||
!Interop.Crypt32.CertAddCertificateLinkToStore(certStore, certificatePal.CertContext, Interop.Crypt32.CertStoreAddDisposition.CERT_STORE_ADD_ALWAYS, IntPtr.Zero))
using (SafeCertContextHandle certContext = certificatePal.GetCertContext())
{
Exception e = Marshal.GetHRForLastWin32Error().ToCryptographicException();
certStore?.Dispose();
throw e;
if (certStore.IsInvalid ||
!Interop.Crypt32.CertAddCertificateLinkToStore(certStore, certContext, Interop.Crypt32.CertStoreAddDisposition.CERT_STORE_ADD_ALWAYS, IntPtr.Zero))
{
Exception e = Marshal.GetHRForLastWin32Error().ToCryptographicException();
certStore.Dispose();
throw e;
}
}

return new StorePal(certStore);
Expand All @@ -144,30 +147,36 @@ internal static partial IExportPal LinkFromCertificateCollection(X509Certificate
IntPtr.Zero,
Interop.Crypt32.CertStoreFlags.CERT_STORE_ENUM_ARCHIVED_FLAG | Interop.Crypt32.CertStoreFlags.CERT_STORE_CREATE_NEW_FLAG,
null);
if (certStore.IsInvalid)
try
{
Exception e = Marshal.GetHRForLastWin32Error().ToCryptographicException();
certStore?.Dispose();
throw e;
}
if (certStore.IsInvalid)
{
throw Marshal.GetHRForLastWin32Error().ToCryptographicException();
}

//
// We use CertAddCertificateLinkToStore to keep a link to the original store, so any property changes get
// applied to the original store. This has a limit of 99 links per cert context however.
//
//
// We use CertAddCertificateLinkToStore to keep a link to the original store, so any property changes get
// applied to the original store. This has a limit of 99 links per cert context however.
//

for (int i = 0; i < certificates.Count; i++)
{
SafeCertContextHandle certContext = ((CertificatePal)certificates[i].Pal!).CertContext;
if (!Interop.Crypt32.CertAddCertificateLinkToStore(certStore, certContext, Interop.Crypt32.CertStoreAddDisposition.CERT_STORE_ADD_ALWAYS, IntPtr.Zero))
for (int i = 0; i < certificates.Count; i++)
{
Exception e = Marshal.GetLastWin32Error().ToCryptographicException();
certStore.Dispose();
throw e;
using (SafeCertContextHandle certContext = ((CertificatePal)certificates[i].Pal!).GetCertContext())
{
if (!Interop.Crypt32.CertAddCertificateLinkToStore(certStore, certContext, Interop.Crypt32.CertStoreAddDisposition.CERT_STORE_ADD_ALWAYS, IntPtr.Zero))
{
throw Marshal.GetLastWin32Error().ToCryptographicException();
}
}
}
}

return new StorePal(certStore);
return new StorePal(certStore);
}
catch
{
certStore.Dispose();
throw;
}
}

internal static partial IStorePal FromSystemStore(string storeName, StoreLocation storeLocation, OpenFlags openFlags)
Expand Down
14 changes: 7 additions & 7 deletions ...curity.Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.Windows.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,17 @@ public void CopyTo(X509Certificate2Collection collection)

public void Add(ICertificatePal certificate)
{
if (!Interop.Crypt32.CertAddCertificateContextToStore(_certStore, ((CertificatePal)certificate).CertContext, Interop.Crypt32.CertStoreAddDisposition.CERT_STORE_ADD_REPLACE_EXISTING_INHERIT_PROPERTIES, IntPtr.Zero))
throw Marshal.GetLastWin32Error().ToCryptographicException();
using (SafeCertContextHandle certContext = ((CertificatePal)certificate).GetCertContext())
{
if (!Interop.Crypt32.CertAddCertificateContextToStore(_certStore, certContext, Interop.Crypt32.CertStoreAddDisposition.CERT_STORE_ADD_REPLACE_EXISTING_INHERIT_PROPERTIES, IntPtr.Zero))
throw Marshal.GetLastWin32Error().ToCryptographicException();
}
}

public void Remove(ICertificatePal certificate)
public unsafe void Remove(ICertificatePal certificate)
{
unsafe
using (SafeCertContextHandle existingCertContext = ((CertificatePal)certificate).GetCertContext())
{
SafeCertContextHandle existingCertContext = ((CertificatePal)certificate).CertContext;
SafeCertContextHandle? enumCertContext = null;
Interop.Crypt32.CERT_CONTEXT* pCertContext = existingCertContext.CertContext;
if (!Interop.crypt32.CertFindCertificateInStore(_certStore, Interop.Crypt32.CertFindType.CERT_FIND_EXISTING, pCertContext, ref enumCertContext))
Expand All @@ -66,8 +68,6 @@ public void Remove(ICertificatePal certificate)
Interop.Crypt32.CERT_CONTEXT* pCertContextToDelete = enumCertContext.Disconnect(); // CertDeleteCertificateFromContext always frees the context (even on error)
if (!Interop.Crypt32.CertDeleteCertificateFromStore(pCertContextToDelete))
throw Marshal.GetLastWin32Error().ToCryptographicException();

GC.KeepAlive(existingCertContext);
}
}

Expand Down
3 changes: 2 additions & 1 deletion ...yptography/src/System/Security/Cryptography/X509Certificates/X509Pal.Windows.PublicKey.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ private static TAlgorithm DecodeECPublicKey<TAlgorithm>(
{
TAlgorithm key;

using (SafeBCryptKeyHandle bCryptKeyHandle = ImportPublicKeyInfo(certificatePal.CertContext, importFlags))
using (SafeCertContextHandle certContext = certificatePal.GetCertContext())
using (SafeBCryptKeyHandle bCryptKeyHandle = ImportPublicKeyInfo(certContext, importFlags))
{
CngKeyBlobFormat blobFormat;
byte[] keyBlob;
Expand Down

0 comments on commit 125b73e

Please sign in to comment.