crypto: refactor argument validation for pbkdf2

Move input argument validation to js, using internal/errors.

Also update docs

* `password` and `salt` may be Buffers or any TypedArrays
* `crypto.DEFAULT_ENCODING` changes the returned derivedKey type

PR-URL: https://github.com/nodejs/node/pull/15746
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

doc/api/crypto.md

@@ -1587,8 +1587,8 @@ changes:
     description: The default encoding for `password` if it is a string changed
                  from `binary` to `utf8`.
 -->
-- `password` {string}
+- `password` {string|Buffer|TypedArray}
-- `salt` {string}
+- `salt` {string|Buffer|TypedArray}
 - `iterations` {number}
 - `keylen` {number}
 - `digest` {string}
@@ -1602,8 +1602,10 @@ applied to derive a key of the requested byte length (`keylen`) from the
 `password`, `salt` and `iterations`.
 
 The supplied `callback` function is called with two arguments: `err` and
-`derivedKey`. If an error occurs, `err` will be set; otherwise `err` will be
+`derivedKey`. If an error occurs while deriving the key, `err` will be set;
-null. The successfully generated `derivedKey` will be passed as a [`Buffer`][].
+otherwise `err` will be null. By default, the successfully generated
+`derivedKey` will be passed to the callback as a [`Buffer`][]. An error will be
+thrown if any of the input arguments specify invalid values or types.
 
 The `iterations` argument must be a number set as high as possible. The
 higher the number of iterations, the more secure the derived key will be,
@@ -1623,6 +1625,18 @@ crypto.pbkdf2('secret', 'salt', 100000, 64, 'sha512', (err, derivedKey) => {
 });
 ```
 
+The `crypto.DEFAULT_ENCODING` may be used to change the way the `derivedKey`
+is passed to the callback:
+
+```js
+const crypto = require('crypto');
+crypto.DEFAULT_ENCODING = 'hex';
+crypto.pbkdf2('secret', 'salt', 100000, 512, 'sha512', (err, derivedKey) => {
+  if (err) throw err;
+  console.log(derivedKey);  // '3745e48...aa39b34'
+});
+```
+
 An array of supported digest functions can be retrieved using
 [`crypto.getHashes()`][].
 
@@ -1643,8 +1657,8 @@ changes:
     description: The default encoding for `password` if it is a string changed
                  from `binary` to `utf8`.
 -->
-- `password` {string}
+- `password` {string|Buffer|TypedArray}
-- `salt` {string}
+- `salt` {string|Buffer|TypedArray}
 - `iterations` {number}
 - `keylen` {number}
 - `digest` {string}
@@ -1673,6 +1687,16 @@ const key = crypto.pbkdf2Sync('secret', 'salt', 100000, 64, 'sha512');
 console.log(key.toString('hex'));  // '3745e48...08d59ae'
 ```
 
+The `crypto.DEFAULT_ENCODING` may be used to change the way the `derivedKey`
+is returned:
+
+```js
+const crypto = require('crypto');
+crypto.DEFAULT_ENCODING = 'hex';
+const key = crypto.pbkdf2Sync('secret', 'salt', 100000, 512, 'sha512');
+console.log(key);  // '3745e48...aa39b34'
+```
+
 An array of supported digest functions can be retrieved using
 [`crypto.getHashes()`][].
 

doc/api/errors.md

@@ -637,6 +637,11 @@ Used when the native call from `process.cpuUsage` cannot be processed properly.
 Used when an invalid value for the `format` argument has been passed to the
 `crypto.ECDH()` class `getPublicKey()` method.
 
+<a id="ERR_CRYPTO_INVALID_DIGEST"></a>
+### ERR_CRYPTO_INVALID_DIGEST
+
+Used when an invalid [crypto digest algorithm][] is specified.
+
 <a id="ERR_DNS_SET_SERVERS_FAILED"></a>
 ### ERR_DNS_SET_SERVERS_FAILED
 
