crypto: fixup scrypt regressions

Fixes a handful of regressions in scrypt support following the refactor. Fixes: #35815 PR-URL: #35821 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rich Trott <[email protected]>
nodejs · Nov 3, 2020 · 79a8fb6 · 79a8fb6
1 parent 8c2b179
commit 79a8fb6
Show file tree

Hide file tree

Showing 8 changed files with 18 additions and 16 deletions.
diff --git a/lib/internal/crypto/webcrypto.js b/lib/internal/crypto/webcrypto.js
@@ -131,7 +131,7 @@ async function deriveBits(algorithm, baseKey, length) {
         .pbkdf2DeriveBits(algorithm, baseKey, length);
     case 'NODE-SCRYPT':
       return lazyRequire('internal/crypto/scrypt')
-        .asyncScryptDeriveBits(algorithm, baseKey, length);
+        .scryptDeriveBits(algorithm, baseKey, length);
     case 'NODE-DH':
       return lazyRequire('internal/crypto/diffiehellman')
         .asyncDeriveBitsDH(algorithm, baseKey, length);

diff --git a/src/crypto/crypto_scrypt.cc b/src/crypto/crypto_scrypt.cc
@@ -28,6 +28,7 @@ ScryptConfig::ScryptConfig(ScryptConfig&& other) noexcept
     N(other.N),
     r(other.r),
     p(other.p),
+    maxmem(other.maxmem),
     length(other.length) {}
 
 ScryptConfig& ScryptConfig::operator=(ScryptConfig&& other) noexcept {
@@ -127,7 +128,7 @@ bool ScryptTraits::DeriveBits(
   ByteSource buf = ByteSource::Allocated(data, params.length);
   unsigned char* ptr = reinterpret_cast<unsigned char*>(data);
 
-  // Botht the pass and salt may be zero-length at this point
+  // Both the pass and salt may be zero-length at this point
 
   if (!EVP_PBE_scrypt(
           params.pass.get(),

diff --git a/src/crypto/crypto_util.cc b/src/crypto/crypto_util.cc
@@ -236,7 +236,9 @@ ByteSource& ByteSource::operator=(ByteSource&& other) noexcept {
 }
 
 std::unique_ptr<BackingStore> ByteSource::ReleaseToBackingStore() {
-  CHECK_NOT_NULL(allocated_data_);
+  // It's ok for allocated_data_ to be nullptr but
+  // only if size_ is not zero.
+  CHECK_IMPLIES(size_ > 0, allocated_data_ != nullptr);
   std::unique_ptr<BackingStore> ptr = ArrayBuffer::NewBackingStore(
       allocated_data_,
       size(),

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
@@ -58,13 +58,16 @@ void Initialize(Local<Object> target,
   PBKDF2Job::Initialize(env, target);
   Random::Initialize(env, target);
   RSAAlg::Initialize(env, target);
-  ScryptJob::Initialize(env, target);
   SecureContext::Initialize(env, target);
   Sign::Initialize(env, target);
   SPKAC::Initialize(env, target);
   Timing::Initialize(env, target);
   Util::Initialize(env, target);
   Verify::Initialize(env, target);
+
+#ifndef OPENSSL_NO_SCRYPT
+  ScryptJob::Initialize(env, target);
+#endif
 }
 
 }  // namespace crypto

diff --git a/test/parallel/test-crypto-scrypt.js b/test/parallel/test-crypto-scrypt.js
@@ -8,7 +8,7 @@ const assert = require('assert');
 const crypto = require('crypto');
 
 const { internalBinding } = require('internal/test/binding');
-if (typeof internalBinding('crypto').scrypt !== 'function')
+if (typeof internalBinding('crypto').ScryptJob !== 'function')
   common.skip('no scrypt support');
 
 const good = [
@@ -156,9 +156,7 @@ for (const options of good) {
 
 for (const options of bad) {
   const expected = {
-    code: 'ERR_CRYPTO_SCRYPT_INVALID_PARAMETER',
-    message: 'Invalid scrypt parameter',
-    name: 'Error',
+    message: /Invalid scrypt param/,
   };
   assert.throws(() => crypto.scrypt('pass', 'salt', 1, options, () => {}),
                 expected);
@@ -168,9 +166,7 @@ for (const options of bad) {
 
 for (const options of toobig) {
   const expected = {
-    message: new RegExp('error:[^:]+:digital envelope routines:' +
-                        '(?:EVP_PBE_scrypt|scrypt_alg):memory limit exceeded'),
-    name: 'Error',
+    message: /Invalid scrypt param/
   };
   assert.throws(() => crypto.scrypt('pass', 'salt', 1, options, () => {}),
                 expected);

diff --git a/test/parallel/test-webcrypto-derivebits.js b/test/parallel/test-webcrypto-derivebits.js
@@ -102,7 +102,7 @@ const { internalBinding } = require('internal/test/binding');
 }
 
 // Test Scrypt bit derivation
-if (typeof internalBinding('crypto').scrypt === 'function') {
+if (typeof internalBinding('crypto').ScryptJob === 'function') {
   async function test(pass, salt, length, expected) {
     const ec = new TextEncoder();
     const key = await subtle.importKey(
@@ -111,7 +111,7 @@ if (typeof internalBinding('crypto').scrypt === 'function') {
       { name: 'NODE-SCRYPT' },
       false, ['deriveBits']);
     const secret = await subtle.deriveBits({
-      name: 'SCRYPT',
+      name: 'NODE-SCRYPT',
       salt: ec.encode(salt),
     }, key, length);
     assert.strictEqual(Buffer.from(secret).toString('hex'), expected);

diff --git a/test/parallel/test-webcrypto-derivekey.js b/test/parallel/test-webcrypto-derivekey.js
@@ -122,7 +122,7 @@ const { internalBinding } = require('internal/test/binding');
 }
 
 // Test Scrypt bit derivation
-if (typeof internalBinding('crypto').scrypt === 'function') {
+if (typeof internalBinding('crypto').ScryptJob === 'function') {
   async function test(pass, salt, expected) {
     const ec = new TextEncoder();
     const key = await subtle.importKey(
@@ -144,7 +144,7 @@ if (typeof internalBinding('crypto').scrypt === 'function') {
   }
 
   const kTests = [
-    ['hello', 'there', 10, 'SHA-256',
+    ['hello', 'there',
      '30ddda6feabaac788eb81cc38f496cd5d9a165d320c537ea05331fe720db1061']
   ];
 

diff --git a/test/sequential/test-async-wrap-getasyncid.js b/test/sequential/test-async-wrap-getasyncid.js
@@ -145,7 +145,7 @@ if (common.hasCrypto) { // eslint-disable-line node-core/crypto-check
     testInitialized(this, 'RandomBytesJob');
   }));
 
-  if (typeof internalBinding('crypto').scrypt === 'function') {
+  if (typeof internalBinding('crypto').ScryptJob === 'function') {
     crypto.scrypt('password', 'salt', 8, common.mustCall(function() {
       testInitialized(this, 'ScryptJob');
     }));