runtime: CryptoStream - crash when bytesToDeliver=amountRead

Created based on Connect feedback: http://connect.microsoft.com/VisualStudio/feedback/details/2604876/bug-in-system-security-cryptography-cryptostream (internal bug 213692)

Description: A case is not handled in the Read method where bytesToDeliver equals amountRead exactly - the ProcessFinalBlock label is never invoked and _finalBlockTransformed is not set to true. This means that a read operation that falls into this case will call FlushFinalBlock on Dispose - which throws an exception in the proprietary stream I’m using for reading. A Flush is never expected on stream used for reading only. The last thing that needs to be checked in the if (bytesToDeliver >= numOutputBytes) block is whether bytesToDeliver has gone to zero, in which case we should go to the ProcessFinalBlock label code. I’ve attached a solution to reproduce and included a copy of CryptoStream that with a one line fix.

Repro Steps: Basically, attempt to read (decrypt) a stream that falls into the “slow” label and into the if (bytesToDeliver >= numOutputBytes) block with bytesToDeliver == numOutputBytes.

Impact: For me, I get intermittent crashes when I read a stream of a particular length that just happens to fall into this case - Flush is called on a stream that accepts no write operations.

About this issue

Original URL
State: closed
Created 7 years ago
Comments: 30 (25 by maintainers)

Most upvoted comments

You can’t, but I can 😉 … I’ll take your comment as ‘go ahead’ signal …

karelz on May 3, 2017

I am thinking that it is actually wrong for a Stream derived class to throw if Flush is called, even if it doesn’t support writing. Some points to corroborate that:

Here it mentions the methods that throw NotImplementedException if a Stream derived type does not support writing, but, Flush is not listed there.
FileStream gives an example of usage of Flush when reading, using that as basis I can see some behavior in a derived type of Stream that supports only reading and can use Flush to reset some internal state.

With this in mind I think that we should not change CryptoStream.

@tah9m9 let’s hear from the experts to decide what to do here, but, perhaps you need to wrap the proprietary stream with a type that just ignores Flush and forward the other methods (I’m guessing that you already did that…)

@ianhays and @karelz let me know if you disagree with the rational above. If you disagree I’ll take a closer look to fully understand the implications of such “fix”.

pjanotti on May 3, 2017