Skip to content

Commit

Permalink
include more details in exception if remote certificate validation fa…
Browse files Browse the repository at this point in the history 
…ils (dotnet#40110)

* include more details in exception if remote certificate validation fails

* fix unit test linking

* feedback from review

* update exception message
  • Loading branch information
@wfurt @Jacksondr5
wfurt authored and Jacksondr5 committed Aug 10, 2020
1 parent ed062dd commit 565c790
Show file tree
Hide file tree
Showing 8 changed files with 186 additions and 88 deletions.
11 changes: 7 additions & 4 deletions src/libraries/System.Net.Security/src/Resources/Strings.resx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,17 +164,20 @@
<data name="net_io_eof" xml:space="preserve">
<value> Received an unexpected EOF or 0 bytes from the transport stream.</value>
</data>
<data name="net_io_async_result" xml:space="preserve">
<value>The parameter: {0} is not valid. Use the object returned from corresponding Begin async call.</value>
</data>
<data name="net_ssl_io_frame" xml:space="preserve">
<value>The handshake failed due to an unexpected packet format.</value>
</data>
<data name="net_ssl_io_renego" xml:space="preserve">
<value>The remote party requested renegotiation when AllowRenegotiation was set to false.</value>
</data>
<data name="net_ssl_io_cert_validation" xml:space="preserve">
<value>The remote certificate is invalid according to the validation procedure.</value>
<value>The remote certificate is invalid according to the validation procedure: {0}</value>
</data>
<data name="net_ssl_io_cert_chain_validation" xml:space="preserve">
<value>The remote certificate is invalid because of errors in the certificate chain: {0}</value>
</data>
<data name="net_ssl_io_cert_custom_validation" xml:space="preserve">
<value>The remote certificate was rejected by the provided RemoteCertificateValidationCallback.</value>
</data>
<data name="net_ssl_io_no_server_cert" xml:space="preserve">
<value>The server mode SSL must use a certificate with the associated private key.</value>
Expand Down
69 changes: 47 additions & 22 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 @@ -25,7 +25,8 @@ internal class SecureChannel

private SslConnectionInfo? _connectionInfo;
private X509Certificate? _selectedClientCertificate;
private bool _isRemoteCertificateAvailable;
private X509Certificate2? _remoteCertificate;
private bool _remoteCertificateExposed;

// These are the MAX encrypt buffer output sizes, not the actual sizes.
private int _headerSize = 5; //ATTN must be set to at least 5 by default
Expand All @@ -39,6 +40,7 @@ internal class SecureChannel

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");
private SslStream? _ssl;

internal SecureChannel(SslAuthenticationOptions sslAuthenticationOptions, SslStream sslStream)
{
Expand All @@ -54,6 +56,7 @@ internal SecureChannel(SslAuthenticationOptions sslAuthenticationOptions, SslStr
_securityContext = null;
_refreshCredentialNeeded = true;
_sslAuthenticationOptions = sslAuthenticationOptions;
_ssl = sslStream;
}

//
Expand Down Expand Up @@ -85,7 +88,16 @@ internal bool IsRemoteCertificateAvailable
{
get
{
return _isRemoteCertificateAvailable;
return _remoteCertificate != null;
}
}

internal X509Certificate? RemoteCertificate
{
get
{
_remoteCertificateExposed = true;
return _remoteCertificate;
}
}

Expand Down Expand Up @@ -164,8 +176,15 @@ internal void SetRefreshCredentialNeeded()

internal void Close()
{
if (!_remoteCertificateExposed)
{
_remoteCertificate?.Dispose();
_remoteCertificate = null;
}

_securityContext?.Dispose();
_credentialsHandle?.Dispose();
_ssl = null;
GC.SuppressFinalize(this);
}

Expand Down Expand Up @@ -585,7 +604,6 @@ private bool AcquireClientCredentials(ref byte[]? thumbPrint)
else
{
_credentialsHandle = SslStreamPal.AcquireCredentialsHandle(selectedCert!, _sslAuthenticationOptions.EnabledSslProtocols, _sslAuthenticationOptions.EncryptionPolicy, _sslAuthenticationOptions.IsServer);

thumbPrint = guessedThumbPrint; // Delay until here in case something above threw.
_selectedClientCertificate = clientCertificate;
}
Expand Down Expand Up @@ -911,22 +929,21 @@ internal SecurityStatusPal Decrypt(byte[]? payload, ref int offset, ref int coun
--*/

//This method validates a remote certificate.
internal bool VerifyRemoteCertificate(RemoteCertValidationCallback? remoteCertValidationCallback, ref ProtocolToken? alertToken)
internal bool VerifyRemoteCertificate(RemoteCertificateValidationCallback? remoteCertValidationCallback, ref ProtocolToken? alertToken, out SslPolicyErrors sslPolicyErrors, out X509ChainStatusFlags chainStatus)
{
SslPolicyErrors sslPolicyErrors = SslPolicyErrors.None;
sslPolicyErrors = SslPolicyErrors.None;
chainStatus = X509ChainStatusFlags.NoError;

// We don't catch exceptions in this method, so it's safe for "accepted" be initialized with true.
bool success = false;
X509Chain? chain = null;
X509Certificate2? remoteCertificateEx = null;
X509Certificate2Collection? remoteCertificateStore = null;

try
{
remoteCertificateEx = CertificateValidationPal.GetRemoteCertificate(_securityContext, out remoteCertificateStore);
_isRemoteCertificateAvailable = remoteCertificateEx != null;
_remoteCertificate = CertificateValidationPal.GetRemoteCertificate(_securityContext, out remoteCertificateStore);

if (remoteCertificateEx == null)
if (_remoteCertificate == null)
{
if (NetEventSource.Log.IsEnabled() && RemoteCertRequired) NetEventSource.Error(this, $"Remote certificate required, but no remote certificate received");
sslPolicyErrors |= SslPolicyErrors.RemoteCertificateNotAvailable;
Expand All @@ -948,38 +965,48 @@ internal bool VerifyRemoteCertificate(RemoteCertValidationCallback? remoteCertVa
sslPolicyErrors |= CertificateValidationPal.VerifyCertificateProperties(
_securityContext!,
chain,
remoteCertificateEx,
_remoteCertificate,
_sslAuthenticationOptions.CheckCertName,
_sslAuthenticationOptions.IsServer,
_sslAuthenticationOptions.TargetHost);
}

if (remoteCertValidationCallback != null)
{
success = remoteCertValidationCallback(_sslAuthenticationOptions.TargetHost, remoteCertificateEx, chain, sslPolicyErrors);
object? sender = _ssl;
if (sender == null)
{
throw new ObjectDisposedException(nameof(SslStream));
}

success = remoteCertValidationCallback(sender, _remoteCertificate, chain, sslPolicyErrors);
}
else
{
if (sslPolicyErrors == SslPolicyErrors.RemoteCertificateNotAvailable && !_sslAuthenticationOptions.RemoteCertRequired)
{
success = true;
}
else
if (!RemoteCertRequired)
{
success = (sslPolicyErrors == SslPolicyErrors.None);
sslPolicyErrors &= ~SslPolicyErrors.RemoteCertificateNotAvailable;
}

success = (sslPolicyErrors == SslPolicyErrors.None);
}

if (NetEventSource.Log.IsEnabled())
{
LogCertificateValidation(remoteCertValidationCallback, sslPolicyErrors, success, chain!);
if (NetEventSource.Log.IsEnabled())
NetEventSource.Info(this, $"Cert validation, remote cert = {remoteCertificateEx}");
NetEventSource.Info(this, $"Cert validation, remote cert = {_remoteCertificate}");
}

if (!success)
{
alertToken = CreateFatalHandshakeAlertToken(sslPolicyErrors, chain!);
if (chain != null)
{
foreach (X509ChainStatus status in chain.ChainStatus)
{
chainStatus |= status.Status;
}
}
}
}
finally
Expand All @@ -1006,8 +1033,6 @@ internal bool VerifyRemoteCertificate(RemoteCertValidationCallback? remoteCertVa
remoteCertificateStore[i].Dispose();
}
}

remoteCertificateEx?.Dispose();
}

return success;
Expand Down Expand Up @@ -1133,7 +1158,7 @@ private static TlsAlertMessage GetAlertMessageFromChain(X509Chain chain)
return TlsAlertMessage.BadCertificate;
}

private void LogCertificateValidation(RemoteCertValidationCallback? remoteCertValidationCallback, SslPolicyErrors sslPolicyErrors, bool success, X509Chain chain)
private void LogCertificateValidation(RemoteCertificateValidationCallback? remoteCertValidationCallback, SslPolicyErrors sslPolicyErrors, bool success, X509Chain chain)
{
if (!NetEventSource.Log.IsEnabled())
return;
Expand Down
17 changes: 14 additions & 3 deletions src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace System.Net.Security
{
internal class SslAuthenticationOptions
{
internal SslAuthenticationOptions(SslClientAuthenticationOptions sslClientAuthenticationOptions, RemoteCertValidationCallback remoteCallback, LocalCertSelectionCallback? localCallback)
internal SslAuthenticationOptions(SslClientAuthenticationOptions sslClientAuthenticationOptions, RemoteCertificateValidationCallback? remoteCallback, LocalCertSelectionCallback? localCallback)
{
Debug.Assert(sslClientAuthenticationOptions.TargetHost != null);

Expand Down Expand Up @@ -78,15 +78,21 @@ internal SslAuthenticationOptions(SslServerAuthenticationOptions sslServerAuthen
CertificateContext = SslStreamCertificateContext.Create(certificateWithKey);
}
}

if (sslServerAuthenticationOptions.RemoteCertificateValidationCallback != null)
{
CertValidationDelegate = sslServerAuthenticationOptions.RemoteCertificateValidationCallback;
}
}

internal SslAuthenticationOptions(ServerOptionsSelectionCallback optionCallback, object? state)
internal SslAuthenticationOptions(ServerOptionsSelectionCallback optionCallback, object? state, RemoteCertificateValidationCallback? remoteCallback)
{
CheckCertName = false;
TargetHost = string.Empty;
IsServer = true;
UserState = state;
ServerOptionDelegate = optionCallback;
CertValidationDelegate = remoteCallback;
}

internal void UpdateOptions(SslServerAuthenticationOptions sslServerAuthenticationOptions)
Expand All @@ -108,6 +114,11 @@ internal void UpdateOptions(SslServerAuthenticationOptions sslServerAuthenticati
// given cert is X509Certificate2 with key. We can use it directly.
CertificateContext = SslStreamCertificateContext.Create(certificateWithKey);
}

if (sslServerAuthenticationOptions.RemoteCertificateValidationCallback != null)
{
CertValidationDelegate = sslServerAuthenticationOptions.RemoteCertificateValidationCallback;
}
}

private static SslProtocols FilterOutIncompatibleSslProtocols(SslProtocols protocols)
Expand Down Expand Up @@ -136,7 +147,7 @@ private static SslProtocols FilterOutIncompatibleSslProtocols(SslProtocols proto
internal EncryptionPolicy EncryptionPolicy { get; set; }
internal bool RemoteCertRequired { get; set; }
internal bool CheckCertName { get; set; }
internal RemoteCertValidationCallback? CertValidationDelegate { get; set; }
internal RemoteCertificateValidationCallback? CertValidationDelegate { get; set; }
internal LocalCertSelectionCallback? CertSelectionDelegate { get; set; }
internal ServerCertSelectionCallback? ServerCertSelectionDelegate { get; set; }
internal CipherSuitesPolicy? CipherSuitesPolicy { get; set; }
Expand Down
26 changes: 20 additions & 6 deletions src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@
using System.Buffers;
using System.ComponentModel;
using System.Diagnostics;
using System.Globalization;
using System.IO;
using System.Runtime.ExceptionServices;
using System.Security.Authentication;
using System.Security.Cryptography.X509Certificates;
using System.Threading;
using System.Threading.Tasks;

Expand Down Expand Up @@ -41,7 +41,7 @@ private enum Framing
private const int InitialHandshakeBufferSize = 4096 + FrameOverhead; // try to fit at least 4K ServerCertificate
private ArrayBuffer _handshakeBuffer;

private void ValidateCreateContext(SslClientAuthenticationOptions sslClientAuthenticationOptions, RemoteCertValidationCallback remoteCallback, LocalCertSelectionCallback? localCallback)
private void ValidateCreateContext(SslClientAuthenticationOptions sslClientAuthenticationOptions, RemoteCertificateValidationCallback? remoteCallback, LocalCertSelectionCallback? localCallback)
{
ThrowIfExceptional();

Expand Down Expand Up @@ -321,9 +321,23 @@ private async Task ForceAuthenticationAsync<TIOAdapter>(TIOAdapter adapter, bool
}

ProtocolToken? alertToken = null;
if (!CompleteHandshake(ref alertToken))
if (!CompleteHandshake(ref alertToken, out SslPolicyErrors sslPolicyErrors, out X509ChainStatusFlags chainStatus))
{
SendAuthResetSignal(alertToken, ExceptionDispatchInfo.Capture(new AuthenticationException(SR.net_ssl_io_cert_validation, null)));
if (_sslAuthenticationOptions!.CertValidationDelegate != null)
{
// there may be some chain errors but the decision was made by custom callback. Details should be tracing if enabled.
SendAuthResetSignal(alertToken, ExceptionDispatchInfo.Capture(new AuthenticationException(SR.net_ssl_io_cert_custom_validation, null)));
}
else if (sslPolicyErrors == SslPolicyErrors.RemoteCertificateChainErrors && chainStatus != X509ChainStatusFlags.NoError)
{
// We failed only because of chain and we have some insight.
SendAuthResetSignal(alertToken, ExceptionDispatchInfo.Capture(new AuthenticationException(SR.Format(SR.net_ssl_io_cert_chain_validation, chainStatus), null)));
}
else
{
// Simple add sslPolicyErrors as crude info.
SendAuthResetSignal(alertToken, ExceptionDispatchInfo.Capture(new AuthenticationException(SR.Format(SR.net_ssl_io_cert_validation, sslPolicyErrors), null)));
}
}
}
finally
Expand Down Expand Up @@ -504,11 +518,11 @@ private void SendAuthResetSignal(ProtocolToken? message, ExceptionDispatchInfo e
//
// - Returns false if failed to verify the Remote Cert
//
private bool CompleteHandshake(ref ProtocolToken? alertToken)
private bool CompleteHandshake(ref ProtocolToken? alertToken, out SslPolicyErrors sslPolicyErrors, out X509ChainStatusFlags chainStatus)
{
_context!.ProcessHandshakeSuccess();

if (!_context.VerifyRemoteCertificate(_sslAuthenticationOptions!.CertValidationDelegate, ref alertToken))
if (!_context.VerifyRemoteCertificate(_sslAuthenticationOptions!.CertValidationDelegate, ref alertToken, out sslPolicyErrors, out chainStatus))
{
_handshakeCompleted = false;
return false;
Expand Down
Loading

0 comments on commit 565c790

Please sign in to comment.