Enable Rfc2898DeriveBytes on Browser WASM #71768

eerhardt · 2022-07-07T15:24:48Z

Marks the APIs as supported on Browser, and enables Rfc2898 tests on Browser WASM.
Use SubtleCrypto deriveBits API to implement one shot Pbkdf2.

Also, make HKDF supported on Browser.

Contributes to #40074

dotnet-issue-labeler · 2022-07-07T15:24:56Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2022-07-07T15:25:01Z

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

Issue Details

Marks the APIs as supported on Browser, and enables Rfc2898 tests on Browser WASM.
Use SubtleCrypto deriveBits API to implement one shot Pbkdf2.

Contributes to #40074

(In draft until #71693 is merged, since this is built on top of that PR.)

Author:	eerhardt
Assignees:	eerhardt
Labels:	`area-System.Security`, `new-api-needs-documentation`
Milestone:	-

ghost · 2022-07-07T16:18:15Z

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

Marks the APIs as supported on Browser, and enables Rfc2898 tests on Browser WASM.
Use SubtleCrypto deriveBits API to implement one shot Pbkdf2.

Contributes to #40074

(In draft until #71693 is merged, since this is built on top of that PR.)

Author:	eerhardt
Assignees:	eerhardt
Labels:	`arch-wasm`, `area-System.Security`, `new-api-needs-documentation`
Milestone:	-

src/libraries/Common/src/System/Security/Cryptography/HashOneShotHelpers.cs

src/libraries/Common/src/System/Security/Cryptography/PasswordBasedEncryption.cs

...em.Security.Cryptography/src/System/Security/Cryptography/HMACHashProvider.Browser.Native.cs

...ystem.Security.Cryptography/src/System/Security/Cryptography/Pbkdf2Implementation.Browser.cs

...tem.Security.Cryptography/src/System/Security/Cryptography/SHAHashProvider.Browser.Native.cs

Marks the APIs as supported on Browser, and enables Rfc2898 tests on Browser WASM. Use SubtleCrypto deriveBits API to implement one shot Pbkdf2. Contributes to dotnet#40074

stephentoub · 2022-07-07T20:14:55Z

...ystem.Security.Cryptography/src/System/Security/Cryptography/Pbkdf2Implementation.Browser.cs

+        {
+            using (Rfc2898DeriveBytes deriveBytes = new Rfc2898DeriveBytes(
+                password.ToArray(),
+                salt.ToArray(),


There's no way to stay in the span universe all the way down? Are we accepting arrays from public entrypoints, passing them around as span, and then ToArray'ing here?

It's possible but we've avoided optimizing the non-one-shot implementation of PBKDF2 when we've talked about it in the past.

Note that this is the way the existing "mobile" platforms are implementing this functionality:

runtime/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/Pbkdf2Implementation.Managed.cs

Lines 27 to 36 in 6a02d5d

using (Rfc2898DeriveBytes deriveBytes = new Rfc2898DeriveBytes(

password.ToArray(),

salt.ToArray(),

iterations,

hashAlgorithmName,

clearPassword: true))

{

byte[] result = deriveBytes.GetBytes(destination.Length);

result.AsSpan().CopyTo(destination);

}

which is used by Android and non-macOS AppleCrypto (iOS, tvOS, etc.).

There's no way to stay in the span universe all the way down?

Not without some decent refactoring to Rfc2898DeriveBytes in order to make the managed algorithm completely "one shot", and then build the instance based Rfc2898DeriveBytes methods on top of it.

Are we accepting arrays from public entrypoints, passing them around as span, and then ToArray'ing here?

In one overload, yes, and in most overloads no. This method is called from these public end-points:

runtime/src/libraries/System.Security.Cryptography/ref/System.Security.Cryptography.cs

Lines 1734 to 1739 in 6a02d5d

public static byte[] Pbkdf2(byte[] password, byte[] salt, int iterations, System.Security.Cryptography.HashAlgorithmName hashAlgorithm, int outputLength) { throw null; }

public static byte[] Pbkdf2(System.ReadOnlySpan<byte> password, System.ReadOnlySpan<byte> salt, int iterations, System.Security.Cryptography.HashAlgorithmName hashAlgorithm, int outputLength) { throw null; }

public static void Pbkdf2(System.ReadOnlySpan<byte> password, System.ReadOnlySpan<byte> salt, System.Span<byte> destination, int iterations, System.Security.Cryptography.HashAlgorithmName hashAlgorithm) { }

public static byte[] Pbkdf2(System.ReadOnlySpan<char> password, System.ReadOnlySpan<byte> salt, int iterations, System.Security.Cryptography.HashAlgorithmName hashAlgorithm, int outputLength) { throw null; }

public static void Pbkdf2(System.ReadOnlySpan<char> password, System.ReadOnlySpan<byte> salt, System.Span<byte> destination, int iterations, System.Security.Cryptography.HashAlgorithmName hashAlgorithm) { }

public static byte[] Pbkdf2(string password, byte[] salt, int iterations, System.Security.Cryptography.HashAlgorithmName hashAlgorithm, int outputLength) { throw null; }

Only the top overload is taking 2 byte[]s, passing them through as spans, and then ToArray'ing here. The bottom overload takes a string, which turns into UTF8 bytes in either a rented array or stackalloc'd span, and then passed in here.

I think it would be simpler to implement a one shot in managed code with IncrementalHash instead of trying to get the streaming PBKDF2 to allocate less. I have a branch with this. Happy to upstream it as a follow up to this.

I have a branch with this. Happy to upstream it as a follow up to this.

I think that would be great!

bartonjs · 2022-07-07T23:30:40Z

src/mono/wasm/runtime/workers/dotnet-crypto-worker.js

@@ -256,6 +256,24 @@ async function sign(type, key, data) {
    return Array.from(new Uint8Array(signResult));
 }

+async function derive_bits(password, salt, iterations, hashAlgorithm, lengthInBytes) {


It feels like this function should either a) take PBKDF2 as a parameter, or b) include it in the name; since it's not applying a general/generic KDF, but a specific one.

The approach I've been taking (for sign - HMAC - and encrypt_decrypt - AES-CBC) is to name the methods the same as the SubtleCrypto APIs, and then taking whatever parameters it needs from the managed code for now. And when we need to add more parameters/options in the future, we would add the parameters/refactor then.

This way it keeps it simple for now, and if we don't need to add anymore in the future it keeps it simple then too.

Adding the parameters now doesn't guarantee that other params can be used in the future without adding more parameters and needing to refactor then as well. For example, if we wanted to add ECDH, we would need to pass a public key as well.

At least that was my reasoning. Thoughts? I can change this approach to one of the above, if you want.

Hm. Since the calling code and the acting code get delivered together I guess the signature is allowed to be modified as needed, so... I suppose it works.

Yeah, if these APIs were public or somehow permanent, I would take a much different approach.

bartonjs · 2022-07-07T23:35:40Z

LGTM, other than the concern about the naming/signature of derive_bits.

eerhardt · 2022-07-08T04:03:05Z

/azp run runtime-wasm

azure-pipelines · 2022-07-08T04:03:14Z

Azure Pipelines successfully started running 1 pipeline(s).

eerhardt · 2022-07-08T17:21:38Z

AOT wasm failures are #71838.

dotnet-issue-labeler bot added area-System.Security new-api-needs-documentation labels Jul 7, 2022

ghost assigned eerhardt Jul 7, 2022

teo-tsirpanis added the arch-wasm WebAssembly architecture label Jul 7, 2022

vcsjones reviewed Jul 7, 2022

View reviewed changes

Enable Rfc2898DeriveBytes on Browser WASM

6906aa2

Marks the APIs as supported on Browser, and enables Rfc2898 tests on Browser WASM. Use SubtleCrypto deriveBits API to implement one shot Pbkdf2. Contributes to dotnet#40074

stephentoub reviewed Jul 7, 2022

View reviewed changes

PR feedback

7e3672f

bartonjs added the cryptographic-docs-impact label Jul 7, 2022

Mark HKDF as supported on Browser and enable tests

31d287e

eerhardt force-pushed the Pbkdf2 branch from cc3cb5d to 31d287e Compare July 7, 2022 23:17

eerhardt marked this pull request as ready for review July 7, 2022 23:17

eerhardt requested a review from lewing as a code owner July 7, 2022 23:17

bartonjs reviewed Jul 7, 2022

View reviewed changes

Fix tests to exclude MD5

559eef9

bartonjs approved these changes Jul 8, 2022

View reviewed changes

eerhardt merged commit 8f75cc9 into dotnet:main Jul 9, 2022

eerhardt deleted the Pbkdf2 branch July 9, 2022 02:51

ghost locked as resolved and limited conversation to collaborators Aug 8, 2022

bartonjs added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Aug 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Rfc2898DeriveBytes on Browser WASM #71768

Enable Rfc2898DeriveBytes on Browser WASM #71768

eerhardt commented Jul 7, 2022 •

edited

Loading

dotnet-issue-labeler bot commented Jul 7, 2022

ghost commented Jul 7, 2022

ghost commented Jul 7, 2022

stephentoub Jul 7, 2022

vcsjones Jul 7, 2022 •

edited

Loading

eerhardt Jul 7, 2022

vcsjones Jul 7, 2022

eerhardt Jul 8, 2022

bartonjs Jul 7, 2022

eerhardt Jul 8, 2022

bartonjs Jul 8, 2022

eerhardt Jul 8, 2022

bartonjs commented Jul 7, 2022

eerhardt commented Jul 8, 2022

azure-pipelines bot commented Jul 8, 2022

eerhardt commented Jul 8, 2022

	using (Rfc2898DeriveBytes deriveBytes = new Rfc2898DeriveBytes(
	password.ToArray(),
	salt.ToArray(),
	iterations,
	hashAlgorithmName,
	clearPassword: true))
	{
	byte[] result = deriveBytes.GetBytes(destination.Length);
	result.AsSpan().CopyTo(destination);
	}

	public static byte[] Pbkdf2(byte[] password, byte[] salt, int iterations, System.Security.Cryptography.HashAlgorithmName hashAlgorithm, int outputLength) { throw null; }
	public static byte[] Pbkdf2(System.ReadOnlySpan<byte> password, System.ReadOnlySpan<byte> salt, int iterations, System.Security.Cryptography.HashAlgorithmName hashAlgorithm, int outputLength) { throw null; }
	public static void Pbkdf2(System.ReadOnlySpan<byte> password, System.ReadOnlySpan<byte> salt, System.Span<byte> destination, int iterations, System.Security.Cryptography.HashAlgorithmName hashAlgorithm) { }
	public static byte[] Pbkdf2(System.ReadOnlySpan<char> password, System.ReadOnlySpan<byte> salt, int iterations, System.Security.Cryptography.HashAlgorithmName hashAlgorithm, int outputLength) { throw null; }
	public static void Pbkdf2(System.ReadOnlySpan<char> password, System.ReadOnlySpan<byte> salt, System.Span<byte> destination, int iterations, System.Security.Cryptography.HashAlgorithmName hashAlgorithm) { }
	public static byte[] Pbkdf2(string password, byte[] salt, int iterations, System.Security.Cryptography.HashAlgorithmName hashAlgorithm, int outputLength) { throw null; }

Enable Rfc2898DeriveBytes on Browser WASM #71768

Enable Rfc2898DeriveBytes on Browser WASM #71768

Conversation

eerhardt commented Jul 7, 2022 • edited Loading

dotnet-issue-labeler bot commented Jul 7, 2022

ghost commented Jul 7, 2022

ghost commented Jul 7, 2022

Choose a reason for hiding this comment

vcsjones Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartonjs commented Jul 7, 2022

eerhardt commented Jul 8, 2022

azure-pipelines bot commented Jul 8, 2022

eerhardt commented Jul 8, 2022

eerhardt commented Jul 7, 2022 •

edited

Loading

vcsjones Jul 7, 2022 •

edited

Loading