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

crypto: fix propagation of "memory limit exceeded" #53300

Merged
merged 1 commit into from
Jun 5, 2024

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented 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: #53291

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 tniessen added crypto Issues and PRs related to the crypto subsystem. openssl Issues and PRs related to the OpenSSL dependency. labels Jun 3, 2024
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jun 3, 2024
Copy link
Member

@panva panva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@panva panva added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jun 3, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 3, 2024
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@panva panva added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 5, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 5, 2024
@nodejs-github-bot nodejs-github-bot merged commit 0281e2c into nodejs:main Jun 5, 2024
72 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 0281e2c

RafaelGSS pushed a commit that referenced this pull request 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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Jun 7, 2024
eliphazbouye pushed a commit to eliphazbouye/node that referenced this pull request 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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
bmeck pushed a commit to bmeck/node that referenced this pull request 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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
marco-ippolito pushed a commit that referenced this pull request 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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
marco-ippolito pushed a commit that referenced this pull request 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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
codebytere added a commit to electron/electron that referenced this pull request Jul 25, 2024
codebytere added a commit to electron/electron that referenced this pull request Jul 26, 2024
jkleinsc pushed a commit to electron/electron that referenced this pull request Jul 26, 2024
* chore: bump node in DEPS to v20.16.0

* test: skip unstable shadow realm gc tests

nodejs/node#52855

* test: extend env for `test-node-output-errors`

nodejs/node#53535

* src: fix typo in env.cc

nodejs/node#53418

* src: reset `process.versions` during pre-execution

nodejs/node#53444

* chore: fixup patch indices

* src,permission: --allow-wasi & prevent WASI exec

nodejs/node#53124

* tls: use SSL_get_peer_tmp_key

nodejs/node#53366

* deps: update c-ares to 1.29.0

nodejs/node#53155

* src: account for OpenSSL unexpected version

* crypto: fix propagation of "memory limit exceeded"

nodejs/node#53300

* process: add process.getBuiltinModule(id)

nodejs/node#52762

* windows 32bit: config change callback needs to be stdcall

c-ares/c-ares@8f265c9

* fix: building with UNICODE

c-ares/c-ares#802

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <[email protected]>
Co-authored-by: Keeley Hammond <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. openssl Issues and PRs related to the OpenSSL dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Invalid scrypt params" error message is unnecessarily generic when exceeding maxmem
4 participants