CryptoStream Memory-based ReadAsync/WriteAsync overrides #47207

NewellClark · 2021-01-20T03:39:24Z

While working on Override Stream ReadAsync/WriteAsync, I found that CryptoStream is missing the memory-based async overrides.

I was able to implement the memory-based ReadAsync without issue. However, WriteAsync has a problem because it calls into TransformBlock on ICryptoTransform, which is a public interface. Ouch.

I went ahead and implemented WriteAsync, and where calls to ICryptoTransform.TransformBlock were made, I check if the underlying ReadOnlyMemory is actually an array skip the copy when it is.

I don't suppose it's feasible to add span-based overloads to ICryptoTransform as default interface implementations? I'd be willing to write up the API proposal in the case that it is.

dotnet-issue-labeler · 2021-01-20T03:39:31Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2021-01-20T03:39:33Z

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq
See info in area-owners.md if you want to be subscribed.

Issue Details

While working on Override Stream ReadAsync/WriteAsync, I found that CryptoStream is missing the memory-based async overrides.

I was able to implement the memory-based ReadAsync without issue. However, WriteAsync has a problem because it calls into TransformBlock on ICryptoTransform, which is a public interface. Ouch.

I went ahead and implemented WriteAsync, and where calls to ICryptoTransform.TransformBlock were made, I check if the underlying ReadOnlyMemory is actually an array skip the copy when it is.

I don't suppose it's feasible to add span-based overloads to ICryptoTransform as default interface implementations? I'd be willing to write up the API proposal in the case that it is.

Author:	NewellClark
Assignees:	-
Labels:	`area-System.Security`, `new-api-needs-documentation`
Milestone:	-

vcsjones · 2021-01-20T04:02:51Z

I don't suppose it's feasible to add span-based overloads to ICryptoTransform as default interface implementations?

This was discussed in #38764 and was ultimately determined DIMs on this interface were too risky.

vcsjones · 2021-01-20T04:18:58Z

...ies/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs

+                    var rentedBuffer = ArrayPool<byte>.Shared.Rent(inputBuffer.Length);
+                    inputBuffer.CopyTo(rentedBuffer);
+                    int result = transform.TransformBlock(rentedBuffer, 0, inputBuffer.Length, outputBuffer, outputOffset);
+                    ArrayPool<byte>.Shared.Return(rentedBuffer);


The rentedBuffer 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 of CryptographicOperations.ZeroMemory in this class for an example.

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.

GrabYourPitchforks · 2021-01-20T04:54:40Z

...ies/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs

