From 986fb5b02d77e07593f92b678b3a108e360d5e5d Mon Sep 17 00:00:00 2001 From: James M Snell Date: Mon, 26 Oct 2020 11:25:42 -0700 Subject: [PATCH] crypto: fixup scrypt regressions Fixes a handful of regressions in scrypt support following the refactor. Fixes: https://github.com/nodejs/node/issues/35815 --- lib/internal/crypto/webcrypto.js | 2 +- src/crypto/crypto_scrypt.cc | 3 ++- src/crypto/crypto_util.cc | 7 ++++++- src/node_crypto.cc | 5 ++++- test/parallel/test-crypto-scrypt.js | 10 +++------- test/parallel/test-webcrypto-derivebits.js | 4 ++-- test/parallel/test-webcrypto-derivekey.js | 4 ++-- test/sequential/test-async-wrap-getasyncid.js | 2 +- 8 files changed, 21 insertions(+), 16 deletions(-) diff --git a/lib/internal/crypto/webcrypto.js b/lib/internal/crypto/webcrypto.js index 0a7101bfcc78c2..81ff9ca5191004 100644 --- 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 index 1af9853a6710e3..39d6b3fd0d8d6a 100644 --- 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(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 index c8a02cc335b66a..1ee51315fa41cd 100644 --- a/src/crypto/crypto_util.cc +++ b/src/crypto/crypto_util.cc @@ -236,7 +236,12 @@ ByteSource& ByteSource::operator=(ByteSource&& other) noexcept { } std::unique_ptr ByteSource::ReleaseToBackingStore() { - CHECK_NOT_NULL(allocated_data_); + CHECK_IMPLIES(size_ == 0, allocated_data_ == nullptr); + if (size_ == 0) { + return ArrayBuffer::NewBackingStore( + nullptr, 0, [](void* data, size_t length, void* deleter_data) {}, + nullptr); + } std::unique_ptr ptr = ArrayBuffer::NewBackingStore( allocated_data_, size(), diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 0c1dbcc52d748c..d25387f1425daa 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -58,13 +58,16 @@ void Initialize(Local 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 index 6c19dee23291bb..aec86d64891518 100644 --- 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 index a8d4d6e673862f..0556b8c15ceba3 100644 --- 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 index 9434da1fb672fb..f072fe01682792 100644 --- 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 index 22fde0916544d6..e37e613b747dc2 100644 --- 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'); }));