Clarify thread safety of crypto one-shots

[GrabYourPitchforks]

.NET has recently introduced several crypto one-shot methods, including:

// Static one-shots on SHA and HMAC classes
public static byte[] HashData(byte[] source) { throw null; }

// Instance one-shots on AesGcm / AesCcm / ChaCha20Poly1305
public void Encrypt(byte[] nonce, byte[] plaintext, byte[] ciphertext, byte[] tag, byte[]? associatedData = null) { }
public void Decrypt(byte[] nonce, byte[] ciphertext, byte[] tag, byte[] plaintext, byte[]? associatedData = null) { }

// Instance one-shots on SymmetricAlgorithm
public byte[] EncryptEcb(byte[] plaintext, PaddingMode paddingMode) { throw null; }

Following our normal threading principles, static one-shot methods are thread-safe, but instance one-shot methods are not guaranteed to be thread-safe. For instance, as of this writing, AesGcm.Encrypt is thread-safe on Windows but not on other operating systems. Which means that the code below could work on a Windows test box but fail in a Linux prod environment.

public class MyClass
{
    private static readonly AesGcm _aes = new AesGcm(GetGlobalKey());

    public void MethodCalledByMultipleThreads(/* ... */)
    {
        _aes.Encrypt(plaintext, nonce, /* ... */);
    }
}

The failure mode could be corruption of the managed buffer, corruption of the underlying native handle, or even nonce reuse (which would destroy GHASH). Considering that keeping singleton SymmetricAlgorithm instances around and using CreateEncryptor / CreateDecryptor as thread-safe ICryptoTransform factories is a well-established pattern, I believe that customers may incorrectly believe that the new crypto one-shots behave similarly.

Given this, how do we want to proceed here? Three options immediately come to mind.

1. Change AesGcm / AesCcm / ChaCha20Poly1305 to be thread-safe on all platforms. The benefit to this is that it makes the pattern above work correctly on all platforms. However, it may complicate the documentation because we would have to say "it's only thread-safe on .NET 6+." And it doesn't address EncryptCbc and friends.

2. Explicitly document these APIs as not thread-safe. This wouldn't change our default stance that static APIs are generally thread-safe and instance APIs are generally not thread-safe. But it would remove ambiguity for these APIs in particular.

3. Do nothing. Assume devs know the default threading behaviors without needing to be told explicitly, and assume that devs won't make the leap from CreateEncryptor being historically thread-safe to EncryptCbc also being thread-safe.

[ghost]

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

Issue Details

---

.NET has recently introduced several crypto one-shot methods, including:

// Static one-shots on SHA and HMAC classes
public static byte[] HashData(byte[] source) { throw null; }

// Instance one-shots on AesGcm / AesCcm / ChaCha20Poly1305
public void Encrypt(byte[] nonce, byte[] plaintext, byte[] ciphertext, byte[] tag, byte[]? associatedData = null) { }
public void Decrypt(byte[] nonce, byte[] ciphertext, byte[] tag, byte[] plaintext, byte[]? associatedData = null) { }

// Instance one-shots on SymmetricAlgorithm
public byte[] EncryptEcb(byte[] plaintext, PaddingMode paddingMode) { throw null; }

Following our normal threading principles, static one-shot methods are thread-safe, but instance one-shot methods are not guaranteed to be thread-safe. For instance, as of this writing, AesGcm.Encrypt is thread-safe on Windows but not on other operating systems. Which means that the code below could work on a Windows test box but fail in a Linux prod environment.

public class MyClass
{
    private static readonly AesGcm _aes = new AesGcm(GetGlobalKey());

    public void MethodCalledByMultipleThreads(/* ... */)
    {
        _aes.Encrypt(plaintext, nonce, /* ... */);
    }
}

The failure mode could be corruption of the managed buffer, corruption of the underlying native handle, or even nonce reuse (which would destroy GHASH). Considering that keeping singleton SymmetricAlgorithm instances around and using CreateEncryptor / CreateDecryptor as thread-safe ICryptoTransform factories is a well-established pattern, I believe that customers may incorrectly believe that the new crypto one-shots behave similarly.

Given this, how do we want to proceed here? Three options immediately come to mind.

1. Change AesGcm / AesCcm / ChaCha20Poly1305 to be thread-safe on all platforms. The benefit to this is that it makes the pattern above work correctly on all platforms. However, it may complicate the documentation because we would have to say "it's only thread-safe on .NET 6+." And it doesn't address EncryptCbc and friends.

2. Explicitly document these APIs as not thread-safe. This wouldn't change our default stance that static APIs are generally thread-safe and instance APIs are generally not thread-safe. But it would remove ambiguity for these APIs in particular.

3. Do nothing. Assume devs know the default threading behaviors without needing to be told explicitly, and assume that devs won't make the leap from CreateEncryptor being historically thread-safe to EncryptCbc also being thread-safe.

Author:	GrabYourPitchforks
Assignees:	-
Labels:	area-System.Security
Milestone:	6.0.0

[AaronRobinsonMSFT]

@GrabYourPitchforks I assume this may have an impact on #49511? Unsure if these APIs will make it to WASM but figured I would link to another thread concern in the crypto space.

[bartonjs]

The static one-shots should be thread-safe, since they're static.

For the instance members, I think my preferences are 3/2/1.

[vcsjones]

I'm indifferent between 2 and 3... documentation makes sense, but then again if we document every single instance API that is not thread safe, throughout the stack - that is a lot of documentation to add. But then yet again, maybe crypto APIs are a bit special and calling out their thread safety is a worthy task.

I don't think the hashing one-shots being thread safe has much bearing on the thread safety of the encryption ones. I largely don't even think of the encryption ones as one-shots since you need to construct an object. Is TransformFinalBlock on ICryptoTransform a "one shot" since it can be used that way?

[bartonjs]

Any pushback on explicitly deciding on "do nothing", @GrabYourPitchforks?

[GrabYourPitchforks]

"Do nothing" is fine. It means that we're mixing thread-safe (CreateEncryptor / CreateDecryptor) and non-thread-safe (EncryptCbc) instance methods on the same type, but we do this with other types (like Dictionary<TKey, TValue>) as well and the world hasn't ended. I was probably just being paranoid. :)

[bartonjs]

Alrighty, then. Closing as no action required 😄.