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

5

10

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!

Morley answered 26/3, 2013 at 15:23 Comment(2)
By the way, were you able to solve this?Leeann
We "solved" by checking for some status that tell us if the user aborted reading and also by checking the .EndOfStream. We didn't care if it is an undefined behavior; it was causing the problem only when the system detected certain flags and we have to stop reading on purpose. As you said, it's an undocumented behavior, but one of many on the Cryptography libs so we deal with it the best we can and if a fix is made in a future version, we'll have to change our code again :S.Morley
F
7

This exception is thrown during Dispose(true). Throwing from Dispose is already a design flaw (https://learn.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();
            }
        }
    }
}
Filiform answered 27/2, 2014 at 14:53 Comment(3)
You may also want to zero CryptoStream's private _InputBuffer and _OutputBuffer members as this exception may leave sensitive information in memory. Looking at referencesource.microsoft.com, this appears to no longer be an issue.Shanell
@WillKrause: Thanks! I've edited the answer to include this information, along with a link to the exact code that you're referring to. As a side note, I'm pretty sure the _InputBuffer / _OutputBuffer issue is mitigated in its severity by the fact that it's only exploitable by using reflection, which is restricted to fully-trusted code by default.Filiform
Note that, at least for read-mode streams, this appears to have been fixed for .NET Core 3.0 as of preview 4: github.com/dotnet/corefx/issues/7779 --> github.com/dotnet/corefx/pull/36048 --> github.com/dotnet/corefx/commit/f99562fFiliform
D
1

As I understand it, the exception is thrown when the last byte read is not a valid padding byte. When you intentionally close the stream early, the last byte read will most likely be considered "invalid padding" and the exception is thrown. Since you're ending intentionally, you should be safe ignoring the exception.

Dumpish answered 26/3, 2013 at 15:47 Comment(0)
P
1

Close calls Dispose(true) which calls FlushFinalBlock which throws the exception, because this is not really the final block.

You can prevent this by overriding the Close method so that it doesn't call FlushFinalBlock:

public class SilentCryptoStream : CryptoStream {
    public SilentCryptoStream(Stream stream, ICryptoTransform transform, CryptoStreamMode mode) :
        base(stream, transform, mode) {
    }

    public override void Close() {
        this.Dispose(false);
        GC.SuppressFinalize(this);
    }
}

(You also need to manually close the underlying stream.)

is it valid to check if reader.EndOfStream is false and then capture the CryptographicException

I think it's OK.

