Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API Proposal]: add Span<byte> key parameter to SymmetricAlgorithm.EncryptCbc and SymmetricAlgorithm.DecryptCbc #111154

Open
DeagleGross opened this issue Jan 7, 2025 · 13 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Security untriaged New issue has not been triaged by the area owner

Comments

@DeagleGross
Copy link

Background and motivation

Nowdays, aspnetcore DataProtection layer uses SymmetricAlgorithm.CreateDecryptor() and SymmetricAlgorithm.CreateEncryptor() for decryption and encryption operations.

While attempting to improve performance in this PR, I came across a limiting API in SymmetricAlgorithm class (and implementations): there is no way to pass in key of algorithm as Span<byte> or ReadOnlySpan<byte> (only byte[] is available) to Encrypt or Decrypt methods.

The only possibilities now are (below only showing encryption flow, but the same applies to decryption):

  1. allocating encryptor and CryptoStream to write a plain text
using (var symmetricAlgorithm = CreateSymmetricAlgorithm())
using (var cryptoTransform = symmetricAlgorithm.CreateEncryptor(encryptionSubkey, iv))
using (var cryptoStream = new CryptoStream(outputStream, cryptoTransform, CryptoStreamMode.Write))
{
    cryptoStream.Write(plaintext.Array!, plaintext.Offset, plaintext.Count);

the problem with this API is that implementation was using MemoryStream as outputStream, which is not that performant.

  1. set value for SymmetricAlgorithm.Key property
using var symmetricAlgorithm = CreateSymmetricAlgorithm(key: encryptionSubkey);
...
symmetricAlgorithm.EncryptCbc(plainText, iv, outputSpan.Slice(start: keyModifierLength + ivLength, length: cipherTextLength));

the problem with this API is that SymmetricAlgorithm.Key {set} is actually cloning a byte array. However, for aspnetcore DataProtection flow there is no need to do it - we could try to rent or stackalloc memory, fill it with key data, pass it in and after it is used clear it out.

API Proposal

Basically the same kind of API exists for validationAlgorithms. In example HMACSHA256.HashData or HMACSHA512.HashData: HMACSHAxxx are implementations of KeyedHashAlgorithm, and I am able to determine the type and then call static method HashData, which accepts validation algorithm key as ReadOnlySpan<byte>:

if (validationAlgorithm is HMACSHA256)
{
    HMACSHA256.HashData(key: validationSubkey, source: hashSource, destination: correctHash);
}
else if (validationAlgorithm is HMACSHA512)
{
    HMACSHA512.HashData(key: validationSubkey, source: hashSource, destination: correctHash);
}
else // not known type, so we only can call `TryComputeHash()` here
{
    // fill in the key for hash compute operation
    validationAlgorithm.Key = validationSubkeyArray;
    validationAlgorithm.TryComputeHash(hashSource, correctHash, out _);
}

I think we could be able to have something similar on the SymmetricAlgorithm type (or on implementations):
note: I am proposing both Encrypt and Decrypt API here, and probably we should add it for all modes (cbc, cfb, ecb)

public abstract class SymmetricAlgorithm : IDisposable
{
public int EncryptCbc(ReadOnlySpan<byte> plaintext, ReadOnlySpan<byte> iv, Span<byte> destination, PaddingMode paddingMode = PaddingMode.PKCS7);
+ public static int EncryptCbc(ReadOnlySpan<byte> key, ReadOnlySpan<byte> plaintext, ReadOnlySpan<byte> iv, Span<byte> destination, PaddingMode paddingMode = PaddingMode.PKCS7);

public byte[] DecryptCbc(ReadOnlySpan<byte> ciphertext, ReadOnlySpan<byte> iv, PaddingMode paddingMode = PaddingMode.PKCS7);
+ public static byte[] DecryptCbc(ReadOnlySpan<byte> key, ReadOnlySpan<byte> ciphertext, ReadOnlySpan<byte> iv, PaddingMode paddingMode = PaddingMode.PKCS7);
}

API Usage

// make a performant stackalloc 
Span<byte> encryptionSubkey= _symmetricAlgorithmSubkeyLengthInBytes<= 128
    ? stackalloc byte[128].Slice(0, _symmetricAlgorithmSubkeyLengthInBytes)
    : new byte[_symmetricAlgorithmSubkeyLengthInBytes];

// run encryptCbc without any `byte[] key` allocations or copy'ing
SymmetricAlgorithm.EncryptCbc(encryptionSubkey, plainText, iv, outputSpan.Slice(start: keyModifierLength + ivLength, length: cipherTextLength));

Alternative Designs

No response

Risks

No response

@DeagleGross DeagleGross added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Security labels Jan 7, 2025
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jan 7, 2025
@bartonjs
Copy link
Member

bartonjs commented Jan 7, 2025

We definitely can't make it be static methods on SymmetricAlgorithm... we don't know what algorithm to use at that layer.

It's even tricky making it static methods on Aes, et al, since we'd be picking a single provider implementation for how we're going to accomplish it.

And then it's tricky as instance methods because for an Aes instance that's backed by a hardware key, it doesn't make sense to specify a key per-call.

Looking at the comment in PR that suggested it, I wonder if we just want something like

public partial class SymmetricAlgorithm
{
    /// <summary>
    ///  Sets the key to use with this instance, but blocks it from being returned from the
    ///  <see cref="Key" /> property.
    /// </summary>
    public void SetOpaqueKey(ReadOnlySpan<byte> key);

    protected virtual void SetOpaqueKeyCore(ReadOnlySpan<byte> key);
}

It would probably have to default to throwing, but we'd wire it up on all of our types. (When overriding the member, a derived type should do whatever they need to do to make Key not return the answer, but then it would start returning again if someone called set_Key)

@vcsjones
Copy link
Member

vcsjones commented Jan 7, 2025

public void SetOpaqueKey(ReadOnlySpan<byte> key);

Are we able to even reasonably implement it? If I recall correctly, some platforms need the original key between each final transformation (CFB on macOS is ringing a bell) because the underlying OS-thing can't be reset.

So in probably more than a few cases we are just going to behind the scenes convert a Span to a byte array. Or a FixedMemoryKeyBox which just hides the allocation as native memory.

@bartonjs
Copy link
Member

bartonjs commented Jan 7, 2025

Levi's comment that inspired this proposal seems to simplify as "calling set_Key makes a copy you can't clear, and that's bad, mm'kay?". So even just FixedMemoryKeyBox addresses that problem.

@vcsjones
Copy link
Member

vcsjones commented Jan 8, 2025

"calling set_Key makes a copy you can't clear, and that's bad, mm'kay?". So even just FixedMemoryKeyBox addresses that problem.

Why do we need a new API for this? Couldn't we just override get and set on Key to work off a FixedMemoryKeyBox in our internal implementations?

It does mean the KeyValue protected field won't hold a value, but since AesImplementation is an internal type it doesn't matter.

@bartonjs
Copy link
Member

bartonjs commented Jan 8, 2025

Be....cause..... uh.... maybe.... he... also.... meant "and no one should be able to get it back?"

If exportability isn't a concern, then, yeah, we could just change our set_Key on sealed/hidden types to avoid the KeyValue field, and our TryEncryptCbc (and friends) to do use the keybox instead of get_Key.

@vcsjones
Copy link
Member

vcsjones commented Jan 8, 2025

It sounds like we could change our internals to work on the key box regardless. If we want a new API to prevent retrieval, that's fine, but I don't see a reason to not use the key box for the get / set.

@bartonjs
Copy link
Member

bartonjs commented Jan 8, 2025

"I bet I know what Kevin's doing this weekend... oh, wait, today's Tuesday."

@vcsjones
Copy link
Member

vcsjones commented Jan 8, 2025

"I bet I know what Kevin's doing this weekend... oh, wait, today's Tuesday."

10:30 PM, weekend, same thing..

We still might want a new API because

ReadOnlySpan<byte>FixedMemoryKeyBox

is more efficient than

ReadOnlySpan<byte>byte[]FixedMemoryKeyBox

Since the key box just wants contiguous memory, not having a new API would force an allocation.

@mgravell
Copy link
Member

mgravell commented Jan 8, 2025

I'm not even remotely qualified to opine on any part of the implementation pieces, but observation on this part:

  public byte[] DecryptCbc(ReadOnlySpan<byte> ciphertext, ReadOnlySpan<byte> iv, PaddingMode paddingMode = PaddingMode.PKCS7);
+ public static byte[] DecryptCbc(ReadOnlySpan<byte> key, ReadOnlySpan<byte> ciphertext, ReadOnlySpan<byte> iv, PaddingMode paddingMode = PaddingMode.PKCS7);

If there is any possible mechanism to avoid that byte[] return, that would seem desirable (again, I'm talking from a position of ignorance here). Common approaches obviously include a Span<byte> destination (as seen in EncryptCbc) and (increasingly, but not ubiquitously) IBufferWriter<byte> destination (the latter being especially useful if the caller can't reasonably predict the size of the required output buffer). Although the security implications of decrypted data and the risk of people casually using pooled buffers etc may make the Span<byte> approach more desirable. Presumably in any event: with an int return for the number of bytes written.

@mgravell
Copy link
Member

mgravell commented Jan 8, 2025

@vcsjones I openly confess my lack of familiarity with this API, but: can you clarify why we would still need a FixedMemoryKeyBox ? It looks to my casual inspection as though this is primarily to allow a: isolation between the member-based and mutable byte[] key (which we no longer need), and b: to allow that to be passed to the specific method via DangerousKeySpan. It seem to me like we can skip FixedMemoryKeyBox and simply pass the span down as-is, with the only caveat being to perhaps add a fixed to guard against GC movement, presumably a single pin covering the entire operation considered as an atomic unit (although multiple calls may be required internally). Since this is a synchronous API, fixed should present no difficulty; and if this was an async API: there is an abstracted pin API in the Memory<T> mataphor that allows for async-compatible pins.

@vcsjones
Copy link
Member

vcsjones commented Jan 8, 2025

I'm not even remotely qualified to opine on any part of the implementation pieces, but observation on this part:

I think we first need to sort out “Can or do we want to even have an API that accepts the key as part of the one shot”. There are some edge cases that make that hard, like AesCng implementation.

Jeremy suggested that the answer might be “no”, no key parameter and a key is really part of an instance of the algorithm.

I am actually not sure that makes the suggested idea disqualifying. The API could be:

public partial class Aes {
    public static int EncryptCbc(
        ReadOnlySpan<byte> plaintext,
        ReadOnlySpan<byte> key,
        ReadOnlySpan<byte> iv,
        Span<byte> destination,
        PaddingMode paddingMode = PaddingMode.PKCS7);
    // Repeat for byte[], int overloads, and Decrypt, and Cfb and Ecb, and other algorithms.
}

That is at least implementable but we are talking about dozens of new APIs.

Being a static though has the disadvantage that it isn’t really extendable. These algorithms are not sealed, and many other developers have derived from these types to add their own implementations. They could just shadow the static with new on their own derived type though.

If there is any possible mechanism to avoid that byte[] return

The one-shots as they exist today already have the usual “bool TryFoo” and “int Foo” overloads so if another one-shot is added we could certainly write to an existing buffer.

@DeagleGross
Copy link
Author

Thanks for explaining @vcsjones, I dont have any expertise in a field, but I find SymmetricAlgorithm API very similar to the HashAlgorithm: both are abstract classes providing a general-purpose API but both requiring key and not having an API to pass it for a "one shot"

public byte[] ComputeHash(byte[] buffer, int offset, int count) { throw null; }

But implementations of HashAlgorithm do have a static method with a key parameter for a one-shot:

public static byte[] HashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source)

Maybe if the cases are similar we can follow the same approach? Or my guess is incorrect?

@mgravell
Copy link
Member

mgravell commented Jan 8, 2025

Being a static though has the disadvantage that it isn’t really extendable.

Agree there's something funky there; any views on static abstract interface methods (C# 11+) limited to suitable runtimes? i.e. something like ISymmetricEncryptor with suitable static abstract method/methods?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Security untriaged New issue has not been triaged by the area owner
Projects
None yet
Development

No branches or pull requests

4 participants