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.

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

Raise warning message for insecure protocols #591

Merged
merged 32 commits into from
Jun 10, 2020
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
0784b17
Cached the protocol version on Managed SNI
DavoudEshtehari Apr 24, 2020
9266e65
Initial changes for SNI Merge testing
cheenamalhotra May 5, 2020
afa3032
Add testing support for .NET Standard/Package reference
cheenamalhotra May 8, 2020
b4cbbef
More improvements + renamed SNI.dll
cheenamalhotra May 12, 2020
b895c14
Receive protocol version
DavoudEshtehari May 12, 2020
7f74273
Receive protocol version
DavoudEshtehari May 12, 2020
4be02e0
Merge remote-tracking branch 'origin/SSL_TLS_WarningMessage' into SSL…
DavoudEshtehari May 12, 2020
c239acc
EOF
cheenamalhotra May 12, 2020
57a3c89
Spell
cheenamalhotra May 12, 2020
cd932fa
Merge remote-tracking branch 'Cheena/temp-sni-merge' into SSL_TLS_War…
DavoudEshtehari May 13, 2020
6acc1b2
Added project configurations
DavoudEshtehari May 13, 2020
ec0ffe2
Merged with Cheena 'temp-sni-merge'
DavoudEshtehari May 13, 2020
a997dc6
Added NetCore protocol warning
DavoudEshtehari May 14, 2020
cc7c620
NetFx SNI changes for auto-load
cheenamalhotra May 15, 2020
962b8e4
Capitalize
cheenamalhotra May 15, 2020
9216b09
Throw unsecure protocol warning at NetCore
DavoudEshtehari May 18, 2020
eefd8ed
Throw unsecure protocol warning at NetFx
DavoudEshtehari May 18, 2020
aeb683e
Merge remote-tracking branch 'Cheena/temp-sni-merge' into SSL_TLS_War…
DavoudEshtehari May 18, 2020
5ccd763
Update new SNI approach changes
DavoudEshtehari May 19, 2020
ab564ce
Udate NetFx & add net_invalid_enum message
DavoudEshtehari May 19, 2020
a708ae3
Update NetCore
DavoudEshtehari May 19, 2020
6345d10
Merge branch 'SSL_TLS_WarningMessage' of github.com:DavoudEshtehari/S…
DavoudEshtehari May 19, 2020
0fd42f3
Revert "Merge branch 'SSL_TLS_WarningMessage' of github.com:DavoudEsh…
DavoudEshtehari May 19, 2020
4c37241
Improved warning
DavoudEshtehari Jun 4, 2020
bc1de6d
Merge remote-tracking branch 'UpStream/master' into SSL_TLS_WarningMe…
DavoudEshtehari Jun 4, 2020
839c901
Amended external functions
DavoudEshtehari Jun 5, 2020
7953af1
Addressed reviews
DavoudEshtehari Jun 5, 2020
8b0870a
Reverted the M.D.S.nuspec
DavoudEshtehari Jun 9, 2020
84ca695
Added summary
DavoudEshtehari Jun 9, 2020
35b7ad6
Amended document
DavoudEshtehari Jun 9, 2020
87fb742
Address reviews
DavoudEshtehari Jun 9, 2020
492bc9f
Merge branch 'master' into SSL_TLS_WarningMessage
cheenamalhotra Jun 10, 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
7 changes: 7 additions & 0 deletions doc/snippets/Microsoft.Data.SqlClient/SqlClientLogger.xml
Original file line number Diff line number Diff line change
Expand Up @@ -37,5 +37,12 @@
<summary>Logs information through a specified method of the current instance type.</summary>
<remarks>To be added.</remarks>
</LogInfo>
<LogWarning>
<param name="type">The type to be logged.</param>
<param name="method">The logging method.</param>
<param name="message">The message to be logged.</param>
<summary>Logs warning through a specified method of the current instance type.</summary>
<remarks>To be added.</remarks>
JRahnama marked this conversation as resolved.
Show resolved Hide resolved
DavoudEshtehari marked this conversation as resolved.
Show resolved Hide resolved
</LogWarning>
</members>
</docs>
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ internal struct SNI_Error
internal static extern uint SNITerminate();

