crypto: fix regression in randomFill/randomBytes#35135
Closed
jasnell wants to merge 4 commits intonodejs:masterfrom
Closed
crypto: fix regression in randomFill/randomBytes#35135jasnell wants to merge 4 commits intonodejs:masterfrom
jasnell wants to merge 4 commits intonodejs:masterfrom
Conversation
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>
Collaborator
|
Review requested:
|
Collaborator
addaleax
approved these changes
Sep 9, 2020
jasnell
commented
Sep 9, 2020
tniessen
approved these changes
Sep 9, 2020
bnoordhuis
approved these changes
Sep 10, 2020
Member
bnoordhuis
left a comment
There was a problem hiding this comment.
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.
lundibundi
approved these changes
Sep 10, 2020
Co-authored-by: Denys Otrishko <shishugi@gmail.com>
lundibundi
reviewed
Sep 10, 2020
|
|
||
| * `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`. |
Member
There was a problem hiding this comment.
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`. |
Member
There was a problem hiding this comment.
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`. |
| * `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`. |
Member
There was a problem hiding this comment.
Suggested change
| must not be larger than `2**31 - 1`. | |
| must be less or equal to `2**31 - 1`. |
|
|
||
| * `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`. |
Member
There was a problem hiding this comment.
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`. |
| * `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`. |
Member
There was a problem hiding this comment.
Suggested change
| must not be larger than `2**31 - 1`. | |
| must be less or equal to `2**31 - 1`. |
lpinca
reviewed
Sep 14, 2020
| CHECK_LE(offset + size, Buffer::Length(args[0])); // Bounds check. | ||
| Environment* env = Environment::GetCurrent(args); | ||
|
|
||
| if (size > INT_MAX) |
Member
There was a problem hiding this comment.
Isn't it better to do proper validation in JS and only use CHECK_LE here?
Member
Author
|
#35093 landed that incorporated most of these changes. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a segfault when a buffer larger than
2**31-1is passed in to randomFill/randomFillSync or when a size larger than2**31-1is 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 toRAND_bytes. That said, I'd like to believe that asking for random values larger than2**31-1is 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), orvcbuild test(Windows) passes