Use checked math with CryptoStream buffer calculations

In .NET Framework, .NET 5, and .NET Core the block scaling logic was used for
`new byte[inputCount / inputBlockSize * outputBlockSize]` (or swap in/out).
That expression throws an OverflowException when the computed size
overflows.

Earlier in .NET 6 we changed to use ArrayPool.Rent.  When ArrayPool.Rent gets a
negative (overflowed) number it instead throws an ArgumentException.

This change restores the OverflowException, so that we're not leaking an
ArgumentException back to the caller.  (While the caller can control the
behavior with the arguments they pass to CryptoStream.Read/Write, the
description and paramName don't map in an obvious manner)

Making the scenario work has virtue, but requires a larger test investment than
we can make at this time.

diff --git a/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs b/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs
index afe137d..9a67b46 100644 (file)
--- a/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs
+++ b/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs
@@ -344,7 +344,7 @@ namespace System.Security.Cryptography
                 if (blocksToProcess > 1 && _transform.CanTransformMultipleBlocks)
                 {
                     // Use ArrayPool.Shared instead of CryptoPool because the array is passed out.
-                    int numWholeBlocksInBytes = blocksToProcess * _inputBlockSize;
+                    int numWholeBlocksInBytes = checked(blocksToProcess * _inputBlockSize);
                     byte[] tempInputBuffer = ArrayPool<byte>.Shared.Rent(numWholeBlocksInBytes);
                     try
                     {
@@ -570,7 +570,7 @@ namespace System.Security.Cryptography
                         int numWholeBlocksInBytes = numWholeBlocks * _inputBlockSize;
 
                         // Use ArrayPool.Shared instead of CryptoPool because the array is passed out.
-                        byte[]? tempOutputBuffer = ArrayPool<byte>.Shared.Rent(numWholeBlocks * _outputBlockSize);
+                        byte[]? tempOutputBuffer = ArrayPool<byte>.Shared.Rent(checked(numWholeBlocks * _outputBlockSize));
                         numOutputBytes = 0;
 
                         try

diff --git a/src/libraries/System.Security.Cryptography.Primitives/tests/CryptoStream.cs b/src/libraries/System.Security.Cryptography.Primitives/tests/CryptoStream.cs
index 86fa123..4112b13 100644 (file)
--- a/src/libraries/System.Security.Cryptography.Primitives/tests/CryptoStream.cs
+++ b/src/libraries/System.Security.Cryptography.Primitives/tests/CryptoStream.cs
@@ -5,6 +5,7 @@ using System.IO;
 using System.IO.Tests;
 using System.Text;
 using System.Threading.Tasks;
+using Microsoft.DotNet.XUnitExtensions;
 using Xunit;
 
 namespace System.Security.Cryptography.Encryption.Tests.Asymmetric
@@ -317,6 +318,74 @@ namespace System.Security.Cryptography.Encryption.Tests.Asymmetric
             }
         }
 
+        [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.Is64BitProcess))]
+        public static void EnormousRead()
+        {
+            // 0x6000_0000 / 3 => 0x2000_0000 * 4 => 0x8000_0000 (overflow)
+            // (output bytes) / (output block size) * (input block size) == (input bytes requested)
+            const int OutputBufferLength = 0x6000_0000;
+            byte[] output;
+
+            try
+            {
+                output = new byte[OutputBufferLength];
+            }
+            catch (OutOfMemoryException)
+            {
+                throw new SkipTestException("Could not create a large enough array");
+            }
+
+            // The input portion doesn't matter, the overflow happens before the call to the inner
+            // stream's read.
+            //
+            // When changing this flow from an OverflowException, there are two reasonable changes:
+            // A really big buffer (but the interior logic clamping the temp read buffer to Array.MaxLength
+            //   will still get it in one read)
+            // A stream that produces more than Array.MaxLength bytes.  Like, oh, 0x8000_0000U of them.
+            byte[] buffer = Array.Empty<byte>();
+
+            using (MemoryStream stream = new MemoryStream(buffer))
+            using (ICryptoTransform transform = new FromBase64Transform())
+            using (CryptoStream cryptoStream = new CryptoStream(stream, transform, CryptoStreamMode.Read))
+            {
+                Assert.Throws<OverflowException>(() => cryptoStream.Read(output, 0, output.Length));
+            }
+        }
+
+        [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.Is64BitProcess))]
+        public static void EnormousWrite()
+        {
+            // 0x6000_0000 / 3 => 0x2000_0000 * 4 => 0x8000_0000 (overflow)
+            // (input bytes) / (input block size) * (output block size) => (output bytes to write)
+            const int InputBufferLength = 0x60000000;
+
+            byte[] buffer;
+
+            try
+            {
+                buffer = new byte[InputBufferLength];
+            }
+            catch (OutOfMemoryException)
+            {
+                throw new SkipTestException("Could not create a large enough array");
+            }
+
+            // In the Read scenario the overflow comes from a reducing transform.
+            // In the Write scenario it comes from an expanding transform.
+            //
+            // When making the write not overflow change the test to use an output stream
+            // that isn't bounded by Array.MaxLength.  e.g. a counting stream, or a stream
+            // that just computes some hash of the input (so total correctness can be measured)
+            byte[] output = Array.Empty<byte>();
+
+            using (MemoryStream stream = new MemoryStream(output))
+            using (ICryptoTransform transform = new ToBase64Transform())
+            using (CryptoStream cryptoStream = new CryptoStream(stream, transform, CryptoStreamMode.Write, leaveOpen: true))
+            {
+                Assert.Throws<OverflowException>(() => cryptoStream.Write(buffer, 0, buffer.Length));
+            }
+        }
+
         private sealed class DerivedCryptoStream : CryptoStream
         {
             public bool DisposeInvoked;