Skip to content
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
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
25 changes: 20 additions & 5 deletions src/libraries/Common/src/Interop/OSX/Interop.CoreFoundation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -131,10 +131,9 @@ internal static unsafe SafeCreateHandle CFStringCreateFromSpan(ReadOnlySpan<char
/// <param name="callbacks">Should be IntPtr.Zero</param>
/// <returns>Returns a pointer to a CFArray on success; otherwise, returns IntPtr.Zero</returns>
[GeneratedDllImport(Interop.Libraries.CoreFoundationLibrary)]
private static partial SafeCreateHandle CFArrayCreate(
private static unsafe partial SafeCreateHandle CFArrayCreate(
IntPtr allocator,
[MarshalAs(UnmanagedType.LPArray)]
IntPtr[] values,
IntPtr* values,
UIntPtr numValues,
IntPtr callbacks);

Expand All @@ -144,9 +143,25 @@ private static partial SafeCreateHandle CFArrayCreate(
/// <param name="values">The values to put in the array</param>
/// <param name="numValues">The number of values in the array</param>
/// <returns>Returns a valid SafeCreateHandle to a CFArray on success; otherwise, returns an invalid SafeCreateHandle</returns>
internal static SafeCreateHandle CFArrayCreate(IntPtr[] values, UIntPtr numValues)
internal static unsafe SafeCreateHandle CFArrayCreate(IntPtr[] values, UIntPtr numValues)
{
return CFArrayCreate(IntPtr.Zero, values, numValues, IntPtr.Zero);
fixed (IntPtr* pValues = values)
{
return CFArrayCreate(IntPtr.Zero, pValues, (UIntPtr)values.Length, IntPtr.Zero);
}
}

/// <summary>
/// Creates a pointer to an unmanaged CFArray containing the input values. Follows the "Create Rule" where if you create it, you delete it.
/// </summary>
/// <param name="values">The values to put in the array</param>
/// <returns>Returns a valid SafeCreateHandle to a CFArray on success; otherwise, returns an invalid SafeCreateHandle</returns>
internal static unsafe SafeCreateHandle CFArrayCreate(Span<IntPtr> values)
{
fixed (IntPtr* pValues = &MemoryMarshal.GetReference(values))
{
return CFArrayCreate(IntPtr.Zero, pValues, (UIntPtr)values.Length, IntPtr.Zero);
}
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,22 @@ private static partial int AppleCryptoNative_SslIsHostnameMatch(
[GeneratedDllImport(Interop.Libraries.AppleCryptoNative, EntryPoint = "AppleCryptoNative_SslSetEnabledCipherSuites")]
internal static unsafe partial int SslSetEnabledCipherSuites(SafeSslHandle sslHandle, uint* cipherSuites, int numCipherSuites);

[GeneratedDllImport(Interop.Libraries.AppleCryptoNative, EntryPoint = "AppleCryptoNative_SslSetCertificateAuthorities")]
internal static partial int SslSetCertificateAuthorities(SafeSslHandle sslHandle, SafeCreateHandle certificateOrArray, int replaceExisting);

internal static unsafe void SslSetCertificateAuthorities(SafeSslHandle sslHandle, Span<IntPtr> certificates, bool replaceExisting)
{
using (SafeCreateHandle cfCertRefs = CoreFoundation.CFArrayCreate(certificates))
{
int osStatus = SslSetCertificateAuthorities(sslHandle, cfCertRefs, replaceExisting ? 1 : 0);

if (osStatus != 0)
{
throw CreateExceptionForOSStatus(osStatus);
}
}
}

internal static void SslSetAcceptClientCert(SafeSslHandle sslHandle)
{
int osStatus = AppleCryptoNative_SslSetAcceptClientCert(sslHandle);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using System.Security.Authentication;
using System.Security.Authentication.ExtendedProtection;
using System.Security.Cryptography;
using System.Security.Cryptography.X509Certificates;
using Microsoft.Win32.SafeHandles;

internal static partial class Interop
Expand Down Expand Up @@ -46,32 +47,32 @@ internal static partial class OpenSsl
return bindingHandle;
}

private static volatile int s_disableTlsResume = -1;

private static bool DisableTlsResume
{
get
{
int disableTlsResume = s_disableTlsResume;
if (disableTlsResume != -1)
{
return disableTlsResume != 0;
}

// First check for the AppContext switch, giving it priority over the environment variable.
if (AppContext.TryGetSwitch(DisableTlsResumeCtxSwitch, out bool value))
{
s_disableTlsResume = value ? 1 : 0;
}
else
{
// AppContext switch wasn't used. Check the environment variable.
private static volatile int s_disableTlsResume = -1;

private static bool DisableTlsResume
{
get
{
int disableTlsResume = s_disableTlsResume;
if (disableTlsResume != -1)
{
return disableTlsResume != 0;
}

// First check for the AppContext switch, giving it priority over the environment variable.
if (AppContext.TryGetSwitch(DisableTlsResumeCtxSwitch, out bool value))
{
s_disableTlsResume = value ? 1 : 0;
}
else
{
// AppContext switch wasn't used. Check the environment variable.
s_disableTlsResume =
Environment.GetEnvironmentVariable(DisableTlsResumeEnvironmentVariable) is string envVar &&
(envVar == "1" || envVar.Equals("true", StringComparison.OrdinalIgnoreCase)) ? 1 : 0;
}
}

return s_disableTlsResume != 0;
return s_disableTlsResume != 0;
}
}

Expand Down Expand Up @@ -147,7 +148,7 @@ internal static SafeSslContextHandle AllocateSslContext(SafeFreeSslCredentials c
// Sets policy and security level
if (!Ssl.SetEncryptionPolicy(sslCtx, sslAuthenticationOptions.EncryptionPolicy))
{
throw new SslException( SR.Format(SR.net_ssl_encryptionpolicy_notsupported, sslAuthenticationOptions.EncryptionPolicy));
throw new SslException(SR.Format(SR.net_ssl_encryptionpolicy_notsupported, sslAuthenticationOptions.EncryptionPolicy));
}
}

Expand Down Expand Up @@ -274,7 +275,7 @@ internal static SafeSslHandle AllocateSslHandle(SafeFreeSslCredentials credentia

if (cacheSslContext)
{
sslAuthenticationOptions.CertificateContext!.SslContexts!.TryGetValue(protocols | (SslProtocols)(hasAlpn ? 1 : 0), out sslCtxHandle);
sslAuthenticationOptions.CertificateContext!.SslContexts!.TryGetValue(protocols | (SslProtocols)(hasAlpn ? 1 : 0), out sslCtxHandle);
}

if (sslCtxHandle == null)
Expand Down Expand Up @@ -339,10 +340,36 @@ internal static SafeSslHandle AllocateSslHandle(SafeFreeSslCredentials credentia
// if server actually requests a certificate.
Ssl.SslSetClientCertCallback(sslHandle, 1);
}

if (sslAuthenticationOptions.IsServer && sslAuthenticationOptions.RemoteCertRequired)
else // sslAuthenticationOptions.IsServer
{
Ssl.SslSetVerifyPeer(sslHandle);
if (sslAuthenticationOptions.RemoteCertRequired)
{
Ssl.SslSetVerifyPeer(sslHandle);
}

if (sslAuthenticationOptions.CertificateContext?.Trust?._sendTrustInHandshake == true)
{
SslCertificateTrust trust = sslAuthenticationOptions.CertificateContext!.Trust!;
X509Certificate2Collection certList = (trust._trustList ?? trust._store!.Certificates);

Debug.Assert(certList != null, "certList != null");
Span<IntPtr> handles = certList.Count <= 256
? stackalloc IntPtr[256]
: new IntPtr[certList.Count];

for (int i = 0; i < certList.Count; i++)
{
handles[i] = certList[i].Handle;
}

if (!Ssl.SslAddClientCAs(sslHandle, handles.Slice(0, certList.Count)))
{
// The method can fail only when the number of cert names exceeds the maximum capacity
// supported by STACK_OF(X509_NAME) structure, which should not happen under normal
// operation.
Debug.Fail("Failed to add issuer to trusted CA list.");
}
}
}
}
catch
Expand Down Expand Up @@ -576,7 +603,7 @@ private static unsafe int AlpnServerSelectCallback(IntPtr ssl, byte** outp, byte
{
*outp = null;
*outlen = 0;
IntPtr sslData = Ssl.SslGetData(ssl);
IntPtr sslData = Ssl.SslGetData(ssl);

// reset application data to avoid dangling pointer.
Ssl.SslSetData(ssl, IntPtr.Zero);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,17 @@ internal static unsafe int SslSetAlpnProtos(SafeSslHandle ssl, Span<byte> serial
[GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslAddExtraChainCert")]
internal static partial bool SslAddExtraChainCert(SafeSslHandle ssl, SafeX509Handle x509);

[GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslAddClientCAs")]
private static unsafe partial bool SslAddClientCAs(SafeSslHandle ssl, IntPtr* x509s, int count);

internal static unsafe bool SslAddClientCAs(SafeSslHandle ssl, Span<IntPtr> x509handles)
{
fixed (IntPtr* pHandles = &MemoryMarshal.GetReference(x509handles))
{
return SslAddClientCAs(ssl, pHandles, x509handles.Length);
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if we should throw here on failure to be consistent with SslSetCertificateAuthorities on macOS.

Copy link
Member Author

Choose a reason for hiding this comment

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

this can fail only when maximum number of STACK_OF(X509_NAME) items is reached (something around MAX_INT), so we would not be likely to throw anyway, I added the propagation of the return code for consistency.

}
}

internal static bool AddExtraChainCertificates(SafeSslHandle ssl, X509Certificate2[] chain)
{
// send pre-computed list of intermediates.
Expand Down
3 changes: 3 additions & 0 deletions src/libraries/System.Net.Security/src/Resources/Strings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -386,4 +386,7 @@
<data name="net_ssl_trust_collection" xml:space="preserve">
<value>Sending trust from collection is not supported on Windows.</value>
</data>
<data name="net_ssl_trust_handshake" xml:space="preserve">
<value>Sending trust in handshake is not supported on this platform.</value>
</data>
</root>
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,44 @@ public SafeDeleteSslContext(SafeFreeSslCredentials credential, SslAuthentication
Dispose();
throw;
}

if (!string.IsNullOrEmpty(sslAuthenticationOptions.TargetHost) && !sslAuthenticationOptions.IsServer)
{
Interop.AppleCrypto.SslSetTargetName(_sslContext, sslAuthenticationOptions.TargetHost);
}

if (sslAuthenticationOptions.CertificateContext == null && sslAuthenticationOptions.CertSelectionDelegate != null)
{
// certificate was not provided but there is user callback. We can break handshake if server asks for certificate
// and we can try to get it based on remote certificate and trusted issuers.
Interop.AppleCrypto.SslBreakOnCertRequested(_sslContext, true);
}

if (sslAuthenticationOptions.IsServer)
{
if (sslAuthenticationOptions.RemoteCertRequired)
{
Interop.AppleCrypto.SslSetAcceptClientCert(_sslContext);
}

if (sslAuthenticationOptions.CertificateContext?.Trust?._sendTrustInHandshake == true)
{
SslCertificateTrust trust = sslAuthenticationOptions.CertificateContext!.Trust!;
X509Certificate2Collection certList = (trust._trustList ?? trust._store!.Certificates);

Debug.Assert(certList != null, "certList != null");
Span<IntPtr> handles = certList.Count <= 256
? stackalloc IntPtr[256]
: new IntPtr[certList.Count];

for (int i = 0; i < certList.Count; i++)
{
handles[i] = certList[i].Handle;
}

Interop.AppleCrypto.SslSetCertificateAuthorities(_sslContext, handles.Slice(0, certList.Count), true);
}
}
}

private static SafeSslHandle CreateSslContext(SafeFreeSslCredentials credential, bool isServer)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@ public static SslCertificateTrust CreateForX509Store(X509Store store, bool sendT
throw new PlatformNotSupportedException(SR.net_ssl_trust_store);
}
#else
if (sendTrustInHandshake)
if (sendTrustInHandshake && !System.OperatingSystem.IsLinux() && !System.OperatingSystem.IsMacOS())
{
// to be removed when implemented.
throw new PlatformNotSupportedException("Not supported yet.");
throw new PlatformNotSupportedException(SR.net_ssl_trust_handshake);
}
#endif
if (!store.IsOpen)
Expand All @@ -41,10 +41,10 @@ public static SslCertificateTrust CreateForX509Store(X509Store store, bool sendT
[UnsupportedOSPlatform("windows")]
public static SslCertificateTrust CreateForX509Collection(X509Certificate2Collection trustList, bool sendTrustInHandshake = false)
{
if (sendTrustInHandshake)
if (sendTrustInHandshake && !System.OperatingSystem.IsLinux() && !System.OperatingSystem.IsMacOS())
{
// to be removed when implemented.
throw new PlatformNotSupportedException("Not supported yet.");
throw new PlatformNotSupportedException(SR.net_ssl_trust_handshake);
}

#if TARGET_WINDOWS
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -747,6 +747,11 @@ private async ValueTask<int> EnsureFullTlsFrameAsync<TIOAdapter>(CancellationTok
return frameSize;
}

if (frameSize < int.MaxValue)
{
_buffer.EnsureAvailableSpace(frameSize - _buffer.EncryptedLength);
}

Comment on lines +750 to +754
Copy link
Member Author

Choose a reason for hiding this comment

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

When supplying store-based SslCertificateTrust, the ServerHello can grow up substantially (well over 8 KB buffer used until now, so we may need to resize the buffer.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this is needed. If the hello is bigger than maximum frame size it should arrive in two (or more) frames. And that should be OK IMHO. If it is not, we should figure out what is happening.
This would be problem already, right? e.g. this is unrelated to sending the list since it is in receiving path.

Copy link
Member Author

@rzikm rzikm Feb 14, 2022

Choose a reason for hiding this comment

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

There is a way for the code to deterministically throw exception without this change in case Root trust cert names are sent in ServerHello (around 120 names):

  • Default buffer size during handshake is 8kB
  • (one of) the incoming frames is larger than 8kB, the size is known before entering the while loop below the added code
  • first read fills in rest of the buffer, part of the frame remains in the inner stream
  • since the frame was known, the call to _buffer.EnsureAvailableSpace is skipped
  • on second iteration, 0 bytes are read since there is no available space to read into
  • throw new IOException(SR.net_io_eof)

Copy link
Member

Choose a reason for hiding this comment

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

Is there chance this will mess up our buffer logic? I briefly look at the EnsureAvailableSpace implementation and it can for example change _activeStart e.g. move the data within the buffer. I'm wondering if we should call EnsureAvailableSpace immediately when we determine frame size...?

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 don't think we can mess things up here. The compacting/moving of the data was happening even before we merged the _internalBuffer and _handshakeBuffer

I'm wondering if we should call EnsureAvailableSpace immediately when we determine frame size...?

That is what is happening here, unless I misunderstand you. The frame size is read in the HaveFullTlsFrame call above and (if the size wasnt' known at that time) at the end of the while loop below.

The alternative place to call EnsureAvailableSpace would be right after we finish processing the previous frame and that does not feel right.

while (_buffer.EncryptedLength < frameSize)
{
// there should be space left to read into
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -248,23 +248,6 @@ private static SecurityStatusPal HandshakeInternal(
{
sslContext = new SafeDeleteSslContext((credential as SafeFreeSslCredentials)!, sslAuthenticationOptions);
context = sslContext;

if (!string.IsNullOrEmpty(sslAuthenticationOptions.TargetHost) && !sslAuthenticationOptions.IsServer)
{
Interop.AppleCrypto.SslSetTargetName(sslContext.SslContext, sslAuthenticationOptions.TargetHost);
}

if (sslAuthenticationOptions.CertificateContext == null && sslAuthenticationOptions.CertSelectionDelegate != null)
{
// certificate was not provided but there is user callback. We can break handshake if server asks for certificate
// and we can try to get it based on remote certificate and trusted issuers.
Interop.AppleCrypto.SslBreakOnCertRequested(sslContext.SslContext, true);
}

if (sslAuthenticationOptions.IsServer && sslAuthenticationOptions.RemoteCertRequired)
{
Interop.AppleCrypto.SslSetAcceptClientCert(sslContext.SslContext);
}
}

if (inputBuffer.Length > 0)
Expand Down
Loading