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: pass empty passphrases to OpenSSL properly #35914

Closed

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Nov 1, 2020

This solves two related problems:

  1. PrivateKeyEncodingConfig was unable to distinguish between "no passphrase" and zero-length passphrases, since both may be stored as ByteSource(nullptr, 0).
  2. OpenSSL uses its default key callback when a cipher was specified, but a nullptr for the passphrase. However, this did happen when an empty passphrase was specified, because malloc(0) is allowed to return a nullptr.

I'm not sure why these problems don't affect older versions.

Fixes: #35898

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/quic

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 1, 2020
@tniessen tniessen added the crypto Issues and PRs related to the crypto subsystem. label Nov 1, 2020
src/crypto/crypto_keys.cc Show resolved Hide resolved
@tniessen tniessen force-pushed the crypto-fix-blank-passphrase-35898 branch from 75b73df to 17654c6 Compare November 1, 2020 22:46
@tniessen tniessen added the security Issues and PRs related to security. label Nov 2, 2020
@tniessen tniessen force-pushed the crypto-fix-blank-passphrase-35898 branch from 17654c6 to 6c6a42d Compare November 2, 2020 16:11
@tniessen tniessen force-pushed the crypto-fix-blank-passphrase-35898 branch from 6c6a42d to 6e328e4 Compare November 2, 2020 16:26
@tniessen tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 2, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 2, 2020
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot
Copy link
Collaborator

tniessen added a commit that referenced this pull request Nov 4, 2020
Fixes: #35898

PR-URL: #35914
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@tniessen
Copy link
Member Author

tniessen commented Nov 4, 2020

Landed in 9aafda5, thank you for reviewing!

@tniessen tniessen closed this Nov 4, 2020
@tniessen tniessen deleted the crypto-fix-blank-passphrase-35898 branch November 4, 2020 15:57
targos pushed a commit that referenced this pull request Nov 4, 2020
Fixes: #35898

PR-URL: #35914
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
// intentionally use a pointer that will likely cause a segmentation fault
// when dereferenced.
CHECK_EQ(pass_len, 0);
pass = reinterpret_cast<char*>(-1);
Copy link
Member

Choose a reason for hiding this comment

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

Somewhat belatedly... forging pointers is technically UB and can lead to miscompiles, even if the pointer is never dereferenced. I get why it's doing what it's doing but I would suggest to just set pass = "";

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. security Issues and PRs related to security.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

generateKeyPair with blank passphrase prompts "Enter PEM pass phrase" in Node 15
10 participants