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

Webcrypto getRandomValues accepts DataView #41480

Closed
LiviaMedeiros opened this issue Jan 11, 2022 · 1 comment
Closed

Webcrypto getRandomValues accepts DataView #41480

LiviaMedeiros opened this issue Jan 11, 2022 · 1 comment

Comments

@LiviaMedeiros
Copy link
Contributor

Version

v16.13.1

Platform

No response

Subsystem

crypto

What steps will reproduce the bug?

const crypto = require('crypto').webcrypto;

const good = crypto.getRandomValues(new Uint16Array(8));
console.log(good);

const shouldBeBad = crypto.getRandomValues(new DataView(new ArrayBuffer(8)));
console.log(shouldBeBad);

const bad = crypto.getRandomValues(new Float32Array(8));
console.log(bad);

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior?

This line:

const shouldBeBad = crypto.getRandomValues(new DataView(new ArrayBuffer(8)));

should throw TypeMismatchError, since DataView instance is not an integer-type TypedArray (in fact, it's not a TypedArray at all).

What do you see instead?

$ node /tmp/node-test.js
Uint16Array(8) [
  11975, 18072,
   7401, 42679,
  61734,  5588,
  62920, 30080
]
DataView {
  byteLength: 8,
  byteOffset: 0,
  buffer: ArrayBuffer {
    [Uint8Contents]: <c6 25 d6 26 46 e0 4d 7a>,
    byteLength: 8
  }
}
node:internal/crypto/random:316
    throw lazyDOMException(
    ^

DOMException [TypeMismatchError]: The data argument must be an integer-type TypedArray
    at Crypto.getRandomValues (node:internal/crypto/random:316:11)
    at Object.<anonymous> (/tmp/node-test.js:9:20)
    at Module._compile (node:internal/modules/cjs/loader:1101:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:17:47

Additional information

Note: the issue is about webcrypto, it doesn't affect crypto.randomFill

W3C specs states that getRandomValues method must throw TypeMismatchError if its argument is not of an integer type.
Within current standard, allowed types may be described in the following ways:

  • Int8Array or Uint8Array or Uint8ClampedArray or Int16Array or Uint16Array or Int32Array or Uint32Array or BigInt64Array or BigUint64Array
  • TypedArray, except for Float32Array and Float64Array
  • ArrayBufferView, except for Float32Array, Float64Array and DataView

Float##Array are prohibited to avoid potential misinterpretation of equivalent of what *(float*) in C does as generating some sane float numbers with uniform distribution.
As explained in w3c/webcrypto#284 (comment), DataView is advisedly prohibited for the same reason.

Fixing this issue would also allow linters and typecheckers to easily assume that any value returned by getRandomValues is guaranteed to be an instance of TypedArray.

LiviaMedeiros added a commit to LiviaMedeiros/node that referenced this issue Jan 11, 2022
prevents Web Crypto API's getRandomValues from accepting DataView

- Fixes: nodejs#41480
- Refs: https://www.w3.org/TR/WebCryptoAPI/#Crypto-method-getRandomValues
@LiviaMedeiros
Copy link
Contributor Author

LiviaMedeiros commented Jan 11, 2022

Draft PR: #41481

Behaviour in patched version:

$ out/Release/node /tmp/node-test.js
Uint16Array(8) [
  56131, 30854,
  59315, 57587,
  50096, 65211,
  62975, 35455
]
node:internal/crypto/random:317
    throw lazyDOMException(
    ^

DOMException [TypeMismatchError]: The data argument must be an integer-type TypedArray
    at Crypto.getRandomValues (node:internal/crypto/random:317:11)
    at Object.<anonymous> (/tmp/node-test.js:6:28)
    at Module._compile (node:internal/modules/cjs/loader:1097:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1149:10)
    at Module.load (node:internal/modules/cjs/loader:975:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:17:47

Node.js v18.0.0-pre

LiviaMedeiros added a commit to LiviaMedeiros/node that referenced this issue Jan 11, 2022
prevents Web Crypto API's getRandomValues from accepting DataView

- Fixes: nodejs#41480
- Refs: https://www.w3.org/TR/WebCryptoAPI/#Crypto-method-getRandomValues
LiviaMedeiros added a commit to LiviaMedeiros/node that referenced this issue Jan 14, 2022
prevents Web Crypto API's getRandomValues from accepting DataView

- Fixes: nodejs#41480
- Refs: https://www.w3.org/TR/WebCryptoAPI/#Crypto-method-getRandomValues
LiviaMedeiros added a commit to LiviaMedeiros/node that referenced this issue Jan 16, 2022
prevents Web Crypto API's getRandomValues from accepting DataView

- Fixes: nodejs#41480
- Refs: https://www.w3.org/TR/WebCryptoAPI/#Crypto-method-getRandomValues
LiviaMedeiros added a commit to LiviaMedeiros/node that referenced this issue Jan 17, 2022
prevents Web Crypto API's getRandomValues from accepting DataView

- Fixes: nodejs#41480
- Refs: https://www.w3.org/TR/WebCryptoAPI/#Crypto-method-getRandomValues
@panva panva closed this as completed in b8de7aa Jan 22, 2022
BethGriggs pushed a commit that referenced this issue Jan 25, 2022
prevents Web Crypto API's getRandomValues from accepting DataView

Fixes: #41480
Refs: https://www.w3.org/TR/WebCryptoAPI/#Crypto-method-getRandomValues

PR-URL: #41481
Reviewed-By: Derek Lewis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Linkgoron pushed a commit to Linkgoron/node that referenced this issue Jan 31, 2022
prevents Web Crypto API's getRandomValues from accepting DataView

Fixes: nodejs#41480
Refs: https://www.w3.org/TR/WebCryptoAPI/#Crypto-method-getRandomValues

PR-URL: nodejs#41481
Reviewed-By: Derek Lewis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit that referenced this issue Feb 28, 2022
prevents Web Crypto API's getRandomValues from accepting DataView

Fixes: #41480
Refs: https://www.w3.org/TR/WebCryptoAPI/#Crypto-method-getRandomValues

PR-URL: #41481
Reviewed-By: Derek Lewis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit that referenced this issue Mar 2, 2022
prevents Web Crypto API's getRandomValues from accepting DataView

Fixes: #41480
Refs: https://www.w3.org/TR/WebCryptoAPI/#Crypto-method-getRandomValues

PR-URL: #41481
Reviewed-By: Derek Lewis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit that referenced this issue Mar 3, 2022
prevents Web Crypto API's getRandomValues from accepting DataView

Fixes: #41480
Refs: https://www.w3.org/TR/WebCryptoAPI/#Crypto-method-getRandomValues

PR-URL: #41481
Reviewed-By: Derek Lewis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit that referenced this issue Mar 14, 2022
prevents Web Crypto API's getRandomValues from accepting DataView

Fixes: #41480
Refs: https://www.w3.org/TR/WebCryptoAPI/#Crypto-method-getRandomValues

PR-URL: #41481
Reviewed-By: Derek Lewis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants