[API Proposal]: Add Span<byte> support to System.Security.Cryptography.ProtectedData

[ChadNedzlek]

Background and motivation

Most cryptographic operations in .NET support passing spans for input and output, however, the ProtectedData type does not,
and still requires allocation of arrays for both input and output.

API Proposal

namespace System.Security.Cryptography;

public partial class ProtectedData {
    public static bool TryProtect(
        ReadOnlySpan<byte> userData,
        DataProtectionScope scope,
        Span<byte> destination,
        out int bytesWritten,
        ReadOnlySpan<byte> optionalEntropy = default);

    public static bool TryUnprotect(
        ReadOnlySpan<byte> encryptedData,
        DataProtectionScope scope,
        Span<byte> destination,
        out int bytesWritten,
        ReadOnlySpan<byte> optionalEntropy = default);

    public static int Protect(
        ReadOnlySpan<byte> userData,
        DataProtectionScope scope,
        Span<byte> destination,
        ReadOnlySpan<byte> optionalEntropy = default);

    public static int Unprotect(
        ReadOnlySpan<byte> encryptedData,
        DataProtectionScope scope,
        Span<byte> destination,
        ReadOnlySpan<byte> optionalEntropy = default);

    // Below: Maybe?
    public static byte[] Protect(
        ReadOnlySpan<byte> userData,
        DataProtectionScope scope,
        ReadOnlySpan<byte> optionalEntropy = default);

    public static byte[] Unprotect(
        ReadOnlySpan<byte> encryptedData,
        DataProtectionScope scope,
        ReadOnlySpan<byte> optionalEntropy = default);
}

API Usage

public void DoStuff(byte [] protectedValue) {
  Span<byte> exposedValue = stackalloc byte[50];
  if (!ProtectedData.TryUnprotect(protectedValue, exposedValue, DataProtectionScope.CurrentUser, out int cb)) {
    throw new InvalidOperationException();
  }
  // use the exposed value
  exposedValue.Clear();
}

Alternative Designs

No response

Risks

Should be low risk, it's adding Span overrides to a byte[] input/output API much like has been done for most other crypto operations.

[dotnet-policy-service]

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

[bartonjs]

I believe that Unprotect always reduces the size of the input, so

byte[] dest = new byte[source.Length];
ProtectedData.TryUnprotect(source, dest, ...);

is always guaranteed to return true. In that case, it's probably goodness to have the non-Try variant... and it might also make sense for Protect.

public partial class ProtectedData
{
    public static int Protect(ReadOnlySpan<byte> source, Span<byte> destination, DataProtectionScope scope);
    public static int Unprotect(ReadOnlySpan<byte> source, Span<byte> destination, DataProtectionScope scope);
}

[bartonjs]

Looks like the optionalEntropy parameter got dropped? It can move to the final position and default to the default (empty) ReadOnlySpan

[vcsjones]

@bartonjs the S.S.C.ProtectedData API and package are marked as "accepts critical fixes only"

We only consider fixes that unblock critical issues

So, we could consider Span APIs here, but, at first glance doesn't seem like it meets the bar.

[bartonjs]

Given the simplicity of it, and that the ability to write to pre-pinned memory might be critical... I think it's goodness to take it.

[vcsjones]

Fair enough. It sounds like you are proposing this.

To throw it out there, if we are adding two of them, do we want to go for our "quadfecta" and add a span in, allocating byte out?

namespace System.Security.Cryptography;

public partial class ProtectedData {
    public static bool TryProtect(
        ReadOnlySpan<byte> userData,
        DataProtectionScope scope,
        Span<byte> destination,
        out int bytesWritten,
        ReadOnlySpan<byte> optionalEntropy = default);

    public static bool TryUnprotect(
        ReadOnlySpan<byte> userData,
        DataProtectionScope scope,
        Span<byte> destination,
        out int bytesWritten,
        ReadOnlySpan<byte> optionalEntropy = default);

    public static int Protect(
        ReadOnlySpan<byte> userData,
        DataProtectionScope scope,
        Span<byte> destination,
        ReadOnlySpan<byte> optionalEntropy = default);

    public static int Unprotect(
        ReadOnlySpan<byte> userData,
        DataProtectionScope scope,
        Span<byte> destination,
        ReadOnlySpan<byte> optionalEntropy = default);

