Skip to content

Commit

Permalink
fix certificate ctx on windows and macOS (#39818)
Browse files Browse the repository at this point in the history 
* fix certificate ctx on windows

* fix linux

* fix osx pal

* feedback from review

* feedback from review
  • Loading branch information
@wfurt
wfurt committed Aug 10, 2020
1 parent 495e929 commit eb243f0
Show file tree
Hide file tree
Showing 12 changed files with 236 additions and 107 deletions.
74 changes: 20 additions & 54 deletions src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,9 @@ private static SafeSslHandle CreateSslContext(SafeFreeSslCredentials credential,
SetProtocols(sslContext, credential.Protocols);
}

if (credential.Certificate != null)
if (credential.CertificateContext != null)
{
SetCertificate(sslContext, credential.Certificate);
SetCertificate(sslContext, credential.CertificateContext);
}

Interop.AppleCrypto.SslBreakOnServerAuth(sslContext, true);
Expand Down Expand Up @@ -315,68 +315,34 @@ private static void SetProtocols(SafeSslHandle sslContext, SslProtocols protocol
Interop.AppleCrypto.SslSetMaxProtocolVersion(sslContext, maxProtocolId);
}

private static void SetCertificate(SafeSslHandle sslContext, X509Certificate2 certificate)
private static void SetCertificate(SafeSslHandle sslContext, SslStreamCertificateContext context)
{
Debug.Assert(sslContext != null, "sslContext != null");
Debug.Assert(certificate != null, "certificate != null");
Debug.Assert(certificate.HasPrivateKey, "certificate.HasPrivateKey");

X509Chain chain = TLSCertificateExtensions.BuildNewChain(
certificate,
includeClientApplicationPolicy: false)!;

using (chain)
{
X509ChainElementCollection elements = chain.ChainElements;

// We need to leave off the EE (first) and root (last) certificate from the intermediates.
X509Certificate2[] intermediateCerts = elements.Count < 3
? Array.Empty<X509Certificate2>()
: new X509Certificate2[elements.Count - 2];

// Build an array which is [
// SecIdentityRef for EE cert,
// SecCertificateRef for intermed0,
// SecCertificateREf for intermed1,
// ...
// ]
IntPtr[] ptrs = new IntPtr[intermediateCerts.Length + 1];

for (int i = 0; i < intermediateCerts.Length; i++)
{
X509Certificate2 intermediateCert = elements[i + 1].Certificate!;
IntPtr[] ptrs = new IntPtr[context!.IntermediateCertificates!.Length + 1];

if (intermediateCert.HasPrivateKey)
{
// In the unlikely event that we get a certificate with a private key from
// a chain, clear it to the certificate.
//
// The current value of intermediateCert is still in elements, which will
// get Disposed at the end of this method. The new value will be
// in the intermediate certs array, which also gets serially Disposed.
intermediateCert = new X509Certificate2(intermediateCert.RawData);
}
for (int i = 0; i < context.IntermediateCertificates.Length; i++)
{
X509Certificate2 intermediateCert = context.IntermediateCertificates[i];

intermediateCerts[i] = intermediateCert;
ptrs[i + 1] = intermediateCert.Handle;
if (intermediateCert.HasPrivateKey)
{
// In the unlikely event that we get a certificate with a private key from
// a chain, clear it to the certificate.
//
// The current value of intermediateCert is still in elements, which will
// get Disposed at the end of this method. The new value will be
// in the intermediate certs array, which also gets serially Disposed.
intermediateCert = new X509Certificate2(intermediateCert.RawData);
}

ptrs[0] = certificate.Handle;

Interop.AppleCrypto.SslSetCertificate(sslContext, ptrs);
ptrs[i + 1] = intermediateCert.Handle;
}

// The X509Chain created all new certs for us, so Dispose them.
// And since the intermediateCerts could have been new instances, Dispose them, too
for (int i = 0; i < elements.Count; i++)
{
elements[i].Certificate!.Dispose();
ptrs[0] = context!.Certificate!.Handle;

if (i < intermediateCerts.Length)
{
intermediateCerts[i].Dispose();
}
}
}
Interop.AppleCrypto.SslSetCertificate(sslContext, ptrs);
}
}
}
22 changes: 7 additions & 15 deletions src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeFreeSslCredentials.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,30 +10,22 @@ namespace System.Net
{
internal sealed class SafeFreeSslCredentials : SafeFreeCredentials
{
public SafeFreeSslCredentials(X509Certificate certificate, SslProtocols protocols, EncryptionPolicy policy)
public SafeFreeSslCredentials(SslStreamCertificateContext? certificateContext, SslProtocols protocols, EncryptionPolicy policy)
: base(IntPtr.Zero, true)
{
Debug.Assert(
certificate == null || certificate is X509Certificate2,
"Only X509Certificate2 certificates are supported at this time");

X509Certificate2? cert = (X509Certificate2?)certificate;

if (cert != null)
if (certificateContext != null)
{
Debug.Assert(cert.HasPrivateKey, "cert.HasPrivateKey");

// Make a defensive copy of the certificate. In some async cases the
// certificate can have been disposed before being provided to the handshake.
//
// This meshes with the Unix (OpenSSL) PAL, because it extracts the private key
// and cert handle (which get up-reffed) to match the API expectations.
cert = new X509Certificate2(cert);
certificateContext = certificateContext.Duplicate();

Debug.Assert(cert.HasPrivateKey, "cert clone.HasPrivateKey");
Debug.Assert(certificateContext.Certificate.HasPrivateKey, "cert clone.HasPrivateKey");
}

Certificate = cert;
CertificateContext = certificateContext;
Protocols = protocols;
Policy = policy;
}
Expand All @@ -42,13 +34,13 @@ public SafeFreeSslCredentials(X509Certificate certificate, SslProtocols protocol

public SslProtocols Protocols { get; }

public X509Certificate2? Certificate { get; }
public SslStreamCertificateContext? CertificateContext { get; }

public override bool IsInvalid => false;

protected override bool ReleaseHandle()
{
Certificate?.Dispose();
CertificateContext?.Certificate.Dispose();
return true;
}
}
Expand Down
15 changes: 11 additions & 4 deletions src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -596,21 +596,27 @@ private bool AcquireClientCredentials(ref byte[]? thumbPrint)
{
if (NetEventSource.Log.IsEnabled())
NetEventSource.Log.UsingCachedCredential(this);

_credentialsHandle = cachedCredentialHandle;
_selectedClientCertificate = clientCertificate;
cachedCred = true;
}
else
{
_credentialsHandle = SslStreamPal.AcquireCredentialsHandle(selectedCert!, _sslAuthenticationOptions.EnabledSslProtocols, _sslAuthenticationOptions.EncryptionPolicy, _sslAuthenticationOptions.IsServer);
if (selectedCert != null)
{
_sslAuthenticationOptions.CertificateContext = SslStreamCertificateContext.Create(selectedCert!);
}

_credentialsHandle = SslStreamPal.AcquireCredentialsHandle(_sslAuthenticationOptions.CertificateContext,
_sslAuthenticationOptions.EnabledSslProtocols, _sslAuthenticationOptions.EncryptionPolicy, _sslAuthenticationOptions.IsServer);

thumbPrint = guessedThumbPrint; // Delay until here in case something above threw.
_selectedClientCertificate = clientCertificate;
}
}
finally
{
if (selectedCert != null)
if (selectedCert != null && _sslAuthenticationOptions.CertificateContext != null)
{
_sslAuthenticationOptions.CertificateContext = SslStreamCertificateContext.Create(selectedCert);
}
Expand Down Expand Up @@ -710,7 +716,8 @@ private bool AcquireServerCredentials(ref byte[]? thumbPrint)
}
else
{
_credentialsHandle = SslStreamPal.AcquireCredentialsHandle(selectedCert, _sslAuthenticationOptions.EnabledSslProtocols, _sslAuthenticationOptions.EncryptionPolicy, _sslAuthenticationOptions.IsServer);
_credentialsHandle = SslStreamPal.AcquireCredentialsHandle(_sslAuthenticationOptions.CertificateContext, _sslAuthenticationOptions.EnabledSslProtocols,
_sslAuthenticationOptions.EncryptionPolicy, _sslAuthenticationOptions.IsServer);
thumbPrint = guessedThumbPrint;
}

Expand Down
8 changes: 8 additions & 0 deletions ...ibraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,14 @@ namespace System.Net.Security
{
public partial class SslStreamCertificateContext
{
private const bool TrimRootCertificate = true;

private SslStreamCertificateContext(X509Certificate2 target, X509Certificate2[] intermediates)
{
Certificate = target;
IntermediateCertificates = intermediates;
}

internal static SslStreamCertificateContext Create(X509Certificate2 target) => Create(target, null);
}
}
9 changes: 9 additions & 0 deletions src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.OSX.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,15 @@ namespace System.Net.Security
{
public partial class SslStreamCertificateContext
{
// No leaf, no root.
private const bool TrimRootCertificate = true;

private SslStreamCertificateContext(X509Certificate2 target, X509Certificate2[] intermediates)
{
Certificate = target;
IntermediateCertificates = intermediates;
}

internal static SslStreamCertificateContext Create(X509Certificate2 target)
{
// On OSX we do not need to build chain unless we are asked for it.
Expand Down
92 changes: 92 additions & 0 deletions ...raries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Windows.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,102 @@ namespace System.Net.Security
{
public partial class SslStreamCertificateContext
{
// No leaf, include root.
private const bool TrimRootCertificate = false;

internal static SslStreamCertificateContext Create(X509Certificate2 target)
{
// On Windows we do not need to build chain unless we are asked for it.
return new SslStreamCertificateContext(target, Array.Empty<X509Certificate2>());
}

private SslStreamCertificateContext(X509Certificate2 target, X509Certificate2[] intermediates)
{
if (intermediates.Length > 0)
{
using (X509Chain chain = new X509Chain())
{
chain.ChainPolicy.VerificationFlags = X509VerificationFlags.AllFlags;
chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck;
chain.ChainPolicy.DisableCertificateDownloads = true;
bool osCanBuildChain = chain.Build(target);

int count = 0;
foreach (X509ChainStatus status in chain.ChainStatus)
{
if (status.Status.HasFlag(X509ChainStatusFlags.PartialChain) || status.Status.HasFlag(X509ChainStatusFlags.NotSignatureValid))
{
osCanBuildChain = false;
break;
}

count++;
}

// OS failed to build the chain but we have at least some intermediates.
// We will try to add them to "Intermediate Certification Authorities" store.
if (!osCanBuildChain)
{
X509Store? store = new X509Store(StoreName.CertificateAuthority, StoreLocation.LocalMachine);

try
{
store.Open(OpenFlags.ReadWrite);
}
catch
{
// If using system store fails, try to fall-back to user store.
store.Dispose();
store = new X509Store(StoreName.CertificateAuthority, StoreLocation.CurrentUser);
try
{
store.Open(OpenFlags.ReadWrite);
}
catch
{
store.Dispose();
store = null;
if (NetEventSource.IsEnabled)
{
NetEventSource.Error(this, $"Failed to open certificate store for intermediates.");
}
}
}

if (store != null)
{
using (store)
{
// Add everything except the root
for (int index = count; index < intermediates.Length - 1; index++)
{
store.Add(intermediates[index]);
}

osCanBuildChain = chain.Build(target);
foreach (X509ChainStatus status in chain.ChainStatus)
{
if (status.Status.HasFlag(X509ChainStatusFlags.PartialChain) || status.Status.HasFlag(X509ChainStatusFlags.NotSignatureValid))
{
osCanBuildChain = false;
break;
}
}

if (!osCanBuildChain)
{
// Add also root to Intermediate CA store so OS can complete building chain.
// (This does not make it trusted.
store.Add(intermediates[intermediates.Length - 1]);
}
}
}
}
}
}

Certificate = target;
IntermediateCertificates = intermediates;
}
}
}
18 changes: 10 additions & 8 deletions src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ public static SslStreamCertificateContext Create(X509Certificate2 target, X509Ce
}

X509Certificate2[] intermediates = Array.Empty<X509Certificate2>();

using (X509Chain chain = new X509Chain())
{
if (additionalCertificates != null)
Expand All @@ -32,11 +31,14 @@ public static SslStreamCertificateContext Create(X509Certificate2 target, X509Ce
chain.ChainPolicy.VerificationFlags = X509VerificationFlags.AllFlags;
chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck;
chain.ChainPolicy.DisableCertificateDownloads = offline;
chain.Build(target);
bool chainStatus = chain.Build(target);

// No leaf, no root.
int count = chain.ChainElements.Count - 2;
if (!chainStatus && NetEventSource.IsEnabled)
{
NetEventSource.Error(null, $"Failed to build chain for {target.Subject}");
}

int count = chain.ChainElements.Count - (TrimRootCertificate ? 1 : 2);
foreach (X509ChainStatus status in chain.ChainStatus)
{
if (status.Status.HasFlag(X509ChainStatusFlags.PartialChain))
Expand All @@ -48,7 +50,7 @@ public static SslStreamCertificateContext Create(X509Certificate2 target, X509Ce
}

// Count can be zero for a self-signed certificate, or a cert issued directly from a root.
if (count > 0)
if (count > 0 && chain.ChainElements.Count > 1)
{
intermediates = new X509Certificate2[count];
for (int i = 0; i < count; i++)
Expand All @@ -70,10 +72,10 @@ public static SslStreamCertificateContext Create(X509Certificate2 target, X509Ce
return new SslStreamCertificateContext(target, intermediates);
}

private SslStreamCertificateContext(X509Certificate2 target, X509Certificate2[] intermediates)
internal SslStreamCertificateContext Duplicate()
{
Certificate = target;
IntermediateCertificates = intermediates;
return new SslStreamCertificateContext(new X509Certificate2(Certificate), IntermediateCertificates);

}
}
}
4 changes: 2 additions & 2 deletions src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.OSX.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,12 @@ public static void VerifyPackageInfo()
}

public static SafeFreeCredentials AcquireCredentialsHandle(
X509Certificate certificate,
SslStreamCertificateContext? certificateContext,
SslProtocols protocols,
EncryptionPolicy policy,
bool isServer)
{
return new SafeFreeSslCredentials(certificate, protocols, policy);
return new SafeFreeSslCredentials(certificateContext, protocols, policy);
}

internal static byte[]? GetNegotiatedApplicationProtocol(SafeDeleteContext? context)
Expand Down
Loading

0 comments on commit eb243f0

Please sign in to comment.