Skip to content

Commit 058a531

Browse files
authored
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.
1 parent b92360a commit 058a531

File tree

2 files changed

+71
-2
lines changed

2 files changed

+71
-2
lines changed

src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -344,7 +344,7 @@ private async ValueTask<int> ReadAsyncCore(Memory<byte> buffer, CancellationToke
344344
if (blocksToProcess > 1 && _transform.CanTransformMultipleBlocks)
345345
{
346346
// Use ArrayPool.Shared instead of CryptoPool because the array is passed out.
347-
int numWholeBlocksInBytes = blocksToProcess * _inputBlockSize;
347+
int numWholeBlocksInBytes = checked(blocksToProcess * _inputBlockSize);
348348
byte[] tempInputBuffer = ArrayPool<byte>.Shared.Rent(numWholeBlocksInBytes);
349349
try
350350
{
@@ -570,7 +570,7 @@ private async ValueTask WriteAsyncCore(ReadOnlyMemory<byte> buffer, Cancellation
570570
int numWholeBlocksInBytes = numWholeBlocks * _inputBlockSize;
571571

572572
// Use ArrayPool.Shared instead of CryptoPool because the array is passed out.
573-
byte[]? tempOutputBuffer = ArrayPool<byte>.Shared.Rent(numWholeBlocks * _outputBlockSize);
573+
byte[]? tempOutputBuffer = ArrayPool<byte>.Shared.Rent(checked(numWholeBlocks * _outputBlockSize));
574574
numOutputBytes = 0;
575575

576576
try

src/libraries/System.Security.Cryptography.Primitives/tests/CryptoStream.cs

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
using System.IO.Tests;
66
using System.Text;
77
using System.Threading.Tasks;
8+
using Microsoft.DotNet.XUnitExtensions;
89
using Xunit;
910

1011
namespace System.Security.Cryptography.Encryption.Tests.Asymmetric
@@ -317,6 +318,74 @@ public static void PaddedAes_PartialRead_Success()
317318
}
318319
}
319320

321+
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.Is64BitProcess))]
322+
public static void EnormousRead()
323+
{
324+
// 0x6000_0000 / 3 => 0x2000_0000 * 4 => 0x8000_0000 (overflow)
325+
// (output bytes) / (output block size) * (input block size) == (input bytes requested)
326+
const int OutputBufferLength = 0x6000_0000;
327+
byte[] output;
328+
329+
try
330+
{
331+
output = new byte[OutputBufferLength];
332+
}
333+
catch (OutOfMemoryException)
334+
{
335+
throw new SkipTestException("Could not create a large enough array");
336+
}
337+
338+
// The input portion doesn't matter, the overflow happens before the call to the inner
339+
// stream's read.
340+
//
341+
// When changing this flow from an OverflowException, there are two reasonable changes:
342+
// A really big buffer (but the interior logic clamping the temp read buffer to Array.MaxLength
343+
// will still get it in one read)
344+
// A stream that produces more than Array.MaxLength bytes. Like, oh, 0x8000_0000U of them.
345+
byte[] buffer = Array.Empty<byte>();
346+
347+
using (MemoryStream stream = new MemoryStream(buffer))
348+
using (ICryptoTransform transform = new FromBase64Transform())
349+
using (CryptoStream cryptoStream = new CryptoStream(stream, transform, CryptoStreamMode.Read))
350+
{
351+
Assert.Throws<OverflowException>(() => cryptoStream.Read(output, 0, output.Length));
352+
}
353+
}
354+
355+
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.Is64BitProcess))]
356+
public static void EnormousWrite()
357+
{
358+
// 0x6000_0000 / 3 => 0x2000_0000 * 4 => 0x8000_0000 (overflow)
359+
// (input bytes) / (input block size) * (output block size) => (output bytes to write)
360+
const int InputBufferLength = 0x60000000;
361+
362+
byte[] buffer;
363+
364+
try
365+
{
366+
buffer = new byte[InputBufferLength];
367+
}
368+
catch (OutOfMemoryException)
369+
{
370+
throw new SkipTestException("Could not create a large enough array");
371+
}
372+
373+
// In the Read scenario the overflow comes from a reducing transform.
374+
// In the Write scenario it comes from an expanding transform.
375+
//
376+
// When making the write not overflow change the test to use an output stream
377+
// that isn't bounded by Array.MaxLength. e.g. a counting stream, or a stream
378+
// that just computes some hash of the input (so total correctness can be measured)
379+
byte[] output = Array.Empty<byte>();
380+
381+
using (MemoryStream stream = new MemoryStream(output))
382+
using (ICryptoTransform transform = new ToBase64Transform())
383+
using (CryptoStream cryptoStream = new CryptoStream(stream, transform, CryptoStreamMode.Write, leaveOpen: true))
384+
{
385+
Assert.Throws<OverflowException>(() => cryptoStream.Write(buffer, 0, buffer.Length));
386+
}
387+
}
388+
320389
private sealed class DerivedCryptoStream : CryptoStream
321390
{
322391
public bool DisposeInvoked;

0 commit comments

Comments
 (0)