Runtime: System.Security.Cryptography.CryptoStream.Dispose throws an exception

[joshfree]

Moved from https://github.com/dotnet/coreclr/issues/4329 on behalf of @kosta-arnorsky

If Dispose method of System.Security.Cryptography.CryptoStream class which uses AES/Rijndael algorithm with padding is called before the ends of decryption a target data stream it throws the exception: System.Security.Cryptography.CryptographicException: Padding is invalid and cannot be removed.
It's especially unpleasant when decryption is interrupted by an uncaught exception:

using (var transform = new System.Security.Cryptography.AesManaged().CreateDecryptor(key, iv))
using (var cryptoStream = new System.Security.Cryptography.CryptoStream(encryptedDataStream, transform, System.Security.Cryptography.CryptoStreamMode.Read))
using (var decryptedStream = new System.IO.MemoryStream())
{
    var buffer = new byte[5000];
    var readed = cryptoStream.Read(buffer, 0, buffer.Length);
    decryptedStream.Write(buffer, 0, readed);
    throw new Exception("Something bad has happend.");
}

Actual Result: Dispose throws the exception:

System.Security.Cryptography.CryptographicException: Padding is invalid and cannot be removed.
   at System.Security.Cryptography.RijndaelManagedTransform.DecryptData(Byte[] inputBuffer, Int32 inputOffset, Int32 inputCount, Byte[]& outputBuffer, Int32 outputOffset, PaddingMode paddingMode, Boolean fLast)
   at System.Security.Cryptography.RijndaelManagedTransform.TransformFinalBlock(Byte[] inputBuffer, Int32 inputOffset, Int32 inputCount)
   at System.Security.Cryptography.CryptoStream.FlushFinalBlock()
   at System.Security.Cryptography.CryptoStream.Dispose(Boolean disposing)
   at System.IO.Stream.Close()
   at System.IO.Stream.Dispose()

I've created a small example to demonstrate this behavior.

[bartonjs]