@@ -202,7 +203,7 @@ public override void SetLength(long value)
        public override Task<int> ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)
        {
            CheckReadArguments(buffer, offset, count);
-            return ReadAsyncInternal(buffer, offset, count, cancellationToken);
+            return ReadAsync(buffer.AsMemory(offset, count), cancellationToken).AsTask();


Possible breaking change: if somebody subclasses CryptoStream and overrides ReadAsync(Memory<byte>, ...) as a wrapper around ReadAsync(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" 😄

GrabYourPitchforks · 2021-01-20T05:03:28Z

...ies/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs

+
+                        return result;
+                    }
+                    catch


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.

...ies/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs

stephentoub · 2021-01-20T14:08:44Z

...ies/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs

@@ -202,7 +203,7 @@ public override void SetLength(long value)
        public override Task<int> ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)
        {
            CheckReadArguments(buffer, offset, count);
-            return ReadAsyncInternal(buffer, offset, count, cancellationToken);
+            return ReadAsync(buffer.AsMemory(offset, count), cancellationToken).AsTask();


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.

stephentoub · 2021-01-20T14:09:26Z

...ies/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs

+                    var rentedBuffer = ArrayPool<byte>.Shared.Rent(inputBuffer.Length);
+                    inputBuffer.CopyTo(rentedBuffer);
+                    int result = transform.TransformBlock(rentedBuffer, 0, inputBuffer.Length, outputBuffer, outputOffset);
+                    ArrayPool<byte>.Shared.Return(rentedBuffer);


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.

…em/Security/Cryptography/CryptoStream.cs Use explicit type Co-authored-by: Stephen Toub <stoub@microsoft.com>

Unintentional breaking change with exceptions that was blocking CI. Note to self: remember to rebuild before running unit tests.

Still need to override CopyTo/CopyToAsync. I just want to make sure that CI will pass with changes so far.

bartonjs · 2021-01-21T16:16:16Z

...ies/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs

@@ -544,8 +565,8 @@ private async ValueTask WriteAsyncCore(byte[] buffer, int offset, int count, Can
                {
                    // not enough to transform a block, so just copy the bytes into the _InputBuffer
                    // and return
-                    Buffer.BlockCopy(buffer, offset, _inputBuffer, _inputBufferIndex, count);
-                    _inputBufferIndex += count;
+                    buffer.Slice(0, buffer.Length).CopyTo(_inputBuffer.AsMemory(_inputBufferIndex));


Suggested change

buffer.Slice(0, buffer.Length).CopyTo(_inputBuffer.AsMemory(_inputBufferIndex));

buffer.CopyTo(_inputBuffer.AsMemory(_inputBufferIndex));

bartonjs · 2021-01-21T16:20:42Z

...ies/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs

+                }
+                else
+                {
+                    var rentedBuffer = ArrayPool<byte>.Shared.Rent(inputBuffer.Length);


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

Suggested change

var rentedBuffer = ArrayPool<byte>.Shared.Rent(inputBuffer.Length);

// Use ArrayPool.Shared instead of CryptoPool because the array is passed out.

byte[] rentedBuffer = ArrayPool<byte>.Shared.Rent(inputBuffer.Length);

bartonjs · 2021-01-21T16:23:04Z

...ies/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs

+                    }
+                    finally
+                    {
+                        CryptographicOperations.ZeroMemory(rentedBuffer.AsSpan(0, inputBuffer.Length));


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.

@bartonjs

- Applied fixes suggested by @bartonjs - Added overrides for CopyTo/CopyToAsync to ensure any temporary buffers are properly cleared.

NewellClark · 2021-01-21T17:20:33Z

@bartonjs I've applied the changes you've suggested
@stephentoub I've implemented overrides of CopyTo and CopyToAsync to ensure temporary buffers are zeroed out.

Quick question: when I apply a change that a reviewer suggested, should I click "resolve conversation", or wait for them to do it?

NewellClark · 2021-01-21T21:34:46Z

Just hit issue #32805. Rerunning.

...ies/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs

…em/Security/Cryptography/CryptoStream.cs

...ies/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs

...aries/System.Security.Cryptography.Primitives/ref/System.Security.Cryptography.Primitives.cs

…em.Security.Cryptography.Primitives.cs

NewellClark added 2 commits January 19, 2021 18:36

Memorify CryptoStream.ReadAsync

c9e983f

Memorify CryptoStream.WriteAsync

b8b0bef

dotnet-issue-labeler bot added area-System.Security new-api-needs-documentation labels Jan 20, 2021

vcsjones reviewed Jan 20, 2021

View reviewed changes

Zeroed memory in rented array

011bfd6

GrabYourPitchforks reviewed Jan 20, 2021

View reviewed changes

stephentoub reviewed Jan 20, 2021

View reviewed changes

NewellClark and others added 3 commits January 20, 2021 10:28

Update src/libraries/System.Security.Cryptography.Primitives/src/Syst…

88f61f9

…em/Security/Cryptography/CryptoStream.cs Use explicit type Co-authored-by: Stephen Toub <stoub@microsoft.com>

Fix exception behavior

259a3b3

Unintentional breaking change with exceptions that was blocking CI. Note to self: remember to rebuild before running unit tests.

[no merge] Pinned array, used finally block

c31f85f

Still need to override CopyTo/CopyToAsync. I just want to make sure that CI will pass with changes so far.

NewellClark mentioned this pull request Jan 21, 2021

Override Stream.ReadAsync/WriteAsync #33789

Closed

bartonjs reviewed Jan 21, 2021

View reviewed changes

Added CopyTo/CopyToAsync overrides

5eebaf6

- Applied fixes suggested by @bartonjs - Added overrides for CopyTo/CopyToAsync to ensure any temporary buffers are properly cleared.

This was referenced Jan 21, 2021

Override Stream ReadAsync/WriteAsync Analyzer dotnet/roslyn-analyzers#4725

Closed

(2/4) Override Stream ReadAsync/WriteAsync Analyzer dotnet/roslyn-analyzers#4726

Merged

NewellClark added 2 commits January 21, 2021 15:25

Merge remote-tracking branch 'dotnet/master' into memorify-CryptoStream

5a81527

Merge remote-tracking branch 'dotnet/master' into memorify-CryptoStream

4046040

Use fully-qualified name in ref

813a1c0

runfoapp bot mentioned this pull request Jan 21, 2021

SSL error when downloading the dotnet install script #46309

Closed

Merge remote-tracking branch 'dotnet/master' into memorify-CryptoStream

bdf4778

NewellClark mentioned this pull request Jan 24, 2021

Memorify DeflateManagedStream #47389

Closed

runfoapp bot mentioned this pull request Jan 28, 2021

nameresolutionpaltests.trygetaddrinfo_externalhost failing on Android #47591

Closed

bartonjs reviewed Feb 22, 2021

View reviewed changes

...ies/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs Show resolved Hide resolved

Update src/libraries/System.Security.Cryptography.Primitives/src/Syst…

9b08e51

…em/Security/Cryptography/CryptoStream.cs

bartonjs reviewed Feb 22, 2021

View reviewed changes

...ies/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs Outdated Show resolved Hide resolved

Undo bad edit

79b086e

bartonjs reviewed Feb 22, 2021

View reviewed changes

...aries/System.Security.Cryptography.Primitives/ref/System.Security.Cryptography.Primitives.cs Outdated Show resolved Hide resolved

Update src/libraries/System.Security.Cryptography.Primitives/ref/Syst…

f506f37

…em.Security.Cryptography.Primitives.cs

bartonjs approved these changes Feb 26, 2021

View reviewed changes

bartonjs merged commit a7669a7 into dotnet:master Feb 26, 2021

dotnet locked as resolved and limited conversation to collaborators Mar 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CryptoStream Memory-based ReadAsync/WriteAsync overrides #47207

CryptoStream Memory-based ReadAsync/WriteAsync overrides #47207

NewellClark commented Jan 20, 2021

dotnet-issue-labeler bot commented Jan 20, 2021

ghost commented Jan 20, 2021

vcsjones commented Jan 20, 2021

vcsjones Jan 20, 2021

NewellClark Jan 20, 2021

stephentoub Jan 20, 2021

NewellClark Jan 20, 2021

GrabYourPitchforks Jan 20, 2021

stephentoub Jan 20, 2021

NewellClark Jan 20, 2021 •

edited

stephentoub Jan 20, 2021 •

edited

GrabYourPitchforks Jan 20, 2021

stephentoub Jan 20, 2021

stephentoub Jan 20, 2021

bartonjs Jan 21, 2021

bartonjs Jan 21, 2021

bartonjs Jan 21, 2021

NewellClark commented Jan 21, 2021 •

edited

NewellClark commented Jan 21, 2021

	buffer.Slice(0, buffer.Length).CopyTo(_inputBuffer.AsMemory(_inputBufferIndex));
	buffer.CopyTo(_inputBuffer.AsMemory(_inputBufferIndex));

	var rentedBuffer = ArrayPool<byte>.Shared.Rent(inputBuffer.Length);
	// Use ArrayPool.Shared instead of CryptoPool because the array is passed out.
	byte[] rentedBuffer = ArrayPool<byte>.Shared.Rent(inputBuffer.Length);

CryptoStream Memory-based ReadAsync/WriteAsync overrides #47207

CryptoStream Memory-based ReadAsync/WriteAsync overrides #47207

Conversation

NewellClark commented Jan 20, 2021

dotnet-issue-labeler bot commented Jan 20, 2021

ghost commented Jan 20, 2021

vcsjones commented Jan 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NewellClark Jan 20, 2021 • edited

Choose a reason for hiding this comment

stephentoub Jan 20, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NewellClark commented Jan 21, 2021 • edited

NewellClark commented Jan 21, 2021

NewellClark Jan 20, 2021 •

edited

stephentoub Jan 20, 2021 •

edited

NewellClark commented Jan 21, 2021 •

edited