Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix certificate ctx on windows and macOS #39818

Merged
merged 6 commits into from
Aug 10, 2020
Merged

fix certificate ctx on windows and macOS #39818

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1a9673f
fix certificate ctx on windows
wfurt Jul 23, 2020
eed8a7a
fix linux
wfurt Jul 23, 2020
b85ff7c
fix osx pal
wfurt Jul 28, 2020
9a60428
feedback from review
wfurt Jul 29, 2020
fe706b9
feedback from review
wfurt Aug 5, 2020
d7fc52e
Merge branch 'master' of https://github.com/dotnet/runtime into winChain
wfurt Aug 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 20 additions & 54 deletions src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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)
wfurt marked this conversation as resolved.
Show resolved Hide resolved
{
_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
Open in desktop
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
Open in desktop
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
Open in desktop
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really want to add all intermediates? Or just the ones that the OS ends up using in a chain?

e.g. Add all of intermediates to chain.ChainPolicy.ExtraStore and build again; then add anything relevant (preferably using a diffing algorithm so that it doesn't potentially reset custom store property overrides).

Using a second chain object (you could copy the policy object over) would make the diff easy, just walk each position and compare that cert.RawData values SequenceEqual.

Ideally, after each add you'd rerun the offline/no-extra build to see if you can avoid invalidating existing higher-scoped entries.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(While a diffing algorithm and a while loop sound potentially expensive, most chains are three items long, so there aren't many iterations)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was considering to add some more complicated logic. But as you said if most chains are 3 long, it does not matter that much. I assumed that adding intermediate once to store is cheap enough so I decided not to care.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's one thing to check:

  • Add an intermediate to the store
  • Open the intermediates store in MMC (e.g. add snap-in, certificates, local computer, then pick Intermediate Certificate Authorities / Certificates in the nav tree)
  • Pick that same certificate, right click, Properties
  • Pick "Enable only the following purposes"
  • Toggle some checkmarks
  • Use the API as you have it
  • Right click on the Certificates entry in the nav bar and refresh
  • Reopen the properties dialog.

If the property resets, we need to do complex stuff here. If it doesn't, we're good. (I think we do "merge properties", which should (generally) be safe, but it's good to test that before we run the risk of altering machine state)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that assumes same CA set between runs, right? (right now we generate unique set for each test run and I had situation where it would build up before adding cleanup to the tests)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It assumes that we ever get something in the intermediates collection that's already in the store. E.g. if what we need is a 4 hop chain:

  • root -> intermed1 -> intermed2 -> end-entity

And intermed1 is already in the store, but intermed2 wasn't; we'll end up calling store.Add(intermed1) which will cause the merge logic to apply. Since it changes system persisted state we need to be careful to ensure it's only additive, not destructive.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is what I did:

  • modified the test helper to save/load certificates from pfx and I disabled the cleanup
  • I run the test and I check both CA.
  • I removed root CA, run tests and verified that it triggers the adding logic
  • I removed root again and I modified properties on intermediate and I also added friendly name
  • run test again. Root CA was added again and intermediate preserved with my modifications

so the conclusion is that adding existing certificate is not destructive.

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]);
bartonjs marked this conversation as resolved.
Show resolved Hide resolved
}

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
Open in desktop
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
Open in desktop
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