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

RandomNumberGenerator.Fill( Span<byte> data ) fails in browser when data length is > 65536. #48584

Closed
zlatanov opened this issue Feb 22, 2021 · 4 comments · Fixed by #48692
Closed
Assignees
Labels

Comments

@zlatanov
Copy link
Contributor

Description

I was investigating some failed tests of mine that were working on Windows and Unix, but were failing in WASM. After checking the logs I found out that browser's API Crypto.getRandomValues supports up to 65536 bytes per call.

Shouldn't the implementation of this API take this in consideration and fill the data in batches?

A small repo:

Span<byte> data = new byte[ 1024 * 1024 ];
RandomNumberGenerator.Fill( data );

Configuration

WasmTestRunner in Chrome V8 engine, more information here:

https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-48470-merge-4413c624cdc8497c86/System.Net.WebSockets.Tests/console.d05686a3.log?sv=2019-07-07&se=2021-03-13T19%3A39%3A49Z&sr=c&sp=rl&sig=wv4tqXOQ6GNNF6I2o9P2YjcU2U512JjKZ%2B0411qSlkA%3D

Regression?

I don't think so.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Feb 22, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Feb 22, 2021

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

Issue Details

Description

I was investigating some failed tests of mine that were working on Windows and Unix, but were failing in WASM. After checking the logs I found out that browser's API Crypto.getRandomValues supports up to 65536 bytes per call.

Shouldn't the implementation of this API take this in consideration and fill the data in batches?

A small repo:

Span<byte> data = new byte[ 1024 * 1024 ];
RandomNumberGenerator.Fill( data );

Configuration

WasmTestRunner in Chrome V8 engine, more information here:

https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-48470-merge-4413c624cdc8497c86/System.Net.WebSockets.Tests/console.d05686a3.log?sv=2019-07-07&se=2021-03-13T19%3A39%3A49Z&sr=c&sp=rl&sig=wv4tqXOQ6GNNF6I2o9P2YjcU2U512JjKZ%2B0411qSlkA%3D

Regression?

I don't think so.

Author: zlatanov
Assignees: -
Labels:

arch-wasm, area-System.Security, bug, untriaged

Milestone: -

@GrabYourPitchforks
Copy link
Member

Here's the code that calls directly into subtle crypto:

if (typeof crypto === 'object' && typeof crypto['getRandomValues'] === 'function') {
// for modern web browsers
// map the work array to the memory buffer passed with the length
var wrkArray = new Uint8Array(Module.HEAPU8.buffer, buffer, bufferLength);
crypto.getRandomValues(wrkArray);
return 0;
} else {

Might be best to apply the fix directly to this code rather than several layers of indirection up the stack at the managed call site (RandomNumberGenerator.cs). That would make sure that all subtle crypto calls get the new behavior, regardless of what control flow paths were taken to get here.

@GrabYourPitchforks
Copy link
Member

GrabYourPitchforks commented Feb 22, 2021

As an aside, I assume the failing unit tests are part of #48470. It's generally not recommended to use random data in unit tests unless you intend to measure entropy. Random data can lead to non-deterministic exercise of edge cases in code, which could lead to unreliable tests or difficult-to-reproduce failures indicating potentially legitimate bugs. Consider instead using static data or generating low-quality "random" input data deterministically (e.g., Enumerable.Range(...) or new Random(fixed_seed)).

@kjpou1 kjpou1 removed the untriaged New issue has not been triaged by the area owner label Feb 23, 2021
@kjpou1 kjpou1 self-assigned this Feb 23, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 24, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 24, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Mar 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants