From e66252654ec329d17c38f5b61af5907651756cbd Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Thu, 24 Apr 2025 11:06:38 -0400 Subject: [PATCH 1/3] Use a custom ReadOnlySpan marshaller to prevent null pointers when marshalling. --- .../Interop.BCryptGenerateSymmetricKey.cs | 9 +- .../NotNullReadOnlySpanMarshaller.cs | 108 ++++++++++++++++++ ...SP800108HmacCounterKdfImplementationCng.cs | 6 +- ...SP800108HmacCounterKdfImplementationCng.cs | 32 +----- .../src/System.Security.Cryptography.csproj | 2 + .../Pbkdf2Implementation.Windows.cs | 52 +++------ ...SP800108HmacCounterKdfImplementationCng.cs | 19 +-- .../tests/Rfc2898OneShotTests.cs | 17 +++ 8 files changed, 160 insertions(+), 85 deletions(-) create mode 100644 src/libraries/Common/src/System/Runtime/InteropServices/NotNullReadOnlySpanMarshaller.cs diff --git a/src/libraries/Common/src/Interop/Windows/BCrypt/Interop.BCryptGenerateSymmetricKey.cs b/src/libraries/Common/src/Interop/Windows/BCrypt/Interop.BCryptGenerateSymmetricKey.cs index 14b2a9fbe9a714..6b48fca0fad4b4 100644 --- a/src/libraries/Common/src/Interop/Windows/BCrypt/Interop.BCryptGenerateSymmetricKey.cs +++ b/src/libraries/Common/src/Interop/Windows/BCrypt/Interop.BCryptGenerateSymmetricKey.cs @@ -3,6 +3,7 @@ using System; using System.Runtime.InteropServices; +using System.Runtime.InteropServices.Marshalling; using Microsoft.Win32.SafeHandles; internal static partial class Interop @@ -10,22 +11,22 @@ internal static partial class Interop internal static partial class BCrypt { [LibraryImport(Libraries.BCrypt)] - internal static unsafe partial NTSTATUS BCryptGenerateSymmetricKey( + internal static partial NTSTATUS BCryptGenerateSymmetricKey( SafeBCryptAlgorithmHandle hAlgorithm, out SafeBCryptKeyHandle phKey, IntPtr pbKeyObject, int cbKeyObject, - byte* pbSecret, + [MarshalUsing(typeof(NotNullReadOnlySpanMarshaller<,>))] ReadOnlySpan pbSecret, int cbSecret, uint dwFlags); [LibraryImport(Libraries.BCrypt)] - internal static unsafe partial NTSTATUS BCryptGenerateSymmetricKey( + internal static partial NTSTATUS BCryptGenerateSymmetricKey( nuint hAlgorithm, out SafeBCryptKeyHandle phKey, IntPtr pbKeyObject, int cbKeyObject, - byte* pbSecret, + [MarshalUsing(typeof(NotNullReadOnlySpanMarshaller<,>))] ReadOnlySpan pbSecret, int cbSecret, uint dwFlags); } diff --git a/src/libraries/Common/src/System/Runtime/InteropServices/NotNullReadOnlySpanMarshaller.cs b/src/libraries/Common/src/System/Runtime/InteropServices/NotNullReadOnlySpanMarshaller.cs new file mode 100644 index 00000000000000..76a252c1aea94f --- /dev/null +++ b/src/libraries/Common/src/System/Runtime/InteropServices/NotNullReadOnlySpanMarshaller.cs @@ -0,0 +1,108 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Collections.Generic; +using System.Runtime.CompilerServices; +using System.Text; + +namespace System.Runtime.InteropServices.Marshalling +{ + [CustomMarshaller(typeof(ReadOnlySpan<>), MarshalMode.ManagedToUnmanagedIn, typeof(NotNullReadOnlySpanMarshaller<,>.ManagedToUnmanagedIn))] + [CustomMarshaller(typeof(ReadOnlySpan<>), MarshalMode.ManagedToUnmanagedOut, typeof(NotNullReadOnlySpanMarshaller<,>.ManagedToUnmanagedOut))] + [ContiguousCollectionMarshaller] + internal static unsafe class NotNullReadOnlySpanMarshaller + where TUnmanagedElement : unmanaged + { + public ref struct ManagedToUnmanagedIn + { + /// + /// Gets the size of the caller-allocated buffer to allocate. + /// + // We'll keep the buffer size at a maximum of 512 bytes to avoid overflowing the stack. + public static int BufferSize => 0x200 / sizeof(TUnmanagedElement); + + private ReadOnlySpan _managedArray; + private TUnmanagedElement* _allocatedMemory; + private Span _span; + + public void FromManaged(ReadOnlySpan managed, Span buffer) + { + _managedArray = managed; + + if (managed.Length <= buffer.Length) + { + _span = buffer[0..managed.Length]; + } + else + { + int bufferSize = checked(managed.Length * sizeof(TUnmanagedElement)); + _allocatedMemory = (TUnmanagedElement*)NativeMemory.Alloc((nuint)bufferSize); + _span = new Span(_allocatedMemory, managed.Length); + } + } + + public ReadOnlySpan GetManagedValuesSource() => _managedArray; + + public Span GetUnmanagedValuesDestination() => _span; + + public ref TUnmanagedElement GetPinnableReference() => ref MemoryMarshal.GetReference(_span); + + public TUnmanagedElement* ToUnmanaged() + { + // Unsafe.AsPointer is safe since buffer must be pinned + return (TUnmanagedElement*)Unsafe.AsPointer(ref GetPinnableReference()); + } + + public void Free() + { + NativeMemory.Free(_allocatedMemory); + } + + public static ref T GetPinnableReference(ReadOnlySpan managed) + { + if (Unsafe.IsNullRef(ref MemoryMarshal.GetReference(managed)) && managed.IsEmpty) + { + T[] notNull = new T[1]; + return ref MemoryMarshal.GetArrayDataReference(notNull); + } + else + { + return ref MemoryMarshal.GetReference(managed); + } + } + } + + public struct ManagedToUnmanagedOut + { + private TUnmanagedElement* _unmanagedArray; + private T[]? _managedValues; + + public void FromUnmanaged(TUnmanagedElement* unmanaged) + { + _unmanagedArray = unmanaged; + } + + public ReadOnlySpan ToManaged() + { + return new ReadOnlySpan(_managedValues!); + } + + public ReadOnlySpan GetUnmanagedValuesSource(int numElements) + { + return new ReadOnlySpan(_unmanagedArray, numElements); + } + + public Span GetManagedValuesDestination(int numElements) + { + _managedValues = new T[numElements]; + return _managedValues; + } + + public void Free() + { + Marshal.FreeCoTaskMem((IntPtr)_unmanagedArray); + } + } + } +} diff --git a/src/libraries/Common/src/System/Security/Cryptography/SP800108HmacCounterKdfImplementationCng.cs b/src/libraries/Common/src/System/Security/Cryptography/SP800108HmacCounterKdfImplementationCng.cs index e02a47a072ada2..fa8706ce6cb05b 100644 --- a/src/libraries/Common/src/System/Security/Cryptography/SP800108HmacCounterKdfImplementationCng.cs +++ b/src/libraries/Common/src/System/Security/Cryptography/SP800108HmacCounterKdfImplementationCng.cs @@ -128,7 +128,7 @@ internal static void DeriveBytesOneShot( } } - private static unsafe SafeBCryptKeyHandle CreateSymmetricKey(byte* symmetricKey, int symmetricKeyLength) + private static SafeBCryptKeyHandle CreateSymmetricKey(ReadOnlySpan symmetricKey) { NTSTATUS generateKeyStatus; SafeBCryptKeyHandle keyHandle; @@ -141,7 +141,7 @@ private static unsafe SafeBCryptKeyHandle CreateSymmetricKey(byte* symmetricKey, pbKeyObject: IntPtr.Zero, cbKeyObject: 0, symmetricKey, - symmetricKeyLength, + symmetricKey.Length, dwFlags: 0); } else @@ -152,7 +152,7 @@ private static unsafe SafeBCryptKeyHandle CreateSymmetricKey(byte* symmetricKey, pbKeyObject: IntPtr.Zero, cbKeyObject: 0, symmetricKey, - symmetricKeyLength, + symmetricKey.Length, dwFlags: 0); } diff --git a/src/libraries/Microsoft.Bcl.Cryptography/src/System/Security/Cryptography/SP800108HmacCounterKdfImplementationCng.cs b/src/libraries/Microsoft.Bcl.Cryptography/src/System/Security/Cryptography/SP800108HmacCounterKdfImplementationCng.cs index 6bbd2f7f1b7fde..f6ff9434a62869 100644 --- a/src/libraries/Microsoft.Bcl.Cryptography/src/System/Security/Cryptography/SP800108HmacCounterKdfImplementationCng.cs +++ b/src/libraries/Microsoft.Bcl.Cryptography/src/System/Security/Cryptography/SP800108HmacCounterKdfImplementationCng.cs @@ -13,7 +13,6 @@ internal unsafe SP800108HmacCounterKdfImplementationCng(ReadOnlySpan key, scoped ReadOnlySpan symmetricKeyMaterial; scoped Span clearSpan = default; - int symmetricKeyMaterialLength; int hashAlgorithmBlockSize = GetHashBlockSize(hashAlgorithm.Name); if (key.Length > hashAlgorithmBlockSize) @@ -28,26 +27,15 @@ internal unsafe SP800108HmacCounterKdfImplementationCng(ReadOnlySpan key, } symmetricKeyMaterial = clearSpan; - symmetricKeyMaterialLength = symmetricKeyMaterial.Length; - } - else if (!key.IsEmpty) - { - symmetricKeyMaterial = key; - symmetricKeyMaterialLength = key.Length; } else { - // CNG requires a non-null pointer even when the length is zero. - symmetricKeyMaterial = [0]; - symmetricKeyMaterialLength = 0; + symmetricKeyMaterial = key; } try { - fixed (byte* pSymmetricKeyMaterial = symmetricKeyMaterial) - { - _keyHandle = CreateSymmetricKey(pSymmetricKeyMaterial, symmetricKeyMaterialLength); - } + _keyHandle = CreateSymmetricKey(symmetricKeyMaterial); } finally { @@ -65,33 +53,21 @@ internal unsafe SP800108HmacCounterKdfImplementationCng(byte[] key, HashAlgorith scoped ReadOnlySpan symmetricKeyMaterial; scoped Span clearSpan = default; - int symmetricKeyMaterialLength; int hashAlgorithmBlockSize = GetHashBlockSize(hashAlgorithm.Name); if (key.Length > hashAlgorithmBlockSize) { clearSpan = HashOneShot(hashAlgorithm, key); symmetricKeyMaterial = clearSpan; - symmetricKeyMaterialLength = symmetricKeyMaterial.Length; - } - else if (key.Length > 0) - { - symmetricKeyMaterial = key; - symmetricKeyMaterialLength = key.Length; } else { - // CNG requires a non-null pointer even when the length is zero. - symmetricKeyMaterial = [0]; - symmetricKeyMaterialLength = 0; + symmetricKeyMaterial = key; } try { - fixed (byte* pSymmetricKeyMaterial = symmetricKeyMaterial) - { - _keyHandle = CreateSymmetricKey(pSymmetricKeyMaterial, symmetricKeyMaterialLength); - } + _keyHandle = CreateSymmetricKey(symmetricKeyMaterial); } finally { diff --git a/src/libraries/System.Security.Cryptography/src/System.Security.Cryptography.csproj b/src/libraries/System.Security.Cryptography/src/System.Security.Cryptography.csproj index 431ad69cefed7e..ab93790a3df4cc 100644 --- a/src/libraries/System.Security.Cryptography/src/System.Security.Cryptography.csproj +++ b/src/libraries/System.Security.Cryptography/src/System.Security.Cryptography.csproj @@ -1514,6 +1514,8 @@ + clearSpan; scoped ReadOnlySpan symmetricKeyMaterial; - int symmetricKeyMaterialLength; - if (password.IsEmpty) - { - // CNG won't accept a null pointer for the password. - symmetricKeyMaterial = [0]; - symmetricKeyMaterialLength = 0; - clearSpan = default; - } - else if (password.Length <= hashBlockSizeBytes) + if (password.Length <= hashBlockSizeBytes) { // Password is small enough to use as-is. symmetricKeyMaterial = password; - symmetricKeyMaterialLength = password.Length; clearSpan = default; } else @@ -108,26 +99,20 @@ private static unsafe void FillKeyDerivation( clearSpan = hashBuffer.Slice(0, hashBufferSize); symmetricKeyMaterial = clearSpan; - symmetricKeyMaterialLength = hashBufferSize; } - Debug.Assert(symmetricKeyMaterial.Length > 0); - NTSTATUS generateKeyStatus; if (Interop.BCrypt.PseudoHandlesSupported) { - fixed (byte* pSymmetricKeyMaterial = symmetricKeyMaterial) - { - generateKeyStatus = Interop.BCrypt.BCryptGenerateSymmetricKey( - (nuint)BCryptAlgPseudoHandle.BCRYPT_PBKDF2_ALG_HANDLE, - out keyHandle, - pbKeyObject: IntPtr.Zero, - cbKeyObject: 0, - pSymmetricKeyMaterial, - symmetricKeyMaterialLength, - dwFlags: 0); - } + generateKeyStatus = Interop.BCrypt.BCryptGenerateSymmetricKey( + (nuint)BCryptAlgPseudoHandle.BCRYPT_PBKDF2_ALG_HANDLE, + out keyHandle, + pbKeyObject: IntPtr.Zero, + cbKeyObject: 0, + symmetricKeyMaterial, + symmetricKeyMaterial.Length, + dwFlags: 0); } else { @@ -151,17 +136,14 @@ private static unsafe void FillKeyDerivation( Interlocked.CompareExchange(ref s_pbkdf2AlgorithmHandle, pbkdf2AlgorithmHandle, null); } - fixed (byte* pSymmetricKeyMaterial = symmetricKeyMaterial) - { - generateKeyStatus = Interop.BCrypt.BCryptGenerateSymmetricKey( - s_pbkdf2AlgorithmHandle, - out keyHandle, - pbKeyObject: IntPtr.Zero, - cbKeyObject: 0, - pSymmetricKeyMaterial, - symmetricKeyMaterialLength, - dwFlags: 0); - } + generateKeyStatus = Interop.BCrypt.BCryptGenerateSymmetricKey( + s_pbkdf2AlgorithmHandle, + out keyHandle, + pbKeyObject: IntPtr.Zero, + cbKeyObject: 0, + symmetricKeyMaterial, + symmetricKeyMaterial.Length, + dwFlags: 0); } CryptographicOperations.ZeroMemory(clearSpan); diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/SP800108HmacCounterKdfImplementationCng.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/SP800108HmacCounterKdfImplementationCng.cs index 84c624120eea42..7bcd1a83c01237 100644 --- a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/SP800108HmacCounterKdfImplementationCng.cs +++ b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/SP800108HmacCounterKdfImplementationCng.cs @@ -7,40 +7,29 @@ namespace System.Security.Cryptography { internal sealed partial class SP800108HmacCounterKdfImplementationCng { - internal unsafe SP800108HmacCounterKdfImplementationCng(ReadOnlySpan key, HashAlgorithmName hashAlgorithm) + internal SP800108HmacCounterKdfImplementationCng(ReadOnlySpan key, HashAlgorithmName hashAlgorithm) { Debug.Assert(hashAlgorithm.Name is not null); scoped ReadOnlySpan symmetricKeyMaterial; scoped Span clearSpan = default; - int symmetricKeyMaterialLength; int hashAlgorithmBlockSize = GetHashBlockSize(hashAlgorithm.Name); if (key.Length > hashAlgorithmBlockSize) { Span buffer = stackalloc byte[512 / 8]; // Largest supported digest is SHA512. - symmetricKeyMaterialLength = HashOneShot(hashAlgorithm, key, buffer); + int symmetricKeyMaterialLength = HashOneShot(hashAlgorithm, key, buffer); clearSpan = buffer.Slice(0, symmetricKeyMaterialLength); symmetricKeyMaterial = clearSpan; } - else if (!key.IsEmpty) - { - symmetricKeyMaterial = key; - symmetricKeyMaterialLength = key.Length; - } else { - // CNG requires a non-null pointer even when the length is zero. - symmetricKeyMaterial = [0]; - symmetricKeyMaterialLength = 0; + symmetricKeyMaterial = key; } try { - fixed (byte* pSymmetricKeyMaterial = symmetricKeyMaterial) - { - _keyHandle = CreateSymmetricKey(pSymmetricKeyMaterial, symmetricKeyMaterialLength); - } + _keyHandle = CreateSymmetricKey(symmetricKeyMaterial); } finally { diff --git a/src/libraries/System.Security.Cryptography/tests/Rfc2898OneShotTests.cs b/src/libraries/System.Security.Cryptography/tests/Rfc2898OneShotTests.cs index 3a54623527d8d6..80a96254a47179 100644 --- a/src/libraries/System.Security.Cryptography/tests/Rfc2898OneShotTests.cs +++ b/src/libraries/System.Security.Cryptography/tests/Rfc2898OneShotTests.cs @@ -208,6 +208,23 @@ public static void Pbkdf2_Password_Salt_Overlapping_Backward() Assert.Equal(expected, output.ToArray()); } + [Fact] + public static void Pbkdf2_Password_EmptySpans() + { + ReadOnlySpan expected = + [ + 0xF7, 0xCE, 0x0B, 0x65, 0x3D, 0x2D, 0x72, 0xA4, 0x10, 0x8C, 0xF5, 0xAB, 0xE9, 0x12, 0xFF, 0xDD + ]; + Span output = stackalloc byte[16]; + Rfc2898DeriveBytes.Pbkdf2( + ReadOnlySpan.Empty, + ReadOnlySpan.Empty, + output, + iterations: 1, + HashAlgorithmName.SHA256); + AssertExtensions.SequenceEqual(expected, output); + } + [Theory] [MemberData(nameof(Pbkdf2_PasswordBytes_Compare_Data))] public static void Pbkdf2_PasswordBytes_Compare( From a0424c6f13c69a23d4750919ce7a97d133f1932d Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Thu, 24 Apr 2025 12:46:06 -0400 Subject: [PATCH 2/3] Use an empty array --- .../Runtime/InteropServices/NotNullReadOnlySpanMarshaller.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/libraries/Common/src/System/Runtime/InteropServices/NotNullReadOnlySpanMarshaller.cs b/src/libraries/Common/src/System/Runtime/InteropServices/NotNullReadOnlySpanMarshaller.cs index 76a252c1aea94f..668611672117e3 100644 --- a/src/libraries/Common/src/System/Runtime/InteropServices/NotNullReadOnlySpanMarshaller.cs +++ b/src/libraries/Common/src/System/Runtime/InteropServices/NotNullReadOnlySpanMarshaller.cs @@ -63,8 +63,7 @@ public static ref T GetPinnableReference(ReadOnlySpan managed) { if (Unsafe.IsNullRef(ref MemoryMarshal.GetReference(managed)) && managed.IsEmpty) { - T[] notNull = new T[1]; - return ref MemoryMarshal.GetArrayDataReference(notNull); + return ref MemoryMarshal.GetArrayDataReference(Array.Empty()); } else { From 9a39402a18fd4e38bf0e301647e1f153262232d9 Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Thu, 24 Apr 2025 13:32:17 -0400 Subject: [PATCH 3/3] Fix build --- .../src/Microsoft.Bcl.Cryptography.csproj | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libraries/Microsoft.Bcl.Cryptography/src/Microsoft.Bcl.Cryptography.csproj b/src/libraries/Microsoft.Bcl.Cryptography/src/Microsoft.Bcl.Cryptography.csproj index 1e0c5be2bbd92b..a4e069c13d4c61 100644 --- a/src/libraries/Microsoft.Bcl.Cryptography/src/Microsoft.Bcl.Cryptography.csproj +++ b/src/libraries/Microsoft.Bcl.Cryptography/src/Microsoft.Bcl.Cryptography.csproj @@ -27,6 +27,8 @@ +