Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

crypto: Only throw if randomBytes arg is a non-number #4682

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions doc/api/crypto.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,23 @@ Generates cryptographically strong pseudo-random data. Usage:
// handle error
}

Note that this function will throw an error if the size argument is
not a number.

An exception will be raised (either thrown, or returned to the
callback) if the size value is negative, or larger than 0x3fffffff.

## crypto.pseudoRandomBytes(size, [callback])

Generates non-cryptographically strong pseudo-random data. Usage is
identical to `crypto.randomBytes`, above.

Note that this function will throw an error if the size argument is
not a number.

An exception will be raised (either thrown, or returned to the
callback) if the size value is negative, or larger than 0x3fffffff.

## crypto.DEFAULT_ENCODING

The default encoding to use for functions that can take either strings
Expand Down
54 changes: 52 additions & 2 deletions lib/crypto.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@ exports.DEFAULT_ENCODING = 'buffer';
try {
var binding = process.binding('crypto');
var SecureContext = binding.SecureContext;
var randomBytes = binding.randomBytes;
var pseudoRandomBytes = binding.pseudoRandomBytes;
var getCiphers = binding.getCiphers;
var getHashes = binding.getHashes;
var crypto = true;
Expand Down Expand Up @@ -550,8 +548,60 @@ function pbkdf2(password, salt, iterations, keylen, callback) {



var max = 0x3fffffff;

Choose a reason for hiding this comment

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

Could be overkill, but since this value is already defined in node_buffer.h is there something to be done with exposing it?

exports.randomBytes = randomBytes;
function randomBytes(size, cb) {
if (typeof size !== 'number')

Choose a reason for hiding this comment

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

Might be missing something here, but I think NaN will make it through all the checks and be passed as 0. I'd do the ~~size at the === 0 check.

throw new Error('size must be a number between 0 and ' + max);

if (size > max || size < 0) {
var er = new Error('size out of range');
if (!cb)
throw er;
return process.nextTick(function() {
cb(er);
});
}

if (size === 0) {
// short circuit
var b = new Buffer(0);
if (!cb)
return b;
return process.nextTick(function() {
cb(null, b);
});
}

binding.randomBytes(~~size, cb);
}

exports.pseudoRandomBytes = pseudoRandomBytes;
function pseudoRandomBytes(size, cb) {
if (typeof size !== 'number')
throw new Error('size must be a number between 0 and ' + max);

if (size > max || size < 0) {
var er = new Error('size out of range');
if (!cb)
throw er;
return process.nextTick(function() {
cb(er);
});
}

if (size === 0) {
// short circuit
var b = new Buffer(0);
if (!cb)
return b;
return process.nextTick(function() {
cb(null, b);
});
}

binding.pseudoRandomBytes(~~size, cb);
}

exports.rng = randomBytes;
exports.prng = pseudoRandomBytes;
Expand Down
2 changes: 1 addition & 1 deletion src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3929,7 +3929,7 @@ Handle<Value> RandomBytes(const Arguments& args) {
// maybe allow a buffer to write to? cuts down on object creation
// when generating random data in a loop
if (!args[0]->IsUint32()) {
Local<String> s = String::New("Argument #1 must be number > 0");
Local<String> s = String::New("Argument #1 must be uint32");
return ThrowException(Exception::TypeError(s));
}

Expand Down
17 changes: 12 additions & 5 deletions test/simple/test-crypto-random.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,7 @@ process.setMaxListeners(256);
[crypto.randomBytes,
crypto.pseudoRandomBytes
].forEach(function(f) {
[-1,
undefined,
[ undefined,
null,
false,
true,
Expand All @@ -49,10 +48,17 @@ process.setMaxListeners(256);
assert.throws(function() { f(value, function() {}); });
});

[0, 1, 2, 4, 16, 256, 1024].forEach(function(len) {
[ -1, 0x3fffffff + 1 ].forEach(function(value) {
assert.throws(function() { f(value); });
f(value, function(er) {
assert(er);
});
});

[0, 1, 1.5, 2, 4, 16, 256, 1024].forEach(function(len) {
f(len, checkCall(function(ex, buf) {
assert.equal(null, ex);
assert.equal(len, buf.length);
assert.equal(Math.floor(len), buf.length);
assert.ok(Buffer.isBuffer(buf));
}));
});
Expand All @@ -67,6 +73,7 @@ function checkCall(cb, desc) {
});

return function() {
return called_ = true, cb.apply(cb, Array.prototype.slice.call(arguments));
called_ = true;
return cb.apply(cb, arguments);
};
}