Since the callstack is for Desktop (RijndaelManaged doesn't exist in CoreFx), I'm moving this to Future. It'll warrant a compat discussion for Desktop, or for being incompatible with Desktop.

[ghost]

Simplified the repro a bit. This repros on both desktop and Core - any crypto algorithm that does this delayed depadding is likely to object to CryptoStream.Dispose() thinking he can call FlushFinalBlock() prematurely.

using System;
using System.IO;
using System.Linq;
using System.Globalization;
using System.Security.Cryptography;

internal static class ConsoleApp5
{
    private static void Main(string[] args)
    {
        byte[] key = "3d20884067e866276312e5ad240767d49198cca0e89e4f586e5234baff7a3bcb".HexToByteArray();
        byte[] iv = "7a18ed9cc88684d868c51b94cdf04e5f".HexToByteArray();
        byte[] cipher = 
            ("a7457de219abe9040299d8f8bb2a05e2b1d18e190591f1902dc698c4fcaf03c0787180a8df12542ff07aaa23e4c9e948d4ec"
           + "b6b53e2aca00df60ba37fc78dc6050c7e0e41fe917415c736f9068821027cfbcdf9588187121dc87ec9439b3a3f8eb97ea7d"
           + "ccf2ae8bdf5e1bc529e0152e").HexToByteArray();

        Aes algorithm = Aes.Create();

        using (MemoryStream encryptedDataStream = new MemoryStream(cipher))
        using (ICryptoTransform transform = algorithm.CreateDecryptor(key, iv))
        {
            CryptoStream cryptoStream = new CryptoStream(encryptedDataStream, transform, CryptoStreamMode.Read);
            byte[] buffer = new byte[5];
            int readed = cryptoStream.Read(buffer, 0, buffer.Length);

            // CryptoStream.Dispose() throws a nuisance exception because it calls FlushFinalBlock on itself which calls ICryptoTransform.TransformFinalBlock, even
            // though it's nowhere near the final block.
            cryptoStream.Dispose();
        }
    }

    private static byte[] HexToByteArray(this string hexString)
    {
        byte[] bytes = new byte[hexString.Length / 2];

        for (int i = 0; i < hexString.Length; i += 2)
        {
            string s = hexString.Substring(i, 2);
            bytes[i / 2] = byte.Parse(s, NumberStyles.HexNumber, null);
        }

        return bytes;
    }
}

[karelz]

Dispose either has to eat the exception (which is bad), or not do the thing which caused it - which is functional breaking change against Desktop, which we probably shouldn't take.

[ATimmeh33]

This issue also occurs for us after our upgrade to ASP.NET Core. Returning a CryptoStream with a FileStreamResult and then aborting the request results in the same exception:

System.Security.Cryptography.CryptographicException: Padding is invalid and cannot be removed.
  at System.Security.Cryptography.CapiSymmetricAlgorithm.DepadBlock(Byte[] block, Int32 offset, Int32 count)
  at System.Security.Cryptography.CapiSymmetricAlgorithm.TransformFinalBlock(Byte[] inputBuffer, Int32 inputOffset, Int32 inputCount)
  at System.Security.Cryptography.CryptoStream.FlushFinalBlock()
  at System.Security.Cryptography.CryptoStream.Dispose(Boolean disposing)
  at System.IO.Stream.Close() 

Why is it necessary to depad the block when it's getting disposed? It seems like any partially read CryptoStream can no longer be disposed without throwing this exception.

Sample code:

private static SymmetricAlgorithm cryptoAlg;

static void Main(string[] args)
{
    cryptoAlg = SymmetricAlgorithm.Create("Aes");
    cryptoAlg.Mode = CipherMode.CBC;
    cryptoAlg.Key = cryptoAlg.IV = new byte[]
    {
        0x0, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7,
        0x8, 0x9, 0xA, 0xB, 0xC, 0xD, 0xE, 0xF,
    };

    string encrypted = Encrypt("Sample string that's bigger than cryptoAlg.BlockSize");

    using (var memoryStream = new MemoryStream(Convert.FromBase64String(encrypted)))
    using (var cryptoStream = new CryptoStream(
        memoryStream,
        cryptoAlg.CreateDecryptor(),
        CryptoStreamMode.Read)
    ) {
        // Partially read the CryptoStream before disposing it.
        cryptoStream.ReadByte();
    }
}

private static string Encrypt(string input)
{
    using (var memoryStream = new MemoryStream())
    using (var encryptor = cryptoAlg.CreateEncryptor())
    using (var cryptoStream = new CryptoStream(
        memoryStream,
        encryptor,
        CryptoStreamMode.Write)
    ) {
        byte[] inputBytes = Encoding.ASCII.GetBytes(input);
        cryptoStream.Write(inputBytes, 0, inputBytes.Length);
        cryptoStream.FlushFinalBlock();
        byte[] outputBytes = memoryStream.ToArray();
        return Convert.ToBase64String(outputBytes);
    }
}

[stephentoub]

Sample code

I tried your sample on both .NET Framework 4.7.2 and .NET Core 2.1/2.2/3.0. I added in the location of corelib just to confirm where it was coming from. I see the same exception in all cases:

c:\Users\stoub\Desktop>dotnet run -c Release -f netcoreapp2.1
C:\Program Files\dotnet\shared\Microsoft.NETCore.App\2.1.7\System.Private.CoreLib.dll

Unhandled Exception: System.Security.Cryptography.CryptographicException: Padding is invalid and cannot be removed.
   at Internal.Cryptography.UniversalCryptoDecryptor.DepadBlock(Byte[] block, Int32 offset, Int32 count)
   at Internal.Cryptography.UniversalCryptoDecryptor.UncheckedTransformFinalBlock(Byte[] inputBuffer, Int32 inputOffset, Int32 inputCount)
   at Internal.Cryptography.UniversalCryptoTransform.TransformFinalBlock(Byte[] inputBuffer, Int32 inputOffset, Int32 inputCount)
   at System.Security.Cryptography.CryptoStream.FlushFinalBlock()
   at System.Security.Cryptography.CryptoStream.Dispose(Boolean disposing)
   at Test.Main(String[] args) in c:\Users\stoub\Desktop\Program.cs:line 25

c:\Users\stoub\Desktop>dotnet run -c Release -f net472
C:\Windows\Microsoft.NET\Framework64\v4.0.30319\mscorlib.dll

Unhandled Exception: System.Security.Cryptography.CryptographicException: Padding is invalid and cannot be removed.
   at System.Security.Cryptography.CapiSymmetricAlgorithm.DepadBlock(Byte[] block, Int32 offset, Int32 count)
   at System.Security.Cryptography.CapiSymmetricAlgorithm.TransformFinalBlock(Byte[] inputBuffer, Int32 inputOffset, Int32 inputCount)
   at System.Security.Cryptography.CryptoStream.FlushFinalBlock()
   at System.Security.Cryptography.CryptoStream.Dispose(Boolean disposing)
   at System.IO.Stream.Close()
   at Test.Main(String[] args) in c:\Users\stoub\Desktop\Program.cs:line 32

Is there a version of the repro that succeeds on netfx but fails on netcore?

Thanks.

[ATimmeh33]

Is there a version of the repo that succeeds on netfx but fails on netcore?

Thanks.

No, problem does indeed occur on both netfx and netcore. Only became apparent after switching to ASP.NET Core though. I'm guessing because it's more efficient with it's resources and thus disposes as soon as a request is aborted.

Our current solution is eating the exception when HttpContext's cancellation token has cancellation requested, but it's more of a temporary fix.

[stephentoub]

Dispose either has to eat the exception (which is bad), or not do the thing which caused it - which is functional breaking change against Desktop, which we probably shouldn't take.

I'd like @bartonjs' opinion, if there's a security reason that we need to do this validation as part of disposal. It seems reasonable otherwise that when just reading you should be able to stop reading whenever you want.

[bartonjs]

Really that call only makes sense when it's in Write mode.

Making Dispose only call FlushFinalBlock in Write mode is a measurable change that we'd probably not take on netfx (at least not without a retargeting change), since we'd stop redundantly (or, in this case, erroneously) calling TransformFinalBlock on the ICryptoTransform; but it's probably a change we can take for Core.

Said another way: it's still a "we'd reduce Desktop compatibility" issue, but probably one we can live with.

[kosta-arnorsky]

@bartonjs, I believe the changes will actually fix more apps than it will break.