From ab58f77ded06e2308d31ec70510ff3f06de68bf0 Mon Sep 17 00:00:00 2001 From: Maxim Lipnin Date: Mon, 19 Jul 2021 21:03:08 +0300 Subject: [PATCH 1/5] Add UnmanagedCallersOnly attribute to SafeDeleteSslContext.ReadFromConnection/WriteToConnection methods --- .../Interop.Ssl.cs | 6 +++--- .../Security/Pal.OSX/SafeDeleteSslContext.cs | 19 +++++++------------ 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs b/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs index 26bf0a3fd111eb..dbf1170de29683 100644 --- a/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs +++ b/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs @@ -119,10 +119,10 @@ private static extern int AppleCryptoNative_SslSetTargetName( private static extern int AppleCryptoNative_SslSetAcceptClientCert(SafeSslHandle sslHandle); [DllImport(Interop.Libraries.AppleCryptoNative, EntryPoint = "AppleCryptoNative_SslSetIoCallbacks")] - internal static extern int SslSetIoCallbacks( + internal static extern unsafe int SslSetIoCallbacks( SafeSslHandle sslHandle, - SSLReadFunc readCallback, - SSLWriteFunc writeCallback); + delegate* unmanaged readCallback, + delegate* unmanaged writeCallback); [DllImport(Interop.Libraries.AppleCryptoNative, EntryPoint = "AppleCryptoNative_SslWrite")] internal static extern unsafe PAL_TlsIo SslWrite(SafeSslHandle sslHandle, byte* writeFrom, int count, out int bytesWritten); diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs index 18585eeea01e18..404d37b2cea37d 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs @@ -5,6 +5,7 @@ using System.Diagnostics; using System.Net.Http; using System.Net.Security; +using System.Runtime.InteropServices; using System.Security.Authentication; using System.Security.Cryptography.X509Certificates; using Microsoft.Win32.SafeHandles; @@ -22,8 +23,6 @@ internal sealed class SafeDeleteSslContext : SafeDeleteContext private const int OSStatus_errSSLWouldBlock = -9803; private const int InitialBufferSize = 2048; private SafeSslHandle _sslContext; - private Interop.AppleCrypto.SSLReadFunc _readCallback; - private Interop.AppleCrypto.SSLWriteFunc _writeCallback; private ArrayBuffer _inputBuffer = new ArrayBuffer(InitialBufferSize); private ArrayBuffer _outputBuffer = new ArrayBuffer(InitialBufferSize); @@ -38,18 +37,12 @@ public SafeDeleteSslContext(SafeFreeSslCredentials credential, SslAuthentication { int osStatus; - unsafe - { - _readCallback = ReadFromConnection; - _writeCallback = WriteToConnection; - } - _sslContext = CreateSslContext(credential, sslAuthenticationOptions.IsServer); osStatus = Interop.AppleCrypto.SslSetIoCallbacks( _sslContext, - _readCallback, - _writeCallback); + &ReadFromConnection, + &WriteToConnection); if (osStatus != 0) { @@ -160,7 +153,8 @@ protected override void Dispose(bool disposing) base.Dispose(disposing); } - private unsafe int WriteToConnection(void* connection, byte* data, void** dataLength) + [UnmanagedCallersOnly] + private static unsafe int WriteToConnection(void* connection, byte* data, void** dataLength) { // We don't pool these buffers and we can't because there's a race between their us in the native // read/write callbacks and being disposed when the SafeHandle is disposed. This race is benign currently, @@ -188,7 +182,8 @@ private unsafe int WriteToConnection(void* connection, byte* data, void** dataLe } } - private unsafe int ReadFromConnection(void* connection, byte* data, void** dataLength) + [UnmanagedCallersOnly] + private static unsafe int ReadFromConnection(void* connection, byte* data, void** dataLength) { try { From ce15cce0868e66e3c44fb2bf6f487140f3aa66f2 Mon Sep 17 00:00:00 2001 From: Maxim Lipnin Date: Tue, 20 Jul 2021 11:13:00 +0300 Subject: [PATCH 2/5] Fix the build Co-authored-by: Jan Kotas --- .../System.Security.Cryptography.Native.Apple/Interop.Ssl.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs b/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs index dbf1170de29683..a1fcc130904870 100644 --- a/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs +++ b/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs @@ -121,8 +121,8 @@ private static extern int AppleCryptoNative_SslSetTargetName( [DllImport(Interop.Libraries.AppleCryptoNative, EntryPoint = "AppleCryptoNative_SslSetIoCallbacks")] internal static extern unsafe int SslSetIoCallbacks( SafeSslHandle sslHandle, - delegate* unmanaged readCallback, - delegate* unmanaged writeCallback); + delegate* unmanaged readCallback, + delegate* unmanaged writeCallback); [DllImport(Interop.Libraries.AppleCryptoNative, EntryPoint = "AppleCryptoNative_SslWrite")] internal static extern unsafe PAL_TlsIo SslWrite(SafeSslHandle sslHandle, byte* writeFrom, int count, out int bytesWritten); From 4a4f6c0c305331db3918007c34f6fe2836f03934 Mon Sep 17 00:00:00 2001 From: Maxim Lipnin Date: Tue, 20 Jul 2021 11:41:36 +0300 Subject: [PATCH 3/5] Fix the build --- .../Net/Security/Pal.OSX/SafeDeleteSslContext.cs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs index 404d37b2cea37d..2771d783c7d825 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs @@ -39,10 +39,13 @@ public SafeDeleteSslContext(SafeFreeSslCredentials credential, SslAuthentication _sslContext = CreateSslContext(credential, sslAuthenticationOptions.IsServer); - osStatus = Interop.AppleCrypto.SslSetIoCallbacks( - _sslContext, - &ReadFromConnection, - &WriteToConnection); + unsafe + { + osStatus = Interop.AppleCrypto.SslSetIoCallbacks( + _sslContext, + &ReadFromConnection, + &WriteToConnection); + } if (osStatus != 0) { From 15655e5bb979c7fb252072ccce3cf4dd322518c0 Mon Sep 17 00:00:00 2001 From: Steve Pfister Date: Thu, 12 Aug 2021 00:15:56 -0400 Subject: [PATCH 4/5] Add SslSetConnection interop method to make sure the right SafeDeleteSslContext instance is associated to an ssl session --- .../Interop.Ssl.cs | 9 +++- .../pal_ssl.c | 8 ++++ .../pal_ssl.h | 7 ++++ .../Security/Pal.OSX/SafeDeleteSslContext.cs | 41 +++++++++++++------ 4 files changed, 51 insertions(+), 14 deletions(-) diff --git a/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs b/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs index a1fcc130904870..f38e94761d6583 100644 --- a/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs +++ b/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs @@ -60,6 +60,11 @@ internal enum PAL_TlsIo [DllImport(Interop.Libraries.AppleCryptoNative, EntryPoint = "AppleCryptoNative_SslCreateContext")] internal static extern System.Net.SafeSslHandle SslCreateContext(int isServer); + [DllImport(Interop.Libraries.AppleCryptoNative, EntryPoint = "AppleCryptoNative_SslSetConnection")] + internal static extern int SslSetConnection( + SafeSslHandle sslHandle, + IntPtr sslConnection); + [DllImport(Interop.Libraries.AppleCryptoNative)] private static extern int AppleCryptoNative_SslSetMinProtocolVersion( SafeSslHandle sslHandle, @@ -121,8 +126,8 @@ private static extern int AppleCryptoNative_SslSetTargetName( [DllImport(Interop.Libraries.AppleCryptoNative, EntryPoint = "AppleCryptoNative_SslSetIoCallbacks")] internal static extern unsafe int SslSetIoCallbacks( SafeSslHandle sslHandle, - delegate* unmanaged readCallback, - delegate* unmanaged writeCallback); + delegate* unmanaged readCallback, + delegate* unmanaged writeCallback); [DllImport(Interop.Libraries.AppleCryptoNative, EntryPoint = "AppleCryptoNative_SslWrite")] internal static extern unsafe PAL_TlsIo SslWrite(SafeSslHandle sslHandle, byte* writeFrom, int count, out int bytesWritten); diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Apple/pal_ssl.c b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Apple/pal_ssl.c index 2b5d3a3d06e52e..2d66847c1731b3 100644 --- a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Apple/pal_ssl.c +++ b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Apple/pal_ssl.c @@ -23,6 +23,14 @@ SSLContextRef AppleCryptoNative_SslCreateContext(int32_t isServer) #pragma clang diagnostic pop } +int32_t AppleCryptoNative_SslSetConnection(SSLContextRef sslContext, SSLConnectionRef sslConnection) +{ +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated-declarations" + return SSLSetConnection(sslContext, sslConnection); +#pragma clang diagnostic pop +} + int32_t AppleCryptoNative_SslSetAcceptClientCert(SSLContextRef sslContext) { #pragma clang diagnostic push diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Apple/pal_ssl.h b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Apple/pal_ssl.h index 8c3b61d530f833..8a992bb9a4bc95 100644 --- a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Apple/pal_ssl.h +++ b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Apple/pal_ssl.h @@ -35,6 +35,13 @@ Returns NULL if an invalid boolean is given for isServer, an SSLContextRef other */ PALEXPORT SSLContextRef AppleCryptoNative_SslCreateContext(int32_t isServer); +/* +Data that is used to uniquely identify an SSL session. + +Returns the result of SSLSetConnection +*/ +PALEXPORT int32_t AppleCryptoNative_SslSetConnection(SSLContextRef sslContext, SSLConnectionRef sslConnection); + /* Indicate that an SSL Context (in server mode) should allow a client to present a mutual auth cert. diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs index 2771d783c7d825..3cd2b92fb8cf03 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs @@ -39,7 +39,11 @@ public SafeDeleteSslContext(SafeFreeSslCredentials credential, SslAuthentication _sslContext = CreateSslContext(credential, sslAuthenticationOptions.IsServer); - unsafe + // Make sure the class instance is associated to the session and is provided + // in the Read/Write callback connection parameter + SslSetConnection(_sslContext); + + unsafe { osStatus = Interop.AppleCrypto.SslSetIoCallbacks( _sslContext, @@ -138,6 +142,13 @@ private static SafeSslHandle CreateSslContext(SafeFreeSslCredentials credential, return sslContext; } + private void SslSetConnection(SafeSslHandle sslContext) + { + GCHandle handle = GCHandle.Alloc(this, GCHandleType.Weak); + + Interop.AppleCrypto.SslSetConnection(sslContext, GCHandle.ToIntPtr(handle)); + } + public override bool IsInvalid => _sslContext?.IsInvalid ?? true; protected override void Dispose(bool disposing) @@ -157,8 +168,11 @@ protected override void Dispose(bool disposing) } [UnmanagedCallersOnly] - private static unsafe int WriteToConnection(void* connection, byte* data, void** dataLength) + private static unsafe int WriteToConnection(IntPtr connection, byte* data, void** dataLength) { + SafeDeleteSslContext? context = (SafeDeleteSslContext?)GCHandle.FromIntPtr(connection).Target; + Debug.Assert(context != null); + // We don't pool these buffers and we can't because there's a race between their us in the native // read/write callbacks and being disposed when the SafeHandle is disposed. This race is benign currently, // but if we were to pool the buffers we would have a potential use-after-free issue. @@ -170,9 +184,9 @@ private static unsafe int WriteToConnection(void* connection, byte* data, void** int toWrite = (int)length; var inputBuffer = new ReadOnlySpan(data, toWrite); - _outputBuffer.EnsureAvailableSpace(toWrite); - inputBuffer.CopyTo(_outputBuffer.AvailableSpan); - _outputBuffer.Commit(toWrite); + context._outputBuffer.EnsureAvailableSpace(toWrite); + inputBuffer.CopyTo(context._outputBuffer.AvailableSpan); + context._outputBuffer.Commit(toWrite); // Since we can enqueue everything, no need to re-assign *dataLength. return OSStatus_noErr; @@ -180,14 +194,17 @@ private static unsafe int WriteToConnection(void* connection, byte* data, void** catch (Exception e) { if (NetEventSource.Log.IsEnabled()) - NetEventSource.Error(this, $"WritingToConnection failed: {e.Message}"); + NetEventSource.Error(context, $"WritingToConnection failed: {e.Message}"); return OSStatus_writErr; } } [UnmanagedCallersOnly] - private static unsafe int ReadFromConnection(void* connection, byte* data, void** dataLength) + private static unsafe int ReadFromConnection(IntPtr connection, byte* data, void** dataLength) { + SafeDeleteSslContext? context = (SafeDeleteSslContext?)GCHandle.FromIntPtr(connection).Target; + Debug.Assert(context != null); + try { ulong toRead = (ulong)*dataLength; @@ -199,16 +216,16 @@ private static unsafe int ReadFromConnection(void* connection, byte* data, void* uint transferred = 0; - if (_inputBuffer.ActiveLength == 0) + if (context._inputBuffer.ActiveLength == 0) { *dataLength = (void*)0; return OSStatus_errSSLWouldBlock; } - int limit = Math.Min((int)toRead, _inputBuffer.ActiveLength); + int limit = Math.Min((int)toRead, context._inputBuffer.ActiveLength); - _inputBuffer.ActiveSpan.Slice(0, limit).CopyTo(new Span(data, limit)); - _inputBuffer.Discard(limit); + context._inputBuffer.ActiveSpan.Slice(0, limit).CopyTo(new Span(data, limit)); + context._inputBuffer.Discard(limit); transferred = (uint)limit; *dataLength = (void*)transferred; @@ -217,7 +234,7 @@ private static unsafe int ReadFromConnection(void* connection, byte* data, void* catch (Exception e) { if (NetEventSource.Log.IsEnabled()) - NetEventSource.Error(this, $"ReadFromConnectionfailed: {e.Message}"); + NetEventSource.Error(context, $"ReadFromConnectionfailed: {e.Message}"); return OSStatus_readErr; } } From 25a1fd438b7a817bfb0d3e2684050c90f58e4aaf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20K=C3=B6plinger?= Date: Thu, 12 Aug 2021 16:03:36 +0200 Subject: [PATCH 5/5] Update entrypoints.c with new DllImport --- .../Unix/System.Security.Cryptography.Native.Apple/entrypoints.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Apple/entrypoints.c b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Apple/entrypoints.c index 5162b7a2193561..db75e90b3a4365 100644 --- a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Apple/entrypoints.c +++ b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Apple/entrypoints.c @@ -78,6 +78,7 @@ static const Entry s_cryptoAppleNative[] = DllImportEntry(AppleCryptoNative_SecKeyCopyExternalRepresentation) DllImportEntry(AppleCryptoNative_SecKeyCopyPublicKey) DllImportEntry(AppleCryptoNative_SslCreateContext) + DllImportEntry(AppleCryptoNative_SslSetConnection) DllImportEntry(AppleCryptoNative_SslSetAcceptClientCert) DllImportEntry(AppleCryptoNative_SslSetMinProtocolVersion) DllImportEntry(AppleCryptoNative_SslSetMaxProtocolVersion)