System.Security.Cryptography.Aes has some strange APIs, which make it difficult to use.

[Vincent-X-Zhang]

Summary

• The parent class of type Aes, SymmetricAlgorithm, implements the IDisposable interface, which is only used to clear array elements. I think it is unnecessary.
• Methods such as public int EncryptCbc(ReadOnlySpan<byte> plaintext, ReadOnlySpan<byte> iv, Span<byte> destination, PaddingMode paddingMode = PaddingMode.PKCS7) do not need the PaddingMode parameter because the Aes class already contains the member variable public virtual PaddingMode Padding.

Motivation

When I use an instance of the Aes class, I often find myself deeply puzzled.

1. An Aes instance allows setting the CipherMode, such as CipherMode.Cfb, but at the same time, it provides a member method like: csharp public byte[] EncryptCbc(ReadOnlySpan<byte> plaintext, ReadOnlySpan<byte> iv, PaddingMode paddingMode = PaddingMode.PKCS7). Why do both of these exist in the same class?
2. An Aes instance allows setting the PaddingMode. So why is there a need to pass PaddingMode again in the method:public byte[] EncryptCbc(ReadOnlySpan<byte> plaintext, ReadOnlySpan<byte> iv, PaddingMode paddingMode = PaddingMode.PKCS7)Wouldn't it be better to turn this instance method into an extension method?
3. The class SymmetricAlgorithm implements the IDisposable interface, which appears to only be used for clearing array elements. Here is the source code:

protected virtual void Dispose(bool disposing)
{
    if (disposing)
    {
        if (KeyValue != null)
        {
            Array.Clear(KeyValue);
            KeyValue = null;
        }
        if (IVValue != null)
        {
            Array.Clear(IVValue);
            IVValue = null;
        }
    }
}

Is it really necessary to dispose?

[huoyaoyuan]

These apis were introduced in very different versions. There are some legacy designs from 20 years ago, which were proved as inconvenient.

For example, the CipherMode property were introduced in .NET Framework 1.1 at 2004, while the EncryptCbc method were introduced in .NET 6 at 2021. You can see the design review of additional methods at #2406.

Is it really necessary to dispose?

When it comes to cryptographic, it can be important to clear sensitive information after use. Dispose is effectively an alias of Clear as you can see in code.

[Vincent-X-Zhang]

It's clear that ReadOnlySpan was introduced in modern .NET to make programs more efficient. However, I think this API design is plagued with code smells, making it harder to extend or refactor.

Take the EncryptCbc method as an example. It actually requires four parameters: plaintext, key, iv, and paddingMode. But the method signature public byte[] EncryptCbc(ReadOnlySpan<byte> plaintext, ReadOnlySpan<byte> iv, PaddingMode paddingMode = PaddingMode.PKCS7) only takes three. Here's how you would use it:

using var aes = Aes.Create();
aes.Key = key;
var result = aes.EncryptCbc(plaintext, iv);

Further, this might create unnecessary ambiguity. For instance:

using var aes = Aes.Create();
aes.Key = key;
aes.PaddingMode = PaddingMode.None;
var result = aes.EncryptCbc(plaintext, iv, PaddingMode.PKCS7);

Some might argue that the missing key parameter is intentional, as it protects sensitive information. But if that’s the case, why not make it an extension method? Here's a potential improvement:

public static class Aes
{
  public static byte[] EncryptCbc(this Aes aes, ReadOnlySpan<byte> plaintext, ReadOnlySpan<byte> key, ReadOnlySpan<byte> iv,  PaddingMode paddingMode = PaddingMode.PKCS7)
   {
       // todo
   }
}

This approach makes the API more explicit, intuitive, and less error-prone. It ensures that all required parameters are passed directly, avoiding confusion about the implicit use of properties like Key and PaddingMode.

[tfenise]

Certain implementations may have unmanaged resources to dispose, like https://referencesource.microsoft.com/#System.Core/System/Security/Cryptography/AesCryptoServiceProvider.cs,270

[Vincent-X-Zhang]

Yes, it does.
Class Aes is no need to inherit interface IDispose, although the certain implementation AesCryptoServiceProvider may have unmanaged resources to dispose.
And the reason why the class Aes needs to dispose as @huoyaoyuan mentioned, "When it comes to cryptographic, it can be important to clear sensitive information after use. Dispose is effectively an alias of Clear as you can see in code."
For me, I don't like the way the Dispose method is used to clear sensitive information.

[tfenise]

If you call Aes.Create(), whose return type is Aes, you may get an AesCryptoServiceProvider (at least in .NET Framework). That is why IDisposable has to be implemented by the base class. This way, you can dispose the Aes instance returned. This is similar to why Stream implements IDisposable, even though the base class does nothing when disposed.

[bartonjs]

The parent class of type Aes, SymmetricAlgorithm, implements the IDisposable interface, which is only used to clear array elements. I think it is unnecessary.

Even were it "unnecessary", it can't stop being IDisposable.

Clearing the key out is important for some people, particularly anyone who has to operate under FedRAMP/FIPS/FISMA data processing standards, which dictate that a secret key must be removed from memory when it is no longer necessary.

And also, that implementation is just the base class behavior. Implementations like AesCryptoServiceProvider in .NET Framework have interop considerations... specifically, SafeHandle values that should be disposed. Sure, those values could be cleaned up by the finalizer, but the main purpose of IDisposable is to be able to clean up native resources without using the finalizer.

Methods such as EncryptCbc do not need the PaddingMode parameter because the Aes class already contains the Padding property.

It also contains the Mode property to distinguish CBC from ECB (et al). The methods were created because the property bag design is bad. The IV property isn't used by all operations. The FeedbackSize property isn't used by all operations. They're still there because we can't remove them... and because leaving them alone makes it easier for someone to work with both .NET and .NET Framework using the legacy CreateEncryptor()/CreateDecryptor() paradigm. The new methods show clearly what data they use, except the one thing they all share, the key, which is what the object itself represents.

Wouldn't it be better to turn this instance method into an extension method?

Why? A) It'd be against design guidelines (don't add extension methods on a type in the same assembly that defines it [except for some special cases]), B) it means that all implementations have to be done by changing the Mode property, et al, and calling CreateEncryptor/CreateDecryptor... and that's actually a bad way to implement them. None of the built-in implementations call CreateEncryptor from their newer methods.

Some might argue that the missing key parameter is intentional, as it protects sensitive information. ... [so make it a method parameter on a static]

The AesCng class supports opening a saved key by name, and not being able to see what the bytes of that key are (reading the Key property when it's in that mode will throw an exception). The design of the new methods is that the Aes (or whatever algorithm) instance represents the combination of an algorithm choice and a key. None of the new methods take a key as that would invalidate the ability to use "external" key references.

avoiding confusion about the implicit use of properties

The logical concept of "the key" is used by all of the new methods, as well as the parameterless CreateEncryptor and CreateDecryptor legacy methods. For most implementations that means the key property, but for AesCng (at least) it might not.

No other property is read by the Encrypt[Mode] or Decrypt[Mode] methods. They're only used by the CreateEncryptor and CreateDecryptor overloads. Which properties get read depend on the value of the Mode property.

If you're writing code for .NET 5+ (nee Core) only, use the Encrypt and Decrypt methods, and ignore everything else except Key. If you're writing code for .NET Framework or .NET Standard you have to use the legacy properties and the CreateEncryptor/CreateDecryptor methods. If you're writing for both you can choose to do #if to try both ways, or stick to the one that works everywhere.