One-shot AES CBC and ECB

[vcsjones]

.NET Core 3.0 has AesCcm and AesGcm type that are simple, one-shot APIs. I really like the API shape of these and would suggest that it might be worth adding AesCbc and AesEcb (sorry) types.

The one-shot (no streaming, no update / finalize) would give a few benefits.

• I like that the mode is part of the type. The modes aren't really interchangeable, and having them as part of the type helps suggest that. That is, as far as using a primitive goes, I can't really "upgrade" from one mode to another like it's some kind of configuration. It requires some thought and consideration as to how each mode behaves.

• Type-per-mode would allow the API shape to fit the mode better (no IV for ECB).

• One-shot usage would be hard to "you're holding it wrong". The API surface would be simple Decrypt and Encrypt methods.

• One-shot API would mean there is no state to carry between calls (like CNG updating the IV).
  That might afford a few more opportunities to use CryptoPool or stack buffers instead of new byte[].

• Do not support some less common types of block padding (PKCS#7 and no padding only). This can help developers make better choices security-wise and push handling down to the underlying implementation.

/cc @GrabYourPitchforks @bartonjs

---

Updated API Proposal

Since the API review we added CFB support, so this adds the CFB members, and adds a mild reshaping to account for CFB being different than ECB and CBC:

Current approved additions, with no updates required:

namespace System.Security.Cryptography
{
    public abstract class SymmetricAlgorithm
    {
        // ECB (Block Aligned, no default padding, no IV)
        public byte[] DecryptEcb(byte[] ciphertext, PaddingMode paddingMode);
        public byte[] DecryptEcb(ReadOnlySpan<byte> ciphertext, PaddingMode paddingMode);
        public int DecryptEcb(ReadOnlySpan<byte> ciphertext, Span<byte> destination, PaddingMode paddingMode);
        public byte[] EncryptEcb(byte[] plaintext, PaddingMode paddingMode);
        public byte[] EncryptEcb(ReadOnlySpan<byte> plaintext, PaddingMode paddingMode);
        public int EncryptEcb(ReadOnlySpan<byte> plaintext, Span<byte> destination, PaddingMode paddingMode);
        public bool TryDecryptEcb(
            ReadOnlySpan<byte> ciphertext, Span<byte> destination, PaddingMode paddingMode, out int bytesWritten);
        protected virtual bool TryDecryptEcbCore(
            ReadOnlySpan<byte> ciphertext, Span<byte> destination, PaddingMode paddingMode, out int bytesWritten);
        public bool TryEncryptEcb(
            ReadOnlySpan<byte> plaintext, Span<byte> destination, PaddingMode paddingMode, out int bytesWritten);
        protected virtual bool TryEncryptEcbCore(
            ReadOnlySpan<byte> plaintext, Span<byte> destination, PaddingMode paddingMode, out int bytesWritten);

        // CBC (Block Aligned, default to PKCS#7 padding, IV required)
        public byte[] DecryptCbc(byte[] ciphertext, byte[] iv, PaddingMode paddingMode = PaddingMode.PKCS7);
        public byte[] DecryptCbc(
            ReadOnlySpan<byte> ciphertext, ReadOnlySpan<byte> iv, PaddingMode paddingMode = PaddingMode.PKCS7);
        public int DecryptCbc(
            ReadOnlySpan<byte> ciphertext, ReadOnlySpan<byte> iv, Span<byte> destination,
            PaddingMode paddingMode = PaddingMode.PKCS7);
        public byte[] EncryptCbc(byte[] plaintext, byte[] iv, PaddingMode paddingMode = PaddingMode.PKCS7);
        public byte[] EncryptCbc(
            ReadOnlySpan<byte> plaintext, ReadOnlySpan<byte> iv, PaddingMode paddingMode = PaddingMode.PKCS7);
        public int EncryptCbc(
            ReadOnlySpan<byte> plaintext, ReadOnlySpan<byte> iv, Span<byte> destination,
            PaddingMode paddingMode = PaddingMode.PKCS7);
        public bool TryDecryptCbc(
            ReadOnlySpan<byte> ciphertext, ReadOnlySpan<byte> iv, Span<byte> destination, out int bytesWritten,
            PaddingMode paddingMode = PaddingMode.PKCS7);
        protected virtual bool TryDecryptCbcCore(
            ReadOnlySpan<byte> ciphertext, ReadOnlySpan<byte> iv, Span<byte> destination, PaddingMode paddingMode,
            out int bytesWritten);
        public bool TryEncryptCbc(
            ReadOnlySpan<byte> plaintext, ReadOnlySpan<byte> iv, Span<byte> destination, out int bytesWritten,
            PaddingMode paddingMode = PaddingMode.PKCS7);
        protected virtual bool TryEncryptCbcCore(
            ReadOnlySpan<byte> plaintext, ReadOnlySpan<byte> iv, Span<byte> destination, PaddingMode paddingMode,
            out int bytesWritten);
    }
}

CFB additions

namespace System.Security.Cryptography
{
    public abstract class SymmetricAlgorithm
    {
        // CFB (Feedback-size aligned, default to No padding, IV required, adds feedback size (default 8))
        public byte[] DecryptCfb(
            byte[] ciphertext, byte[] iv, PaddingMode paddingMode = PaddingMode.None, int feedbackSizeBits = 8);
        public byte[] DecryptCfb(
            ReadOnlySpan<byte> ciphertext, ReadOnlySpan<byte> iv, PaddingMode paddingMode = PaddingMode.None,
            int feedbackSizeBits = 8);
        public int DecryptCfb(
            ReadOnlySpan<byte> ciphertext, ReadOnlySpan<byte> iv, Span<byte> destination,
            PaddingMode paddingMode = PaddingMode.None, int feedbackSizeBits = 8);

        public byte[] EncryptCfb(
            byte[] plaintext, byte[] iv, PaddingMode paddingMode = PaddingMode.None, int feedbackSizeBits = 8);
        public byte[] EncryptCfb(
            ReadOnlySpan<byte> plaintext, ReadOnlySpan<byte> iv, PaddingMode paddingMode = PaddingMode.None,
            int feedbackSizeBits = 8);
        public int EncryptCfb(
            ReadOnlySpan<byte> plaintext, ReadOnlySpan<byte> iv, Span<byte> destination,
            PaddingMode paddingMode = PaddingMode.None, int feedbackSizeBits = 8);

        public bool TryDecryptCfb(
            ReadOnlySpan<byte> ciphertext, ReadOnlySpan<byte> iv, Span<byte> destination, out int bytesWritten,
            PaddingMode paddingMode = PaddingMode.None, int feedbackSizeBits = 8);

        public bool TryEncryptCfb(
            ReadOnlySpan<byte> plaintext, ReadOnlySpan<byte> iv, Span<byte> destination, out int bytesWritten,
            PaddingMode paddingMode = PaddingMode.None, int feedbackSizeBits = 8);

        protected virtual bool TryDecryptCfbCore(
            ReadOnlySpan<byte> ciphertext, ReadOnlySpan<byte> iv, Span<byte> destination, PaddingMode paddingMode,
            int feedbackSizeBits, out int bytesWritten);

        protected virtual bool TryEncryptCfbCore(
            ReadOnlySpan<byte> plaintext, ReadOnlySpan<byte> iv, Span<byte> destination, PaddingMode paddingMode,
            int feedbackSizeBits, out int bytesWritten);
    }
}

Modifications of existing approved members:

namespace System.Security.Cryptography
{
    public abstract class SymmetricAlgorithm
    {
-       public int GetCiphertextLength(PaddingMode paddingMode, int plaintextLength);
+       public int GetCiphertextLengthEcb(int plaintextLength, PaddingMode paddingMode);
+       public int GetCiphertextLengthCbc(int plaintextLength, PaddingMode paddingMode = PaddingMode.PKCS7);
+       public int GetCiphertextLengthCfb(
+           int plaintextLength, PaddingMode paddingMode = PaddingMode.None, int feedbackSizeBits = 8);
    }
}

Previously discussed, outdated, API Proposal

namespace System.Security.Cryptography {
    public abstract class SymmetricAlgorithm {

        public byte[] EncryptEcb(
            byte[] plaintext,
            PaddingMode paddingMode = default);
        public byte[] EncryptEcb(
            ReadOnlySpan<byte> plaintext,
            PaddingMode paddingMode = default);
        public int EncryptEcb(
            ReadOnlySpan<byte> plaintext,
            Span<byte> destination,
            PaddingMode paddingMode = default);
        public bool TryEncryptEcb(
            ReadOnlySpan<byte> plaintext,
            Span<byte> destination,
            out int bytesWritten,
            PaddingMode paddingMode = default);

        public byte[] EncryptCbc(
            byte[] plaintext,
            byte[] iv,
            PaddingMode paddingMode = default);
        public byte[] EncryptCbc(
            ReadOnlySpan<byte> plaintext,
            ReadOnlySpan<byte> iv,
            PaddingMode paddingMode = default);
        public int EncryptCbc(
            ReadOnlySpan<byte> plaintext,
            ReadOnlySpan<byte> iv,
            Span<byte> destination,
            PaddingMode paddingMode = default);
        public bool TryEncryptCbc(
            ReadOnlySpan<byte> plaintext,
            ReadOnlySpan<byte> iv,
            Span<byte> destination,
            out int bytesWritten,
            PaddingMode paddingMode = default);

        public byte[] DecryptEcb(
            byte[] ciphertext,
            PaddingMode paddingMode = default);
        public byte[] DecryptEcb(
            ReadOnlySpan<byte> ciphertext,
            PaddingMode paddingMode = default);
        public int DecryptEcb(
            ReadOnlySpan<byte> ciphertext,
            Span<byte> destination,
            PaddingMode paddingMode = default);
        public bool TryDecryptEcb(
            ReadOnlySpan<byte> ciphertext,
            Span<byte> destination,
            out int bytesWritten,
            PaddingMode paddingMode = default);

        public byte[] DecryptCbc(
            byte[] ciphertext,
            byte[] iv,
            PaddingMode paddingMode = default);
        public byte[] DecryptCbc(
            ReadOnlySpan<byte> ciphertext,
            ReadOnlySpan<byte> iv,
            PaddingMode paddingMode = default);
        public int DecryptCbc(
            ReadOnlySpan<byte> ciphertext,
            ReadOnlySpan<byte> iv,
            Span<byte> destination,
            PaddingMode paddingMode = default);
        public bool TryDecryptCbc(
            ReadOnlySpan<byte> ciphertext,
            ReadOnlySpan<byte> iv,
            Span<byte> destination,
            out int bytesWritten,
            PaddingMode paddingMode = default);


        protected virtual bool TryEncryptCbcCore(
            ReadOnlySpan<byte> plaintext,
            ReadOnlySpan<byte> iv,
            Span<byte> destination,
            PaddingMode paddingMode,
            out int bytesWritten);
        protected virtual bool TryDecryptCbcCore(
            ReadOnlySpan<byte> ciphertext,
            ReadOnlySpan<byte> iv,
            Span<byte> destination,
            PaddingMode paddingMode,
            out int bytesWritten);
        protected virtual bool TryEncryptEcbCore(
            ReadOnlySpan<byte> plaintext,
            Span<byte> destination,
            PaddingMode paddingMode,
            out int bytesWritten);
        protected virtual bool TryDecryptEcbCore(
            ReadOnlySpan<byte> ciphertext,
            Span<byte> destination,
            PaddingMode paddingMode,
            out int bytesWritten);

        public int GetCiphertextLength(PaddingMode paddingMode, int plaintextLength);
    }
}

[scalablecory]

Do we want to make it easier to use ECB? :)