@@ -1355,6 +1360,7 @@ closed.
 [Node.js Error Codes]: #nodejs-error-codes
 [V8's stack trace API]: https://github.com/v8/v8/wiki/Stack-Trace-API
 [WHATWG URL API]: url.html#url_the_whatwg_url_api
+[crypto digest algorithm]: crypto.html#crypto_crypto_gethashes
 [domains]: domain.html
 [event emitter-based]: events.html#events_class_eventemitter
 [file descriptors]: https://en.wikipedia.org/wiki/File_descriptor

lib/internal/crypto/pbkdf2.js

@@ -5,9 +5,13 @@ const {
   getDefaultEncoding,
   toBuf
 } = require('internal/crypto/util');
+const { isArrayBufferView } = require('internal/util/types');
 const {
   PBKDF2
 } = process.binding('crypto');
+const {
+  INT_MAX
+} = process.binding('constants').crypto;
 
 function pbkdf2(password, salt, iterations, keylen, digest, callback) {
   if (typeof digest === 'function') {
@@ -34,10 +38,39 @@ function _pbkdf2(password, salt, iterations, keylen, digest, callback) {
   password = toBuf(password);
   salt = toBuf(salt);
 
+  if (!isArrayBufferView(password)) {
+    throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'password',
+                               ['string', 'Buffer', 'TypedArray']);
+  }
+
+  if (!isArrayBufferView(salt)) {
+    throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'salt',
+                               ['string', 'Buffer', 'TypedArray']);
+  }
+
+  if (typeof iterations !== 'number')
+    throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'iterations', 'number');
+
+  if (iterations < 0)
+    throw new errors.RangeError('ERR_OUT_OF_RANGE', 'iterations');
+
+  if (typeof keylen !== 'number')
+    throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'keylen', 'number');
+
+  if (keylen < 0 ||
+      !Number.isFinite(keylen) ||
+      keylen > INT_MAX) {
+    throw new errors.RangeError('ERR_OUT_OF_RANGE', 'keylen');
+  }
+
   const encoding = getDefaultEncoding();
 
-  if (encoding === 'buffer')
+  if (encoding === 'buffer') {
-    return PBKDF2(password, salt, iterations, keylen, digest, callback);
+    const ret = PBKDF2(password, salt, iterations, keylen, digest, callback);
+    if (ret === -1)
+      throw new errors.TypeError('ERR_CRYPTO_INVALID_DIGEST', digest);
+    return ret;
+  }
 
   // at this point, we need to handle encodings.
   if (callback) {
@@ -46,9 +79,12 @@ function _pbkdf2(password, salt, iterations, keylen, digest, callback) {
         ret = ret.toString(encoding);
       callback(er, ret);
     }
-    PBKDF2(password, salt, iterations, keylen, digest, next);
+    if (PBKDF2(password, salt, iterations, keylen, digest, next) === -1)
+      throw new errors.TypeError('ERR_CRYPTO_INVALID_DIGEST', digest);
   } else {
-    var ret = PBKDF2(password, salt, iterations, keylen, digest);
+    const ret = PBKDF2(password, salt, iterations, keylen, digest);
+    if (ret === -1)
+      throw new errors.TypeError('ERR_CRYPTO_INVALID_DIGEST', digest);
     return ret.toString(encoding);
   }
 }

lib/internal/errors.js

@@ -157,6 +157,7 @@ E('ERR_CRYPTO_ECDH_INVALID_FORMAT', 'Invalid ECDH format: %s');
 E('ERR_CRYPTO_HASH_DIGEST_NO_UTF16', 'hash.digest() does not support UTF-16');
 E('ERR_CRYPTO_HASH_FINALIZED', 'Digest already called');
 E('ERR_CRYPTO_HASH_UPDATE_FAILED', 'Hash update failed');
+E('ERR_CRYPTO_INVALID_DIGEST', 'Invalid digest: %s');
 E('ERR_CRYPTO_SIGN_KEY_REQUIRED', 'No key provided to sign');
 E('ERR_DNS_SET_SERVERS_FAILED', (err, servers) =>
   `c-ares failed to set servers: "${err}" [${servers}]`);

src/node_constants.cc

@@ -1180,6 +1180,7 @@ void DefineCryptoConstants(Local<Object> target) {
                               "defaultCipherList",
                               default_cipher_list);
 #endif
