[wasm][crypto] RandomNumberGenerator mapped to Web Crypto getRandomValues #42728

kjpou1 · 2020-09-25T10:05:27Z

Uses Web Crypto API getRandomValues if available.
Add javascript bridge implementation library to Native source tree. …
- Javascript checks for crypto interface and uses crypto.getRandomValues
- Add api bridge call when building for emscripten browser.
- If we couldn't find a proper implementation, as Math.random() is not suitable we will abort.


ABORT: no cryptographic support found getRandomValues. Consider polyfilling it if you want to use something insecure like Math.random(), e.g. put this in a --pre-js: var crypto = { getRandomValues: function(array) { for (var i = 0; i < array.length; i++) array[i] = (Math.random()*256)|0 } };

closes #42727

Part of #40074 and #40074 (comment)

…lues - Uses Web Crypto API [`getRandomValues`](https://www.w3.org/TR/WebCryptoAPI/#Crypto-method-getRandomValues) if available. - Falls back to `/dev/urandom` as default if the crypto library is missing.

ghost · 2020-09-25T10:05:32Z

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @jeffhandley
See info in area-owners.md if you want to be subscribed.

steveisok · 2020-09-25T10:37:24Z

Doesn't /dev/urandom result in a call to Crypto.getRandomValues via the emscripten implementation?

kjpou1 · 2020-09-25T12:17:36Z

Looks like it does: https://github.com/paulshapiro/emscripten/blob/master/src/library_fs.js#L1296

We can close this if we want to continue using the default implementation

src/libraries/Native/Unix/System.Native/pal_random.c

GrabYourPitchforks · 2020-09-26T01:51:39Z

Falls back to /dev/urandom as default if the crypto library is missing.

Is this even possible? Is there any wasm host which does not provide a subtle crypto implementation? And if there is, why not force the host to polyfill as in #42731?

steveisok · 2020-09-26T13:19:57Z

Falls back to /dev/urandom as default if the crypto library is missing.

Is this even possible? Is there any wasm host which does not provide a subtle crypto implementation? And if there is, why not force the host to polyfill as in #42731?

This is just about the best way to call https://developer.mozilla.org/en-US/docs/Web/API/Crypto/getRandomValues. It's the only API that isn't promised based and we can actually invoke.

To be honest, this already works and unless I'm missing something, I would recommend we close the PR.

GrabYourPitchforks · 2020-09-26T16:54:31Z

Let me rephrase the requirement:

RandomNumberGenerator's built-in implementation must call Crypto.getRandomValues or otherwise use a high-quality source of entropy. It would be better if it made this call directly, but it would also be acceptable to make this call indirectly, assuming there's no low-quality entropy fallback code path. The emscripten implementation doesn't seem to fulfill this latter requirement.

src/libraries/Native/Unix/System.Native/pal_random.c

lewing · 2020-09-26T18:40:30Z

emscripten-core/emscripten#12240 landed recently which will expose getentropy as a very thin wrapper over subtle crypto which is what the existing wasm /dev/random uses. The code does fall back to requiring crypto.randomBytes on node and fails if that is not supported. I propose we stick with using /dev/random until we update emscripten then call getentropy from pal_random for wasm. Does this satisfy all the requirements?

lewing · 2020-09-26T21:27:53Z

Falls back to /dev/urandom as default if the crypto library is missing.

Is this even possible? Is there any wasm host which does not provide a subtle crypto implementation? And if there is, why not force the host to polyfill as in #42731?

This just works because emscripten falls back to node's crypto.randomBytes for both /dev/random and /dev/urandom any code should handle the node case and not fall back to /dev/urandom

lewing

This has several issues, it should be implemented in terms of calling getentropy regardless of where the implementation lives.

…crypto-getRandomValues

- Javascript checks for crypto interface and uses `crypto.getRandomValues` - Add api bridge call when building for emscripten browser. - separate out into browser subdirectory - If we couldn't find a proper implementation, as Math.random() is not suitable we will abort. ``` ABORT: no cryptographic support found getRandomValues. Consider polyfilling it if you want to use something insecure like Math.random(), e.g. put this in a --pre-js: var crypto = { getRandomValues: function(array) { for (var i = 0; i < array.length; i++) array[i] = (Math.random()*256)|0 } }; ```

…ingle value.

src/libraries/Native/Unix/System.Native/browser/pal_random_browser.h

src/libraries/Native/Unix/System.Native/browser/pal_entropy_bridge.js

…crypto-getRandomValues

jkotas · 2020-09-30T14:49:42Z

src/libraries/Native/Unix/System.Native/pal_random.js

+            return 0;
+        } else {
+            // we couldn't find a proper implementation, as Math.random() is not suitable
+            abort("no cryptographic support found for getRandomValues. Consider polyfilling it if you want to use something insecure like Math.random(), e.g. put this in a --pre-js: var crypto = { getRandomValues: function(array) { for (var i = 0; i < array.length; i++) array[i] = (Math.random()*256)|0 } };");


I do not think we should be suggesting an unsecure polyfill in the error message.

I think we should leave it for the documentation to discuss the polyfill workarounds. The recommended workaround should be something that tries to be secure as much as possible, like the MSR library discussed in the other PR. If somebody wants to use insecure polyfill, I would leave it up to them to figure out how to do it.

cc @GrabYourPitchforks

Changed to let normal managed exception to handle error.

WASM-ERR: WASM-ERR: Unhandled Exception: WASM-ERR: System.Security.Cryptography.CryptographicException: Error occurred during a cryptographic operation. WASM-ERR: at Interop.GetCryptographicallySecureRandomBytes(Byte* buffer, Int32 length) WASM-ERR: at System.Security.Cryptography.RandomNumberGeneratorImplementation.GetBytes(Byte* pbBuffer, Int32 count) WASM-ERR: at System.Security.Cryptography.RandomNumberGeneratorImplementation.GetBytes(Span`1 data) WASM-ERR: at System.Security.Cryptography.RandomNumberGeneratorImplementation.GetNonZeroBytes(Span`1 data) WASM-ERR: at System.Security.Cryptography.RandomNumberGeneratorImplementation.GetNonZeroBytes(Byte[] data)

Does this work?

src/mono/wasm/runtime-test.js

CoffeeFlux

LGTM modulo the things Jan mentioned. Thanks!

src/mono/wasm/runtime-test.js

src/libraries/Native/Unix/System.Native/pal_random.c

lewing

Address the open reviews and this looks good.

src/mono/wasm/runtime-test.js

Co-authored-by: Ryan Lucia <ryan@luciaonline.net>

…crypto-getRandomValues

…allows the managed code exception flow to continue as normal.

[wasm][crypto] RandomNumberGenerator mapped to Web Crypto getRandomVa…

f0a662c

…lues - Uses Web Crypto API [`getRandomValues`](https://www.w3.org/TR/WebCryptoAPI/#Crypto-method-getRandomValues) if available. - Falls back to `/dev/urandom` as default if the crypto library is missing.

kjpou1 requested a review from marek-safar as a code owner September 25, 2020 10:05

Dotnet-GitSync-Bot added the area-System.Security label Sep 25, 2020

kjpou1 requested review from CoffeeFlux, akoeplinger, lewing and steveisok September 25, 2020 10:06

kjpou1 mentioned this pull request Sep 25, 2020

[wasm][tests] Use a polyfill library to provide Web Crypto API #42731

Closed

jkotas reviewed Sep 25, 2020

View reviewed changes

src/libraries/Native/Unix/System.Native/pal_random.c Outdated Show resolved Hide resolved

kjpou1 requested a review from jkotas September 25, 2020 14:06

jkotas reviewed Sep 26, 2020

View reviewed changes

src/libraries/Native/Unix/System.Native/pal_random.c Outdated Show resolved Hide resolved

lewing requested changes Sep 26, 2020

View reviewed changes

kjpou1 added 5 commits September 28, 2020 09:10

Remove the emscripten interface code from the driver.c.

eb46e2f

Merge branch 'master' of https://github.com/dotnet/runtime into wasm-…

ce149c9

…crypto-getRandomValues

remove extraneous code comment

878cfcf

Move emscripten definition around.

289efef

Address review comment

b5d372b

marek-safar added the arch-wasm WebAssembly architecture label Sep 28, 2020

kjpou1 added 4 commits September 29, 2020 06:33

Merge branch 'master' of https://github.com/dotnet/runtime into wasm-…

f657f4f

…crypto-getRandomValues

Change tests to set random values of the buffer instead of return a s…

88fa846

…ingle value.

Remove old test code

2b8987d

kjpou1 requested a review from lewing September 29, 2020 07:47

Incorporate the PAL bridge layer into the --js-library build process

ce13ad1

jkotas reviewed Sep 29, 2020

View reviewed changes

src/libraries/Native/Unix/System.Native/browser/pal_random_browser.h Outdated Show resolved Hide resolved

jkotas reviewed Sep 29, 2020

View reviewed changes

src/libraries/Native/Unix/System.Native/browser/pal_random_browser.h Outdated Show resolved Hide resolved

jkotas reviewed Sep 29, 2020

View reviewed changes

src/libraries/Native/Unix/System.Native/browser/pal_entropy_bridge.js Outdated Show resolved Hide resolved

This was referenced Sep 29, 2020

Test initial triage jaredpar/runfo#50

Closed

Test initial triage jaredpar/runfo#51

Closed

kjpou1 added 2 commits September 30, 2020 06:22

Merge branch 'master' of https://github.com/dotnet/runtime into wasm-…

4eb6639

…crypto-getRandomValues

Address review comments about directory structure and naming

5e593bb

kjpou1 requested a review from jkotas September 30, 2020 06:49

jkotas reviewed Sep 30, 2020

View reviewed changes

src/mono/wasm/runtime-test.js Show resolved Hide resolved

runfoapp bot mentioned this pull request Sep 30, 2020

Mono System.Text.Json.Tests on Windows timing out #42677

Open

CoffeeFlux reviewed Sep 30, 2020

View reviewed changes

src/mono/wasm/runtime-test.js Show resolved Hide resolved

src/mono/wasm/runtime-test.js Outdated Show resolved Hide resolved

src/mono/wasm/runtime-test.js Outdated Show resolved Hide resolved

marek-safar reviewed Sep 30, 2020

View reviewed changes

src/libraries/Native/Unix/System.Native/pal_random.c Show resolved Hide resolved

src/libraries/Native/Unix/System.Native/pal_random.c Show resolved Hide resolved

lewing reviewed Oct 1, 2020

View reviewed changes

src/mono/wasm/runtime-test.js Show resolved Hide resolved

kjpou1 and others added 5 commits October 1, 2020 05:48

Update src/mono/wasm/runtime-test.js

76de399

Co-authored-by: Ryan Lucia <ryan@luciaonline.net>

Merge branch 'master' of https://github.com/dotnet/runtime into wasm-…

75a81ce

…crypto-getRandomValues

Add note about insecure code for testing purposes

8077f7c

Formatting

ec16f1a

Return -1 if crypto does not exist instead of aborting from js. This …

7b7e9d2

…allows the managed code exception flow to continue as normal.

kjpou1 requested review from lewing, marek-safar, CoffeeFlux and jkotas October 1, 2020 06:54

jkotas approved these changes Oct 1, 2020

View reviewed changes

lewing approved these changes Oct 1, 2020

View reviewed changes

marek-safar merged commit e573cac into dotnet:master Oct 1, 2020

kjpou1 deleted the wasm-crypto-getRandomValues branch October 6, 2020 04:02

ghost locked as resolved and limited conversation to collaborators Dec 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wasm][crypto] RandomNumberGenerator mapped to Web Crypto getRandomValues #42728

[wasm][crypto] RandomNumberGenerator mapped to Web Crypto getRandomValues #42728

kjpou1 commented Sep 25, 2020 •

edited

Loading

ghost commented Sep 25, 2020

steveisok commented Sep 25, 2020

kjpou1 commented Sep 25, 2020

GrabYourPitchforks commented Sep 26, 2020

steveisok commented Sep 26, 2020

GrabYourPitchforks commented Sep 26, 2020 •

edited

Loading

lewing commented Sep 26, 2020 •

edited

Loading

lewing commented Sep 26, 2020 •

edited

Loading

lewing left a comment

jkotas Sep 30, 2020

kjpou1 Oct 1, 2020

CoffeeFlux left a comment

lewing left a comment

[wasm][crypto] RandomNumberGenerator mapped to Web Crypto getRandomValues #42728

[wasm][crypto] RandomNumberGenerator mapped to Web Crypto getRandomValues #42728

Conversation

kjpou1 commented Sep 25, 2020 • edited Loading

ghost commented Sep 25, 2020

steveisok commented Sep 25, 2020

kjpou1 commented Sep 25, 2020

GrabYourPitchforks commented Sep 26, 2020

steveisok commented Sep 26, 2020

GrabYourPitchforks commented Sep 26, 2020 • edited Loading

lewing commented Sep 26, 2020 • edited Loading

lewing commented Sep 26, 2020 • edited Loading

lewing left a comment

Choose a reason for hiding this comment

jkotas Sep 30, 2020

Choose a reason for hiding this comment

kjpou1 Oct 1, 2020

Choose a reason for hiding this comment

CoffeeFlux left a comment

Choose a reason for hiding this comment

lewing left a comment

Choose a reason for hiding this comment

kjpou1 commented Sep 25, 2020 •

edited

Loading

GrabYourPitchforks commented Sep 26, 2020 •

edited

Loading

lewing commented Sep 26, 2020 •

edited

Loading

lewing commented Sep 26, 2020 •

edited

Loading