From c9e983f21b1f5a0660f17bd641fefeb1cf47bc79 Mon Sep 17 00:00:00 2001 From: NewellClark Date: Tue, 19 Jan 2021 18:36:14 -0500 Subject: [PATCH 01/11] Memorify CryptoStream.ReadAsync --- .../Security/Cryptography/CryptoStream.cs | 49 ++++++++++--------- 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs b/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs index ff5003d7bc78a4..076b534c1528be 100644 --- a/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs +++ b/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs @@ -202,7 +202,7 @@ public override void SetLength(long value) public override Task 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(); } public override IAsyncResult BeginRead(byte[] buffer, int offset, int count, AsyncCallback? callback, object? state) => @@ -211,7 +211,7 @@ public override IAsyncResult BeginRead(byte[] buffer, int offset, int count, Asy public override int EndRead(IAsyncResult asyncResult) => TaskToApm.End(asyncResult); - private async Task ReadAsyncInternal(byte[] buffer, int offset, int count, CancellationToken cancellationToken) + public override async ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) { // To avoid a race with a stream's position pointer & generating race // conditions with internal buffer indexes in our own streams that @@ -222,7 +222,7 @@ private async Task ReadAsyncInternal(byte[] buffer, int offset, int count, await AsyncActiveSemaphore.WaitAsync(cancellationToken).ConfigureAwait(false); try { - return await ReadAsyncCore(buffer, offset, count, cancellationToken, useAsync: true).ConfigureAwait(false); + return await ReadAsyncCore(buffer, cancellationToken, useAsync: true).ConfigureAwait(false); } finally { @@ -268,7 +268,10 @@ public override void WriteByte(byte value) public override int Read(byte[] buffer, int offset, int count) { CheckReadArguments(buffer, offset, count); - return ReadAsyncCore(buffer, offset, count, default(CancellationToken), useAsync: false).GetAwaiter().GetResult(); + var completedValueTask = ReadAsyncCore(buffer.AsMemory(offset, count), default(CancellationToken), useAsync: false); + Debug.Assert(completedValueTask.IsCompleted); + + return completedValueTask.GetAwaiter().GetResult(); } private void CheckReadArguments(byte[] buffer, int offset, int count) @@ -278,22 +281,22 @@ private void CheckReadArguments(byte[] buffer, int offset, int count) throw new NotSupportedException(SR.NotSupported_UnreadableStream); } - private async Task ReadAsyncCore(byte[] buffer, int offset, int count, CancellationToken cancellationToken, bool useAsync) + private async ValueTask ReadAsyncCore(Memory buffer, CancellationToken cancellationToken, bool useAsync) { // read <= count bytes from the input stream, transforming as we go. // Basic idea: first we deliver any bytes we already have in the // _OutputBuffer, because we know they're good. Then, if asked to deliver // more bytes, we read & transform a block at a time until either there are // no bytes ready or we've delivered enough. - int bytesToDeliver = count; - int currentOutputIndex = offset; + int bytesToDeliver = buffer.Length; + int currentOutputIndex = 0; Debug.Assert(_outputBuffer != null); if (_outputBufferIndex != 0) { // we have some already-transformed bytes in the output buffer - if (_outputBufferIndex <= count) + if (_outputBufferIndex <= buffer.Length) { - Buffer.BlockCopy(_outputBuffer, 0, buffer, offset, _outputBufferIndex); + _outputBuffer.AsSpan(0, _outputBufferIndex).CopyTo(buffer.Span); bytesToDeliver -= _outputBufferIndex; currentOutputIndex += _outputBufferIndex; int toClear = _outputBuffer.Length - _outputBufferIndex; @@ -302,14 +305,14 @@ private async Task ReadAsyncCore(byte[] buffer, int offset, int count, Canc } else { - Buffer.BlockCopy(_outputBuffer, 0, buffer, offset, count); - Buffer.BlockCopy(_outputBuffer, count, _outputBuffer, 0, _outputBufferIndex - count); - _outputBufferIndex -= count; + _outputBuffer.AsSpan(0, buffer.Length).CopyTo(buffer.Span); + Buffer.BlockCopy(_outputBuffer, buffer.Length, _outputBuffer, 0, _outputBufferIndex - buffer.Length); + _outputBufferIndex -= buffer.Length; int toClear = _outputBuffer.Length - _outputBufferIndex; CryptographicOperations.ZeroMemory(new Span(_outputBuffer, _outputBufferIndex, toClear)); - return (count); + return buffer.Length; } } // _finalBlockTransformed == true implies we're at the end of the input stream @@ -319,7 +322,7 @@ private async Task ReadAsyncCore(byte[] buffer, int offset, int count, Canc // eventually, we'll just always return 0 here because there's no more to read if (_finalBlockTransformed) { - return (count - bytesToDeliver); + return buffer.Length - bytesToDeliver; } // ok, now loop until we've delivered enough or there's nothing available int amountRead = 0; @@ -373,7 +376,7 @@ await _stream.ReadAsync(new Memory(tempInputBuffer, _inputBufferIndex, num // Use ArrayPool.Shared instead of CryptoPool because the array is passed out. tempOutputBuffer = ArrayPool.Shared.Rent(numWholeReadBlocks * _outputBlockSize); numOutputBytes = _transform.TransformBlock(tempInputBuffer, 0, numWholeReadBlocksInBytes, tempOutputBuffer, 0); - Buffer.BlockCopy(tempOutputBuffer, 0, buffer, currentOutputIndex, numOutputBytes); + tempOutputBuffer.AsSpan(0, numOutputBytes).CopyTo(buffer.Span.Slice(currentOutputIndex)); // Clear what was written while we know how much that was CryptographicOperations.ZeroMemory(new Span(tempOutputBuffer, 0, numOutputBytes)); @@ -429,22 +432,22 @@ await _stream.ReadAsync(new Memory(_inputBuffer, _inputBufferIndex, _input if (bytesToDeliver >= numOutputBytes) { - Buffer.BlockCopy(_outputBuffer, 0, buffer, currentOutputIndex, numOutputBytes); + _outputBuffer.AsSpan(0, numOutputBytes).CopyTo(buffer.Span.Slice(currentOutputIndex)); CryptographicOperations.ZeroMemory(new Span(_outputBuffer, 0, numOutputBytes)); currentOutputIndex += numOutputBytes; bytesToDeliver -= numOutputBytes; } else { - Buffer.BlockCopy(_outputBuffer, 0, buffer, currentOutputIndex, bytesToDeliver); + _outputBuffer.AsSpan(0, bytesToDeliver).CopyTo(buffer.Span.Slice(currentOutputIndex)); _outputBufferIndex = numOutputBytes - bytesToDeliver; Buffer.BlockCopy(_outputBuffer, bytesToDeliver, _outputBuffer, 0, _outputBufferIndex); int toClear = _outputBuffer.Length - _outputBufferIndex; CryptographicOperations.ZeroMemory(new Span(_outputBuffer, _outputBufferIndex, toClear)); - return count; + return buffer.Length; } } - return count; + return buffer.Length; ProcessFinalBlock: // if so, then call TransformFinalBlock to get whatever is left @@ -458,20 +461,20 @@ await _stream.ReadAsync(new Memory(_inputBuffer, _inputBufferIndex, _input // now, return either everything we just got or just what's asked for, whichever is smaller if (bytesToDeliver < _outputBufferIndex) { - Buffer.BlockCopy(_outputBuffer, 0, buffer, currentOutputIndex, bytesToDeliver); + _outputBuffer.AsSpan(0, bytesToDeliver).CopyTo(buffer.Span.Slice(currentOutputIndex)); _outputBufferIndex -= bytesToDeliver; Buffer.BlockCopy(_outputBuffer, bytesToDeliver, _outputBuffer, 0, _outputBufferIndex); int toClear = _outputBuffer.Length - _outputBufferIndex; CryptographicOperations.ZeroMemory(new Span(_outputBuffer, _outputBufferIndex, toClear)); - return (count); + return buffer.Length; } else { - Buffer.BlockCopy(_outputBuffer, 0, buffer, currentOutputIndex, _outputBufferIndex); + _outputBuffer.AsSpan(0, _outputBufferIndex).CopyTo(buffer.Span.Slice(currentOutputIndex)); bytesToDeliver -= _outputBufferIndex; _outputBufferIndex = 0; CryptographicOperations.ZeroMemory(_outputBuffer); - return (count - bytesToDeliver); + return buffer.Length - bytesToDeliver; } } From b8b0bef97a0480aa10b761d1c791a6297d0f019c Mon Sep 17 00:00:00 2001 From: NewellClark Date: Tue, 19 Jan 2021 22:13:19 -0500 Subject: [PATCH 02/11] Memorify CryptoStream.WriteAsync --- ...System.Security.Cryptography.Primitives.cs | 2 + .../Security/Cryptography/CryptoStream.cs | 51 +++++++++++++------ 2 files changed, 38 insertions(+), 15 deletions(-) diff --git a/src/libraries/System.Security.Cryptography.Primitives/ref/System.Security.Cryptography.Primitives.cs b/src/libraries/System.Security.Cryptography.Primitives/ref/System.Security.Cryptography.Primitives.cs index dbe106511bcdbf..7876f89c42e703 100644 --- a/src/libraries/System.Security.Cryptography.Primitives/ref/System.Security.Cryptography.Primitives.cs +++ b/src/libraries/System.Security.Cryptography.Primitives/ref/System.Security.Cryptography.Primitives.cs @@ -85,11 +85,13 @@ public void FlushFinalBlock() { } public System.Threading.Tasks.ValueTask FlushFinalBlockAsync(System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } public override int Read(byte[] buffer, int offset, int count) { throw null; } public override System.Threading.Tasks.Task ReadAsync(byte[] buffer, int offset, int count, System.Threading.CancellationToken cancellationToken) { throw null; } + public override System.Threading.Tasks.ValueTask ReadAsync(System.Memory buffer, System.Threading.CancellationToken cancellationToken = default) { throw null; } public override int ReadByte() { throw null; } public override long Seek(long offset, System.IO.SeekOrigin origin) { throw null; } public override void SetLength(long value) { } public override void Write(byte[] buffer, int offset, int count) { } public override System.Threading.Tasks.Task WriteAsync(byte[] buffer, int offset, int count, System.Threading.CancellationToken cancellationToken) { throw null; } + public override System.Threading.Tasks.ValueTask WriteAsync(System.ReadOnlyMemory buffer, System.Threading.CancellationToken cancellationToken = default) { throw null; } public override void WriteByte(byte value) { } } public enum CryptoStreamMode diff --git a/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs b/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs index 076b534c1528be..4c90b29a1bc7c6 100644 --- a/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs +++ b/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs @@ -5,6 +5,7 @@ using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.IO; +using System.Runtime.InteropServices; using System.Threading; using System.Threading.Tasks; @@ -211,6 +212,7 @@ public override IAsyncResult BeginRead(byte[] buffer, int offset, int count, Asy public override int EndRead(IAsyncResult asyncResult) => TaskToApm.End(asyncResult); + /// public override async ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) { // To avoid a race with a stream's position pointer & generating race @@ -481,7 +483,7 @@ await _stream.ReadAsync(new Memory(_inputBuffer, _inputBufferIndex, _input public override Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) { CheckWriteArguments(buffer, offset, count); - return WriteAsyncInternal(buffer, offset, count, cancellationToken); + return WriteAsync(buffer.AsMemory(offset, count), cancellationToken).AsTask(); } public override IAsyncResult BeginWrite(byte[] buffer, int offset, int count, AsyncCallback? callback, object? state) => @@ -490,7 +492,8 @@ public override IAsyncResult BeginWrite(byte[] buffer, int offset, int count, As public override void EndWrite(IAsyncResult asyncResult) => TaskToApm.End(asyncResult); - private async Task WriteAsyncInternal(byte[] buffer, int offset, int count, CancellationToken cancellationToken) + /// + public override async ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationToken cancellationToken = default) { // To avoid a race with a stream's position pointer & generating race // conditions with internal buffer indexes in our own streams that @@ -501,7 +504,7 @@ private async Task WriteAsyncInternal(byte[] buffer, int offset, int count, Canc await AsyncActiveSemaphore.WaitAsync(cancellationToken).ConfigureAwait(false); try { - await WriteAsyncCore(buffer, offset, count, cancellationToken, useAsync: true).ConfigureAwait(false); + await WriteAsyncCore(buffer, cancellationToken, useAsync: true).ConfigureAwait(false); } finally { @@ -512,7 +515,7 @@ private async Task WriteAsyncInternal(byte[] buffer, int offset, int count, Canc public override void Write(byte[] buffer, int offset, int count) { CheckWriteArguments(buffer, offset, count); - WriteAsyncCore(buffer, offset, count, default(CancellationToken), useAsync: false).AsTask().GetAwaiter().GetResult(); + WriteAsyncCore(buffer.AsMemory(offset, count), default, useAsync: false).AsTask().GetAwaiter().GetResult(); } private void CheckWriteArguments(byte[] buffer, int offset, int count) @@ -522,22 +525,22 @@ private void CheckWriteArguments(byte[] buffer, int offset, int count) throw new NotSupportedException(SR.NotSupported_UnwritableStream); } - private async ValueTask WriteAsyncCore(byte[] buffer, int offset, int count, CancellationToken cancellationToken, bool useAsync) + private async ValueTask WriteAsyncCore(ReadOnlyMemory buffer, CancellationToken cancellationToken, bool useAsync) { // write <= count bytes to the output stream, transforming as we go. // Basic idea: using bytes in the _InputBuffer first, make whole blocks, // transform them, and write them out. Cache any remaining bytes in the _InputBuffer. - int bytesToWrite = count; - int currentInputIndex = offset; + int bytesToWrite = buffer.Length; + int currentInputIndex = 0; // if we have some bytes in the _InputBuffer, we have to deal with those first, // so let's try to make an entire block out of it if (_inputBufferIndex > 0) { Debug.Assert(_inputBuffer != null); - if (count >= _inputBlockSize - _inputBufferIndex) + if (buffer.Length >= _inputBlockSize - _inputBufferIndex) { // we have enough to transform at least a block, so fill the input block - Buffer.BlockCopy(buffer, offset, _inputBuffer, _inputBufferIndex, _inputBlockSize - _inputBufferIndex); + buffer.Slice(0, _inputBlockSize - _inputBufferIndex).CopyTo(_inputBuffer.AsMemory(_inputBufferIndex)); currentInputIndex += (_inputBlockSize - _inputBufferIndex); bytesToWrite -= (_inputBlockSize - _inputBufferIndex); _inputBufferIndex = _inputBlockSize; @@ -547,8 +550,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)); + _inputBufferIndex += buffer.Length; return; } } @@ -588,8 +591,7 @@ private async ValueTask WriteAsyncCore(byte[] buffer, int offset, int count, Can try { - numOutputBytes = - _transform.TransformBlock(buffer, currentInputIndex, numWholeBlocksInBytes, tempOutputBuffer, 0); + numOutputBytes = TransformBlock(_transform, buffer.Slice(currentInputIndex, numWholeBlocksInBytes), tempOutputBuffer, 0); if (useAsync) { @@ -617,7 +619,7 @@ private async ValueTask WriteAsyncCore(byte[] buffer, int offset, int count, Can { Debug.Assert(_outputBuffer != null); // do it the slow way - numOutputBytes = _transform.TransformBlock(buffer, currentInputIndex, _inputBlockSize, _outputBuffer, 0); + numOutputBytes = TransformBlock(_transform, buffer.Slice(currentInputIndex, _inputBlockSize), _outputBuffer, 0); if (useAsync) await _stream.WriteAsync(new ReadOnlyMemory(_outputBuffer, 0, numOutputBytes), cancellationToken).ConfigureAwait(false); @@ -633,12 +635,31 @@ private async ValueTask WriteAsyncCore(byte[] buffer, int offset, int count, Can Debug.Assert(_inputBuffer != null); // In this case, we don't have an entire block's worth left, so store it up in the // input buffer, which by now must be empty. - Buffer.BlockCopy(buffer, currentInputIndex, _inputBuffer, 0, bytesToWrite); + buffer.Slice(currentInputIndex, bytesToWrite).CopyTo(_inputBuffer); _inputBufferIndex += bytesToWrite; return; } } return; + + static int TransformBlock(ICryptoTransform transform, ReadOnlyMemory inputBuffer, byte[] outputBuffer, int outputOffset) + { + if (MemoryMarshal.TryGetArray(inputBuffer, out ArraySegment segment)) + { + // Skip the copy if readonlymemory is actually an array. + Debug.Assert(segment.Array is not null); + return transform.TransformBlock(segment.Array, segment.Offset, inputBuffer.Length, outputBuffer, outputOffset); + } + else + { + var rentedBuffer = ArrayPool.Shared.Rent(inputBuffer.Length); + inputBuffer.CopyTo(rentedBuffer); + int result = transform.TransformBlock(rentedBuffer, 0, inputBuffer.Length, outputBuffer, outputOffset); + ArrayPool.Shared.Return(rentedBuffer); + + return result; + } + } } public void Clear() From 011bfd67845226151d09a6f7380be5751c6d62a2 Mon Sep 17 00:00:00 2001 From: NewellClark Date: Tue, 19 Jan 2021 23:41:00 -0500 Subject: [PATCH 03/11] Zeroed memory in rented array --- .../Security/Cryptography/CryptoStream.cs | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs b/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs index 4c90b29a1bc7c6..f9b82550fe7ddc 100644 --- a/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs +++ b/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs @@ -653,11 +653,23 @@ static int TransformBlock(ICryptoTransform transform, ReadOnlyMemory input else { var rentedBuffer = ArrayPool.Shared.Rent(inputBuffer.Length); - inputBuffer.CopyTo(rentedBuffer); - int result = transform.TransformBlock(rentedBuffer, 0, inputBuffer.Length, outputBuffer, outputOffset); - ArrayPool.Shared.Return(rentedBuffer); + try + { + inputBuffer.CopyTo(rentedBuffer); + int result = transform.TransformBlock(rentedBuffer, 0, inputBuffer.Length, outputBuffer, outputOffset); + CryptographicOperations.ZeroMemory(rentedBuffer.AsSpan(0, inputBuffer.Length)); + ArrayPool.Shared.Return(rentedBuffer); + rentedBuffer = null; - return result; + return result; + } + catch + { + CryptographicOperations.ZeroMemory(rentedBuffer.AsSpan(0, inputBuffer.Length)); + rentedBuffer = null; + + throw; + } } } } From 88f61f90ff8eaab0c0197e78137528a3bb9e3186 Mon Sep 17 00:00:00 2001 From: Newell Clark Date: Wed, 20 Jan 2021 10:28:27 -0500 Subject: [PATCH 04/11] Update src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs Use explicit type Co-authored-by: Stephen Toub --- .../src/System/Security/Cryptography/CryptoStream.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs b/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs index f9b82550fe7ddc..fc3b693862023f 100644 --- a/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs +++ b/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs @@ -270,7 +270,7 @@ public override void WriteByte(byte value) public override int Read(byte[] buffer, int offset, int count) { CheckReadArguments(buffer, offset, count); - var completedValueTask = ReadAsyncCore(buffer.AsMemory(offset, count), default(CancellationToken), useAsync: false); + ValueTask completedValueTask = ReadAsyncCore(buffer.AsMemory(offset, count), default(CancellationToken), useAsync: false); Debug.Assert(completedValueTask.IsCompleted); return completedValueTask.GetAwaiter().GetResult(); From 259a3b331a321d4aad617e134db8d27602c40b14 Mon Sep 17 00:00:00 2001 From: NewellClark Date: Wed, 20 Jan 2021 20:56:27 -0500 Subject: [PATCH 05/11] Fix exception behavior Unintentional breaking change with exceptions that was blocking CI. Note to self: remember to rebuild before running unit tests. --- .../Security/Cryptography/CryptoStream.cs | 43 +++++++++++++------ 1 file changed, 29 insertions(+), 14 deletions(-) diff --git a/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs b/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs index fc3b693862023f..915d20f4f82627 100644 --- a/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs +++ b/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs @@ -203,17 +203,18 @@ public override void SetLength(long value) public override Task ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) { CheckReadArguments(buffer, offset, count); - return ReadAsync(buffer.AsMemory(offset, count), cancellationToken).AsTask(); + return ReadAsyncInternal(buffer.AsMemory(offset, count), cancellationToken).AsTask(); } - public override IAsyncResult BeginRead(byte[] buffer, int offset, int count, AsyncCallback? callback, object? state) => - TaskToApm.Begin(ReadAsync(buffer, offset, count, CancellationToken.None), callback, state); + public override ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) + { + if (!CanRead) + return ValueTask.FromException(new NotSupportedException(SR.NotSupported_UnreadableStream)); - public override int EndRead(IAsyncResult asyncResult) => - TaskToApm.End(asyncResult); + return ReadAsyncInternal(buffer, cancellationToken); + } - /// - public override async ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) + private async ValueTask ReadAsyncInternal(Memory buffer, CancellationToken cancellationToken = default) { // To avoid a race with a stream's position pointer & generating race // conditions with internal buffer indexes in our own streams that @@ -232,6 +233,12 @@ public override async ValueTask ReadAsync(Memory buffer, Cancellation } } + public override IAsyncResult BeginRead(byte[] buffer, int offset, int count, AsyncCallback? callback, object? state) => + TaskToApm.Begin(ReadAsync(buffer, offset, count, CancellationToken.None), callback, state); + + public override int EndRead(IAsyncResult asyncResult) => + TaskToApm.End(asyncResult); + public override int ReadByte() { // If we have enough bytes in the buffer such that reading 1 will still leave bytes @@ -483,17 +490,19 @@ await _stream.ReadAsync(new Memory(_inputBuffer, _inputBufferIndex, _input public override Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) { CheckWriteArguments(buffer, offset, count); - return WriteAsync(buffer.AsMemory(offset, count), cancellationToken).AsTask(); + return WriteAsyncInternal(buffer.AsMemory(offset, count), cancellationToken).AsTask(); } - public override IAsyncResult BeginWrite(byte[] buffer, int offset, int count, AsyncCallback? callback, object? state) => - TaskToApm.Begin(WriteAsync(buffer, offset, count, CancellationToken.None), callback, state); + /// + public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationToken cancellationToken = default) + { + if (!CanWrite) + return ValueTask.FromException(new NotSupportedException(SR.NotSupported_UnwritableStream)); - public override void EndWrite(IAsyncResult asyncResult) => - TaskToApm.End(asyncResult); + return WriteAsyncInternal(buffer, cancellationToken); + } - /// - public override async ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationToken cancellationToken = default) + private async ValueTask WriteAsyncInternal(ReadOnlyMemory buffer, CancellationToken cancellationToken = default) { // To avoid a race with a stream's position pointer & generating race // conditions with internal buffer indexes in our own streams that @@ -512,6 +521,12 @@ public override async ValueTask WriteAsync(ReadOnlyMemory buffer, Cancella } } + public override IAsyncResult BeginWrite(byte[] buffer, int offset, int count, AsyncCallback? callback, object? state) => + TaskToApm.Begin(WriteAsync(buffer, offset, count, CancellationToken.None), callback, state); + + public override void EndWrite(IAsyncResult asyncResult) => + TaskToApm.End(asyncResult); + public override void Write(byte[] buffer, int offset, int count) { CheckWriteArguments(buffer, offset, count); From c31f85ff1ae345b5583a5b7b6a8ce4811a545e1f Mon Sep 17 00:00:00 2001 From: NewellClark Date: Thu, 21 Jan 2021 09:29:34 -0500 Subject: [PATCH 06/11] [no merge] Pinned array, used finally block Still need to override CopyTo/CopyToAsync. I just want to make sure that CI will pass with changes so far. --- .../Security/Cryptography/CryptoStream.cs | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs b/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs index 915d20f4f82627..ee935735fac115 100644 --- a/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs +++ b/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs @@ -657,7 +657,7 @@ private async ValueTask WriteAsyncCore(ReadOnlyMemory buffer, Cancellation } return; - static int TransformBlock(ICryptoTransform transform, ReadOnlyMemory inputBuffer, byte[] outputBuffer, int outputOffset) + unsafe static int TransformBlock(ICryptoTransform transform, ReadOnlyMemory inputBuffer, byte[] outputBuffer, int outputOffset) { if (MemoryMarshal.TryGetArray(inputBuffer, out ArraySegment segment)) { @@ -668,23 +668,24 @@ static int TransformBlock(ICryptoTransform transform, ReadOnlyMemory input else { var rentedBuffer = ArrayPool.Shared.Rent(inputBuffer.Length); + int result = default; try { - inputBuffer.CopyTo(rentedBuffer); - int result = transform.TransformBlock(rentedBuffer, 0, inputBuffer.Length, outputBuffer, outputOffset); - CryptographicOperations.ZeroMemory(rentedBuffer.AsSpan(0, inputBuffer.Length)); - ArrayPool.Shared.Return(rentedBuffer); - rentedBuffer = null; - - return result; + // Pin the rented buffer for security. + fixed (byte* _ = &rentedBuffer[0]) + { + inputBuffer.CopyTo(rentedBuffer); + result = transform.TransformBlock(rentedBuffer, 0, inputBuffer.Length, outputBuffer, outputOffset); + } } - catch + finally { CryptographicOperations.ZeroMemory(rentedBuffer.AsSpan(0, inputBuffer.Length)); - rentedBuffer = null; - - throw; } + ArrayPool.Shared.Return(rentedBuffer); + rentedBuffer = null; + + return result; } } } From 5eebaf6d3890e2f4d6a4b4e6f56b18c747f768e1 Mon Sep 17 00:00:00 2001 From: NewellClark Date: Thu, 21 Jan 2021 12:12:33 -0500 Subject: [PATCH 07/11] Added CopyTo/CopyToAsync overrides - Applied fixes suggested by @bartonjs - Added overrides for CopyTo/CopyToAsync to ensure any temporary buffers are properly cleared. --- ...System.Security.Cryptography.Primitives.cs | 4 + .../Security/Cryptography/CryptoStream.cs | 105 ++++++++++++++++-- 2 files changed, 98 insertions(+), 11 deletions(-) diff --git a/src/libraries/System.Security.Cryptography.Primitives/ref/System.Security.Cryptography.Primitives.cs b/src/libraries/System.Security.Cryptography.Primitives/ref/System.Security.Cryptography.Primitives.cs index 7876f89c42e703..a7d002262f5b7c 100644 --- a/src/libraries/System.Security.Cryptography.Primitives/ref/System.Security.Cryptography.Primitives.cs +++ b/src/libraries/System.Security.Cryptography.Primitives/ref/System.Security.Cryptography.Primitives.cs @@ -4,6 +4,8 @@ // Changes to this file must follow the https://aka.ms/api-review process. // ------------------------------------------------------------------------------ +using System.IO; + namespace System.Security.Cryptography { public abstract partial class AsymmetricAlgorithm : System.IDisposable @@ -75,6 +77,8 @@ public CryptoStream(System.IO.Stream stream, System.Security.Cryptography.ICrypt public override System.IAsyncResult BeginRead(byte[] buffer, int offset, int count, System.AsyncCallback? callback, object? state) { throw null; } public override System.IAsyncResult BeginWrite(byte[] buffer, int offset, int count, System.AsyncCallback? callback, object? state) { throw null; } public void Clear() { } + public override void CopyTo(Stream destination, int bufferSize) { throw null; } + public override System.Threading.Tasks.Task CopyToAsync(System.IO.Stream destination, int bufferSize, System.Threading.CancellationToken cancellationToken) { throw null; } protected override void Dispose(bool disposing) { } public override System.Threading.Tasks.ValueTask DisposeAsync() { throw null; } public override int EndRead(System.IAsyncResult asyncResult) { throw null; } diff --git a/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs b/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs index ee935735fac115..7b2796584b3bc3 100644 --- a/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs +++ b/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs @@ -206,6 +206,7 @@ public override Task ReadAsync(byte[] buffer, int offset, int count, Cancel return ReadAsyncInternal(buffer.AsMemory(offset, count), cancellationToken).AsTask(); } + /// public override ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) { if (!CanRead) @@ -565,7 +566,7 @@ private async ValueTask WriteAsyncCore(ReadOnlyMemory buffer, Cancellation { // not enough to transform a block, so just copy the bytes into the _InputBuffer // and return - buffer.Slice(0, buffer.Length).CopyTo(_inputBuffer.AsMemory(_inputBufferIndex)); + buffer.CopyTo(_inputBuffer.AsMemory(_inputBufferIndex)); _inputBufferIndex += buffer.Length; return; } @@ -667,29 +668,111 @@ unsafe static int TransformBlock(ICryptoTransform transform, ReadOnlyMemory.Shared.Rent(inputBuffer.Length); + // Use ArrayPool.Shared instead of CryptoPool because the array is passed out. + byte[] rentedBuffer = ArrayPool.Shared.Rent(inputBuffer.Length); int result = default; - try + + // Pin the rented buffer for security. + fixed (byte* _ = &rentedBuffer[0]) { - // Pin the rented buffer for security. - fixed (byte* _ = &rentedBuffer[0]) + try { inputBuffer.CopyTo(rentedBuffer); result = transform.TransformBlock(rentedBuffer, 0, inputBuffer.Length, outputBuffer, outputOffset); } + finally + { + CryptographicOperations.ZeroMemory(rentedBuffer.AsSpan(0, inputBuffer.Length)); + } } - finally - { - CryptographicOperations.ZeroMemory(rentedBuffer.AsSpan(0, inputBuffer.Length)); - } - ArrayPool.Shared.Return(rentedBuffer); - rentedBuffer = null; + ArrayPool.Shared.Return(rentedBuffer); + rentedBuffer = null!; return result; } } } + /// + public unsafe override void CopyTo(Stream destination, int bufferSize) + { + CheckCopyToArguments(destination, bufferSize); + + // Use ArrayPool.Shared instead of CryptoPool because the array is passed out. + byte[] rentedBuffer = ArrayPool.Shared.Rent(bufferSize); + // Pin the array for security. + fixed (byte* _ = &rentedBuffer[0]) + { + try + { + int bytesRead; + do + { + bytesRead = Read(rentedBuffer, 0, bufferSize); + destination.Write(rentedBuffer, 0, bytesRead); + } while (bytesRead > 0); + } + finally + { + CryptographicOperations.ZeroMemory(rentedBuffer.AsSpan(0, bufferSize)); + } + } + ArrayPool.Shared.Return(rentedBuffer); + rentedBuffer = null!; + } + + /// + public override Task CopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken) + { + CheckCopyToArguments(destination, bufferSize); + return CopyToAsyncInternal(destination, bufferSize, cancellationToken); + } + + private async Task CopyToAsyncInternal(Stream destination, int bufferSize, CancellationToken cancellationToken) + { + // Use ArrayPool.Shared instead of CryptoPool because the array is passed out. + byte[] rentedBuffer = ArrayPool.Shared.Rent(bufferSize); + // Pin the array for security. + GCHandle pinHandle = GCHandle.Alloc(rentedBuffer, GCHandleType.Pinned); + try + { + int bytesRead; + do + { + bytesRead = await ReadAsync(rentedBuffer.AsMemory(0, bufferSize), cancellationToken).ConfigureAwait(false); + await destination.WriteAsync(rentedBuffer.AsMemory(0, bytesRead), cancellationToken).ConfigureAwait(false); + } while (bytesRead > 0); + } + finally + { + CryptographicOperations.ZeroMemory(rentedBuffer.AsSpan(0, bufferSize)); + pinHandle.Free(); + } + ArrayPool.Shared.Return(rentedBuffer); + rentedBuffer = null!; + } + + private void CheckCopyToArguments(Stream destination, int bufferSize) + { + if (destination is null) + throw new ArgumentNullException(nameof(destination)); + + EnsureNotDisposed(destination, nameof(destination)); + + if (!destination.CanWrite) + throw new NotSupportedException(SR.NotSupported_UnwritableStream); + if (bufferSize <= 0) + throw new ArgumentOutOfRangeException(nameof(bufferSize), SR.ArgumentOutOfRange_NeedPosNum); + if (!CanRead) + throw new NotSupportedException(SR.NotSupported_UnreadableStream); + } + + private static void EnsureNotDisposed(Stream stream, string objectName) + { + if (!stream.CanRead && !stream.CanWrite) + throw new ObjectDisposedException(objectName); + } + public void Clear() { Close(); From 813a1c0920f70ea075466f0b4fd1dc7582d03ee5 Mon Sep 17 00:00:00 2001 From: NewellClark Date: Thu, 21 Jan 2021 16:47:03 -0500 Subject: [PATCH 08/11] Use fully-qualified name in ref --- .../ref/System.Security.Cryptography.Primitives.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Security.Cryptography.Primitives/ref/System.Security.Cryptography.Primitives.cs b/src/libraries/System.Security.Cryptography.Primitives/ref/System.Security.Cryptography.Primitives.cs index a7d002262f5b7c..c694c44374b18c 100644 --- a/src/libraries/System.Security.Cryptography.Primitives/ref/System.Security.Cryptography.Primitives.cs +++ b/src/libraries/System.Security.Cryptography.Primitives/ref/System.Security.Cryptography.Primitives.cs @@ -77,7 +77,7 @@ public CryptoStream(System.IO.Stream stream, System.Security.Cryptography.ICrypt public override System.IAsyncResult BeginRead(byte[] buffer, int offset, int count, System.AsyncCallback? callback, object? state) { throw null; } public override System.IAsyncResult BeginWrite(byte[] buffer, int offset, int count, System.AsyncCallback? callback, object? state) { throw null; } public void Clear() { } - public override void CopyTo(Stream destination, int bufferSize) { throw null; } + public override void CopyTo(System.IO.Stream destination, int bufferSize) { throw null; } public override System.Threading.Tasks.Task CopyToAsync(System.IO.Stream destination, int bufferSize, System.Threading.CancellationToken cancellationToken) { throw null; } protected override void Dispose(bool disposing) { } public override System.Threading.Tasks.ValueTask DisposeAsync() { throw null; } From 9b08e518366494873f753543e86442db0e487ccb Mon Sep 17 00:00:00 2001 From: Jeremy Barton Date: Mon, 22 Feb 2021 10:26:56 -0800 Subject: [PATCH 09/11] Update src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs --- .../src/System/Security/Cryptography/CryptoStream.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs b/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs index 7b2796584b3bc3..11e9a968509f88 100644 --- a/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs +++ b/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs @@ -658,7 +658,7 @@ private async ValueTask WriteAsyncCore(ReadOnlyMemory buffer, Cancellation } return; - unsafe static int TransformBlock(ICryptoTransform transform, ReadOnlyMemory inputBuffer, byte[] outputBuffer, int outputOffset) + private unsafe static int TransformBlock(ICryptoTransform transform, ReadOnlyMemory inputBuffer, byte[] outputBuffer, int outputOffset) { if (MemoryMarshal.TryGetArray(inputBuffer, out ArraySegment segment)) { From 79b086ecb4730169d4848ddcccb4dd7fd89ef429 Mon Sep 17 00:00:00 2001 From: Jeremy Barton Date: Mon, 22 Feb 2021 13:21:24 -0800 Subject: [PATCH 10/11] Undo bad edit --- .../src/System/Security/Cryptography/CryptoStream.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs b/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs index 11e9a968509f88..7b2796584b3bc3 100644 --- a/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs +++ b/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs @@ -658,7 +658,7 @@ private async ValueTask WriteAsyncCore(ReadOnlyMemory buffer, Cancellation } return; - private unsafe static int TransformBlock(ICryptoTransform transform, ReadOnlyMemory inputBuffer, byte[] outputBuffer, int outputOffset) + unsafe static int TransformBlock(ICryptoTransform transform, ReadOnlyMemory inputBuffer, byte[] outputBuffer, int outputOffset) { if (MemoryMarshal.TryGetArray(inputBuffer, out ArraySegment segment)) { From f506f37e23909d29d3ec926e31102af56f6f42cf Mon Sep 17 00:00:00 2001 From: Jeremy Barton Date: Mon, 22 Feb 2021 13:23:49 -0800 Subject: [PATCH 11/11] Update src/libraries/System.Security.Cryptography.Primitives/ref/System.Security.Cryptography.Primitives.cs --- .../ref/System.Security.Cryptography.Primitives.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/libraries/System.Security.Cryptography.Primitives/ref/System.Security.Cryptography.Primitives.cs b/src/libraries/System.Security.Cryptography.Primitives/ref/System.Security.Cryptography.Primitives.cs index c694c44374b18c..5e9515946979a8 100644 --- a/src/libraries/System.Security.Cryptography.Primitives/ref/System.Security.Cryptography.Primitives.cs +++ b/src/libraries/System.Security.Cryptography.Primitives/ref/System.Security.Cryptography.Primitives.cs @@ -4,8 +4,6 @@ // Changes to this file must follow the https://aka.ms/api-review process. // ------------------------------------------------------------------------------ -using System.IO; - namespace System.Security.Cryptography { public abstract partial class AsymmetricAlgorithm : System.IDisposable