[DllImport(SNI, CallingConvention = CallingConvention.Cdecl, EntryPoint = "SNIWaitForSSLHandshakeToCompleteWrapper")]
internal static extern uint SNIWaitForSSLHandshakeToComplete([In] SNIHandle pConn, int dwMilliseconds);
internal static extern uint SNIWaitForSSLHandshakeToComplete([In] SNIHandle pConn, int dwMilliseconds, out uint pProtocolVersion);
DavoudEshtehari marked this conversation as resolved.
Show resolved Hide resolved

[DllImport(SNI, CallingConvention = CallingConvention.Cdecl)]
internal static extern uint UnmanagedIsTokenRestricted([In] IntPtr token, [MarshalAs(UnmanagedType.Bool)] out bool isRestricted);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,10 @@ internal abstract class SNIHandle

public abstract void ReturnPacket(SNIPacket packet);

/// <summary>
/// Gets a value that indicates the security protocol used to authenticate this connection.
/// </summary>
public virtual int ProtocolVersion { get; } = 0;
#if DEBUG
/// <summary>
/// Test handle for killing underlying connection
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ public Guid ConnectionId
}
}

public int ProtocolVersion => _lowerHandle.ProtocolVersion;

/// <summary>
/// Constructor
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ internal sealed class SNIMarsHandle : SNIHandle

public override int ReserveHeaderSize => SNISMUXHeader.HEADER_LENGTH;

public override int ProtocolVersion => _connection.ProtocolVersion;

/// <summary>
/// Dispose object
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,21 @@ public override uint Status
}
}

public override int ProtocolVersion
{
get
{
try
{
return (int)_sslStream.SslProtocol;
}
catch
{
return base.ProtocolVersion;
}
}
}

public override uint CheckConnection()
{
long scopeID = SqlClientEventSource.Log.SNIScopeEnterEvent("<sc.SNI.SNINpHandle.CheckConnection |SNI|INFO>");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,21 @@ public override uint Status
}
}

public override int ProtocolVersion
{
get
{
try
{
return (int)_sslStream.SslProtocol;
}
catch
{
return base.ProtocolVersion;
}
}
}

/// <summary>
/// Constructor
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,9 @@ private void LoadSSPILibrary()
// No - Op
}

private void WaitForSSLHandShakeToComplete(ref uint error)
private void WaitForSSLHandShakeToComplete(ref uint error, ref int protocolVersion)
{
// No - Op

}

private SNIErrorDetails GetSniErrorDetails()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,15 +75,13 @@ private void LoadSSPILibrary()
}
}