+  NODE_DEFINE_CONSTANT(target, INT_MAX);
 }
 
 void DefineZlibConstants(Local<Object> target) {

src/node_crypto.cc

@@ -5330,7 +5330,6 @@ void PBKDF2(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
 
   const EVP_MD* digest = nullptr;
-  const char* type_error = nullptr;
   char* pass = nullptr;
   char* salt = nullptr;
   int passlen = -1;
@@ -5341,54 +5340,19 @@ void PBKDF2(const FunctionCallbackInfo<Value>& args) {
   PBKDF2Request* req = nullptr;
   Local<Object> obj;
 
-  if (args.Length() != 5 && args.Length() != 6) {
-    type_error = "Bad parameter";
-    goto err;
-  }
-
-  THROW_AND_RETURN_IF_NOT_BUFFER(args[0], "Pass phrase");
   passlen = Buffer::Length(args[0]);
-  if (passlen < 0) {
-    type_error = "Bad password";
-    goto err;
-  }
-
-  THROW_AND_RETURN_IF_NOT_BUFFER(args[1], "Salt");
 
   pass = node::Malloc(passlen);
   memcpy(pass, Buffer::Data(args[0]), passlen);
 
   saltlen = Buffer::Length(args[1]);
-  if (saltlen < 0) {
-    type_error = "Bad salt";
-    goto err;
-  }
 
   salt = node::Malloc(saltlen);
   memcpy(salt, Buffer::Data(args[1]), saltlen);
 
-  if (!args[2]->IsNumber()) {
-    type_error = "Iterations not a number";
-    goto err;
-  }
-
   iter = args[2]->Int32Value();
-  if (iter < 0) {
-    type_error = "Bad iterations";
-    goto err;
-  }
-
-  if (!args[3]->IsNumber()) {
-    type_error = "Key length not a number";
-    goto err;
-  }
 
   raw_keylen = args[3]->NumberValue();
-  if (raw_keylen < 0.0 || isnan(raw_keylen) || isinf(raw_keylen) ||
-      raw_keylen > INT_MAX) {
-    type_error = "Bad key length";
-    goto err;
-  }
 
   keylen = static_cast<int>(raw_keylen);
 
@@ -5396,8 +5360,10 @@ void PBKDF2(const FunctionCallbackInfo<Value>& args) {
     node::Utf8Value digest_name(env->isolate(), args[4]);
     digest = EVP_get_digestbyname(*digest_name);
     if (digest == nullptr) {
-      type_error = "Bad digest name";
+      free(salt);
-      goto err;
+      free(pass);
+      args.GetReturnValue().Set(-1);
+      return;
     }
   }
 
@@ -5443,12 +5409,6 @@ void PBKDF2(const FunctionCallbackInfo<Value>& args) {
     else
       args.GetReturnValue().Set(argv[1]);
   }
-  return;
-
- err:
-  free(salt);
-  free(pass);
-  return env->ThrowTypeError(type_error);
 }
 
 

test/parallel/test-crypto-pbkdf2.js

@@ -6,6 +6,8 @@ if (!common.hasCrypto)
 const assert = require('assert');
 const crypto = require('crypto');
 
+const { INT_MAX } = process.binding('constants').crypto;
+
 //
 // Test PBKDF2 with RFC 6070 test vectors (except #4)
 //
@@ -63,33 +65,17 @@ common.expectsError(
   }
 );
 
-// Should not work with Infinity key length
+[Infinity, -Infinity, NaN, -1, 4073741824, INT_MAX + 1].forEach((i) => {
-assert.throws(function() {
+  common.expectsError(
-  crypto.pbkdf2('password', 'salt', 1, Infinity, 'sha256',
+    () => {
+      crypto.pbkdf2('password', 'salt', 1, i, 'sha256',
                     common.mustNotCall());
-}, /^TypeError: Bad key length$/);
+    }, {
-
+      code: 'ERR_OUT_OF_RANGE',
-// Should not work with negative Infinity key length
+      type: RangeError,
-assert.throws(function() {
+      message: 'The "keylen" argument is out of range'
-  crypto.pbkdf2('password', 'salt', 1, -Infinity, 'sha256',
+    });
-                common.mustNotCall());
+});
-}, /^TypeError: Bad key length$/);
-
-// Should not work with NaN key length
-assert.throws(function() {
-  crypto.pbkdf2('password', 'salt', 1, NaN, 'sha256', common.mustNotCall());
-}, /^TypeError: Bad key length$/);
-
-// Should not work with negative key length
-assert.throws(function() {
-  crypto.pbkdf2('password', 'salt', 1, -1, 'sha256', common.mustNotCall());
-}, /^TypeError: Bad key length$/);
-
-// Should not work with key length that does not fit into 32 signed bits
-assert.throws(function() {
-  crypto.pbkdf2('password', 'salt', 1, 4073741824, 'sha256',
-                common.mustNotCall());
-}, /^TypeError: Bad key length$/);
 
 // Should not get FATAL ERROR with empty password and salt
 // https://github.com/nodejs/node/issues/8571
@@ -114,3 +100,106 @@ common.expectsError(
     type: TypeError,
     message: 'The "digest" argument must be one of type string or null'
   });
+
+[1, {}, [], true, undefined, null].forEach((i) => {
+  common.expectsError(
+    () => crypto.pbkdf2(i, 'salt', 8, 8, 'sha256', common.mustNotCall()),
+    {
+      code: 'ERR_INVALID_ARG_TYPE',
+      type: TypeError,
+      message: 'The "password" argument must be one of type string, ' +
+               'Buffer, or TypedArray'
+    }
+  );
+
+  common.expectsError(
+    () => crypto.pbkdf2('pass', i, 8, 8, 'sha256', common.mustNotCall()),
+    {
+      code: 'ERR_INVALID_ARG_TYPE',
+      type: TypeError,
+      message: 'The "salt" argument must be one of type string, ' +
+               'Buffer, or TypedArray'
+    }
+  );
+
+  common.expectsError(
+    () => crypto.pbkdf2Sync(i, 'salt', 8, 8, 'sha256'),
+    {
+      code: 'ERR_INVALID_ARG_TYPE',
+      type: TypeError,
+      message: 'The "password" argument must be one of type string, ' +
+               'Buffer, or TypedArray'
+    }
+  );
+
+  common.expectsError(
+    () => crypto.pbkdf2Sync('pass', i, 8, 8, 'sha256'),
+    {
+      code: 'ERR_INVALID_ARG_TYPE',
+      type: TypeError,
+      message: 'The "salt" argument must be one of type string, ' +
+               'Buffer, or TypedArray'
+    }
+  );
+});
+
+['test', {}, [], true, undefined, null].forEach((i) => {
+  common.expectsError(
+    () => crypto.pbkdf2('pass', 'salt', i, 8, 'sha256', common.mustNotCall()),
+    {
+      code: 'ERR_INVALID_ARG_TYPE',
+      type: TypeError,
+      message: 'The "iterations" argument must be of type number'
+    }
+  );
+
+  common.expectsError(
+    () => crypto.pbkdf2Sync('pass', 'salt', i, 8, 'sha256'),
+    {
+      code: 'ERR_INVALID_ARG_TYPE',
+      type: TypeError,
+      message: 'The "iterations" argument must be of type number'
+    }
+  );
+});
+
+// Any TypedArray should work for password and salt
+crypto.pbkdf2(new Uint8Array(10), 'salt', 8, 8, 'sha256', common.mustCall());
+crypto.pbkdf2('pass', new Uint8Array(10), 8, 8, 'sha256', common.mustCall());
+crypto.pbkdf2(new Uint16Array(10), 'salt', 8, 8, 'sha256', common.mustCall());
+crypto.pbkdf2('pass', new Uint16Array(10), 8, 8, 'sha256', common.mustCall());
+crypto.pbkdf2(new Uint32Array(10), 'salt', 8, 8, 'sha256', common.mustCall());
+crypto.pbkdf2('pass', new Uint32Array(10), 8, 8, 'sha256', common.mustCall());
+crypto.pbkdf2(new Float32Array(10), 'salt', 8, 8, 'sha256', common.mustCall());
+crypto.pbkdf2('pass', new Float32Array(10), 8, 8, 'sha256', common.mustCall());
+crypto.pbkdf2(new Float64Array(10), 'salt', 8, 8, 'sha256', common.mustCall());
+crypto.pbkdf2('pass', new Float64Array(10), 8, 8, 'sha256', common.mustCall());
+
+crypto.pbkdf2Sync(new Uint8Array(10), 'salt', 8, 8, 'sha256');
+crypto.pbkdf2Sync('pass', new Uint8Array(10), 8, 8, 'sha256');
+crypto.pbkdf2Sync(new Uint16Array(10), 'salt', 8, 8, 'sha256');
+crypto.pbkdf2Sync('pass', new Uint16Array(10), 8, 8, 'sha256');
+crypto.pbkdf2Sync(new Uint32Array(10), 'salt', 8, 8, 'sha256');
+crypto.pbkdf2Sync('pass', new Uint32Array(10), 8, 8, 'sha256');
+crypto.pbkdf2Sync(new Float32Array(10), 'salt', 8, 8, 'sha256');
+crypto.pbkdf2Sync('pass', new Float32Array(10), 8, 8, 'sha256');
+crypto.pbkdf2Sync(new Float64Array(10), 'salt', 8, 8, 'sha256');
+crypto.pbkdf2Sync('pass', new Float64Array(10), 8, 8, 'sha256');
+
+common.expectsError(
+  () => crypto.pbkdf2('pass', 'salt', 8, 8, 'md55', common.mustNotCall()),
+  {
+    code: 'ERR_CRYPTO_INVALID_DIGEST',
+    type: TypeError,
+    message: 'Invalid digest: md55'
+  }
+);
+
+common.expectsError(
+  () => crypto.pbkdf2Sync('pass', 'salt', 8, 8, 'md55'),
+  {
+    code: 'ERR_CRYPTO_INVALID_DIGEST',
+    type: TypeError,
+    message: 'Invalid digest: md55'
+  }
+);