    // Below: Maybe?
    public static byte[] Protect(
        ReadOnlySpan<byte> userData,
        DataProtectionScope scope,
        ReadOnlySpan<byte> optionalEntropy = default);

    public static byte[] Unprotect(
        ReadOnlySpan<byte> userData,
        DataProtectionScope scope,
        ReadOnlySpan<byte> optionalEntropy = default);
}

[bartonjs]

It's starting to feel like overkill, but may as well take the full set to review.

Was flipping destination and scope intentional? (source, destination, scope, out written, optionalEntropy=default) looks like the right order to me given RSA.TrySignHash (the example in FDGv3 for how to order the parameters), but maybe we've recently felt differently about that order and you're remembering something I'm not?

[vcsjones]

but maybe we've recently felt differently about that order and you're remembering something I'm not?

I waffled on that. I used AesGcm and SP800108 as inspiration.

In order, I end up thinking things as

1. All inputs (userData, scope)
2. Things we write to (destination) (kind of an out, so at the end)
3. Actual out parameters
4. Optional parameters

Which is how GCM looks

public void Decrypt (ReadOnlySpan<byte> nonce, 
ReadOnlySpan<byte> ciphertext, 
ReadOnlySpan<byte> tag,
Span<byte> plaintext, ReadOnlySpan<byte> associatedData = default);

Granted, scope feels a little different because it isn't an input into an algorithm itself, but I didn't care to make the distinction.

[vcsjones]

One thing to consider here, is that ProtectedData on .NET Framework is a type-forward. We can't partially type-forward, so that would mean

1. The new API will not be available on .NET Framework, only .NET (Core), even if you take a dependency on a package.
2. We undo the type forward (no idea of the implications of that, but my first thought is "sounds bad")
3. We put these on a different class than ProtectedData that does not type forward at all.

@ChadNedzlek was your expectation to be able to use the span-overloads from a down level .NET platform?

[ChadNedzlek]

@vcsjones No, it was not. Only forward versions of .NET

[bartonjs]

Video

• We flipped destination and scope to match guidelines
• We discussed whether we need a GetEstimatedSize, and decided no because it's not feasible to implement.

namespace System.Security.Cryptography;

public partial class ProtectedData {
    public static bool TryProtect(
        ReadOnlySpan<byte> userData,
        Span<byte> destination,
        DataProtectionScope scope,
        out int bytesWritten,
        ReadOnlySpan<byte> optionalEntropy = default);

    public static bool TryUnprotect(
        ReadOnlySpan<byte> encryptedData,
        Span<byte> destination,
        DataProtectionScope scope,
        out int bytesWritten,
        ReadOnlySpan<byte> optionalEntropy = default);

    public static int Protect(
        ReadOnlySpan<byte> userData,
        Span<byte> destination,
        DataProtectionScope scope,
        ReadOnlySpan<byte> optionalEntropy = default);

    public static int Unprotect(
        ReadOnlySpan<byte> encryptedData,
        Span<byte> destination,
        DataProtectionScope scope,
        ReadOnlySpan<byte> optionalEntropy = default);

    public static byte[] Protect(
        ReadOnlySpan<byte> userData,
        DataProtectionScope scope,
        ReadOnlySpan<byte> optionalEntropy = default);

    public static byte[] Unprotect(
        ReadOnlySpan<byte> encryptedData,
        DataProtectionScope scope,
        ReadOnlySpan<byte> optionalEntropy = default);
}

[vcsjones]

@ChadNedzlek Jeremy mentioned you were looking to implement this, so I'll assign this to you for now. Feel free to unassign yourself if that is no the case.

[ChadNedzlek]

Got the code written, now just need to copy paste the existing tests 6 times. :-)

[bartonjs]

Simple functional tests can avoid duplication by changing the current class to an abstract, making all of the test methods instance, and deciding on a common shape to do the verification with. Then use derived classes to adapt the one form to another.

Usually our "least common denominator" is the array-in/array-out version. Like in https://github.com/dotnet/runtime/blob/main/src/libraries/System.Security.Cryptography/tests/HKDFTests.cs.

Then, for things like "the Try returns false/0 for too small" (and the non-Try throws) and a particular test show that it works for a goldilocks-sized buffer and a too-big buffer (accuracy of length was verified by the other tests, really), methods on the appropriate derived type (which the derived types at the bottom of the HKDFTests do).