Try/catch blocks for ArrayPool in System.Security.Cryptography.HashAlgorithm

[qtoyam]

All funcs in HashAlgorithm that use ArrayPool don't use try/catch blocks to ensure return array (and ZeroMemory).
For example:

runtime/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/HashAlgorithm.cs

Lines 136 to 159 in 911da68

	private async Task<byte[]> ComputeHashAsyncCore(
	Stream inputStream,
	CancellationToken cancellationToken)
	{
	// Use ArrayPool.Shared instead of CryptoPool because the array is passed out.
	byte[] rented = ArrayPool<byte>.Shared.Rent(4096);
	Memory<byte> buffer = rented;
	int clearLimit = 0;
	int bytesRead;
	while ((bytesRead = await inputStream.ReadAsync(buffer, cancellationToken).ConfigureAwait(false)) > 0)
	{
	if (bytesRead > clearLimit)
	{
	clearLimit = bytesRead;
	}
	HashCore(rented, 0, bytesRead);
	}
	CryptographicOperations.ZeroMemory(rented.AsSpan(0, clearLimit));
	ArrayPool<byte>.Shared.Return(rented, clearArray: false);
	return CaptureHashCodeAndReinitialize();
	}

Cancellation can be requested, so array will not be returned and memory will not be zero out.

I suggest fix:

Fix

        private async Task<byte[]> ComputeHashAsyncCore(
            Stream inputStream,
            CancellationToken cancellationToken)
        {
            // Use ArrayPool.Shared instead of CryptoPool because the array is passed out.
            byte[] rented = ArrayPool<byte>.Shared.Rent(4096);
            int clearLimit = 0;
            try
            {
                Memory<byte> buffer = rented;
                int bytesRead;

                while ((bytesRead = await inputStream.ReadAsync(buffer, cancellationToken).ConfigureAwait(false)) > 0)
                {
                    if (bytesRead > clearLimit)
                    {
                        clearLimit = bytesRead;
                    }

                    HashCore(rented, 0, bytesRead);
                }

                return CaptureHashCodeAndReinitialize();
            }
            finally
            {
                CryptographicOperations.ZeroMemory(rented.AsSpan(0, clearLimit));
                ArrayPool<byte>.Shared.Return(rented, clearArray: false);
            }
        }

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

---

All funcs in HashAlgorithm that use ArrayPool don't use try/catch blocks to ensure return array (and ZeroMemory).
For example:

runtime/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/HashAlgorithm.cs

Lines 136 to 159 in 911da68

	private async Task<byte[]> ComputeHashAsyncCore(
	Stream inputStream,
	CancellationToken cancellationToken)
	{
	// Use ArrayPool.Shared instead of CryptoPool because the array is passed out.
	byte[] rented = ArrayPool<byte>.Shared.Rent(4096);
	Memory<byte> buffer = rented;
	int clearLimit = 0;
	int bytesRead;
	while ((bytesRead = await inputStream.ReadAsync(buffer, cancellationToken).ConfigureAwait(false)) > 0)
	{
	if (bytesRead > clearLimit)
	{
	clearLimit = bytesRead;
	}
	HashCore(rented, 0, bytesRead);
	}
	CryptographicOperations.ZeroMemory(rented.AsSpan(0, clearLimit));
	ArrayPool<byte>.Shared.Return(rented, clearArray: false);
	return CaptureHashCodeAndReinitialize();
	}

Cancellation can be requested, so array will not be returned and memory will not be zero out.

I suggest fix:

Fix

        private async Task<byte[]> ComputeHashAsyncCore(
            Stream inputStream,
            CancellationToken cancellationToken)
        {
            // Use ArrayPool.Shared instead of CryptoPool because the array is passed out.
            byte[] rented = ArrayPool<byte>.Shared.Rent(4096);
            int clearLimit = 0;
            try
            {
                Memory<byte> buffer = rented;
                int bytesRead;

                while ((bytesRead = await inputStream.ReadAsync(buffer, cancellationToken).ConfigureAwait(false)) > 0)
                {
                    if (bytesRead > clearLimit)
                    {
                        clearLimit = bytesRead;
                    }

                    HashCore(rented, 0, bytesRead);
                }

                return CaptureHashCodeAndReinitialize();
            }
            finally
            {
                CryptographicOperations.ZeroMemory(rented.AsSpan(0, clearLimit));
                ArrayPool<byte>.Shared.Return(rented, clearArray: false);
            }
        }

Author:	qtoyam
Assignees:	-
Labels:	area-System.Security, untriaged
Milestone:	-

[MichalPetryka]

The recommendation is to not use try finally with ArrayPool in order to avoid memory corruption caused by things possibly still holding a reference to the array due to the exception.

[stephentoub]

The primary reason we haven't is because the extra code and complexity generally isn't worth it. If the occasional array isn't returned to the pool, it doesn't meaningfully impact performance... even the pool itself may drop arrays if it's full.

[vcsjones]

This was likely left out of the try/finally intentionally. To start with some background from @GrabYourPitchforks:

We also recommend not releasing the array back to the array pool in a finally block. Generally speaking, if an exception occurs, safer to abandon the array rather than return it.

The reason for this is that if you call an API and that API throws an exception, it has likely left its input arguments in an indeterminate state. At that point the safest thing to do would be to abandon them. Again, speaking in generalities.

Don't mistake returning back to the pool as "releasing resources" like a typical dispose. Returning back to the pool really says "I guarantee nobody else is using this and it's safe for somebody else to claim it." But how do you make that guarantee in an exceptional code path?

In this particular case, the rented buffer is being passed down in to a virtual/abstract method, HashCore. Generally, when buffers are passed to user code (anyone can implement HashCore in their own derived classes) and an exception occurs, we abandon the array instead of assuming it is safe to return to the pool.

[stephentoub]

Again, speaking in generalities

There are many situations where it's no more risky to return to the pool in an exceptional case than in a successful case. I'd argue it's the vast majority case. Which is why I say the primary reason is the extra complexity, mental overhead, and even small perf hit on the success path (e.g. lack of inlining due to the EH). My general stance is there's no need to return on exception, but it's ok to do so if there's a likely benefit, e.g. because there's reason to believe exceptions may be a bit more common (e.g. we shouldn't ship an analyzer that says don't return in a finally block). In more sensitive areas, like crypto, that can of course shift the equation.

[bartonjs]

The patterns in HashAlgorithm were intentional.

• HashCore isn't expected to throw, so there's not a strong feeling of unnecessary GC load here
• HashCore is a virtual method and the type hierarchy permits non-inbox implementations. So we can't just say by inspection "nothing here would do something weird like put a delayed call to Array.Clear in a finalizer".
• Not having a finally is generally better.

So we have

• The number of finally-based returns isn't much different than success-only.
• Returning in a finally has a (very) small risk of having the array have two different owners
• Not having a finally wins unless it's obvious that it's needed.

Net result: No finally is warranted.