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

Support sites with invalid IDN in SslStream #82934

Merged
merged 4 commits into from
Mar 10, 2023
Merged

Support sites with invalid IDN in SslStream #82934

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
749fe7b
initial test
wfurt Mar 1, 2023
c17e019
'update'
wfurt Mar 3, 2023
2264e06
feedback from review
wfurt Mar 5, 2023
a491fc4
android
wfurt Mar 8, 2023
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
49 changes: 25 additions & 24 deletions src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Diagnostics;
using System.Globalization;
using System.IO;
using System.Net;
using System.Net.Security;
Expand All @@ -25,7 +24,6 @@ internal static partial class OpenSsl
private const string TlsCacheSizeCtxName = "System.Net.Security.TlsCacheSize";
private const string TlsCacheSizeEnvironmentVariable = "DOTNET_SYSTEM_NET_SECURITY_TLSCACHESIZE";
private const SslProtocols FakeAlpnSslProtocol = (SslProtocols)1; // used to distinguish server sessions with ALPN
private static readonly IdnMapping s_idnMapping = new IdnMapping();
private static readonly ConcurrentDictionary<SslProtocols, SafeSslContextHandle> s_clientSslContexts = new ConcurrentDictionary<SslProtocols, SafeSslContextHandle>();

#region internal methods
Expand Down Expand Up @@ -385,21 +383,22 @@ internal static SafeSslHandle AllocateSslHandle(SslAuthenticationOptions sslAuth

if (sslAuthenticationOptions.IsClient)
{
// The IdnMapping converts unicode input into the IDNA punycode sequence.
string punyCode = string.IsNullOrEmpty(sslAuthenticationOptions.TargetHost) ? string.Empty : s_idnMapping.GetAscii(sslAuthenticationOptions.TargetHost!);

// Similar to windows behavior, set SNI on openssl by default for client context, ignore errors.
if (!Ssl.SslSetTlsExtHostName(sslHandle, punyCode))
if (!string.IsNullOrEmpty(sslAuthenticationOptions.TargetHost))
{
Crypto.ErrClearError();
}
// Similar to windows behavior, set SNI on openssl by default for client context, ignore errors.
if (!Ssl.SslSetTlsExtHostName(sslHandle, sslAuthenticationOptions.TargetHost))
{
Crypto.ErrClearError();
}

if (cacheSslContext && !string.IsNullOrEmpty(punyCode))
{
sslCtxHandle.TrySetSession(sslHandle, punyCode);
bool ignored = false;
sslCtxHandle.DangerousAddRef(ref ignored);
sslHandle.SslContextHandle = sslCtxHandle;

if (cacheSslContext)
{
sslCtxHandle.TrySetSession(sslHandle, sslAuthenticationOptions.TargetHost);
bool ignored = false;
sslCtxHandle.DangerousAddRef(ref ignored);
sslHandle.SslContextHandle = sslCtxHandle;
}
}

// relevant to TLS 1.3 only: if user supplied a client cert or cert callback,
Expand Down Expand Up @@ -745,16 +744,18 @@ private static unsafe int NewSessionCallback(IntPtr ssl, IntPtr session)
Debug.Assert(session != IntPtr.Zero);

IntPtr ptr = Ssl.SslGetData(ssl);
Debug.Assert(ptr != IntPtr.Zero);
GCHandle gch = GCHandle.FromIntPtr(ptr);

SafeSslContextHandle? ctxHandle = gch.Target as SafeSslContextHandle;
// There is no relation between SafeSslContextHandle and SafeSslHandle so the handle
// may be released while the ssl session is still active.
if (ctxHandle != null && ctxHandle.TryAddSession(Ssl.SslGetServerName(ssl), session))
if (ptr != IntPtr.Zero)
{
// offered session was stored in our cache.
return 1;
GCHandle gch = GCHandle.FromIntPtr(ptr);

SafeSslContextHandle? ctxHandle = gch.Target as SafeSslContextHandle;
// There is no relation between SafeSslContextHandle and SafeSslHandle so the handle
// may be released while the ssl session is still active.
if (ctxHandle != null && ctxHandle.TryAddSession(Ssl.SslGetServerName(ssl), session))
{
// offered session was stored in our cache.
return 1;
}
}

// OpenSSL will destroy session.
Expand Down
16 changes: 3 additions & 13 deletions src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics;
using System.Globalization;
using System.Runtime.InteropServices;
using System.Security.Authentication.ExtendedProtection;
using System.Security.Cryptography.X509Certificates;
Expand Down Expand Up @@ -334,9 +333,6 @@ internal abstract partial class SafeDeleteContext : DebugSafeHandle
internal abstract partial class SafeDeleteContext : SafeHandle
{
#endif
private const string dummyStr = " ";
private static readonly IdnMapping s_idnMapping = new IdnMapping();

protected SafeFreeCredentials? _EffectiveCredential;

//-------------------------------------------------------------------
Expand Down Expand Up @@ -453,18 +449,12 @@ internal static unsafe int InitializeSecurityContext(
}
}

if (targetName == null || targetName.Length == 0)
{
targetName = dummyStr;
}

string punyCode = s_idnMapping.GetAscii(targetName);
fixed (char* namePtr = punyCode)
fixed (char* namePtr = targetName)
{
errorCode = MustRunInitializeSecurityContext(
ref inCredentials,
isContextAbsent,
(byte*)(((object)targetName == (object)dummyStr) ? null : namePtr),
(byte*)namePtr,
inFlags,
endianness,
&inSecurityBufferDescriptor,
Expand Down Expand Up @@ -514,7 +504,7 @@ internal static unsafe int InitializeSecurityContext(
errorCode = MustRunInitializeSecurityContext(
ref inCredentials,
isContextAbsent,
(byte*)(((object)targetName == (object)dummyStr) ? null : namePtr),
(byte*)namePtr,
inFlags,
endianness,
&inSecurityBufferDescriptor,
Expand Down
38 changes: 36 additions & 2 deletions src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Collections.Generic;
using System.Diagnostics;
using System.Globalization;
using System.Runtime.InteropServices;
using System.Security.Authentication;
using System.Security.Cryptography.X509Certificates;
Expand All @@ -11,6 +12,8 @@ namespace System.Net.Security
{
internal sealed class SslAuthenticationOptions
{
private static readonly IdnMapping s_idnMapping = new IdnMapping();

// Simplified version of IPAddressParser.Parse to avoid allocations and dependencies.
// It purposely ignores scopeId as we don't really use so we do not need to map it to actual interface id.
private static unsafe bool IsValidAddress(ReadOnlySpan<char> ipSpan)
Expand Down Expand Up @@ -46,6 +49,20 @@ private static unsafe bool IsValidAddress(ReadOnlySpan<char> ipSpan)
return false;
}

private static unsafe bool IsSafeDnsString(ReadOnlySpan<char> name)
{
for (int i = 0; i < name.Length; i++)
Copy link
Member

Choose a reason for hiding this comment

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

This could use the new IndexOfAnyValues<T>, but not sure if the allocation of static map is worth it for such short strings.

Copy link
Member

@MihaZupan MihaZupan Mar 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, this should use IndexOfAnyValues. The allocation there is very small, we'll only store a bitmap for the ASCII range.

// Alphanumeric or '-', '.', '_'
private static readonly IndexOfAnyValues<char> s_safeDnsChars =
    IndexOfAnyValues.Create("-.0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz");

private static bool IsSafeDnsString(ReadOnlySpan<char> name) =>
    name.IndexOfAnyExcept(s_safeDnsChars) < 0;

Copy link
Member Author

Choose a reason for hiding this comment

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

IndexOfAnyValues crossed my mind but the set of characters I don't want is large for Unicode. I was not aware of IndexOfAnyExcept

Copy link
Member Author

Choose a reason for hiding this comment

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

updated. Just to be sure: this would only match that ASCII characters, not their Unicode equivalent (if any), right @MihaZupan ?

Copy link
Member

Choose a reason for hiding this comment

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

That's right, the set of values you pass to Create is "ordinal".

{
if (!char.IsAsciiLetterOrDigit(name[i]) && name[i] != '.' && name[i] != '-' && name[i] != '_')
{
return false;
}
}

return true;
}


internal SslAuthenticationOptions()
{
TargetHost = string.Empty;
Expand Down Expand Up @@ -86,13 +103,30 @@ internal void UpdateOptions(SslClientAuthenticationOptions sslClientAuthenticati
if (!string.IsNullOrEmpty(sslClientAuthenticationOptions.TargetHost))
{
// RFC 6066 section 3 says to exclude trailing dot from fully qualified DNS hostname
TargetHost = sslClientAuthenticationOptions.TargetHost.TrimEnd('.');
string targetHost = sslClientAuthenticationOptions.TargetHost.TrimEnd('.');

// RFC 6066 forbids IP literals
if (IsValidAddress(TargetHost))
if (IsValidAddress(targetHost))
{
TargetHost = string.Empty;
}
else
{
try
{
TargetHost = s_idnMapping.GetAscii(targetHost);
}
catch (ArgumentException)
{
if (!IsSafeDnsString(targetHost))
{
throw;
}

// Seems like name that does not confrom to IDN but apers somewhat valid according to orogional DNS rfc.
TargetHost = targetHost;
}
}
}

// Client specific options.
Expand Down
81 changes: 81 additions & 0 deletions src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamSniTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,87 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout(
Assert.Equal(string.Empty, server.TargetHostName);
}

[Theory]
[InlineData("\u00E1b\u00E7d\u00EB.com")]
[InlineData("\u05D1\u05F1.com")]
[InlineData("\u30B6\u30C7\u30D8.com")]
public async Task SslStream_ValidIdn_Success(string name)
{
(SslStream client, SslStream server) = TestHelper.GetConnectedSslStreams();
using (client)
using (server)
{
using X509Certificate2 serverCertificate = Configuration.Certificates.GetServerCertificate();
using X509Certificate2 clientCertificate = Configuration.Certificates.GetClientCertificate();

SslServerAuthenticationOptions serverOptions = new SslServerAuthenticationOptions() { ServerCertificate = serverCertificate };
SslClientAuthenticationOptions clientOptions = new SslClientAuthenticationOptions()
{
TargetHost = name,
CertificateChainPolicy = new X509ChainPolicy() { VerificationFlags = X509VerificationFlags.IgnoreInvalidName },
RemoteCertificateValidationCallback = (sender, certificate, chain, sslPolicyErrors) => true
};

await TestConfiguration.WhenAllOrAnyFailedWithTimeout(
client.AuthenticateAsClientAsync(clientOptions, default),
server.AuthenticateAsServerAsync(serverOptions, default));

await TestHelper.PingPong(client, server, default);
Assert.Equal(name, server.TargetHostName);
}
}

[Theory]
[InlineData("www-.volal.cz")]
[InlineData("www-.colorhexa.com")]
[InlineData("xn--www-7m0a.thegratuit.com")]
public async Task SslStream_SafeInvalidIdn_Success(string name)
{
(SslStream client, SslStream server) = TestHelper.GetConnectedSslStreams();
using (client)
using (server)
{
using X509Certificate2 serverCertificate = Configuration.Certificates.GetServerCertificate();
using X509Certificate2 clientCertificate = Configuration.Certificates.GetClientCertificate();

SslServerAuthenticationOptions serverOptions = new SslServerAuthenticationOptions() { ServerCertificate = serverCertificate };
SslClientAuthenticationOptions clientOptions = new SslClientAuthenticationOptions()
{
TargetHost = name,
CertificateChainPolicy = new X509ChainPolicy() { VerificationFlags = X509VerificationFlags.IgnoreInvalidName },
RemoteCertificateValidationCallback = (sender, certificate, chain, sslPolicyErrors) => true
};

await TestConfiguration.WhenAllOrAnyFailedWithTimeout(
client.AuthenticateAsClientAsync(clientOptions, default),
server.AuthenticateAsServerAsync(serverOptions, default));

await TestHelper.PingPong(client, server, default);
Assert.Equal(name, server.TargetHostName);
}
}

[Theory]
[InlineData("\u0000\u00E7d\u00EB.com")]
public async Task SslStream_UnsafeInvalidIdn_Throws(string name)
{
(SslStream client, SslStream server) = TestHelper.GetConnectedSslStreams();
using (client)
using (server)
{
using X509Certificate2 serverCertificate = Configuration.Certificates.GetServerCertificate();

SslClientAuthenticationOptions clientOptions = new SslClientAuthenticationOptions()
{
TargetHost = name,
CertificateChainPolicy = new X509ChainPolicy() { VerificationFlags = X509VerificationFlags.IgnoreInvalidName },
RemoteCertificateValidationCallback = (sender, certificate, chain, sslPolicyErrors) => true
};

await Assert.ThrowsAsync<ArgumentException>(() => client.AuthenticateAsClientAsync(clientOptions, default));
}
}

private static Func<Task> WithAggregateExceptionUnwrapping(Func<Task> a)
{
return async () => {
Expand Down