Provide a temporary RSA decryption buffer when dest is too small (dot…

…net#36601) * Provide a temporary RSA decryption buffer when dest is too small * Remove the semi-redundant second span variable * Add tests to ensure that destination is not tainted when decryption fails * Make Decrypt_WrongKey_OAEP_SHA256 conditional
stark-lang · Apr 6, 2019 · df5bba1 · df5bba1
2 parents 8e61998 + 85f3729
commit df5bba1
Show file tree

Hide file tree

Showing 7 changed files with 91 additions and 25 deletions.
diff --git a/src/Common/src/System/Security/Cryptography/RSAOpenSsl.cs b/src/Common/src/System/Security/Cryptography/RSAOpenSsl.cs
@@ -130,6 +130,54 @@ public override bool TryDecrypt(
             SafeRsaHandle key = _key.Value;
             CheckInvalidKey(key);
 
+            int keySizeBytes = Interop.Crypto.RsaSize(key);
+
+            // OpenSSL does not take a length value for the destination, so it can write out of bounds.
+            // To prevent the OOB write, decrypt into a temporary buffer.
+            if (destination.Length < keySizeBytes)
+            {
+                Span<byte> tmp = stackalloc byte[0];
+                byte[] rent = null;
+
+                // RSA up through 4096 stackalloc
+                if (keySizeBytes <= 512)
+                {
+                    tmp = stackalloc byte[keySizeBytes];
+                }
+                else
+                {
+                    rent = ArrayPool<byte>.Shared.Rent(keySizeBytes);
+                    tmp = rent;
+                }
+
+                bool ret = TryDecrypt(key, data, tmp, rsaPadding, oaepProcessor, out bytesWritten);
+
+                if (ret)
+                {
+                    tmp = tmp.Slice(0, bytesWritten);
+
+                    if (bytesWritten > destination.Length)
+                    {
+                        ret = false;
+                        bytesWritten = 0;
+                    }
+                    else
+                    {
+                        tmp.CopyTo(destination);
+                    }
+
+                    CryptographicOperations.ZeroMemory(tmp);
+                }
+
+                if (rent != null)
+                {
+                    // Already cleared
+                    ArrayPool<byte>.Shared.Return(rent);
+                }
+
+                return ret;
+            }
+
             return TryDecrypt(key, data, destination, rsaPadding, oaepProcessor, out bytesWritten);
         }
 

diff --git a/...ts/System/Security/Cryptography/AlgorithmImplementations/RSA/EncryptDecrypt.netcoreapp.cs b/...ts/System/Security/Cryptography/AlgorithmImplementations/RSA/EncryptDecrypt.netcoreapp.cs
@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
+using System.Linq;
 using Xunit;
 
 namespace System.Security.Cryptography.Rsa.Tests
@@ -44,21 +45,13 @@ public void Decrypt_VariousSizeSpans_Success()
                 Assert.Equal(0, bytesWritten);
                 Assert.Equal<byte>(new byte[actual.Length], actual);
 
-                // Just right... but that may be insufficient on Unix, where with padding the output destination
-                // may need to be larger than the actual decrypted content.
+                // Just right.
                 actual = new byte[TestData.HelloBytes.Length];
                 bool decrypted = rsa.TryDecrypt(cipherBytes, actual, RSAEncryptionPadding.OaepSHA1, out bytesWritten);
-                if (RSAFactory.SupportsDecryptingIntoExactSpaceRequired || decrypted)
-                {
-                    Assert.True(decrypted);
-                    Assert.Equal(TestData.HelloBytes.Length, bytesWritten);
-                    Assert.Equal<byte>(TestData.HelloBytes, actual);
-                }
-                else
-                {
-                    Assert.Equal(0, bytesWritten);
-                    Assert.Equal<byte>(new byte[actual.Length], actual);
-                }
+
+                Assert.True(decrypted);
+                Assert.Equal(TestData.HelloBytes.Length, bytesWritten);
+                Assert.Equal<byte>(TestData.HelloBytes, actual);
 
                 // Bigger than needed
                 actual = new byte[TestData.HelloBytes.Length + 1000];
@@ -94,5 +87,42 @@ public void Encrypt_VariousSizeSpans_Success()
                 Assert.Equal(cipherBytes.Length, bytesWritten);
             }
         }
