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

isaacs · 2013-01-29T22:33:29Z

If it is out of the acceptable range, then return a more sensible error message. (Throw for sync, call the callback for async.)

Implements the compromise suggested in #4583.

trevnorris · 2013-01-30T01:14:22Z

lib/crypto.js

@@ -550,8 +548,60 @@ function pbkdf2(password, salt, iterations, keylen, callback) {



+var max = 0x3fffffff;


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

If it is out of the acceptable range, then return a more sensible error. (Throw for sync, call the callback for async.) Fixes nodejsGH-4583

isaacs · 2013-01-30T02:07:16Z

Updated for consistency.

I think if we're going to throw, it's ok to give a little more information, even if that's not specifically why it threw. Throws are aggressive and should generally only happen in development unless something is very strange. May as well let the programmer know at that point that their shit will break if the number is too big or negative, since they may have a way to prevent that in the future.

trevnorris · 2013-01-30T03:21:33Z

@isaacs is it too much to allow the user to pass a number string (e.g. '456')? It seems that if the argument is expecting a number then type coercion in that case might be fine.

Also, it seems there are at least several different ways specific values are checked. Beyond deciding when the throw will happen I think we need another discussion on what is thrown. For example if the argument is expecting a uint then must we make sure it has no decimal part, or can we just floor the argument?

I've created a gist that I'll be updating with ways to check argument values under certain circumstances (https://gist.github.com/4670377). Think it might be good to have a discussion about this.

trevnorris · 2013-01-30T03:25:19Z

lib/crypto.js

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


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.

Nodejs-Jenkins · 2013-03-13T23:25:42Z

Build triggered.

isaacs · 2013-05-21T00:29:18Z

Closing as discussed in #4583

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [nodejs#4682](nodejs/node#4682) * Previously deprecated Buffer APIs are removed [nodejs#5048](nodejs/node#5048), [nodejs#4594](nodejs/node#4594) * Improved error handling [nodejs#4514](nodejs/node#4514) * Cluster * Worker emitted as first argument in 'message' event [nodejs#5361](nodejs/node#5361). * Crypto * Improved error handling [nodejs#3100](nodejs/node#3100), [nodejs#5611](nodejs/node#5611) * Simplified Certificate class bindings [nodejs#5382](nodejs/node#5382) * Improved control over FIPS mode [nodejs#5181](nodejs/node#5181) * pbkdf2 digest overloading is deprecated [nodejs#4047](nodejs/node#4047) * Dependencies * Reintroduce shared c-ares build support [nodejs#5775](nodejs/node#5775). * V8 updated to 5.0.71.31 [nodejs#6111](nodejs/node#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [nodejs#4921](nodejs/node#4921). * Domains * Clear stack when no error handler [nodejs#4659](nodejs/node#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [nodejs#3594](nodejs/node#3594) * FS apis can now accept and return paths as Buffers [nodejs#5616](nodejs/node#5616). * Error handling and type checking improvements [nodejs#5616](nodejs/node#5616), [nodejs#5590](nodejs/node#5590), [nodejs#4518](nodejs/node#4518), [nodejs#3917](nodejs/node#3917). * fs.read's string interface is deprecated [nodejs#4525](nodejs/node#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [nodejs#4557](nodejs/node#4557). * Modules * Current directory is now prioritized for local lookups [nodejs#5689](nodejs/node#5689) * Symbolic links are preserved when requiring modules [nodejs#5950](nodejs/node#5950) * Net * DNS hints no longer implicitly set [nodejs#6021](nodejs/node#6021). * Improved error handling and type checking [nodejs#5981](nodejs/node#5981), [nodejs#5733](nodejs/node#5733), [nodejs#2904](nodejs/node#2904) * Path * Improved type checking [nodejs#5348](nodejs/node#5348). * Process * Introduce process warnings API [nodejs#4782](nodejs/node#4782). * Throw exception when non-function passed to nextTick [nodejs#3860](nodejs/node#3860). * Readline * Emit key info unconditionally [nodejs#6024](nodejs/node#6024) * REPL * Assignment to `_` will emit a warning. [nodejs#5535](nodejs/node#5535) * Timers * Fail early when callback is not a function [nodejs#4362](nodejs/node#4362) * TLS * Rename 'clientError' to 'tlsClientError' [nodejs#4557](nodejs/node#4557) * SHA1 used for sessionIdContext [nodejs#3866](nodejs/node#3866) * TTY * Previously deprecated setRawMode wrapper is removed [nodejs#2528](nodejs/node#2528). * Util * Changes to Error object formatting [nodejs#4582](nodejs/node#4582). * Windows * Windows XP and Vista are no longer supported [nodejs#5167](nodejs/node#5167), [nodejs#5167](nodejs/node#5167).

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [nodejs#4682](nodejs/node#4682) * Previously deprecated Buffer APIs are removed [nodejs#5048](nodejs/node#5048), [nodejs#4594](nodejs/node#4594) * Improved error handling [nodejs#4514](nodejs/node#4514) * Cluster * Worker emitted as first argument in 'message' event [nodejs#5361](nodejs/node#5361). * Crypto * Improved error handling [nodejs#3100](nodejs/node#3100), [nodejs#5611](nodejs/node#5611) * Simplified Certificate class bindings [nodejs#5382](nodejs/node#5382) * Improved control over FIPS mode [nodejs#5181](nodejs/node#5181) * pbkdf2 digest overloading is deprecated [nodejs#4047](nodejs/node#4047) * Dependencies * Reintroduce shared c-ares build support [nodejs#5775](nodejs/node#5775). * V8 updated to 5.0.71.31 [nodejs#6111](nodejs/node#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [nodejs#4921](nodejs/node#4921). * Domains * Clear stack when no error handler [nodejs#4659](nodejs/node#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [nodejs#3594](nodejs/node#3594) * FS apis can now accept and return paths as Buffers [nodejs#5616](nodejs/node#5616). * Error handling and type checking improvements [nodejs#5616](nodejs/node#5616), [nodejs#5590](nodejs/node#5590), [nodejs#4518](nodejs/node#4518), [nodejs#3917](nodejs/node#3917). * fs.read's string interface is deprecated [nodejs#4525](nodejs/node#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [nodejs#4557](nodejs/node#4557). * Modules * Current directory is now prioritized for local lookups [nodejs#5689](nodejs/node#5689) * Symbolic links are preserved when requiring modules [nodejs#5950](nodejs/node#5950) * Net * DNS hints no longer implicitly set [nodejs#6021](nodejs/node#6021). * Improved error handling and type checking [nodejs#5981](nodejs/node#5981), [nodejs#5733](nodejs/node#5733), [nodejs#2904](nodejs/node#2904) * OS X * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7 [nodejs#6402](nodejs/node#6402). * Path * Improved type checking [nodejs#5348](nodejs/node#5348). * Process * Introduce process warnings API [nodejs#4782](nodejs/node#4782). * Throw exception when non-function passed to nextTick [nodejs#3860](nodejs/node#3860). * Readline * Emit key info unconditionally [nodejs#6024](nodejs/node#6024) * REPL * Assignment to `_` will emit a warning. [nodejs#5535](nodejs/node#5535) * Timers * Fail early when callback is not a function [nodejs#4362](nodejs/node#4362) * TLS * Rename 'clientError' to 'tlsClientError' [nodejs#4557](nodejs/node#4557) * SHA1 used for sessionIdContext [nodejs#3866](nodejs/node#3866) * TTY * Previously deprecated setRawMode wrapper is removed [nodejs#2528](nodejs/node#2528). * Util * Changes to Error object formatting [nodejs#4582](nodejs/node#4582). * Windows * Windows XP and Vista are no longer supported [nodejs#5167](nodejs/node#5167), [nodejs#5167](nodejs/node#5167).

trevnorris reviewed Jan 30, 2013
View reviewed changes

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

b7198f7

If it is out of the acceptable range, then return a more sensible error. (Throw for sync, call the callback for async.) Fixes nodejsGH-4583

trevnorris reviewed Jan 30, 2013
View reviewed changes

isaacs closed this May 21, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

isaacs commented Jan 29, 2013

trevnorris Jan 30, 2013

isaacs commented Jan 30, 2013

trevnorris commented Jan 30, 2013

trevnorris Jan 30, 2013

Nodejs-Jenkins commented Mar 13, 2013

isaacs commented May 21, 2013

		@@ -550,8 +548,60 @@ function pbkdf2(password, salt, iterations, keylen, callback) {



		var max = 0x3fffffff;

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

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

Conversation

isaacs commented Jan 29, 2013

trevnorris Jan 30, 2013

Choose a reason for hiding this comment

isaacs commented Jan 30, 2013

trevnorris commented Jan 30, 2013

trevnorris Jan 30, 2013

Choose a reason for hiding this comment

Nodejs-Jenkins commented Mar 13, 2013

isaacs commented May 21, 2013