Handle null signature destinations for ECDsa / DSA / RSA

Handle null signature destinations. When RSA/ECDSA/DSA use a "null" span as a destination for a signature, a nullptr was handed to CNG. CNG would interpret this as asking for the signature length. It would set the output length and return success without actually computing the signature. This changes the p/invokes to prevent null values for the signature destination.
jww3 · Oct 25, 2023 · 1410793 · 1410793
1 parent b2b1c7c
commit 1410793
Show file tree

Hide file tree

Showing 6 changed files with 119 additions and 9 deletions.
diff --git a/src/libraries/Common/src/Interop/Windows/BCrypt/Interop.BCryptSignHash.cs b/src/libraries/Common/src/Interop/Windows/BCrypt/Interop.BCryptSignHash.cs
@@ -3,6 +3,7 @@
 
 using System;
 using System.Runtime.InteropServices;
+using Internal.Cryptography;
 
 using Microsoft.Win32.SafeHandles;
 
@@ -30,7 +31,7 @@ internal static unsafe NTSTATUS BCryptSignHashPkcs1(
         {
             fixed (char* pHashAlgorithmName = hashAlgorithmName)
             fixed (byte* pHash = &MemoryMarshal.GetReference(hash))
-            fixed (byte* pDest = &MemoryMarshal.GetReference(destination))
+            fixed (byte* pDest = &Helpers.GetNonNullPinnableReference(destination))
             {
                 BCRYPT_PKCS1_PADDING_INFO paddingInfo = default;
                 paddingInfo.pszAlgId = (IntPtr)pHashAlgorithmName;
@@ -56,7 +57,7 @@ internal static unsafe NTSTATUS BCryptSignHashPss(
         {
             fixed (char* pHashAlgorithmName = hashAlgorithmName)
             fixed (byte* pHash = &MemoryMarshal.GetReference(hash))
-            fixed (byte* pDest = &MemoryMarshal.GetReference(destination))
+            fixed (byte* pDest = &Helpers.GetNonNullPinnableReference(destination))
             {
                 BCRYPT_PSS_PADDING_INFO paddingInfo = default;
                 paddingInfo.pszAlgId = (IntPtr)pHashAlgorithmName;

diff --git a/src/libraries/Common/src/Interop/Windows/NCrypt/Interop.SignVerify.cs b/src/libraries/Common/src/Interop/Windows/NCrypt/Interop.SignVerify.cs
@@ -3,14 +3,24 @@
 
 using System;
 using System.Runtime.InteropServices;
+using Internal.Cryptography;
 using Microsoft.Win32.SafeHandles;
 
 internal static partial class Interop
 {
     internal static partial class NCrypt
     {
         [LibraryImport(Libraries.NCrypt, StringMarshalling = StringMarshalling.Utf16)]
-        internal static unsafe partial ErrorCode NCryptSignHash(SafeNCryptKeyHandle hKey, void* pPaddingInfo, ReadOnlySpan<byte> pbHashValue, int cbHashValue, Span<byte> pbSignature, int cbSignature, out int pcbResult, AsymmetricPaddingMode dwFlags);
+        private static unsafe partial ErrorCode NCryptSignHash(SafeNCryptKeyHandle hKey, void* pPaddingInfo, byte* pbHashValue, int cbHashValue, byte* pbSignature, int cbSignature, out int pcbResult, AsymmetricPaddingMode dwFlags);
+
+        internal static unsafe ErrorCode NCryptSignHash(SafeNCryptKeyHandle hKey, void* pPaddingInfo, ReadOnlySpan<byte> pbHashValue, Span<byte> pbSignature, out int pcbResult, AsymmetricPaddingMode dwFlags)
+        {
+            fixed (byte* pHash = &MemoryMarshal.GetReference(pbHashValue))
+            fixed (byte* pSignature = &Helpers.GetNonNullPinnableReference(pbSignature))
+            {
+                return NCryptSignHash(hKey, pPaddingInfo, pHash, pbHashValue.Length, pSignature, pbSignature.Length, out pcbResult, dwFlags);
+            }
+        }
 
         [LibraryImport(Libraries.NCrypt, StringMarshalling = StringMarshalling.Utf16)]
         internal static unsafe partial ErrorCode NCryptVerifySignature(SafeNCryptKeyHandle hKey, void* pPaddingInfo, ReadOnlySpan<byte> pbHashValue, int cbHashValue, ReadOnlySpan<byte> pbSignature, int cbSignature, AsymmetricPaddingMode dwFlags);

diff --git a/...s/Common/tests/System/Security/Cryptography/AlgorithmImplementations/DSA/DSASignVerify.cs b/...s/Common/tests/System/Security/Cryptography/AlgorithmImplementations/DSA/DSASignVerify.cs
@@ -391,6 +391,25 @@ public void Verify2048WithSha1()
             }
         }
 
+        [Fact]
+        public void SignData_NullSignature_Fails()
+        {
+            using (DSA dsa = DSAFactory.Create())
+            {
+                dsa.ImportParameters(DSATestData.GetDSA1024Params());
+
+                bool result = dsa.TrySignData(
+                    "hello"u8,
+                    (Span<byte>)null,
+                    HashAlgorithmName.SHA1,
+                    DSASignatureFormat.IeeeP1363FixedFieldConcatenation,
+                    out int bytesWritten);
+
+                Assert.False(result);
+                Assert.Equal(0, bytesWritten);
+            }
+        }
+
         private void SignAndVerify(byte[] data, string hashAlgorithmName, DSAParameters dsaParameters, int expectedSignatureLength)
         {
             using (DSA dsa = DSAFactory.Create())

diff --git a/...es/Common/tests/System/Security/Cryptography/AlgorithmImplementations/ECDsa/ECDsaTests.cs b/...es/Common/tests/System/Security/Cryptography/AlgorithmImplementations/ECDsa/ECDsaTests.cs
@@ -86,6 +86,32 @@ public void VerifyHash_InvalidArguments_Throws(ECDsa ecdsa)
             AssertExtensions.Throws<ArgumentNullException>("hash", () => ecdsa.VerifyHash(null, null));
             AssertExtensions.Throws<ArgumentNullException>("signature", () => ecdsa.VerifyHash(new byte[0], null));
         }
+
+        [Theory]
+        [MemberData(nameof(RealImplementations))]
+        public void SignHash_NullSignature_Fails(ECDsa ecdsa)
+        {
+            byte[] hash = RandomNumberGenerator.GetBytes(SHA256.HashSizeInBytes);
+
+            AssertExtensions.Throws<ArgumentException>("destination", () =>
+                ecdsa.SignHash(hash, (Span<byte>)null, DSASignatureFormat.IeeeP1363FixedFieldConcatenation));
+
+            bool result = ecdsa.TrySignHash(hash, (Span<byte>)null, DSASignatureFormat.IeeeP1363FixedFieldConcatenation, out int bytesWritten);
+            Assert.False(result);
+            Assert.Equal(0, bytesWritten);
+        }
+
+        [Theory]
+        [MemberData(nameof(RealImplementations))]
+        public void SignData_NullSignature_Fails(ECDsa ecdsa)
+        {
+            AssertExtensions.Throws<ArgumentException>("destination", () =>
+                ecdsa.SignData("hello"u8, (Span<byte>)null, HashAlgorithmName.SHA256, DSASignatureFormat.IeeeP1363FixedFieldConcatenation));
+
+            bool result = ecdsa.TrySignData("hello"u8, (Span<byte>)null, HashAlgorithmName.SHA256, DSASignatureFormat.IeeeP1363FixedFieldConcatenation, out int bytesWritten);
+            Assert.False(result);
+            Assert.Equal(0, bytesWritten);
+        }
     }
 
     [SkipOnPlatform(TestPlatforms.Browser, "Not supported on Browser")]

diff --git a/...ries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/SignVerify.cs b/...ries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/SignVerify.cs
@@ -1429,6 +1429,60 @@ public void PssSignature_WrongLength()
             }
         }
 
+        [ConditionalTheory]
+        [InlineData(true)]
+        [InlineData(false)]
+        public void SignHash_NullSignature_Fails(bool usePss)
+        {
+            if (!SupportsPss)
+            {
+                throw new SkipTestException("Platform does not support PSS");
+            }
+
+            RSASignaturePadding padding = usePss ? RSASignaturePadding.Pss : RSASignaturePadding.Pkcs1;
+
+            using (RSA rsa = RSA.Create())
+            {
+                byte[] hash = RandomNumberGenerator.GetBytes(SHA256.HashSizeInBytes);
+
+                AssertExtensions.Throws<ArgumentException>("destination", () =>
+                    rsa.SignHash(hash, (Span<byte>)null, HashAlgorithmName.SHA256, padding));
+
+                bool result = rsa.TrySignHash(
+                    hash,
+                    (Span<byte>)null,
+                    HashAlgorithmName.SHA256,
+                    padding,
+                    out int bytesWritten);
+
+                Assert.False(result);
+                Assert.Equal(0, bytesWritten);
+            }
+        }
+
+        [ConditionalTheory]
+        [InlineData(true)]
+        [InlineData(false)]
+        public void SignData_NullSignature_Fails(bool usePss)
+        {
+            if (!SupportsPss)
+            {
+                throw new SkipTestException("Platform does not support PSS");
+            }
+
+            RSASignaturePadding padding = usePss ? RSASignaturePadding.Pss : RSASignaturePadding.Pkcs1;
+
+            using (RSA rsa = RSA.Create())
+            {
+                AssertExtensions.Throws<ArgumentException>("destination", () =>
+                    rsa.SignData("hello"u8, (Span<byte>)null, HashAlgorithmName.SHA256, padding));
+
+                bool result = rsa.TrySignData("hello"u8, (Span<byte>)null, HashAlgorithmName.SHA256, padding, out int bytesWritten);
+                Assert.False(result);
+                Assert.Equal(0, bytesWritten);
+            }
+        }
+
         private void ExpectSignature(
             byte[] expectedSignature,
             byte[] data,

diff --git a/...ies/System.Security.Cryptography/src/System/Security/Cryptography/CngCommon.SignVerify.cs b/...ies/System.Security.Cryptography/src/System/Security/Cryptography/CngCommon.SignVerify.cs
@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System;
+using System.Diagnostics;
 using Internal.Cryptography;
 using Microsoft.Win32.SafeHandles;
 using ErrorCode = Interop.NCrypt.ErrorCode;
@@ -20,22 +21,22 @@ public static unsafe byte[] SignHash(this SafeNCryptKeyHandle keyHandle, ReadOnl
 #endif
             byte[] signature = new byte[estimatedSize];
             int numBytesNeeded;
-            ErrorCode errorCode = Interop.NCrypt.NCryptSignHash(keyHandle, pPaddingInfo, hash, hash.Length, signature, signature.Length, out numBytesNeeded, paddingMode);
+            ErrorCode errorCode = Interop.NCrypt.NCryptSignHash(keyHandle, pPaddingInfo, hash, signature, out numBytesNeeded, paddingMode);
 
             if (errorCode == ErrorCode.STATUS_UNSUCCESSFUL)
             {
-                errorCode = Interop.NCrypt.NCryptSignHash(keyHandle, pPaddingInfo, hash, hash.Length, signature, signature.Length, out numBytesNeeded, paddingMode);
+                errorCode = Interop.NCrypt.NCryptSignHash(keyHandle, pPaddingInfo, hash, signature, out numBytesNeeded, paddingMode);
             }
 
             if (errorCode.IsBufferTooSmall())
             {
                 signature = new byte[numBytesNeeded];
-                errorCode = Interop.NCrypt.NCryptSignHash(keyHandle, pPaddingInfo, hash, hash.Length, signature, signature.Length, out numBytesNeeded, paddingMode);
+                errorCode = Interop.NCrypt.NCryptSignHash(keyHandle, pPaddingInfo, hash, signature, out numBytesNeeded, paddingMode);
             }
 
             if (errorCode == ErrorCode.STATUS_UNSUCCESSFUL)
             {
-                errorCode = Interop.NCrypt.NCryptSignHash(keyHandle, pPaddingInfo, hash, hash.Length, signature, signature.Length, out numBytesNeeded, paddingMode);
+                errorCode = Interop.NCrypt.NCryptSignHash(keyHandle, pPaddingInfo, hash, signature, out numBytesNeeded, paddingMode);
             }
 
             if (errorCode != ErrorCode.ERROR_SUCCESS)
@@ -53,16 +54,15 @@ public static unsafe bool TrySignHash(this SafeNCryptKeyHandle keyHandle, ReadOn
                     keyHandle,
                     pPaddingInfo,
                     hash,
-                    hash.Length,
                     signature,
-                    signature.Length,
                     out int numBytesNeeded,
                     paddingMode);
 
                 switch (error)
                 {
                     case ErrorCode.ERROR_SUCCESS:
                         bytesWritten = numBytesNeeded;
+                        Debug.Assert(bytesWritten <= signature.Length);
                         return true;
 
                     case ErrorCode code when code.IsBufferTooSmall():