From 8f8e4c24beb476db6271d7371600e0d84ec65b66 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Tue, 12 Dec 2023 09:40:12 +0100 Subject: [PATCH 1/4] Fix calculation of channel bindings hash in managed NTLM implementation --- .../NegotiateAuthenticationPal.ManagedNtlm.cs | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs b/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs index 95be2c7fc8984e..5efa48293046a1 100644 --- a/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs +++ b/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs @@ -424,10 +424,23 @@ private unsafe void WriteChannelBindingHash(Span hashBuffer) { if (_channelBinding != null) { - IntPtr cbtData = _channelBinding.DangerousGetHandle(); - int cbtDataSize = _channelBinding.Size; - int written = MD5.HashData(new Span((void*)cbtData, cbtDataSize), hashBuffer); - Debug.Assert(written == MD5.HashSizeInBytes); + int secChannelBindingsSize = Marshal.SizeOf(); + IntPtr cbtData = (nint)_channelBinding.DangerousGetHandle() + secChannelBindingsSize; + int cbtDataSize = _channelBinding.Size - secChannelBindingsSize; + + // Channel bindings are calculated according to RFC 4121, section 4.1.1.2, + // so we need to include zeroed initiator fields and length prefix for the + // application data. + Span prefix = stackalloc byte[sizeof(uint) * 5]; + prefix.Clear(); + BinaryPrimitives.WriteInt32LittleEndian(prefix.Slice(sizeof(uint) * 4), cbtDataSize); + using (var md5 = IncrementalHash.CreateHash(HashAlgorithmName.MD5)) + { + md5.AppendData(prefix); + md5.AppendData(new Span((void*)cbtData, cbtDataSize)); + int written = md5.GetHashAndReset(hashBuffer); + Debug.Assert(written == MD5.HashSizeInBytes); + } } else { From c21a5d7e003f8fb02ee04b41bdf0c1e4592c3f39 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Tue, 12 Dec 2023 16:26:42 +0100 Subject: [PATCH 2/4] Use sizeof(SecChannelBindings) consistently --- .../System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs | 6 +++--- .../src/System/Net/NegotiateAuthenticationPal.Unix.cs | 2 +- .../Net/Security/Pal.Managed/SafeChannelBindingHandle.cs | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs b/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs index 5efa48293046a1..147dd35e4e194c 100644 --- a/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs +++ b/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs @@ -424,9 +424,9 @@ private unsafe void WriteChannelBindingHash(Span hashBuffer) { if (_channelBinding != null) { - int secChannelBindingsSize = Marshal.SizeOf(); - IntPtr cbtData = (nint)_channelBinding.DangerousGetHandle() + secChannelBindingsSize; - int cbtDataSize = _channelBinding.Size - secChannelBindingsSize; + int appDataOffset = sizeof(SecChannelBindings); + IntPtr cbtData = (nint)_channelBinding.DangerousGetHandle() + appDataOffset; + int cbtDataSize = _channelBinding.Size - appDataOffset; // Channel bindings are calculated according to RFC 4121, section 4.1.1.2, // so we need to include zeroed initiator fields and length prefix for the diff --git a/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.Unix.cs b/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.Unix.cs index d3b39321b12bf2..d7696ed3eb7fc1 100644 --- a/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.Unix.cs +++ b/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.Unix.cs @@ -587,7 +587,7 @@ private NegotiateAuthenticationStatusCode InitializeSecurityContext( { // If a TLS channel binding token (cbt) is available then get the pointer // to the application specific data. - int appDataOffset = Marshal.SizeOf(); + int appDataOffset = sizeof(SecChannelBindings); Debug.Assert(appDataOffset < channelBinding.Size); IntPtr cbtAppData = channelBinding.DangerousGetHandle() + appDataOffset; int cbtAppDataSize = channelBinding.Size - appDataOffset; diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.Managed/SafeChannelBindingHandle.cs b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.Managed/SafeChannelBindingHandle.cs index 35daf739b88af8..0a2cf623e934c4 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.Managed/SafeChannelBindingHandle.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.Managed/SafeChannelBindingHandle.cs @@ -11,7 +11,7 @@ namespace System.Net.Security internal sealed class SafeChannelBindingHandle : ChannelBinding { private const int CertHashMaxSize = 128; - private static readonly int s_secChannelBindingSize = Marshal.SizeOf(); + private static readonly int s_secChannelBindingSize = sizeof(SecChannelBindings); private readonly int _cbtPrefixByteArraySize; internal int Length { get; private set; } From e78d14c36ac6114d4478cc39c136e4290f1108ef Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Tue, 12 Dec 2023 17:27:58 +0100 Subject: [PATCH 3/4] Use Unsafe.SizeOf(). --- .../Net/NegotiateAuthenticationPal.ManagedNtlm.cs | 3 ++- .../src/System/Net/NegotiateAuthenticationPal.Unix.cs | 3 ++- .../Security/Pal.Managed/SafeChannelBindingHandle.cs | 11 ++++++----- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs b/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs index 147dd35e4e194c..9e31364c7f20b7 100644 --- a/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs +++ b/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs @@ -8,6 +8,7 @@ using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Net.Security; +using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Runtime.Versioning; using System.Security.Authentication.ExtendedProtection; @@ -424,7 +425,7 @@ private unsafe void WriteChannelBindingHash(Span hashBuffer) { if (_channelBinding != null) { - int appDataOffset = sizeof(SecChannelBindings); + int appDataOffset = Unsafe.SizeOf(); IntPtr cbtData = (nint)_channelBinding.DangerousGetHandle() + appDataOffset; int cbtDataSize = _channelBinding.Size - appDataOffset; diff --git a/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.Unix.cs b/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.Unix.cs index d7696ed3eb7fc1..027c541be70ca0 100644 --- a/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.Unix.cs +++ b/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.Unix.cs @@ -8,6 +8,7 @@ using System.Diagnostics.CodeAnalysis; using System.Globalization; using System.Net.Security; +using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Security; using System.Security.Authentication.ExtendedProtection; @@ -587,7 +588,7 @@ private NegotiateAuthenticationStatusCode InitializeSecurityContext( { // If a TLS channel binding token (cbt) is available then get the pointer // to the application specific data. - int appDataOffset = sizeof(SecChannelBindings); + int appDataOffset = Unsafe.SizeOf(); Debug.Assert(appDataOffset < channelBinding.Size); IntPtr cbtAppData = channelBinding.DangerousGetHandle() + appDataOffset; int cbtAppDataSize = channelBinding.Size - appDataOffset; diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.Managed/SafeChannelBindingHandle.cs b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.Managed/SafeChannelBindingHandle.cs index 0a2cf623e934c4..c3c6f1c7f2b7db 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.Managed/SafeChannelBindingHandle.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.Managed/SafeChannelBindingHandle.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Diagnostics; +using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Security.Authentication.ExtendedProtection; using System.Text; @@ -11,7 +12,7 @@ namespace System.Net.Security internal sealed class SafeChannelBindingHandle : ChannelBinding { private const int CertHashMaxSize = 128; - private static readonly int s_secChannelBindingSize = sizeof(SecChannelBindings); + private static int SecChannelBindingSize => Unsafe.SizeOf(); private readonly int _cbtPrefixByteArraySize; internal int Length { get; private set; } @@ -36,8 +37,8 @@ internal unsafe SafeChannelBindingHandle(ChannelBindingKind kind) "tls-unique:"u8; _cbtPrefixByteArraySize = cbtPrefix.Length; - handle = Marshal.AllocHGlobal(s_secChannelBindingSize + _cbtPrefixByteArraySize + CertHashMaxSize); - IntPtr cbtPrefixPtr = handle + s_secChannelBindingSize; + handle = Marshal.AllocHGlobal(SecChannelBindingSize + _cbtPrefixByteArraySize + CertHashMaxSize); + IntPtr cbtPrefixPtr = handle + SecChannelBindingSize; cbtPrefix.CopyTo(new Span((byte*)cbtPrefixPtr, cbtPrefix.Length)); CertHashPtr = cbtPrefixPtr + _cbtPrefixByteArraySize; Length = CertHashMaxSize; @@ -46,12 +47,12 @@ internal unsafe SafeChannelBindingHandle(ChannelBindingKind kind) internal void SetCertHashLength(int certHashLength) { int cbtLength = _cbtPrefixByteArraySize + certHashLength; - Length = s_secChannelBindingSize + cbtLength; + Length = SecChannelBindingSize + cbtLength; SecChannelBindings channelBindings = new SecChannelBindings() { ApplicationDataLength = cbtLength, - ApplicationDataOffset = s_secChannelBindingSize + ApplicationDataOffset = SecChannelBindingSize }; Marshal.StructureToPtr(channelBindings, handle, true); } From ecfd015510c068b4474e65fcb12edbcc17fe3431 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Tue, 12 Dec 2023 19:12:42 +0100 Subject: [PATCH 4/4] Use unsafe + sizeof --- .../src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs | 3 +-- .../src/System/Net/NegotiateAuthenticationPal.Unix.cs | 5 ++--- .../Net/Security/Pal.Managed/SafeChannelBindingHandle.cs | 3 +-- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs b/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs index 9e31364c7f20b7..147dd35e4e194c 100644 --- a/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs +++ b/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs @@ -8,7 +8,6 @@ using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Net.Security; -using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Runtime.Versioning; using System.Security.Authentication.ExtendedProtection; @@ -425,7 +424,7 @@ private unsafe void WriteChannelBindingHash(Span hashBuffer) { if (_channelBinding != null) { - int appDataOffset = Unsafe.SizeOf(); + int appDataOffset = sizeof(SecChannelBindings); IntPtr cbtData = (nint)_channelBinding.DangerousGetHandle() + appDataOffset; int cbtDataSize = _channelBinding.Size - appDataOffset; diff --git a/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.Unix.cs b/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.Unix.cs index 027c541be70ca0..e1735a65a6f97e 100644 --- a/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.Unix.cs +++ b/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.Unix.cs @@ -8,7 +8,6 @@ using System.Diagnostics.CodeAnalysis; using System.Globalization; using System.Net.Security; -using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Security; using System.Security.Authentication.ExtendedProtection; @@ -544,7 +543,7 @@ Interop.NetSecurityNative.Status status } } - private NegotiateAuthenticationStatusCode InitializeSecurityContext( + private unsafe NegotiateAuthenticationStatusCode InitializeSecurityContext( ref SafeGssCredHandle credentialsHandle, ref SafeGssContextHandle? contextHandle, ref SafeGssNameHandle? targetNameHandle, @@ -588,7 +587,7 @@ private NegotiateAuthenticationStatusCode InitializeSecurityContext( { // If a TLS channel binding token (cbt) is available then get the pointer // to the application specific data. - int appDataOffset = Unsafe.SizeOf(); + int appDataOffset = sizeof(SecChannelBindings); Debug.Assert(appDataOffset < channelBinding.Size); IntPtr cbtAppData = channelBinding.DangerousGetHandle() + appDataOffset; int cbtAppDataSize = channelBinding.Size - appDataOffset; diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.Managed/SafeChannelBindingHandle.cs b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.Managed/SafeChannelBindingHandle.cs index c3c6f1c7f2b7db..0b8699fd0eaa04 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.Managed/SafeChannelBindingHandle.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.Managed/SafeChannelBindingHandle.cs @@ -2,7 +2,6 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Diagnostics; -using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Security.Authentication.ExtendedProtection; using System.Text; @@ -12,7 +11,7 @@ namespace System.Net.Security internal sealed class SafeChannelBindingHandle : ChannelBinding { private const int CertHashMaxSize = 128; - private static int SecChannelBindingSize => Unsafe.SizeOf(); + private static unsafe int SecChannelBindingSize => sizeof(SecChannelBindings); private readonly int _cbtPrefixByteArraySize; internal int Length { get; private set; }