Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Invalid scrypt params" error message is unnecessarily generic when exceeding maxmem #53291

Closed
greguz opened this issue Jun 3, 2024 · 4 comments · Fixed by #53300
Closed
Labels
crypto Issues and PRs related to the crypto subsystem.

Comments

@greguz
Copy link
Contributor

greguz commented Jun 3, 2024

Version

v20.13.1

Platform

Linux lindell 6.9.3-arch1-1 #1 SMP PREEMPT_DYNAMIC Fri, 31 May 2024 15:14:45 +0000 x86_64 GNU/Linux

Subsystem

node:crypto

What steps will reproduce the bug?

Reading the official scrypt docs, I see that the cost parameter should be a power of 2.

Following the OWASP docs, I wanted to use 2 power 17 to feed the cost parameter.

const { scryptSync, randomBytes } = require('node:crypto')

const secret = 'shhh'
const salt = randomBytes(16)

// throws RangeError: Invalid scrypt params, same with scrypt()
scryptSync(secret, salt, 64, {
  blockSize: 8,
  cost: 131072, // 2^17
  parallelization: 1,
})

How often does it reproduce? Is there a required condition?

Always reproduce.

What is the expected behavior? Why is that the expected behavior?

Hash is correctly derived.

What do you see instead?

A RangeError with the "Invalid scrypt params" message.

Additional information

No response

@greguz
Copy link
Contributor Author

greguz commented Jun 3, 2024

Also reproduces with Node.js v20.14.0.

@marco-ippolito marco-ippolito added the crypto Issues and PRs related to the crypto subsystem. label Jun 3, 2024
@panva
Copy link
Member

panva commented Jun 3, 2024

These parameters are exceeding the default memory upper bound option. It is an error when (approximately) 128 * N * r > maxmem, the default maxmem is 32 * 1024 * 1024.

@greguz
Copy link
Contributor Author

greguz commented Jun 3, 2024

Sorry, my bad. I got confused with the N and r aliases, plus the generic error message. Now I see the Node.js docs are in fact correct. Thank you for the fast answer.

@greguz greguz closed this as completed Jun 3, 2024
@tniessen tniessen reopened this Jun 3, 2024
@tniessen tniessen changed the title Invalid scrypt params error when there're valid (per official Node.js documentation) "Invalid scrypt params" error message is unnecessarily generic when exceeding maxmem Jun 3, 2024
tniessen added a commit to tniessen/node that referenced this issue Jun 3, 2024
When we throw ERR_CRYPTO_INVALID_SCRYPT_PARAMS after a call to
EVP_PBE_scrypt, check if OpenSSL reported an error and if so, append the
OpenSSL error message to the default generic error message. In
particular, this catches cases when `maxmem` is not sufficient, which
otherwise is difficult to identify because our documentation only
provides an approximation of the required `maxmem` value.

Fixes: nodejs#53291
@tniessen
Copy link
Member

tniessen commented Jun 3, 2024

OpenSSL unfortunately does not consistently report errors during the scrypt parameter validation, but it does if maxmem is insufficient. We might as well forward that error message: #53300

nodejs-github-bot pushed a commit that referenced this issue Jun 5, 2024
When we throw ERR_CRYPTO_INVALID_SCRYPT_PARAMS after a call to
EVP_PBE_scrypt, check if OpenSSL reported an error and if so, append the
OpenSSL error message to the default generic error message. In
particular, this catches cases when `maxmem` is not sufficient, which
otherwise is difficult to identify because our documentation only
provides an approximation of the required `maxmem` value.

Fixes: #53291
PR-URL: #53300
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
RafaelGSS pushed a commit that referenced this issue Jun 7, 2024
When we throw ERR_CRYPTO_INVALID_SCRYPT_PARAMS after a call to
EVP_PBE_scrypt, check if OpenSSL reported an error and if so, append the
OpenSSL error message to the default generic error message. In
particular, this catches cases when `maxmem` is not sufficient, which
otherwise is difficult to identify because our documentation only
provides an approximation of the required `maxmem` value.

Fixes: #53291
PR-URL: #53300
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
sophoniie pushed a commit to sophoniie/node that referenced this issue Jun 20, 2024
When we throw ERR_CRYPTO_INVALID_SCRYPT_PARAMS after a call to
EVP_PBE_scrypt, check if OpenSSL reported an error and if so, append the
OpenSSL error message to the default generic error message. In
particular, this catches cases when `maxmem` is not sufficient, which
otherwise is difficult to identify because our documentation only
provides an approximation of the required `maxmem` value.

Fixes: nodejs#53291
PR-URL: nodejs#53300
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
When we throw ERR_CRYPTO_INVALID_SCRYPT_PARAMS after a call to
EVP_PBE_scrypt, check if OpenSSL reported an error and if so, append the
OpenSSL error message to the default generic error message. In
particular, this catches cases when `maxmem` is not sufficient, which
otherwise is difficult to identify because our documentation only
provides an approximation of the required `maxmem` value.

Fixes: nodejs#53291
PR-URL: nodejs#53300
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
marco-ippolito pushed a commit that referenced this issue Jul 19, 2024
When we throw ERR_CRYPTO_INVALID_SCRYPT_PARAMS after a call to
EVP_PBE_scrypt, check if OpenSSL reported an error and if so, append the
OpenSSL error message to the default generic error message. In
particular, this catches cases when `maxmem` is not sufficient, which
otherwise is difficult to identify because our documentation only
provides an approximation of the required `maxmem` value.

Fixes: #53291
PR-URL: #53300
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
marco-ippolito pushed a commit that referenced this issue Jul 19, 2024
When we throw ERR_CRYPTO_INVALID_SCRYPT_PARAMS after a call to
EVP_PBE_scrypt, check if OpenSSL reported an error and if so, append the
OpenSSL error message to the default generic error message. In
particular, this catches cases when `maxmem` is not sufficient, which
otherwise is difficult to identify because our documentation only
provides an approximation of the required `maxmem` value.

Fixes: #53291
PR-URL: #53300
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants