crypto: fix regression in randomFill/randomBytes #35135

jasnell · 2020-09-09T21:51:25Z

Fixes a segfault when a buffer larger than 2**31-1 is passed in to randomFill/randomFillSync or when a size larger than 2**31-1 is passed in to randomBytes.

I opened this one separate because we should decide if this is the way we want to handle this. This PR adds a throw if the size is larger than 2**31-1. Alternatively, we could allow larger values by splitting it into multiple calls to RAND_bytes. That said, I'd like to believe that asking for random values larger than 2**31-1 is going to be exceedingly rare (I'd really hope), so I think the throw is sufficient here.

Related to: #35132

Signed-off-by: James M Snell jasnell@gmail.com

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Fixes a segfault when a buffer larger than 2**31-1 is passed in to randomFill/randomFillSync or when a size larger than 2**31-1 is passed in to randomBytes. Signed-off-by: James M Snell <jasnell@gmail.com>

nodejs-github-bot · 2020-09-09T21:51:30Z

Review requested:

@nodejs/crypto

nodejs-github-bot · 2020-09-09T22:11:54Z

CI: https://ci.nodejs.org/job/node-test-pull-request/33120/

test/common/index.js

bnoordhuis

Seems fine to me. Requesting more than a few kilobytes of randomness is already questionable (what do you need so much entropy for?), let alone 2 GB.

doc/api/crypto.md

Co-authored-by: Denys Otrishko <shishugi@gmail.com>

lundibundi · 2020-09-10T17:28:25Z

doc/api/crypto.md

@@ -2691,9 +2691,11 @@ changes:
    description: The `buffer` argument may be any `TypedArray` or `DataView`.
 -->

-* `buffer` {Buffer|TypedArray|DataView} Must be supplied.
+* `buffer` {Buffer|TypedArray|DataView} Must be supplied. The
+  size of the provided `buffer` must not be larger than `2**31 - 1`.


Ditto

Suggested change

size of the provided `buffer` must not be larger than `2**31 - 1`.

size of the provided `buffer` must be less or equal to `2**31 - 1`.

FWIW, I think less than or equal to would be the way to go here and in the other three examples, but I'm not blocking. Whatever wording is used is fine by me. Can always be changed later.

Suggested change

size of the provided `buffer` must not be larger than `2**31 - 1`.

size of the provided `buffer` must be less than or equal to `2**31 - 1`.

lundibundi · 2020-09-10T17:28:33Z

doc/api/crypto.md

 * `offset` {number} **Default:** `0`
-* `size` {number} **Default:** `buffer.length - offset`
+* `size` {number} **Default:** `buffer.length - offset`. The `size`
+  must not be larger than `2**31 - 1`.


Suggested change

must not be larger than `2**31 - 1`.

must be less or equal to `2**31 - 1`.

lundibundi · 2020-09-10T17:28:42Z

doc/api/crypto.md

@@ -2737,9 +2739,11 @@ changes:
    description: The `buffer` argument may be any `TypedArray` or `DataView`.
 -->

-* `buffer` {Buffer|TypedArray|DataView} Must be supplied.
+* `buffer` {Buffer|TypedArray|DataView} Must be supplied. The size
+  of the provided `buffer` must not be larger than `2**31 - 1`.


Suggested change

of the provided `buffer` must not be larger than `2**31 - 1`.

of the provided `buffer` must be less or equal to `2**31 - 1`.

lundibundi · 2020-09-10T17:28:50Z

doc/api/crypto.md

 * `offset` {number} **Default:** `0`
-* `size` {number} **Default:** `buffer.length - offset`
+* `size` {number} **Default:** `buffer.length - offset`.  The `size`
+  must not be larger than `2**31 - 1`.


Suggested change

must not be larger than `2**31 - 1`.

must be less or equal to `2**31 - 1`.

lpinca · 2020-09-14T05:33:08Z

src/node_crypto.cc