private void WaitForSSLHandShakeToComplete(ref uint error)
private void WaitForSSLHandShakeToComplete(ref uint error, ref int protocolVersion)
{
if (TdsParserStateObjectFactory.UseManagedSNI)
return;
cheenamalhotra marked this conversation as resolved.
Show resolved Hide resolved
// in the case where an async connection is made, encryption is used and Windows Authentication is used,
// wait for SSL handshake to complete, so that the SSL context is fully negotiated before we try to use its
// Channel Bindings as part of the Windows Authentication context build (SSL handshake must complete
// before calling SNISecGenClientContext).
error = _physicalStateObj.WaitForSSLHandShakeToComplete();
error = _physicalStateObj.WaitForSSLHandShakeToComplete(out protocolVersion);
if (error != TdsEnums.SNI_SUCCESS)
{
_physicalStateObj.AddError(ProcessSNIError(_physicalStateObj));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
using System.Diagnostics;
using System.Globalization;
using System.IO;
using System.Reflection;
using System.Security.Authentication;
using System.Text;
using System.Threading;
using System.Threading.Tasks;
Expand All @@ -19,6 +21,7 @@
using Microsoft.Data.SqlClient.DataClassification;
using Microsoft.Data.SqlClient.Server;
using Microsoft.Data.SqlTypes;
using Newtonsoft.Json.Serialization;

namespace Microsoft.Data.SqlClient
{
Expand All @@ -39,6 +42,9 @@ internal struct SNIErrorDetails
internal sealed partial class TdsParser
{
private static int _objectTypeCount; // EventSource counter
private readonly SqlClientLogger _logger = new SqlClientLogger();
private readonly string _typeName;

internal readonly int _objectID = Interlocked.Increment(ref _objectTypeCount);
internal int ObjectID => _objectID;

Expand Down Expand Up @@ -174,6 +180,7 @@ internal TdsParser(bool MARS, bool fAsynchronous)

_physicalStateObj = TdsParserStateObjectFactory.Singleton.CreateTdsParserStateObject(this);
DataClassificationVersion = TdsEnums.DATA_CLASSIFICATION_NOT_ENABLED;
_typeName = GetType().Name;
}

internal SqlInternalConnectionTds Connection
Expand Down Expand Up @@ -900,7 +907,15 @@ private PreLoginHandshakeStatus ConsumePreLoginHandshake(bool encrypt, bool trus
ThrowExceptionAndWarning(_physicalStateObj);
}

WaitForSSLHandShakeToComplete(ref error);
int protocolVersion = 0;
WaitForSSLHandShakeToComplete(ref error, ref protocolVersion);

SslProtocols protocol = (SslProtocols)protocolVersion;
string warningMessage = protocol.GetProtocolWarning();
if(!string.IsNullOrEmpty(warningMessage))
{
_logger.LogWarning(_typeName, MethodBase.GetCurrentMethod().Name, warningMessage);
}

// create a new packet encryption changes the internal packet size
_physicalStateObj.ClearAllWritePackets();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using System.Diagnostics;
using System.Globalization;
using System.Security;
using System.Security.Authentication;
using System.Text;
using Microsoft.Data.Common;
using Microsoft.Data.SqlTypes;
Expand Down Expand Up @@ -917,4 +918,59 @@ private void ParseMultipartName()

internal static readonly MultiPartTableName Null = new MultiPartTableName(new string[] { null, null, null, null });
}

internal static class SslProtocolsHelper
{
private static string ToFriendlyName(this SslProtocols protocol)
{
string name;

/* The SslProtocols.Tls13 is supported by netcoreapp3.1 and later
* This driver does not support this version yet!
if ((protocol & SslProtocols.Tls13) == SslProtocols.Tls13)
{
name = "TLS 1.3";
}*/
if((protocol & SslProtocols.Tls12) == SslProtocols.Tls12)
JRahnama marked this conversation as resolved.
Show resolved Hide resolved
{
name = "TLS 1.2";
}
else if ((protocol & SslProtocols.Tls11) == SslProtocols.Tls11)
{
name = "TLS 1.1";
}
else if ((protocol & SslProtocols.Tls) == SslProtocols.Tls)
{
name = "TLS 1.0";
}
#pragma warning disable CS0618 // Type or member is obsolete: SSL is depricated
else if ((protocol & SslProtocols.Ssl3) == SslProtocols.Ssl3)
{
name = "SSL 3.0";
}
else if ((protocol & SslProtocols.Ssl2) == SslProtocols.Ssl2)
#pragma warning restore CS0618 // Type or member is obsolete: SSL is depricated
{
name = "SSL 2.0";
}
else
{
name = protocol.ToString();
}

return name;
}

public static string GetProtocolWarning(this SslProtocols protocol)
DavoudEshtehari marked this conversation as resolved.
Show resolved Hide resolved
{
string message = string.Empty;
#pragma warning disable CS0618 // Type or member is obsolete : SSL is depricated
if ((protocol & (SslProtocols.Ssl2 | SslProtocols.Ssl3 | SslProtocols.Tls | SslProtocols.Tls11)) != SslProtocols.None)
#pragma warning restore CS0618 // Type or member is obsolete : SSL is depricated
{
message = SRHelper.Format(SR.SEC_ProtocolWarning, protocol.ToFriendlyName());
}
return message;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -774,7 +774,7 @@ private void ResetCancelAndProcessAttention()

internal abstract uint EnableSsl(ref uint info);

internal abstract uint WaitForSSLHandShakeToComplete();
internal abstract uint WaitForSSLHandShakeToComplete(out int protocolVersion);

internal abstract void Dispose();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,11 @@ internal override uint EnableMars(ref uint info)
return TdsEnums.SNI_ERROR;
}

internal override uint EnableSsl(ref uint info) => SNIProxy.Singleton.EnableSsl(Handle, info);
internal override uint EnableSsl(ref uint info)
{
uint result = SNIProxy.Singleton.EnableSsl(Handle, info);
return result;
}
DavoudEshtehari marked this conversation as resolved.
Show resolved Hide resolved

internal override uint SetConnectionBufferSize(ref uint unsignedPacketSize) => SNIProxy.Singleton.SetConnectionBufferSize(Handle, unsignedPacketSize);

Expand All @@ -220,6 +224,10 @@ internal override uint GenerateSspiClientContext(byte[] receivedBuff, uint recei
return 0;
}

internal override uint WaitForSSLHandShakeToComplete() => 0;
internal override uint WaitForSSLHandShakeToComplete(out int protocolVersion)
{
protocolVersion = Handle.ProtocolVersion;
return 0;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,33 @@
using System.Collections.Generic;
using System.Diagnostics;
using System.Runtime.InteropServices;
using System.Security.Authentication;
using System.Threading.Tasks;
using Microsoft.Data.Common;

namespace Microsoft.Data.SqlClient
{
internal class TdsParserStateObjectNative : TdsParserStateObject
{
// protocol versions from native sni
[Flags]
private enum NativeProtocols
{
SP_PROT_SSL2_SERVER = 0x00000004,
SP_PROT_SSL2_CLIENT = 0x00000008,
SP_PROT_SSL3_SERVER = 0x00000010,
SP_PROT_SSL3_CLIENT = 0x00000020,
SP_PROT_TLS1_0_SERVER = 0x00000040,
SP_PROT_TLS1_0_CLIENT = 0x00000080,
SP_PROT_TLS1_1_SERVER = 0x00000100,
SP_PROT_TLS1_1_CLIENT = 0x00000200,
SP_PROT_TLS1_2_SERVER = 0x00000400,
SP_PROT_TLS1_2_CLIENT = 0x00000800,
SP_PROT_TLS1_3_SERVER = 0x00001000,
SP_PROT_TLS1_3_CLIENT = 0x00002000,
SP_PROT_NONE = 0x0
}

private SNIHandle _sessionHandle = null; // the SNI handle we're to work on

private SNIPacket _sniPacket = null; // Will have to re-vamp this for MARS
Expand Down Expand Up @@ -300,7 +320,8 @@ internal override uint EnableMars(ref uint info)
internal override uint EnableSsl(ref uint info)
{
// Add SSL (Encryption) SNI provider.
return SNINativeMethodWrapper.SNIAddProvider(Handle, SNINativeMethodWrapper.ProviderEnum.SSL_PROV, ref info);
uint result = SNINativeMethodWrapper.SNIAddProvider(Handle, SNINativeMethodWrapper.ProviderEnum.SSL_PROV, ref info);
return result;
DavoudEshtehari marked this conversation as resolved.
Show resolved Hide resolved
}

internal override uint SetConnectionBufferSize(ref uint unsignedPacketSize)
Expand All @@ -309,8 +330,49 @@ internal override uint SetConnectionBufferSize(ref uint unsignedPacketSize)
internal override uint GenerateSspiClientContext(byte[] receivedBuff, uint receivedLength, ref byte[] sendBuff, ref uint sendLength, byte[] _sniSpnBuffer)
=> SNINativeMethodWrapper.SNISecGenClientContext(Handle, receivedBuff, receivedLength, sendBuff, ref sendLength, _sniSpnBuffer);

internal override uint WaitForSSLHandShakeToComplete()
=> SNINativeMethodWrapper.SNIWaitForSSLHandshakeToComplete(Handle, GetTimeoutRemaining());
internal override uint WaitForSSLHandShakeToComplete(out int protocolVersion)
{
uint returnValue = SNINativeMethodWrapper.SNIWaitForSSLHandshakeToComplete(Handle, GetTimeoutRemaining(), out uint nativeProtocolVersion);
var nativeProtocol = (NativeProtocols)nativeProtocolVersion;
cheenamalhotra marked this conversation as resolved.
Show resolved Hide resolved

/* The SslProtocols.Tls13 is supported by netcoreapp3.1 and later
* This driver does not support this version yet!
if (nativeProtocol.HasFlag(NativeProtocols.SP_PROT_TLS1_3_CLIENT) || nativeProtocol.HasFlag(NativeProtocols.SP_PROT_TLS1_3_SERVER))
{
protocolVersion = (int)SslProtocols.Tls13;
}*/
if (nativeProtocol.HasFlag(NativeProtocols.SP_PROT_TLS1_2_CLIENT) || nativeProtocol.HasFlag(NativeProtocols.SP_PROT_TLS1_2_SERVER))
{
protocolVersion = (int)SslProtocols.Tls12;
}
else if (nativeProtocol.HasFlag(NativeProtocols.SP_PROT_TLS1_1_CLIENT) || nativeProtocol.HasFlag(NativeProtocols.SP_PROT_TLS1_1_SERVER))
{
protocolVersion = (int)SslProtocols.Tls11;
}
else if (nativeProtocol.HasFlag(NativeProtocols.SP_PROT_TLS1_0_CLIENT) || nativeProtocol.HasFlag(NativeProtocols.SP_PROT_TLS1_0_SERVER))
{
protocolVersion = (int)SslProtocols.Tls;
}
else if (nativeProtocol.HasFlag(NativeProtocols.SP_PROT_SSL3_CLIENT) || nativeProtocol.HasFlag(NativeProtocols.SP_PROT_SSL3_SERVER))
{
#pragma warning disable CS0618 // Type or member is obsolete : SSL is depricated
protocolVersion = (int)SslProtocols.Ssl3;
}
else if (nativeProtocol.HasFlag(NativeProtocols.SP_PROT_SSL2_CLIENT) || nativeProtocol.HasFlag(NativeProtocols.SP_PROT_SSL2_SERVER))
{
protocolVersion = (int)SslProtocols.Ssl2;
#pragma warning restore CS0618 // Type or member is obsolete : SSL is depricated
}
else if(nativeProtocol.HasFlag(NativeProtocols.SP_PROT_NONE))
{
protocolVersion = (int)SslProtocols.None;
karinazhou marked this conversation as resolved.
Show resolved Hide resolved
}
else
{
throw new ArgumentException(SRHelper.Format(SRHelper.net_invalid_enum, nameof(NativeProtocols)), nameof(NativeProtocols));
}
return returnValue;
}

internal override void DisposePacketCache()
{
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions src/Microsoft.Data.SqlClient/netcore/src/Resources/SR.resx
Original file line number Diff line number Diff line change
Expand Up @@ -1863,4 +1863,7 @@
<data name="SQL_BulkLoadCannotConvertValueWithoutRowNo" xml:space="preserve">
<value>The given value{0} of type {1} from the data source cannot be converted to type {2} for Column {3} [{4}].</value>
</data>
<data name="SEC_ProtocolWarning" xml:space="preserve">
<value>Security Warning: The negotiated {0} is an insecure protocol and is supported for backward compatibility only. The recommended protocol version is TLS 1.2 and later.</value>
</data>
</root>
Loading