-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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: fix regression in randomFill/randomBytes #35135
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -2631,7 +2631,7 @@ changes: | |||||
`ERR_INVALID_CALLBACK`. | ||||||
--> | ||||||
|
||||||
* `size` {number} | ||||||
* `size` {number} The `size` must be less or equal to `2**31 - 1`. | ||||||
* `callback` {Function} | ||||||
* `err` {Error} | ||||||
* `buf` {Buffer} | ||||||
|
@@ -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`. | ||||||
* `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`. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
* Returns: {Buffer|TypedArray|DataView} The object passed as `buffer` argument. | ||||||
|
||||||
Synchronous version of [`crypto.randomFill()`][]. | ||||||
|
@@ -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`. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
* `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`. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
* `callback` {Function} `function(err, buf) {}`. | ||||||
|
||||||
This function is similar to [`crypto.randomBytes()`][] but requires the first | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5855,6 +5855,7 @@ struct RandomBytesJob : public CryptoJob { | |
|
||
|
||
void RandomBytes(const FunctionCallbackInfo<Value>& args) { | ||
Environment* env = Environment::GetCurrent(args); | ||
CHECK(args[0]->IsArrayBufferView()); // buffer; wrap object retains ref. | ||
CHECK(args[1]->IsUint32()); // offset | ||
CHECK(args[2]->IsUint32()); // size | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't it better to do proper validation in JS and only use |
||
return THROW_ERR_OUT_OF_RANGE(env, "buffer is too large"); | ||
|
||
std::unique_ptr<RandomBytesJob> job(new RandomBytesJob(env)); | ||
job->data = reinterpret_cast<unsigned char*>(Buffer::Data(args[0])) + offset; | ||
job->size = size; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
'use strict'; | ||
const common = require('../common'); | ||
if (!common.hasCrypto) | ||
common.skip('missing crypto'); | ||
|
||
const { Buffer } = require('buffer'); | ||
const assert = require('assert'); | ||
|
||
const { | ||
randomFill, | ||
randomFillSync, | ||
randomBytes | ||
} = require('crypto'); | ||
|
||
let kData; | ||
try { | ||
kData = Buffer.alloc(2 ** 31 + 1); | ||
} catch { | ||
common.skip('not enough memory'); | ||
} | ||
|
||
assert.throws(() => randomFill(kData, common.mustNotCall()), { | ||
code: 'ERR_OUT_OF_RANGE' | ||
}); | ||
|
||
assert.throws(() => randomFillSync(kData), { | ||
code: 'ERR_OUT_OF_RANGE' | ||
}); | ||
|
||
assert.throws(() => randomBytes(2 ** 31 + 1), { | ||
code: 'ERR_OUT_OF_RANGE' | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.