+
+        [Fact]
+        public void Decrypt_WrongKey_Pkcs7()
+        {
+            Decrypt_WrongKey(RSAEncryptionPadding.Pkcs1);
+        }
+
+        [Fact]
+        public void Decrypt_WrongKey_OAEP_SHA1()
+        {
+            Decrypt_WrongKey(RSAEncryptionPadding.OaepSHA1);
+        }
+
+        [ConditionalFact(nameof(SupportsSha2Oaep))]
+        public void Decrypt_WrongKey_OAEP_SHA256()
+        {
+            Decrypt_WrongKey(RSAEncryptionPadding.OaepSHA256);
+        }
+
+        private static void Decrypt_WrongKey(RSAEncryptionPadding padding)
+        {
+            using (RSA rsa1 = RSAFactory.Create())
+            using (RSA rsa2 = RSAFactory.Create())
+            {
+                byte[] encrypted = rsa1.Encrypt(TestData.HelloBytes, padding);
+                byte[] buf = new byte[encrypted.Length];
+                buf.AsSpan().Fill(0xCA);
+
+                int bytesWritten = 0;
+
+                Assert.ThrowsAny<CryptographicException>(
+                    () => rsa2.TryDecrypt(encrypted, buf, padding, out bytesWritten));
+
+                Assert.Equal(0, bytesWritten);
+                Assert.True(buf.All(b => b == 0xCA));
+            }
+        }
     }
 }
diff --git a/src/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/RSAFactory.cs b/src/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/RSAFactory.cs
@@ -12,7 +12,6 @@ public interface IRSAProvider
         bool SupportsLargeExponent { get; }
         bool SupportsSha2Oaep { get; }
         bool SupportsPss { get; }
-        bool SupportsDecryptingIntoExactSpaceRequired { get; }
     }
 
     public static partial class RSAFactory
@@ -41,7 +40,5 @@ public static RSA Create(RSAParameters rsaParameters)
         public static bool SupportsSha2Oaep => s_provider.SupportsSha2Oaep;
 
         public static bool SupportsPss => s_provider.SupportsPss;
-
-        public static bool SupportsDecryptingIntoExactSpaceRequired => s_provider.SupportsDecryptingIntoExactSpaceRequired;
     }
 }
diff --git a/src/System.Security.Cryptography.Algorithms/tests/DefaultRSAProvider.cs b/src/System.Security.Cryptography.Algorithms/tests/DefaultRSAProvider.cs
@@ -52,9 +52,6 @@ public bool Supports384PrivateKey
 
         public bool SupportsPss { get; } =
             !PlatformDetection.IsFullFramework || !(RSA.Create() is RSACryptoServiceProvider);
-
-        public bool SupportsDecryptingIntoExactSpaceRequired => RuntimeInformation.IsOSPlatform(OSPlatform.Windows);
-
     }
 
     public partial class RSAFactory

diff --git a/src/System.Security.Cryptography.Cng/tests/RSACngProvider.cs b/src/System.Security.Cryptography.Cng/tests/RSACngProvider.cs
@@ -35,8 +35,6 @@ public bool Supports384PrivateKey
         public bool SupportsSha2Oaep => true;
 
         public bool SupportsPss => true;
-
-        public bool SupportsDecryptingIntoExactSpaceRequired => true;
     }
 
     public partial class RSAFactory

diff --git a/src/System.Security.Cryptography.Csp/tests/RSACryptoServiceProviderProvider.cs b/src/System.Security.Cryptography.Csp/tests/RSACryptoServiceProviderProvider.cs
@@ -19,8 +19,6 @@ public class RSACryptoServiceProviderProvider : IRSAProvider
         public bool SupportsSha2Oaep => false;
 
         public bool SupportsPss => false;
-
-        public bool SupportsDecryptingIntoExactSpaceRequired => RuntimeInformation.IsOSPlatform(OSPlatform.Windows);
     }
 
     public partial class RSAFactory

diff --git a/src/System.Security.Cryptography.OpenSsl/tests/RSAOpenSslProvider.cs b/src/System.Security.Cryptography.OpenSsl/tests/RSAOpenSslProvider.cs
@@ -17,8 +17,6 @@ public class RSAOpenSslProvider : IRSAProvider
         public bool SupportsSha2Oaep => true;
 
         public bool SupportsPss => true;
-
-        public bool SupportsDecryptingIntoExactSpaceRequired => false;
     }
 
     public partial class RSAFactory