Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

System.Security.Cryptography.CryptoStream.Dispose throws an exception #7779

Closed
joshfree opened this Issue Apr 15, 2016 · 9 comments

Comments

Projects
None yet
6 participants
@joshfree
Copy link
Member

joshfree commented Apr 15, 2016

Moved from dotnet/coreclr#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

This comment has been minimized.

Copy link
Member

bartonjs commented Apr 15, 2016

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.

@bartonjs bartonjs modified the milestones: Future, 1.0.0-rtm Apr 15, 2016

@joshfree joshfree assigned ghost and unassigned bartonjs May 2, 2016

@joshfree joshfree modified the milestones: 1.1.0, Future May 2, 2016

@ghost

This comment has been minimized.

Copy link

ghost commented May 11, 2016

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

This comment has been minimized.

Copy link
Member

karelz commented Nov 16, 2016

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.

@karelz karelz closed this Nov 16, 2016

@ATimmeh33

This comment has been minimized.

Copy link

ATimmeh33 commented Jan 31, 2019

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

This comment has been minimized.

Copy link
Member

stephentoub commented Jan 31, 2019

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

This comment has been minimized.

Copy link

ATimmeh33 commented Jan 31, 2019

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

This comment has been minimized.

Copy link
Member

stephentoub commented Jan 31, 2019

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

This comment has been minimized.

Copy link
Member

bartonjs commented Feb 2, 2019

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

This comment has been minimized.

Copy link

kosta-arnorsky commented Feb 4, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.