From 6bcfb6f847a7adfb62c6e463a77d0338495ee9d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Fri, 14 Sep 2018 13:02:44 +0200 Subject: [PATCH] crypto: deprecate digest == null in PBKDF2 I assume that permitting digest === null was unintentional when digest === undefined was deprecated since their behavior was equivalent. The sha1 default for digest === null has somehow made it through refactoring of the PBKDF2 module multiple times, even though digest === undefined has been EOL for some time now. This change deprecates setting digest to null so we can fix the behavior in Node.js 12 or so. --- doc/api/crypto.md | 8 ++++---- doc/api/deprecations.md | 17 ++++++++++++----- lib/internal/crypto/pbkdf2.js | 8 +++++++- test/parallel/test-crypto-pbkdf2.js | 7 ++++++- 4 files changed, 29 insertions(+), 11 deletions(-) diff --git a/doc/api/crypto.md b/doc/api/crypto.md index 06d9281ba47f5c..b7bf532d2f71e5 100644 --- a/doc/api/crypto.md +++ b/doc/api/crypto.md @@ -1786,8 +1786,8 @@ 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. -If `digest` is `null`, `'sha1'` will be used. This behavior will be deprecated -in a future version of Node.js. +If `digest` is `null`, `'sha1'` will be used. This behavior is deprecated, +please specify a `digest` explicitely. 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, @@ -1852,8 +1852,8 @@ applied to derive a key of the requested byte length (`keylen`) from the If an error occurs an `Error` will be thrown, otherwise the derived key will be returned as a [`Buffer`][]. -If `digest` is `null`, `'sha1'` will be used. This behavior will be deprecated -in a future version of Node.js. +If `digest` is `null`, `'sha1'` will be used. This behavior is deprecated, +please specify a `digest` explicitely. 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, diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index 4eb094f7ada447..b14599f26b05e9 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -227,24 +227,31 @@ to the `constants` property exposed by the relevant module. For instance, ### DEP0009: crypto.pbkdf2 without digest -Type: End-of-Life +Type: Runtime Use of the [`crypto.pbkdf2()`][] API without specifying a digest was deprecated in Node.js 6.0 because the method defaulted to using the non-recommended `'SHA1'` digest. Previously, a deprecation warning was printed. Starting in -Node.js 8.0.0, calling `crypto.pbkdf2()` or `crypto.pbkdf2Sync()` with an -undefined `digest` will throw a `TypeError`. +Node.js 8.0.0, calling `crypto.pbkdf2()` or `crypto.pbkdf2Sync()` with +`digest` set to `undefined` will throw a `TypeError`. + +Beginning in Node.js REPLACEME, calling these functions with `digest` set to +`null` will print a deprecation warning to align with the behavior when `digest` +is `undefined`. ### DEP0010: crypto.createCredentials diff --git a/lib/internal/crypto/pbkdf2.js b/lib/internal/crypto/pbkdf2.js index 3567a91e08f8b8..616892d98f04f7 100644 --- a/lib/internal/crypto/pbkdf2.js +++ b/lib/internal/crypto/pbkdf2.js @@ -5,6 +5,7 @@ const { AsyncWrap, Providers } = internalBinding('async_wrap'); const { Buffer } = require('buffer'); const { pbkdf2: _pbkdf2 } = internalBinding('crypto'); const { validateUint32 } = require('internal/validators'); +const { deprecate } = require('internal/util'); const { ERR_CRYPTO_INVALID_DIGEST, ERR_CRYPTO_PBKDF2_ERROR, @@ -51,11 +52,16 @@ function pbkdf2Sync(password, salt, iterations, keylen, digest) { return keybuf.toString(encoding); } +const defaultDigest = deprecate(() => 'sha1', + 'Calling pbkdf2 or pbkdf2Sync with "digest" ' + + 'set to null is deprecated.', + 'DEP0009'); + function check(password, salt, iterations, keylen, digest) { if (typeof digest !== 'string') { if (digest !== null) throw new ERR_INVALID_ARG_TYPE('digest', ['string', 'null'], digest); - digest = 'sha1'; + digest = defaultDigest(); } password = validateArrayBufferView(password, 'password'); diff --git a/test/parallel/test-crypto-pbkdf2.js b/test/parallel/test-crypto-pbkdf2.js index 0f5d4618ea156c..db12cf14fcc168 100644 --- a/test/parallel/test-crypto-pbkdf2.js +++ b/test/parallel/test-crypto-pbkdf2.js @@ -6,6 +6,11 @@ if (!common.hasCrypto) const assert = require('assert'); const crypto = require('crypto'); +common.expectWarning( + 'DeprecationWarning', + 'Calling pbkdf2 or pbkdf2Sync with "digest" set to null is deprecated.', + 'DEP0009'); + // // Test PBKDF2 with RFC 6070 test vectors (except #4) // @@ -64,7 +69,7 @@ assert.throws( ); assert.throws( - () => crypto.pbkdf2Sync('password', 'salt', -1, 20, null), + () => crypto.pbkdf2Sync('password', 'salt', -1, 20, 'sha1'), { code: 'ERR_OUT_OF_RANGE', name: 'RangeError [ERR_OUT_OF_RANGE]',