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

generateKeyPair with blank passphrase prompts "Enter PEM pass phrase" in Node 15 #35898

Closed
davidje13 opened this issue Oct 31, 2020 · 4 comments
Assignees
Labels
confirmed-bug Issues with confirmed bugs. crypto Issues and PRs related to the crypto subsystem. security Issues and PRs related to security.

Comments

@davidje13
Copy link

  • Version: v15.0.1
  • Platform: Darwin DaveMBP.local 18.7.0 Darwin Kernel Version 18.7.0: Mon Aug 31 20:53:32 PDT 2020; root:xnu-4903.278.44~1/RELEASE_X86_64 x86_64
  • Subsystem: crypto

What steps will reproduce the bug?

const crypto = require('crypto');

crypto.generateKeyPair('rsa', {
  modulusLength: 2048,
  privateKeyEncoding: {
    type: 'pkcs8',
    format: 'pem',
    cipher: 'aes-256-cbc',
    passphrase: '', // <-- blank string passphrase
  },
  publicKeyEncoding: { type: 'spki', format: 'pem' },
},  (err, publicKey, privateKey) => console.log(`got key\n\n${publicKey}\n\n${privateKey}`));

What is the expected behaviour?

In NodeJS 14 and below, the above generates an output without any prompts.

What do you see instead?

Since NodeJS 15, the above issues a prompt on the terminal:

Enter PEM pass phrase:

Which hangs until the user provides input (i.e. forever on a CI server).

Additional information

It seems reasonable for a blank string to be rejected as an input here if a cipher is being used, but it should either work or throw an exception. Triggering a command-line prompt is not a good user experience, and makes this relatively difficult to track-down.

In my particular case, I allow users of my project to configure a blank passphrase to mean "don't bother encrypting this", which I can achieve myself by detecting a blank passphrase and passing undefined for both cipher and passphrase in Node 15, which is fine. My personal preference would be for this to throw if given a blank passphrase, but that would still be a breaking change from 14, so maybe the way to go is to allow blank passphrases as before.

@Trott
Copy link
Member

Trott commented Nov 1, 2020

@nodejs/crypto @jasnell

@tniessen tniessen self-assigned this Nov 1, 2020
@tniessen tniessen added confirmed-bug Issues with confirmed bugs. crypto Issues and PRs related to the crypto subsystem. labels Nov 1, 2020
@tniessen
Copy link
Member

tniessen commented Nov 1, 2020

Fix in #35914

@tniessen tniessen added the security Issues and PRs related to security. label Nov 2, 2020
@tniessen
Copy link
Member

tniessen commented Nov 2, 2020

Adding the security label as a precaution due to the DoS potential resulting from this. This does not crash the process, but can cause it to become unresponsive.

tniessen added a commit to tniessen/node that referenced this issue Nov 2, 2020
@tniessen
Copy link
Member

tniessen commented Nov 4, 2020

Thank you for the report @davidje13, this is really good to know! The fix will likely be included in the next release of Node.js 15.

targos pushed a commit that referenced this issue 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. crypto Issues and PRs related to the crypto subsystem. security Issues and PRs related to security.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@Trott @tniessen @davidje13 and others