Pharmacology answered 12/2, 2014 at 2:35 Comment(26)
I'm not so sure capturing the exception is okay. Most likely nobody will be affected, but the point of Dispose is to release unmanaged resources. So unless the exception is thrown on the very last step after everything else has been released, it is likely that capturing it and ignoring it will lead to a resource or memory leak. Also, how do you know Dispose(false) will behave exactly as you say?Leeann
>"it is likely that capturing it and ignoring it will lead to a resource or memory leak" No. The CryptoStream is clever and wraps FlushFinalBlock and the base stream closing in a try block while other cleanup code is in the finally block. Also, CryptoStream doesn't own any unmanaged resources. It has nothing to leak.Pharmacology
>"Also, how do you know Dispose(false) will behave exactly as you say?" I've looked at the source code. The boolean disposing parameter only enables calling the FlushFinalBlock (if not yet flushed) and the base stream closing.Pharmacology
I checked the assembly with DotPeek, and calling Dispose(false) won't close the underlying stream. This is different from what you found, so it is likely that it varies according to implementation. Catching the exception is something I'd like to avoid, as the CryptoStream is one of many layered streams, and used in many places. I guess I could derive a new class and catch the exception there, but I'd rather prevent the exception going off in the first place.Leeann
>"calling Dispose(false) won't close the underlying stream." Yes. I've specifically wrote about that since the beginning. "This is different from what you found" No, this is the same. Re-read my answer - (You also need to manually close the underlying stream.) and my comment - The boolean disposing parameter only enables calling the FlushFinalBlock (if not yet flushed) and the base stream closing. Pharmacology
Okay. I was expecting somebody knew a way to cleanly close the CryptoStream though. Like, preventing the problem instead of hacking it away. I'm sorry I caused you so many inconveniences.Leeann
@PandaPajama That's what I've tried to do. I've researched the CryptoStream sources thoroughly. There is currently NO way to prevent it. It's not the matter of knowledge. If there was a way, I'd have found it.Pharmacology
Unfortunately neither of your two solutions are usable in a production environment. CryptoStream has a spec; overriding the Close functionality may introduce other problems, even if not in this particular case, maybe in a future version of that class, or one offered by a different vendor. It is interesting that you have researched it yourself, but I would prefer a categorical answer like your capitalized NO from an authoritative source, and that was the point of the bounty. It seemed nobody else cares about this, so whatever...Leeann
Catching the exception in this particular case seems future-safe and production-ok. Basically, you abort some operation, so it's natural that the exception is thrown and you're in your right to catch it in this situation. Catch it and check for the conditions.Pharmacology
There is a huge difference between "seems" and "is". This particular implementation of CryptoStream does not act according to specification and is therefore bugged. The specification doesn't even mention the possibility of such an exception. I brought this up because this doesn't throw an exception on the PSM Mono libraries, and I found this exception when porting back to the MS implementation.Leeann
It is also not production safe, because there may be (now or on the future, and on this or other implementation of that class) an exception that you may be inadvertently catching and ignoring with that hack.Leeann
"an exception that you may be inadvertently catching and ignoring with that hack" This cannot happen: I proposed to catch the exception ONLY in the explicit situation when you're basically aborting an operation (and you do know when you're doing that). You're not surprised when you get ThreadAbortException when you call thread.Abort(), do you? It's an exception. You should catch it and assess the situation to decide whether to re-throw it or continue. That's the purpose of the catch block.Pharmacology
PSM is awful. It's very old and outdated.Pharmacology
To repeat myself: you've done checking the relevant part of the stream. You don't care about any additional stream data (If you did care, you'd read it to end to check the encryption correctness). You're doing an abort operation which should throw the exception, but may not do that in some implementations. It's not exceptional - you're 100% expecting it. Catch it. try{ cryptoStreamReader.Close(); } catch() {}Pharmacology
msdn.microsoft.com/en-us/library/… "...calling the Close method. Doing so flushes the stream and causes all remain blocks of data to be processed by the CryptoStream object." msdn.microsoft.com/en-us/library/… "The AES algorithm is essentially the Rijndael"Pharmacology
msdn.microsoft.com/en-us/library/… "CryptographicException" Here it is referencesource-beta.microsoft.com/#mscorlib/system/security/… .Pharmacology
You seem to be missing my point. The problem here is not whether or not I can catch that exception. Of course I can. The problem is with the specification of Close(). It never mentions it can throw such an exception, therefore the situation in which that exception is thrown is something that was not thought of possible when the library was written. Relying on undefined behavior is very dangerous: other versions of the library may throw a different exception, crash the program, or format your hard disk. I need a documented way to prevent this problem from happening.Leeann
What's so awful and outdated about PSM? I find it to be very useful. Perhaps you may wish to enlighten me on what's so awful and outdated about it? Perhaps even propose an alternative? Notice that the buggy library here is the official Microsoft one, not the one in PSM.Leeann
Unfortunately there is not much left to discuss. The solution you propose, even though it makes the program run, is unacceptable for my purposes. If you have a solution that is indeed documented, post it as a new answer, and we'll discuss that, but this discussion will not go anywhere, and flooding the comments is not very constructive anyways.Leeann
"The problem is with the specification of Close(). It never mentions it can throw such an exception" Both Stream.Close and Stream.Dispose are virtual. Noone can control (and thus document) the behavior of the overrides. Calling a virtual method can result in any exception. In your case it's RjindaelManaged class that throws. And it's you, who "connect" them together (by passing the Aesmanaged to the CryptoStream's constructor).Pharmacology
"Relying on undefined behavior is very dangerous: other versions of the library may throw a different exception, crash the program, or format your hard disk." Yes, of course. Any IDisposable.Dispose() override may format your hard drive. The IDisposable.Dispose() documentation will never tell you that (as it's not known beforehead) or prevent it.Pharmacology
"What's so awful and outdated about PSM?" If by PSM you mean PlayStation Mobile Studio, then it's outdated, because it uses a very old version of Mono. Sony has never really updated it other than some small fixes.Pharmacology
"I need a documented way to prevent this problem from happening." What's the documented way to prevent the int a = 1 / 0; from throwing the DivisionByZeroException? What's the documented way to prevent the new Stack<int>().Pop() from throwing the InvalidOperationException? Answering these question will help you to answer your original question.Pharmacology
There is no mention of CryptoStream.Close (not Stream.Close) throwing a CryptographicException anywhere. "Undefined behavior" is not the same as "exception". Dividing by 0 is defined to throw an exception; Popping an empty stack is defined to throw an exception, and as long as it is defined that way, you can rely on the specification and know what you're going to get. CryptoStream (not Stream) is not defined to throw a CryptographicException. This is undefined behavior, and relying on it is dangerous.Leeann
I assume you have extensively used PSM in order to claim that it is awful, so I suppose you use an alternative that uses a much more recent version of Mono (support for c# 4 is plenty recent in my book though), which, for the purposes of this thread, does throw an exception in this case. Care to share it? No? Then what was the purpose of that comment?Leeann
let us continue this discussion in chatLeeann
R
0

Can you turn off padding?

// aes.Padding = PaddingMode.PKCS7;
aes.Padding = PaddingMode.None;
Raker answered 26/3, 2013 at 15:46 Comment(5)
I tried turning padding off, but if I do so, AES encryption will fail. As I understand AES needs the padding.Morley
@Morley - It makes your test program run correctly without exceptions.Raker
You are right, but when I check the output there is garbage on it :S, try replacing the while and write to a file: File.WriteAllText("c:\\test_decrypted", reader.ReadToEnd());. If I comment the line cryptoStream.FlushFinalBlock(); the error I get is the exception I mentioned before when closing before EOF.Morley
I tried it and got some corruption at the end of the file. I removed cryptoStream.FlushFinalBlock() and with padding set to none, it worked with no exceptions and no corruption.Raker
This is really weird, if I do that, I get this exception: "Length of the data to encrypt is invalid." when encrypting so the file is not generated :S.Morley
M
0

My solution was to, in my derived class, add this to my Dispose(bool) override:

    protected override void Dispose(bool disposing)
    {
        // CryptoStream.Dispose(bool) has a bug in read mode. If the reader doesn't read all the way to the end of the stream, it throws an exception while trying to
        // read the final block during Dispose(). We'll work around this here by moving to the end of the stream for them. This avoids the thrown exception and
        // allows everything to be cleaned up (disposed, wiped from memory, etc.) properly.
        if ((disposing) &&
            (CanRead) &&
            (m_TransformMode == CryptoStreamMode.Read))
        {
            const int BUFFER_SIZE = 32768;
            byte[] buffer = new byte[BUFFER_SIZE];

            while (Read(buffer, 0, BUFFER_SIZE) == BUFFER_SIZE)
            {
            }
        }

        base.Dispose(disposing);
        ...

By making sure the stream is always read to the end, the internal issue in the CryptStream.Dispose is avoided. Of course, you need to weigh this against the nature of what you are reading, to be sure it doesn't have a negative impact. Only use it against a source of a known finite length.

Mm answered 6/7, 2015 at 14:22 Comment(0)

© 2022 - 2024 — McMap. All rights reserved.