@@ -5863,7 +5864,10 @@ void RandomBytes(const FunctionCallbackInfo<Value>& args) {
  const uint32_t size = args[2].As<Uint32>()->Value();
  CHECK_GE(offset + size, offset);  // Overflow check.
  CHECK_LE(offset + size, Buffer::Length(args[0]));  // Bounds check.
-  Environment* env = Environment::GetCurrent(args);
+
+  if (size > INT_MAX)


Isn't it better to do proper validation in JS and only use CHECK_LE here?

jasnell · 2020-10-08T00:36:50Z

#35093 landed that incorporated most of these changes.

crypto: fix regression in randomFill/randomBytes

4d516dd

Fixes a segfault when a buffer larger than 2**31-1 is passed in to randomFill/randomFillSync or when a size larger than 2**31-1 is passed in to randomBytes. Signed-off-by: James M Snell <jasnell@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Sep 9, 2020

fixup! crypto: fix regression in randomFill/randomBytes

e0749df

jasnell requested a review from addaleax September 9, 2020 21:54

jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 9, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 9, 2020

addaleax approved these changes Sep 9, 2020

View reviewed changes

test/common/index.js Outdated Show resolved Hide resolved

jasnell commented Sep 9, 2020

View reviewed changes

test/common/index.js Outdated Show resolved Hide resolved

[Squash] fixup

4c8c193

jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 9, 2020

tniessen approved these changes Sep 9, 2020

View reviewed changes

bnoordhuis approved these changes Sep 10, 2020

View reviewed changes

lundibundi approved these changes Sep 10, 2020

View reviewed changes

doc/api/crypto.md Outdated Show resolved Hide resolved

[squash] nit

399f61d

Co-authored-by: Denys Otrishko <shishugi@gmail.com>

lundibundi reviewed Sep 10, 2020

View reviewed changes

Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 13, 2020

lpinca reviewed Sep 14, 2020

View reviewed changes

jasnell closed this Oct 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: fix regression in randomFill/randomBytes #35135

crypto: fix regression in randomFill/randomBytes #35135

jasnell commented Sep 9, 2020 •

edited

Loading

nodejs-github-bot commented Sep 9, 2020

nodejs-github-bot commented Sep 9, 2020

bnoordhuis left a comment

lundibundi Sep 10, 2020

Trott Sep 16, 2020 •

edited

Loading

lundibundi Sep 10, 2020

lundibundi Sep 10, 2020

lundibundi Sep 10, 2020

lpinca Sep 14, 2020

jasnell commented Oct 8, 2020

	size of the provided `buffer` must not be larger than `2**31 - 1`.
	size of the provided `buffer` must be less or equal to `2**31 - 1`.

	must not be larger than `2**31 - 1`.
	must be less or equal to `2**31 - 1`.

	of the provided `buffer` must not be larger than `2**31 - 1`.
	of the provided `buffer` must be less or equal to `2**31 - 1`.

crypto: fix regression in randomFill/randomBytes #35135

crypto: fix regression in randomFill/randomBytes #35135

Conversation

jasnell commented Sep 9, 2020 • edited Loading

Checklist

nodejs-github-bot commented Sep 9, 2020

nodejs-github-bot commented Sep 9, 2020

bnoordhuis left a comment

Choose a reason for hiding this comment

lundibundi Sep 10, 2020

Choose a reason for hiding this comment

Trott Sep 16, 2020 • edited Loading

Choose a reason for hiding this comment

lundibundi Sep 10, 2020

Choose a reason for hiding this comment

lundibundi Sep 10, 2020

Choose a reason for hiding this comment

lundibundi Sep 10, 2020

Choose a reason for hiding this comment

lpinca Sep 14, 2020

Choose a reason for hiding this comment

jasnell commented Oct 8, 2020

jasnell commented Sep 9, 2020 •

edited

Loading

Trott Sep 16, 2020 •

edited

Loading