[vcsjones]

@scalablecory I even pre-apologized for the suggestion at the very beginning 😅.

If it were up to me ECB of any kind would be in an out of band package and removed from shared framework. That said, I know there is a very real need for it by some folks, so I threw it out there as an idea.

When / if this comes to an API proposal the thought can be put further to the test.

[bartonjs]

ECB is important as the building block for things that we don't have intrinsically, like AES-KW (IIRC that uses ECB)--if nothing else, pretty much every other mode can be written in terms of ECB.

I think the one shots are good, but I don't know that we want distinct types for classic modes. I had pictured these as just EncryptEcb, EncryptCbc, etc methods on SymmetricAlgorithm -- the drawback is the IV property, which was already weird.

And I don't think it makes sense to drop support for the alternate padding schemes.

AesCcm and AesGcm got new types because they're fundamentally different concepts than "just AES", and we wanted to not have any mode where the IV (nonce) got reused because of a property existing where it shouldn't have. And the notion of "what tag sizes are supported" is different per tag algorithm (could have been EncryptGcm, DecryptGcm, SupportedGcmTagSizes, etc, I guess...

Really the problem I want to avoid here is that when AES2 comes around we don't end up with type explosions from { AES, AES2, 3DES } x { CBC, ECB, CTR, ... }. The places that it makes sense to be a bit polymorphic we should be, and CBC/ECB/CTR/etc feel more like different methods to me (verbs/adverbs) than different classes (nouns).

[vcsjones]

I don't know that we want distinct types for classic modes. I had pictured these as just EncryptEcb, EncryptCbc, etc methods on SymmetricAlgorithm.

Yes, I'm not a fan of the IV property since that makes the developer think the IV has the same use / lifetime as the key. Padding with CTR is also somewhat weird, too. FeedbackSize is also a bit off for Aes. The current design feels very "do all the things" for different modes.

The places that it makes sense to be a bit polymorphic we should be

The usage of AES is rarely polymorphic in my own uses - the usage of AES is almost always specific to what mode it is in. AES ECB can only be used as a building block for key-wrap or rare occurrences of "I have only one block to encrypt with one time use keys". CBC needs a CSPRNG IV and does not tolerate predictable IVs. CTR tolerates predictable IVs and can be a counter, or it can be zero if the key is used once. Etc, etc.

So while the API shape may be polymorphic I don't really think it can ever be used as such. My thought was to encode that in the type.

And I don't think it makes sense to drop support for the alternate padding schemes.

Fair enough. I don't have strong arguments to remove it other than I will personally avoid them. I was hoping to push the padding handling down to the implementation.

[bartonjs]

The usage of AES is rarely polymorphic in my own uses

Well, I mean more like "RC2, DES, 3DES, and AES all support ECB, CBC, ...", so something like

partial class SymmetricAlgorithm
{
    // default(PaddingMode) => use the property?
    public int EncryptEcb(ReadOnlySpan<byte> source, Span<byte> destination, PaddingMode paddingMode = default);
    public int DecryptEcb(ReadOnlySpan<byte> source, Span<byte> destination, PaddingMode paddingMode = default);
    protected virtual bool TryTransformCbc(ReadOnlySpan<byte> source, ReadOnlySpan<byte> iv, Span<byte> destination, PaddingMode paddingMode, bool encrypt);

    // Maybe change iv to Span and add an optional parameter for "generate it for me",
    // but perhaps either "generate it for me" or "I already have one" is a named alternative,
    // such as EncryptCbcWithIv, EncryptCbcWithRandomIv
    public int EncryptCbc(ReadOnlySpan<byte> plaintext, ReadOnlySpan<byte> iv, Span<byte> destination, PaddingMode paddingMode = default);
    public int DecryptCbc(ReadOnlySpan<byte> ciphertext, ReadOnlySpan<byte> iv, Span<byte> destination, PaddingMode paddingMode = default);

    // Split nonce and counter? Unify them?
    public int EncryptCtr(ReadOnlySpan<byte> plaintext, ..., Span<byte> destination);
    public int DecryptCtr(ReadOnlySpan<byte> ciphertext, ..., Span<byte> destination);

    // I suppose for consistency, feedbackSize == 0 means "use the property".
    // Same IV questions as CBC
    public int EncryptCfb(..., int feedbackSize = 0);
    public int DecryptCfb(..., int feedbackSize = 0);

// OFB, CTS?, etc.
}

All of the public methods (span-accepting, byte[] returning, Span-Try, etc) can do argument validation (and, when appropriate, random-value filling) then defer the single (per mode) protected virtual bool TryTransform[Mode](...) (which could be written in terms of CreateTransform for the base class). It's the Template Method Pattern, but doesn't need the Core suffix since it got a weird name with the addition of the encrypt (vs decrypt) Boolean.

No IV/Nonce value would ever be read from the IV property, but Padding and FeedbackSize seem like they can (as you rarely want to change them).

[vcsjones]

Well, I mean more like "RC2, DES, 3DES, and AES all support ECB, CBC,

Okay, if per-type modes is not looking like a good idea, then I would propose:

namespace System.Security.Cryptography {
    public abstract class SymmetricAlgorithm {
        public byte[] EncryptEcb(ReadOnlySpan<byte> plaintext, PaddingMode paddingMode = default);
        public bool TryEncyptEcb(ReadOnlySpan<byte> plaintext, Span<byte> destination, out int bytesWritten, PaddingMode paddingMode = default);
        public byte[] EncryptCbc(ReadOnlySpan<byte> plaintext, ReadOnlySpan<byte> iv, PaddingMode paddingMode = default);
        public bool TryEncyptCbc(ReadOnlySpan<byte> plaintext, ReadOnlySpan<byte> iv, Span<byte> destination, out int bytesWritten, PaddingMode paddingMode = default);

        public byte[] DecryptEcb(ReadOnlySpan<byte> ciphertext, PaddingMode paddingMode = default);
        public bool TryDecryptEcb(ReadOnlySpan<byte> ciphertext, Span<byte> destination, out int bytesWritten, PaddingMode paddingMode = default);
        public byte[] DecryptCbc(ReadOnlySpan<byte> ciphertext, ReadOnlySpan<byte> iv, PaddingMode paddingMode = default);
        public bool TryDecryptCbc(ReadOnlySpan<byte> ciphertext, ReadOnlySpan<byte> iv, Span<byte> destination, out int bytesWritten, PaddingMode paddingMode = default);
    }
}

Thoughts.

1. It didn't make sense to me to do CFB/OFB since it's missing. (Something is causing the LocallocLarge test to run slowly #4647)
2. CTS appears to be inconsistently supported? Is it CNG only? Doesn't work on my Linux box.
3. I see AES CTR in your proposal. Was your intention to build CTR on top of ECB?

[bartonjs]

public virtual

• ✓ DO prefer protected accessibility over public accessibility for virtual members. Public members should provide extensibility (if required) by calling into a protected virtual member.

CFB/OFB/CTS/CTR/etc

Yeah, they wouldn't need to be done yet, but having a pattern worked out ahead of time seems virtuous. (CFB, at least, is on the 5.0 plan).

[vcsjones]

DO prefer protected accessibility over public accessibility for virtual members. Public members should provide extensibility (if required) by calling into a protected virtual member.

Hm. That one is new to me. I struggle to find this documentation in the repository. There's this but doesn't mention that. Anyway, seems like a reasonable thing, updated.

[bartonjs]

That one is at https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/virtual-members.

As part of updating the guidelines for a 3rd edition of Framework Design Guidelines we're also looking at how to better (more modernly?) share the guidelines. (Since there's a published book involved there's a bit of copyright/contract complication that slows things down a bit)

[GrabYourPitchforks]

Just throwing this out there for discussion...

If we were to reinvent the SymmetricAlgorithm class today, would we still have properties like Key on it? It was already mentioned earlier that the IV property was problematic.

In my ideal world, the API would look something like below. (Ignore for now that it violates the FDG.)

public abstract class SymmetricAlgorithm2 : IDisposable
{
    public abstract int Decrypt(ROS<byte> src, Span<byte> dest);
    public abstract int Encrypt(ROS<byte> src, Span<byte> dest);

    public abstract void Dispose();
}

public sealed class AesAlgorithm2 : SymmetricAlgorithm2
{
    public AesAlgorithm(ReadOnlySpan<byte> key, ChainingMode chainingMode, PaddingMode paddingMode);
}

Basically, everything about how the algorithm behaves is fixed at construction time, and there's just Encrypt and Decrypt. Maybe I'm too influenced by the BCrypt* APIs? :)

Edit: I should clarify that this sketch API is only useful for one-shot. It's useless for streaming.

[vcsjones]

would we still have properties like Key on it?

Probably? Keys are usually tied to some lifetime of "thing which can do encryption with the key" like a BCRYPT_KEY_HANDLE. Even in one-shot scenarios using the same key many times makes plenty of sense. The IV on the other hand has a lifetime associated with each encrypt / decrypt call.

If we were to reinvent the SymmetricAlgorithm class today..

I seem to have very different tastes from y'all on this. In my "ideal" world the mode is part of the type (hence my original suggestion) or at least something that is "compile time", not some attribute on a thing. The mode simply dictates too much of the API shape for me, and like CNG / BCrypt, you end up with some invariant structure of "stuff that only applies to the mode" like pPaddingInfo being void*.

I covered a lot of that above, so I won't reiterate it, but between feedback size, IV, nonces, etc, and some other modes of AES that are even more bizarre like IGE which has two IVs, trying to cram the mode into a single API surface doesn't make a lot of sense to me.

If the mode dictates what information is necessary for it to function, then a "base" encrypt / decrypt doesn't work.

The sketched API above doesn't have an IV in it anywhere, for example. Where does it go? It can't go on Encrypt / Decrypt since SymmetricAlgorithm doesn't have it, making it a property makes the inheritance work but I dislike that for reasons already stated. I think many people mistakenly believe the IV's usage lifetime is the same as the key's then when they "learn cryptography through intellisense".

You can have a method per-mode, or a type per-mode, and now I'm back to where I started.

[bartonjs]

would we still have properties like Key on it?

No, I wouldn't bother with letting you get it back 😄. Maybe an export method, definitely not a property.

In my ideal world, the API would look something like below. [Candidate for SymmetricAlgorithm2]

I agree with @vcsjones, the mode inputs don't lend themselves to a general "encrypt/decrypt" virtual. Looking at the current model (ignoring CreateEncryptor(key,iv)), the strawman SymmetricAlgorithm2, and what we did for GCM/CCM, the instance is "the key", and then we need to model how things work.

Maybe I'm too influenced by the BCrypt* APIs? :)

Perhaps, but we can't (shouldn't/won't) take a PVOID for "uh, extra stuff needed for the mode...", like the IV/Nonce/TagSize/etc.

You can have a method per-mode, or a type per-mode, and now I'm back to where I started.

Yeah, and both are fine-ish. Type-per-mode makes things like PBES less pleasant since there's no unifying representation of (e.g.) CBC, which is why I'm favoring method(-pair)-per-mode. Type-per-algorithm-and-mode also makes for a lot more namespace-level noise for IntelliSense. (Type-per-mode DOES have the benefit of suggesting that you shouldn't mix modes with the same key (or, at least, not semi-suggesting mixing modes is OK), but I don't feel it does that "strongly" enough to offset the value lost to things like the PBES implementations for processing PKCS8/PKCS12).

[vcsjones]

No, I wouldn't bother with letting you get it back

Sure I agree with that. I took the question to mean "is the key an attribute of the object instance or method call". I meant to say it should not be a method parameter.

[vcsjones]

which is why I'm favoring method(-pair)-per-mode

Well, I suppose you can fix that with moar types...

public interface ICbcMode {
    bool TryEncyptCbc(ReadOnlySpan<byte> plaintext, ReadOnlySpan<byte> iv, Span<byte> destination, out int bytesWritten, PaddingMode paddingMode = default);
    bool TryDecyptCbc(ReadOnlySpan<byte> ciphertext, ReadOnlySpan<byte> iv, Span<byte> destination, out int bytesWritten, PaddingMode paddingMode = default);
}

public class AesCbc : ICbcMode {
}

public class TripleDesCbc : ICbcMode {
}

I'm not sure if I am serious or not, but I see your point.

Type-per-algorithm-and-mode also makes for a lot more namespace-level noise for IntelliSense

Could stick it in a new Primitives namespace.

or, at least, not semi-suggesting mixing modes is OK

I think part of what I don't like about mode methods is that it forever implies that any new mode might apply to all future algorithms. Perhaps AES2 won't define any of these modes. Perhaps it'll only define GCM-SIV.

[bartonjs]

I think part of what I don't like about mode methods is that it forever implies that any new mode might apply to all future algorithms.

There is a bit of a point-in-time "things that work everywhere" (not CCM/GCM) for deciding things can go on SymmetricAlgorithm, but that was really already done by the Mode property. The ultimate recourse is "NotSupportedException" if some future algorithm is not defined for ECB/CBC/etc modes.

But since we use SymmetricAlgorithm for block ciphers, and the block modes apply to all block ciphers, I don't really see a place where it would fall apart. (If we added a stream cipher, it'd fall apart, but so would the current model. If we ever added one it'd probably need a new base class already, or just be its own, standalone, thing.)