WASM error handling in SubtleCrypto through web worker

[AaronRobinsonMSFT]

The work to enable WASM to use SubtleCrypto, #65966, had some follow ups on how to handle error cases. Specifically, the communication channel with the web worker could be hardened to convey more errors/failures. This could be accomplished through throwing an exception or the web worker communicating a failure case explicitly.

Source hints:

• crypto-worker.ts
  • see read_response and consider throwing an error.
• dotnet-crypto-worker.js
  • The reading in of a request could be including in the try/catch in await_request.
  • The await_request function could return a bespoke "error occurred" message instead of an exception string.

See #69741

/cc @radical @ericstj @layomia

[ghost]

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

Issue Details

---

The work to enable WASM to use SubtleCrypto, #65966, had some follow ups on how to handle error cases. Specifically, the communication channel with the web worker could be hardened to convey more errors/failures. This could be accomplished through throwing an exception or the web worker communicating a failure case explicitly.

Source hints:

• crypto-worker.ts
  • see read_response and consider throwing an error.
• dotnet-crypto-worker.js
  • The reading in of a request could be including in the try/catch in await_request.
  • The await_request function could return a bespoke "error occurred" message instead of an exception string.

/cc @radical @ericstj @layomia

Author:	AaronRobinsonMSFT
Assignees:	-
Labels:	arch-wasm, area-System.Security
Milestone:	7.0.0

[radical]

Another suggestion would be for the tests to confirm when running on browser that it did actually use the worker implementation.

[AaronRobinsonMSFT]

Another suggestion would be for the tests to confirm when running on browser that it did actually use the worker implementation.

I think this is being done. See src/tests/BuildWasmApps/Wasm.Build.Tests/NativeLibraryTests.cs. There is a check for console output. I commented on this in the JavaScript file.

[radical]

I think this is being done. See src/tests/BuildWasmApps/Wasm.Build.Tests/NativeLibraryTests.cs. There is a check for console output. I commented on this in the JavaScript file.

Yes, but the xharness webserver is setup in two different places for Wasm.Build.Tests, and the library tests. I was just suggesting that we add some kinda confirmation for the library tests also, so it doesn't accidentally start using the fallback, and we don't find out.

[eerhardt]

Note: when fixing this, we also need to ensure that the "lock" added in #70185 is cleared on error.

[lewing]

What is left to do here?

[radical]

I think this is being done. See src/tests/BuildWasmApps/Wasm.Build.Tests/NativeLibraryTests.cs. There is a check for console output. I commented on this in the JavaScript file.

Yes, but the xharness webserver is setup in two different places for Wasm.Build.Tests, and the library tests. I was just suggesting that we add some kinda confirmation for the library tests also, so it doesn't accidentally start using the fallback, and we don't find out.

Only this, if @eerhardt thinks we should do it.

[eerhardt]

that we add some kinda confirmation for the library tests

@radical - do you know how we would do this in the libraries tests?

[radical]

@radical - do you know how we would do this in the libraries tests?

One simple check would be to call Interop.BrowserCrypto.CanUseSubtleCrypto via reflection, and compare it against an environment variable which indicates whether we are expecting to use subtlecrypto. We can do this as:

1. Add a separate test that does this
2. Or do this is all the relevant test class' .ctor . This would catch the case where some tests do use subtlecrypto, then one causes an error, and the subsequent tests would end up using the managed implementation. This can work because, once we hit an error which disables the use of subtlecrypto, Interop.BrowserCrypto.CanUseSubtleCrypto should return false.

runtime/src/mono/wasm/runtime/crypto-worker.ts

Lines 120 to 123 in ed2a5a1

	worker.onerror = event => {
	console.warn(`MONO_WASM: Error in Crypto WebWorker. Cryptography digest calls will fallback to managed implementation. Error: ${event.message}`);
	mono_wasm_crypto = null;
	};

[eerhardt]

This can work because, once we hit an error which disables the use of subtlecrypto, Interop.BrowserCrypto.CanUseSubtleCrypto should return false.

This is actually a problem in the current code, because of the following C# code:

runtime/src/libraries/Common/src/Interop/Browser/System.Security.Cryptography.Native.Browser/Interop.SubtleCrypto.cs

Line 21 in 8f75cc9

internal static readonly bool CanUseSubtleCrypto = CanUseSubtleCryptoImpl() == 1;

This only gets called once, and then cached in the field. So if an error happens subsequently, we don't P/Invoke back into JS code to see if mono_wasm_crypto is null or not.

How fast is the P/Invokes in WASM? Should this CanUseSubtleCrypto always call into the JS code every time it is invoked?

[bartonjs]

This can work because, once we hit an error which disables the use of subtlecrypto, Interop.BrowserCrypto.CanUseSubtleCrypto should return false.

I dunno. I think (in all sincerity) we should just let all crypto operations blow up.