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: fail early if passphrase is too long #27010

Conversation

tniessen
Copy link
Member

This causes OpenSSL to fail early if the decryption passphrase is too long, and produces a somewhat helpful error message. OpenSSL gives us a buffer of limited size (currently 1024 bytes), so there is no way to pass longer passphrases.

Refs: #25208

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

This causes OpenSSL to fail early if the decryption passphrase is too
long, and produces a somewhat helpful error message.

Refs: nodejs#25208
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Mar 30, 2019
@tniessen
Copy link
Member Author

This was @sam-github's idea in #25208 (comment), thanks Sam! :)

doc/api/crypto.md Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
@tniessen
Copy link
Member Author

I think this is technically semver-major? So cc @nodejs/tsc

@tniessen tniessen added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 31, 2019
@rvagg
Copy link
Member

rvagg commented Apr 1, 2019

Oooo, tricky. Where does this fail without this fix and what does that failure look like? It's not from the createPrivateKey() call at the moment as far as I can tell.
I'm tempted to say this is a bugfix and therefore not breaking, but we've been very careful with error messages / codes etc. in the last few years so maybe we're forced to make it semver-major depending on what failure looks like. If it's something like a silent failure then I'd say it's a bugfix and can be semver-patch.

@tniessen
Copy link
Member Author

tniessen commented Apr 1, 2019

@rvagg It is indeed tricky! The failure without this patch is unpredictable. createPrivateKey might fail with the message bad decrypt if the PKCS#7 padding is invalid after decryption, this is the most likely behavior. However, if the PKCS#7 padding matches by chance, it can also result in an asn1 encoding error. And there is a tiny chance that both decryption and ASN.1 decoding succeed (either correctly or incorrectly, but without producing an error), and some other API call would randomly fail later when trying to use the key.

Note that this should also be an extremely rare case, supplying such a long passphrase does not make sense since the entropy of the passphrase would far exceed the entropy of the derived decryption key.

@rvagg
Copy link
Member

rvagg commented Apr 2, 2019

OK, so I'm going to go out on a limb and suggest that those failure modes mean we have bugs in our interface and therefore this should be semver-patch.

Anyone else have an opinion? @tniessen what's your position?

@tniessen
Copy link
Member Author

tniessen commented Apr 2, 2019

I'm usually leaning towards semver-patch too easily 😅 This does change the error message and code, but on the other hand, it also provides a stable solution instead of the current unpredictable behavior. Personally, I feel that this should land on all release lines where that is possible, simply to get rid of the unpredictability, but I am also fine with a TSC decision to treat this as semver-major.

those failure modes mean we have bugs in our interface and therefore this should be semver-patch.

From that perspective, this certainly is a bugfix, we should not have accepted passphrases that do not fit into the buffer in the first place.

@sam-github
Copy link
Contributor

We've been more lax about semver-major's recently. In this case, someone would have to be relying on passing a passphrase that is too large, and it getting truncated... which is pretty obscure. I'd be OK with semver-patch, mostly because I don't want this to float until the fall for 13.x, continually causing backport conflict.

@tniessen tniessen removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 2, 2019
@tniessen
Copy link
Member Author

tniessen commented Apr 2, 2019

I removed the semver-major label, feel free to chime in @nodejs/tsc.

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 3, 2019
@danbev
Copy link
Contributor

danbev commented Apr 3, 2019

Re-build of failing node-test-commit-linux (✔️)

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Apr 4, 2019
This causes OpenSSL to fail early if the decryption passphrase is too
long, and produces a somewhat helpful error message.

PR-URL: nodejs#27010
Refs: nodejs#25208
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
@BridgeAR
Copy link
Member

BridgeAR commented Apr 4, 2019

Landed in 73bca57 🎉

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants