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

crypto: reduce range of size to int max #38096

Closed
wants to merge 1 commit into from

Conversation

Ayase-252
Copy link
Member

@Ayase-252 Ayase-252 commented Apr 5, 2021

Background

A bump of max length of Buffer to 2 ** 32 - 1(https://bugs.chromium.org/p/v8/issues/detail?id=4153#c66) breaks validation of sizeargument of randomBytes. When passing a over-large size like 2 * 31), it causes abort in v14 as described in #38090.

Impact of the PR

For v15, the overlarge size is catched in C++ code, and an Error will throw currently:

Welcome to Node.js v15.12.0.
Type ".help" for more information.
> crypto.randomBytes(2147483648)
Uncaught RangeError: buffer is too large
    at randomFillSync (node:internal/crypto/random:128:15)
    at Object.randomBytes (node:internal/crypto/random:96:5) {
  code: 'ERR_OUT_OF_RANGE'

After the PR, an JS-level Error will throw:

> crypto.randomBytes(2147483648)
Uncaught:
RangeError [ERR_OUT_OF_RANGE]: The value of "size" is out of range. It must be >= 0 && <= 2147483647. Received 2147483648
    at new NodeError (node:internal/errors:349:5)
    at assertSize (node:internal/crypto/random:80:11)
    at Object.randomBytes (node:internal/crypto/random:93:10)
    at REPL1:1:8
    at Script.runInThisContext (node:vm:131:12)
    at REPLServer.defaultEval (node:repl:522:29)
    at bound (node:domain:416:15)
    at REPLServer.runBound [as eval] (node:domain:427:12)
    at REPLServer.onLine (node:repl:844:10)
    at REPLServer.emit (node:events:381:22) {
  code: 'ERR_OUT_OF_RANGE'
}

For v14, executing crypto.randomBytes(2147483648) will abort immediately

Welcome to Node.js v14.16.0.
Type ".help" for more information.
>  crypto.randomBytes(2147483648)
[1]    96221 segmentation fault  node

If this PR is backported successfully, it will throw the same Error described above instead of abort.

Refs: #38090

@nodejs-github-bot nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Apr 5, 2021
@nodejs-github-bot

This comment has been minimized.

Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rather allow buffer sizes as large as 2 ** 32 - 1 (for 64-bit platforms)?

@RaisinTen
Copy link
Contributor

A bump of max length of Buffer to 2 ** 32

@Ayase-252 Did you mean this in the PR description?

- A bump of max length of Buffer to 2 ** 32 
+ A bump of max length of Buffer to 2 ** 32 - 1

@Ayase-252
Copy link
Member Author

A bump of max length of Buffer to 2 ** 32

@Ayase-252 Did you mean this in the PR description?

- A bump of max length of Buffer to 2 ** 32 
+ A bump of max length of Buffer to 2 ** 32 - 1

Yes, my mistake, I will edit in the OP too

@Ayase-252
Copy link
Member Author

Should we rather allow buffer sizes as large as 2 ** 32 - 1 (for 64-bit platforms)?

Yes. It could be. But I don’t know find how crypto.random allocates buffer and why 2**31 - 1 is a limitation factor so far. May investigate tomorrow.

@nodejs-github-bot

This comment has been minimized.

@Linkgoron
Copy link
Member

Linkgoron commented Apr 5, 2021

@Ayase-252 I think you referenced the wrong issue in the commit message.

@Ayase-252
Copy link
Member Author

@Ayase-252 I think you referenced the wrong issue in the commit message.

Thanks! Commit message is amended.

@jasnell
Copy link
Member

jasnell commented Apr 6, 2021

Should we rather allow buffer sizes as large as 2 ** 32 - 1 (for 64-bit platforms)?

For now, no. Openssl is still pretty limited here, using int for many sizes. Enforcing the current limit is the right thing to do here.

@RaisinTen
Copy link
Contributor

Instead of Refs, should we use Fixes in the commit message, as this stops the segfault?

@Ayase-252
Copy link
Member Author

@RaisinTen I don't think this PR fixes #38090 until change is backported to v14 later. May it be more approriate to use Fixes in a backport commit?

@nodejs-github-bot

This comment has been minimized.

@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 6, 2021
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

jasnell pushed a commit that referenced this pull request Apr 12, 2021
Refs: #38090

PR-URL: #38096
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@jasnell
Copy link
Member

jasnell commented Apr 12, 2021

Landed in 993ed19

@jasnell jasnell closed this Apr 12, 2021
@Ayase-252 Ayase-252 deleted the fix/38390 branch April 13, 2021 14:25
BethGriggs pushed a commit that referenced this pull request Apr 15, 2021
Refs: #38090

PR-URL: #38096
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
targos pushed a commit that referenced this pull request May 1, 2021
Refs: #38090

PR-URL: #38096
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
danielleadams pushed a commit that referenced this pull request May 8, 2021
Refs: #38090

PR-URL: #38096
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants