Skip to content

Commit

Permalink
fix TLS resume with client certificates (#79898)
Browse files Browse the repository at this point in the history
* fix TLS resume with client certificates

* fix windows

* fix unused warning

* undo #79128 test change

* remove dead code

* fix resolve
  • Loading branch information
wfurt authored Jan 19, 2023
1 parent b6f12dd commit 8300e38
Show file tree
Hide file tree
Showing 15 changed files with 256 additions and 38 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,8 @@ internal static SafeSslHandle AllocateSslHandle(SslAuthenticationOptions sslAuth
if (!Interop.Ssl.Capabilities.Tls13Supported ||
string.IsNullOrEmpty(sslAuthenticationOptions.TargetHost) ||
sslAuthenticationOptions.CertificateContext != null ||
sslAuthenticationOptions.CertSelectionDelegate != null)
sslAuthenticationOptions.ClientCertificates?.Count > 0 ||
sslAuthenticationOptions.CertSelectionDelegate != null)
{
cacheSslContext = false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ internal bool TryAddSession(IntPtr namePtr, IntPtr session)
if (!string.IsNullOrEmpty(targetName))
{
// We do this only for lookup in RemoveSession.
// Since this is part of chache manipulation and no function impact it is done here.
// Since this is part of cache manipulation and no function impact it is done here.
// This will use strdup() so it is safe to pass in raw pointer.
Interop.Ssl.SessionSetHostname(session, namePtr);

Expand Down
16 changes: 16 additions & 0 deletions src/libraries/Common/src/Interop/Windows/SspiCli/Interop.SSPI.cs
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ internal enum ContextAttribute
SECPKG_ATTR_ISSUER_LIST_EX = 0x59, // returns SecPkgContext_IssuerListInfoEx
SECPKG_ATTR_CLIENT_CERT_POLICY = 0x60, // sets SecPkgCred_ClientCertCtlPolicy
SECPKG_ATTR_CONNECTION_INFO = 0x5A, // returns SecPkgContext_ConnectionInfo
SECPKG_ATTR_SESSION_INFO = 0x5D, // sets SecPkgContext_SessionInfo
SECPKG_ATTR_CIPHER_INFO = 0x64, // returns SecPkgContext_CipherInfo
SECPKG_ATTR_REMOTE_CERT_CHAIN = 0x67, // returns PCCERT_CONTEXT
SECPKG_ATTR_UI_INFO = 0x68, // sets SEcPkgContext_UiInfo
Expand Down Expand Up @@ -331,6 +332,21 @@ internal unsafe struct SecPkgCred_ClientCertPolicy
public char* pwszSslCtlIdentifier;
}

[StructLayout(LayoutKind.Sequential)]
internal unsafe struct SecPkgContext_SessionInfo
{
public uint dwFlags;
public uint cbSessionId;
public fixed byte rgbSessionId[32];

[Flags]
public enum Flags
{
Zero = 0,
SSL_SESSION_RECONNECT = 0x01,
};
}

[LibraryImport(Interop.Libraries.SspiCli, SetLastError = true)]
internal static partial int EncryptMessage(
ref CredHandle contextHandle,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Globalization;
using System.Runtime.InteropServices;
using System.Security.Authentication.ExtendedProtection;
using System.Security.Cryptography.X509Certificates;
using Microsoft.Win32.SafeHandles;

namespace System.Net.Security
Expand Down Expand Up @@ -310,10 +311,15 @@ public static unsafe int AcquireCredentialsHandle(

internal sealed class SafeFreeCredential_SECURITY : SafeFreeCredentials
{
#pragma warning disable 0649
// This is used only by SslStream but it is included elsewhere
public X509Certificate? LocalCertificate;
#pragma warning restore 0649
public SafeFreeCredential_SECURITY() : base() { }

protected override bool ReleaseHandle()
{
LocalCertificate?.Dispose();
return Interop.SspiCli.FreeCredentialsHandle(ref _handle) == 0;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ internal static SslPolicyErrors VerifyCertificateProperties(
}

// Check if the local certificate has been sent to the peer during the handshake.
internal static bool IsLocalCertificateUsed(SafeDeleteContext? securityContext)
internal static bool IsLocalCertificateUsed(SafeFreeCredentials? _, SafeDeleteContext? securityContext)
{
SafeSslHandle? sslContext = ((SafeDeleteSslContext?)securityContext)?.SslContext;
if (sslContext == null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ internal static SslPolicyErrors VerifyCertificateProperties(

// This is only called when we selected local client certificate.
// Currently this is only when Apple crypto asked for it.
internal static bool IsLocalCertificateUsed(SafeDeleteContext? _) => true;
internal static bool IsLocalCertificateUsed(SafeFreeCredentials? _1, SafeDeleteContext? _2) => true;

//
// Used only by client SSL code, never returns null.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ internal static SslPolicyErrors VerifyCertificateProperties(

// This is only called when we selected local client certificate.
// Currently this is only when OpenSSL needs it because peer asked.
internal static bool IsLocalCertificateUsed(SafeDeleteContext? _) => true;
internal static bool IsLocalCertificateUsed(SafeFreeCredentials? _1, SafeDeleteContext? _2) => true;

//
// Used only by client SSL code, never returns null.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Security.Cryptography;
using System.Security.Cryptography.X509Certificates;
using System.Security.Principal;
using static Interop.SspiCli;

namespace System.Net
{
Expand Down Expand Up @@ -90,8 +91,23 @@ internal static SslPolicyErrors VerifyCertificateProperties(
}

// Check that local certificate was used by schannel.
internal static bool IsLocalCertificateUsed(SafeDeleteContext securityContext)
internal static bool IsLocalCertificateUsed(SafeFreeCredentials? _credentialsHandle, SafeDeleteContext securityContext)
{
SecPkgContext_SessionInfo info = default;
// fails on Server 2008 and older. We will fall-back to probing LOCAL_CERT_CONTEXT in that case.
if (SSPIWrapper.QueryBlittableContextAttributes(
GlobalSSPI.SSPISecureChannel,
securityContext,
Interop.SspiCli.ContextAttribute.SECPKG_ATTR_SESSION_INFO,
ref info) &&
((SecPkgContext_SessionInfo.Flags)info.dwFlags).HasFlag(SecPkgContext_SessionInfo.Flags.SSL_SESSION_RECONNECT))
{
// This is TLS Resumed session. Windows can fail to query the local cert bellow.
// Instead, we will determine the usage form used credentials.
SafeFreeCredential_SECURITY creds = (SafeFreeCredential_SECURITY)_credentialsHandle!;
return creds.LocalCertificate != null;
}

SafeFreeCertContext? localContext = null;
try
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ private bool CompleteHandshake(ref ProtocolToken? alertToken, out SslPolicyError
return true;
}

if (_selectedClientCertificate != null && !CertificateValidationPal.IsLocalCertificateUsed(_securityContext!))
if (_selectedClientCertificate != null && !CertificateValidationPal.IsLocalCertificateUsed(_credentialsHandle, _securityContext!))
{
// We may select client cert but it may not be used.
// This is primarily an issue on Windows with credential caching.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@ public partial class SslStream
private int _trailerSize = 16;
private int _maxDataSize = 16354;

private bool _refreshCredentialNeeded = true;

private static readonly Oid s_serverAuthOid = new Oid("1.3.6.1.5.5.7.3.1", "1.3.6.1.5.5.7.3.1");
private static readonly Oid s_clientAuthOid = new Oid("1.3.6.1.5.5.7.3.2", "1.3.6.1.5.5.7.3.2");

Expand Down Expand Up @@ -104,11 +102,6 @@ internal bool RemoteCertRequired
}
}

internal void SetRefreshCredentialNeeded()
{
_refreshCredentialNeeded = true;
}

internal void CloseContext()
{
if (!_remoteCertificateExposed)
Expand Down Expand Up @@ -489,6 +482,7 @@ private string[] GetRequestCertificateAuthorities()
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"Selected cert = {selectedCert}");

_selectedClientCertificate = clientCertificate;

return selectedCert;
}

Expand Down Expand Up @@ -529,7 +523,7 @@ This will not restart a session but helps minimizing the number of handles we cr
--*/

private bool AcquireClientCredentials(ref byte[]? thumbPrint)
private bool AcquireClientCredentials(ref byte[]? thumbPrint, bool newCredentialsRequested = false)
{
// Acquire possible Client Certificate information and set it on the handle.

Expand Down Expand Up @@ -594,7 +588,7 @@ private bool AcquireClientCredentials(ref byte[]? thumbPrint)
_sslAuthenticationOptions.CertificateContext ??= SslStreamCertificateContext.Create(selectedCert!);
}

_credentialsHandle = AcquireCredentialsHandle(_sslAuthenticationOptions);
_credentialsHandle = AcquireCredentialsHandle(_sslAuthenticationOptions, newCredentialsRequested);
thumbPrint = guessedThumbPrint; // Delay until here in case something above threw.
}
}
Expand Down Expand Up @@ -689,8 +683,11 @@ private bool AcquireServerCredentials(ref byte[]? thumbPrint)
//
byte[] guessedThumbPrint = selectedCert.GetCertHash();
bool sendTrustedList = _sslAuthenticationOptions.CertificateContext!.Trust?._sendTrustInHandshake ?? false;
SafeFreeCredentials? cachedCredentialHandle = SslSessionsCache.TryCachedCredential(guessedThumbPrint, _sslAuthenticationOptions.EnabledSslProtocols, _sslAuthenticationOptions.IsServer, _sslAuthenticationOptions.EncryptionPolicy, sendTrustedList);

SafeFreeCredentials? cachedCredentialHandle = SslSessionsCache.TryCachedCredential(guessedThumbPrint,
_sslAuthenticationOptions.EnabledSslProtocols,
_sslAuthenticationOptions.IsServer,
_sslAuthenticationOptions.EncryptionPolicy,
sendTrustedList);
if (cachedCredentialHandle != null)
{
_credentialsHandle = cachedCredentialHandle;
Expand All @@ -705,9 +702,9 @@ private bool AcquireServerCredentials(ref byte[]? thumbPrint)
return cachedCred;
}

private static SafeFreeCredentials? AcquireCredentialsHandle(SslAuthenticationOptions sslAuthenticationOptions)
private static SafeFreeCredentials? AcquireCredentialsHandle(SslAuthenticationOptions sslAuthenticationOptions, bool newCredentialsRequested = false)
{
SafeFreeCredentials? cred = SslStreamPal.AcquireCredentialsHandle(sslAuthenticationOptions);
SafeFreeCredentials? cred = SslStreamPal.AcquireCredentialsHandle(sslAuthenticationOptions, newCredentialsRequested);

if (sslAuthenticationOptions.CertificateContext != null && cred != null)
{
Expand Down Expand Up @@ -761,16 +758,6 @@ internal ProtocolToken NextMessage(ReadOnlySpan<byte> incomingBuffer)
{
byte[]? nextmsg = null;
SecurityStatusPal status = GenerateToken(incomingBuffer, ref nextmsg);

if (!_sslAuthenticationOptions.IsServer && status.ErrorCode == SecurityStatusPalErrorCode.CredentialsNeeded)
{
if (NetEventSource.Log.IsEnabled())
NetEventSource.Info(this, "NextMessage() returned SecurityStatusPal.CredentialsNeeded");

SetRefreshCredentialNeeded();
status = GenerateToken(incomingBuffer, ref nextmsg);
}

ProtocolToken token = new ProtocolToken(nextmsg, status);

if (NetEventSource.Log.IsEnabled())
Expand Down Expand Up @@ -806,6 +793,10 @@ private SecurityStatusPal GenerateToken(ReadOnlySpan<byte> inputBuffer, ref byte
bool sendTrustList = false;
byte[]? thumbPrint = null;

// We need to try get credentials at the beginning.
// _credentialsHandle may be always null on some platforms but
// _securityContext will be allocated on first call.
bool refreshCredentialNeeded = _securityContext == null;
//
// Looping through ASC or ISC with potentially cached credential that could have been
// already disposed from a different thread before ISC or ASC dir increment a cred ref count.
Expand All @@ -815,7 +806,7 @@ private SecurityStatusPal GenerateToken(ReadOnlySpan<byte> inputBuffer, ref byte
do
{
thumbPrint = null;
if (_refreshCredentialNeeded)
if (refreshCredentialNeeded)
{
cachedCreds = _sslAuthenticationOptions.IsServer
? AcquireServerCredentials(ref thumbPrint)
Expand Down Expand Up @@ -865,15 +856,31 @@ private SecurityStatusPal GenerateToken(ReadOnlySpan<byte> inputBuffer, ref byte
_sslAuthenticationOptions,
SelectClientCertificate
);

if (status.ErrorCode == SecurityStatusPalErrorCode.CredentialsNeeded)
{
refreshCredentialNeeded = true;
cachedCreds = AcquireClientCredentials(ref thumbPrint, newCredentialsRequested: true);

if (NetEventSource.Log.IsEnabled())
NetEventSource.Info(this, "InitializeSecurityContext() returned 'CredentialsNeeded'.");

status = SslStreamPal.InitializeSecurityContext(
ref _credentialsHandle!,
ref _securityContext,
_sslAuthenticationOptions.TargetHost,
inputBuffer,
ref result,
_sslAuthenticationOptions,
SelectClientCertificate);
}
}
} while (cachedCreds && _credentialsHandle == null);
}
finally
{
if (_refreshCredentialNeeded)
if (refreshCredentialNeeded)
{
_refreshCredentialNeeded = false;

//
// Assuming the ISC or ASC has referenced the credential,
// we want to call dispose so to decrement the effective ref count.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public static SecurityStatusPal Renegotiate(
throw new PlatformNotSupportedException();
}

public static SafeFreeCredentials? AcquireCredentialsHandle(SslAuthenticationOptions sslAuthenticationOptions)
public static SafeFreeCredentials? AcquireCredentialsHandle(SslAuthenticationOptions _1, bool _2)
{
return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ public static SecurityStatusPal Renegotiate(
throw new PlatformNotSupportedException();
}

public static SafeFreeCredentials? AcquireCredentialsHandle(SslAuthenticationOptions sslAuthenticationOptions)
public static SafeFreeCredentials? AcquireCredentialsHandle(SslAuthenticationOptions _1, bool _2)
{
return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public static SecurityStatusPal InitializeSecurityContext(
return HandshakeInternal(ref context, inputBuffer, ref outputBuffer, sslAuthenticationOptions, clientCertificateSelectionCallback);
}

public static SafeFreeCredentials? AcquireCredentialsHandle(SslAuthenticationOptions sslAuthenticationOptions)
public static SafeFreeCredentials? AcquireCredentialsHandle(SslAuthenticationOptions _1, bool _2)
{
return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ public static SecurityStatusPal Renegotiate(
return status;
}

public static SafeFreeCredentials AcquireCredentialsHandle(SslAuthenticationOptions sslAuthenticationOptions)
public static SafeFreeCredentials AcquireCredentialsHandle(SslAuthenticationOptions sslAuthenticationOptions, bool newCredentialsRequested)
{
try
{
Expand All @@ -191,6 +191,16 @@ public static SafeFreeCredentials AcquireCredentialsHandle(SslAuthenticationOpti
AttachCertificateStore(cred, certificateContext.Trust._store!);
}

// Windows can fail to get local credentials in case of TLS Resume.
// We will store associated certificate in credentials and use it in case
// of TLS resume. It will be disposed when the credentials are.
if (newCredentialsRequested && sslAuthenticationOptions.CertificateContext != null)
{
SafeFreeCredential_SECURITY handle = (SafeFreeCredential_SECURITY)cred;
// We need to create copy to avoid Disposal issue.
handle.LocalCertificate = new X509Certificate2(sslAuthenticationOptions.CertificateContext.Certificate);
}

return cred;
}
catch (Win32Exception e)
Expand Down
Loading

0 comments on commit 8300e38

Please sign in to comment.