TherentedBuffer contains sensitive data and is being returned to a shared pool. We should clear the array before returning it to the pool so that when some other API rents it, it isn’t populated with the sensitive data. See other usages ofCryptographicOperations.ZeroMemory in this class for an example.

Fixed.

If this is a real concern, CryptoStream should probably override CopyTo/CopyToAsync as well, to clear the temporary buffer that might be used as part of the copy.

I can do that. I'll implement all suggested changes once I figure out why it's failing CI. It passed when I ran the tests on my machine. I must be doing something wrong.

Possible breaking change: if somebody subclassesCryptoStream and overridesReadAsync(Memory<byte>, ...) as a wrapper aroundReadAsync(byte[], ...), the code will now stack overflow. I don't know if anybody is likely to have done this in practice. But it's a potential hazard of having one virtual method begin to dispatch to a different already-existing virtual method.

@bartonjs is there a pattern for handling this?

If we're concerned about that, both overloads of ReadAsync can delegate to a non-virtual ReadAsyncCore.

I've never seen a type derived from CryptoStream, though. I'm sure someone somewhere has done it, but I don't think we need to be too concerned (famous last words). If we were actually concerned, we'd potentially want to take it a step further and have the ReadAsync memory overload check whether the type was derived or not, delegating to the base implementation if it was, in case someone had overridden ReadAsync(byte[], ...) to do something special, in which case it would be a breaking change for ReadAsync(Memory, ...) to not use it.

Would we want to do reflection to see if the derived type has actually overridden ReadAsync(byte[]...) so we can possibly skip the memory copy?

Would we want to do reflection

The answer to that question is pretty much always "no" 😄

The catch block here is unnecessary. Instead, consider restructuring the outer try block as the following pseudocode:

byte[]rented=ArrayPool.Rent();try{input.CopyTo(rented);Transform(from:rented,to:output);}finally{ZeroMem(rented);}ArrayPool.Return(rented);

By putting a finally block around the code where the rented buffer contains potentially sensitive data, we can ensure that the whole thing is zeroed out whether the operation completes successfully or fails.

We could also consider pinning the temporary buffer as part of this operation to provide further protection against copies of the data being made.

Note to other reviewers: Should we skip the array pool entirely and instead use pre-pinned arrays? If an adversary can force the crypto transform to fail, they can force the application to abandon a bunch of Gen2 arrays, which could lead to perf degradation.

If we're concerned about that, both overloads of ReadAsync can delegate to a non-virtual ReadAsyncCore.

I've never seen a type derived from CryptoStream, though. I'm sure someone somewhere has done it, but I don't think we need to be too concerned (famous last words). If we were actually concerned, we'd potentially want to take it a step further and have the ReadAsync memory overload check whether the type was derived or not, delegating to the base implementation if it was, in case someone had overridden ReadAsync(byte[], ...) to do something special, in which case it would be a breaking change for ReadAsync(Memory, ...) to not use it.

If this is a real concern, CryptoStream should probably override CopyTo/CopyToAsync as well, to clear the temporary buffer that might be used as part of the copy.

var shouldn't be used here, and please add the comment explaining why this isn't using CryptoPool:

If the array is being pinned for the operation that's presumably so that it can be populated and cleared without GC compaction applying, so I'd expect the clear to be inside the pin.