Stopping decryption before EOF throws exception: Padding is invalid and cannot be removed

Question

This is the scenario we have: We have huge encrypted files, in the order of gigabytes that we can decrypt correctly if we read them until the end. The problem arises when we are reading and detect some flag in the file, then we stop reading and call reader.Close(), what happens is that a CryptographicException: "Padding is invalid and cannot be removed." is thrown. I have this small console app that reproduce this behavior, to test it just run it, it will create a file in your C:\ drive and then it will read line by line when pressing any key, and will stop when pressing 'q'.

using System;
using System.IO;
using System.Security.Cryptography;

namespace encryptSample
{
    class Program
    {
        static void Main(string[] args)
        {
            var transform = CreateCryptoTransform(true);
            // first create encrypted file
            using (FileStream destination = new FileStream("c:\\test_enc.txt", FileMode.OpenOrCreate, FileAccess.Write, FileShare.ReadWrite))
            {
                using (CryptoStream cryptoStream = new CryptoStream(destination, transform, CryptoStreamMode.Write))
                {
                    using (StreamWriter source = new StreamWriter(cryptoStream))
                    {
                        for (int i = 0; i < 1000; i++)
                        {
                            source.WriteLine("This is just random text to fill the file and show what happens when I stop reading in the middle - " + i);
                        }
                        // Also tried this line, but is the same with or without it
                        cryptoStream.FlushFinalBlock();
                    }
                }
            }

            StreamReader reader;
            ICryptoTransform transformDec;
            CryptoStream cryptoStreamReader;

            transformDec = CreateCryptoTransform(false);
            FileStream fileStream = new FileStream("c:\\test_enc.txt", FileMode.Open, FileAccess.Read, FileShare.ReadWrite);
            cryptoStreamReader = new CryptoStream(fileStream, transformDec, CryptoStreamMode.Read);
            reader = new StreamReader(cryptoStreamReader);

            while (Console.In.ReadLine() != "q")
            {
                Console.WriteLine(reader.ReadLine());
            }

            try
            {
                cryptoStreamReader.Close();
                reader.Close();
                reader.Dispose();
            }
            catch (CryptographicException ex)
            {
                if (reader.EndOfStream)
                    throw;

            }
        }

        private static ICryptoTransform CreateCryptoTransform(bool encrypt)
        {
            byte[] salt = new byte[] { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 }; // Must be at least eight bytes.  MAKE THIS SALTIER!
            const int iterations = 1042; // Recommendation is >= 1000.
            const string password = "123456";

            AesManaged aes = new AesManaged();
            aes.BlockSize = aes.LegalBlockSizes[0].MaxSize;
            aes.KeySize = aes.LegalKeySizes[0].MaxSize;
            // NB: Rfc2898DeriveBytes initialization and subsequent calls to   GetBytes   must be eactly the same, including order, on both the encryption and decryption sides.
            Rfc2898DeriveBytes key = new Rfc2898DeriveBytes(password, salt, iterations);
            aes.Key = key.GetBytes(aes.KeySize / 8);
            aes.IV = key.GetBytes(aes.BlockSize / 8);
            aes.Mode = CipherMode.CBC;
            aes.Padding = PaddingMode.PKCS7;
            ICryptoTransform transform = encrypt ? aes.CreateEncryptor(aes.Key, aes.IV) : aes.CreateDecryptor(aes.Key, aes.IV);
            return transform;
        }

    }
}

In our original class, we do the reader.Close during the Dispose(). My question is, is it valid to check if reader.EndOfStream is false and then capture the CryptographicException? Or there is something wrong in the encryption/decryption methods? Maybe we are missing something.

Regards!

Answer 1

This exception is thrown during Dispose(true). Throwing from Dispose is already a design flaw (https://docs.microsoft.com/en-us/visualstudio/code-quality/ca1065-do-not-raise-exceptions-in-unexpected-locations#dispose-methods), but it's even worse since this exception is thrown even before the underlying stream is closed.

This means that anything that receives a Stream that might be a CryptoStream needs to work around this and either close the underlying Stream themselves in a 'catch' block (essentially needing a reference to something completely unrelated), or somehow warn all listeners that the stream may still be open (e.g., "don't try to delete the underlying file -- it's still open!").

No, in my book, this is a pretty big oversight, and the other answers don't seem to address the fundamental issue. CryptoStream takes ownership of the passed-in stream, so it takes on the responsibility to close the underlying stream before control leaves Dispose(true), end of story.

Ideally, it should also never throw under circumstances that are not truly exceptional (such as "we stopped reading early, because the decrypted data is in the wrong format and it's a waste of time to continue reading").

Our solution was basically this (update: but be warned -- as Will Krause pointed out in the comments, this could leave sensitive information lying around in the private _InputBuffer and _OutputBuffer fields that can be accessed via reflection. Versions 4.5 and above of the .NET Framework don't have this problem.):

internal sealed class SilentCryptoStream : CryptoStream
{
    private readonly Stream underlyingStream;

    public SilentCryptoStream(Stream stream, ICryptoTransform transform, CryptoStreamMode mode)
        : base(stream, transform, mode)
    {
        // stream is already implicitly validated non-null in the base constructor.
        this.underlyingStream = stream;
    }

    protected override void Dispose(bool disposing)
    {
        try
        {
            base.Dispose(disposing);
        }
        catch (CryptographicException)
        {
            if (disposing)
            {
                this.underlyingStream.Dispose();
            }
        }
    }
}