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: decode missing passphrase errors #25208

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
6 participants
@tniessen
Copy link
Member

tniessen commented Dec 24, 2018

When a user attempts to load an encrypted key without supplying a passphrase, a cryptic OpenSSL error is thrown. This change intercepts the OpenSSL error and throws a nice error code instead.

I would prefer to treat this as semver-minor.

cc @nodejs/crypto

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
@bnoordhuis
Copy link
Member

bnoordhuis left a comment

LGTM. Nice.

I would prefer to treat this as semver-minor.

Error message changes are normally semver-major. It would probably require an TSC exception to downgrade this to semver-minor.

Show resolved Hide resolved src/node_crypto.cc Outdated
@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Dec 27, 2018

Thanks, Ben. My primary concern is the difficulty of backporting other things, I guess having added key objects will make backporting difficult enough even without semver-major changes. (I am still unsure whether we can backport key objects to v10.x.) cc @nodejs/tsc for the semverity.

@mcollina
Copy link
Member

mcollina left a comment

Code LGTM. According to our rules, those types of changes are semver-major.
Did Key objects ship into a release yet?

@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Dec 28, 2018

Did Key objects ship into a release yet?

@mcollina Yes, #25175 (v11.6.0).

@mcollina

This comment has been minimized.

Copy link
Member

mcollina commented Dec 28, 2018

I’m +1 in shipping it as semver-minor in 11.

IMHO this type of PR is the reason why I prefer new features to go out as experimental for a bit.

@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Dec 29, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/19865/

@mcollina This would have changed existing errors with or without key objects, and making it experimental would not have helped with backporting. If we decide to treat this as semver-major, I would like to wait a little before landing it to make sure that we have a chance to backport all important semver-minor / patch commits first.

@mcollina

This comment has been minimized.

Copy link
Member

mcollina commented Dec 29, 2018

To make things clear: is this changing the errors of other APIs apart from the new Key? In that case.. this should really be semver-major.

@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Dec 30, 2018

@mcollina Yes, it does, consider this example which does not use key objects:

const crypto = require('crypto');

const key = `
-----BEGIN ENCRYPTED PRIVATE KEY-----
MIIC2TBTBgkqhkiG9w0BBQ0wRjAlBgkqhkiG9w0BBQwwGAQSIFFvMaBFyBvqqhY6
yTV2fMVVAgIUczAdBglghkgBZQMEASoEEGRetyFtHhnJ7TZTM2qolWkEggKAFg/h
GERtM1loEd+u8VAtLwTzBiXE5pmRpp/hX/1HrbBnzFjAsNtWlEtzpSuxuCoXtMst
uKRB8qveHlfTQPzopkRZtljfOkD1DhdJz8BXSZrFmVkMrUq6m4Y/rnqTqI5JmtmQ
qAXTBbl7u8TwMnqIaoSInEHnc+aiFT3KJuIq6PZy2rGKWGW2WB/OML2gANvHBI9n
gyOo4VZHNsR6VBbCRJErUFhF5Wk2/YJD9ejnvXH6pJFqZYvnCFjkSlR+4MdCHBSo
Ld0IoFjQ6X1uLLglFf/rQGKEQruLjTKmz6oe8nZIzrOoLmArir0DGTakEt0K6mha
0M5s9zNkdMd7XRns0uvmYHzbpNVWpUP5YUmf1BJLjTHex51Msjoz6v6ixinel852
5lS2wtVwXp8MXG9iofvMEDocmvn60vuksmgwxMccRWX2zAt8ixFefzIjM0KzPRpt
ByJP0B733u+DI0Y5bsiJVAxl7Gr8Io5k6Uk0nZziVK8+vDXLF2BNetp4kRM/XBaM
N/DcosGiAxOeJqSA45ethV8cHGZVuNOsCXSVomVoKIxgWhkyBzXv9sIbRSSGWfJQ
edWEV9t4RTCgIu+622JZFzw1PbWtEu4R38v0JZQN3zxkYPC7nFIfmx9unUWucoup
ZYbvlzjyNZ6VI8jDvvqy+XmaY+FZcSgPTGCz/4KArxJuSvE8gJULUS7Y7JCuDjjL
h04pYsl8WMA3UH2/CxiFv75vXZI0q2HKUnNNawrQG83zPfBiVrDQARifCkPmzsCd
tHd8A/agDAeg9rmat6PRC4d0to6pUg7v5ZR9VZkRWMJiPMtuH4fh/2L/ys/9EihG
CZJe6XTZkgFAp9gzGg==
-----END ENCRYPTED PRIVATE KEY-----
`;

const message = Buffer.from('Hello world', 'utf8');
crypto.publicEncrypt(key, message);

With node <= 11.6.0:

internal/crypto/cipher.js:53
    return method(data, format, type, passphrase, buffer, padding);
           ^

Error: error:06065064:digital envelope routines:EVP_DecryptFinal_ex:bad decrypt

(And even weirder error messages for about 0.4% of all encrypted private keys due to PKCS padding, this caused issues such as #22978).

With this patch, the error message is consistent when no passphrase was supplied:

internal/crypto/cipher.js:52
    return method(data, format, type, passphrase, buffer, padding);
           ^

TypeError: Passphrase required for encrypted key

In that case.. this should really be semver-major.

Okay, then let's wait until we know about the backporting situation in crypto before landing this. Thanks for discussing this, Matteo!

@tniessen tniessen added semver-major and removed semver-minor labels Dec 30, 2018

Show resolved Hide resolved src/node_crypto.cc Outdated
Show resolved Hide resolved src/node_crypto.cc Outdated

@tniessen tniessen force-pushed the tniessen:crypto-throw-if-encrypted branch from d2202aa to 82e0fee Jan 4, 2019

@sam-github
Copy link
Member

sam-github left a comment

Improving the error codes is a good idea. Its semver-major, though. Landing is OK IMO. I put a suggestion that, if possible, might make the changes less intrusive. Also, Tobias, just FYI, I I had to put aside some work I was doing to make all crypto errors have .code, .function, .library, .reason properties, like the tls_wrap/SSL errors now do. I'll pick it up again after TLS1.3. Its related, because sticking to the SSL error scheme would fit into that work (again, if possible).

Show resolved Hide resolved src/node_crypto.cc Outdated
Show resolved Hide resolved src/node_crypto.cc Outdated
Show resolved Hide resolved doc/api/errors.md Outdated
len = len > buflen ? buflen : len;
memcpy(buf, u, len);
memcpy(buf, passphrase, len);

This comment has been minimized.

Copy link
@sam-github

sam-github Jan 4, 2019

Member

The existing code was like this, but looking at openssl source, couldn't we return -1 down below if there is no passphrase? That would cause openssl to immediately error with PEM_R_BAD_PASSWORD_READ, rather than try to decrypt the PEM with a zero-length passphrase, and then fail later with some semi-random decoding error because the decrypted data is garbage.

As long as the passphrase callback is only made if a passphrase is actually needed (isn't this the case?), that would be more direct, as it would make the returned SSL error actually be informative, so you wouldn't need to override it.

Just a thought, maybe it won't work, but it seems possible.

This comment has been minimized.

Copy link
@sam-github

sam-github Jan 4, 2019

Member

And actually, perhaps we should be returning -1 if the passphrase is longer than the buffer openssl is giving us... if the passphrase is truncated its going to do a bad decrypt, and lead to strange error codes, seems to me that its better to fail immediately with -1.

This comment has been minimized.

Copy link
@tniessen

tniessen Jan 15, 2019

Author Member

Based on the fact that we are not doing this already, I did not think there was a way to let OpenSSL know that there is no password. Sounds like a good idea though!

@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Jan 15, 2019

Also, Tobias, just FYI, I I had to put aside some work I was doing to make all crypto errors have .code, .function, .library, .reason properties

Thanks for letting me know! I just looked at the PR and so far, it seems to only affect SSL, not crypto, right? Generally, having such a code on all crypto errors would be desirable, as long as it does not become too OpenSSL specific.

@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Mar 28, 2019

@tniessen Any chance of getting this landed so it can be in 12.x?

@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Mar 28, 2019

@sam-github It's on my list, I assume it makes sense to wait for #26868 to land first? I'll try to get this into node 12!

@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Mar 28, 2019

#26868 had an unrelated test failure so I resumed, I'm hopeful it can land within a couple hours.

crypto: decode missing passphrase errors
When a user attempts to load an encrypted key without supplying a
passphrase, a cryptic OpenSSL error is thrown. This change intercepts
the OpenSSL error and throws a nice error code instead.

@tniessen tniessen force-pushed the tniessen:crypto-throw-if-encrypted branch from 82e0fee to 36de9dc Mar 29, 2019

@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Mar 29, 2019

@sam-github I rebased the changes. If I use your suggestion and return -1 instead of throwing myself, the error becomes:

  Comparison {
+   code: 'ERR_OSSL_PEM_BAD_PASSWORD_READ',
+   message: 'error:09078068:PEM routines:d2i_PKCS8PrivateKey_bio:bad password read',
+   type: [Function: Error] {
+     prepareStackTrace: undefined,
+     stackTraceLimit: 0
+   }
-   code: 'ERR_CRYPTO_READ_KEY',
-   message: 'Passphrase required for encrypted key',
-   type: [Function: TypeError]
  }

What is your preference? I have a slight preference for returning -1 and using the OpenSSL error, also in case the buffer is too small as you suggested before. No need to do anything fancy on our side :)

@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Mar 29, 2019

BAD_PASSWORD_READ means specifically that the password could not be read, right? Not that the password was the wrong password?

I like the .code better. I've mixed feelings about the .message, the one you used is definitely more readable, but its also inconsistent.

Just for background, if you take a quick look at all the calls to ThrowCryptoError(), you can see that (usually) a default message is passed along with the error code. In existing code (before and after my change to decorate the Error), that message is only used when err == 0, its so there can be something in the .message if for some reason there wasn't an openssl error.

Aside: I'm a bit suspicious of this. ThrowCryptoError() is called only when an openssl API returned failure, so if err == 0, then that's an openssl API bug. That is surely possible, but shouldn't be typical, and maybe the bugs ThrowCryptoError() is working around were specific to 1.0.2, and have been fixed for years. I'd like to look into this some time, its possible its basically dead code.

Anyhow, I changed ThrowCryptoError() so that the user-supplied message string was used allways, not just as a default, thinking it would make more human-readable .message strings, but then backed that out:

  1. it's VERY semver-major
  2. they were more human-readable, but they lacked the actual reason for failure, which shows up in the long-style openssl message, as well as .reason. The actual reason was still around, visible in the .code and .reason, but it seemed to me that not having the .message be more complete wasn't helpful, in many situations its probably only the .message string that will be displayed to a user, and while the long-style string is pretty low-level in its format, its also very informative

What I think I'd prefer, longer term, is for ThrowCryptoError to essentially make .message = default_message + ": " + .reason, so we have a human-readable prefix that node_crypto.cc provides, that gives some node.js specific context to the error, but its then followed by the openssl-provided reason for the failure. That gives us more readable errors without loss of information, and with full details still available in the openssl-specific Error properties (function/reason/library).

As far as this PR goes, I think the way you have it now is reasonable. Its consistent with everything else in node_crypto.cc, and informative enough that users will understand what went wrong.

@tniessen tniessen added this to the 12.0.0 milestone Mar 29, 2019

@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Mar 29, 2019

BAD_PASSWORD_READ means specifically that the password could not be read, right?

Yes.

I like the .code better. I've mixed feelings about the .message, the one you used is definitely more readable, but its also inconsistent.

I agree. It's the inconsistency that bothers me.

I'd like to look into this some time, its possible its basically dead code.

Whenever I have to use ThrowCryptoError, I hope that the default message will never be used. If we try to include those in the error messages, we should revisit all of them. Some refer to specific OpenSSL APIs and are most likely outdated.

As far as this PR goes, I think the way you have it now is reasonable. Its consistent with everything else in node_crypto.cc, and informative enough that users will understand what went wrong.

I am still not sure what's better... We could let OpenSSL deal with it and end up with an OpenSSL error code. That might be good because:

  1. It reduces the complexity of our own implementation -- we don't need PasswordCallbackInfo anymore.
  2. It is consistent with all other OpenSSL errors thrown in crypto and we don't need to add a new error code.

On the other hand,

  1. The error message is less expressive.
  2. If I add the check for the buffer size separately, both error conditions (no password / password buffer too small) will result in the same error, which is undesirable.

Thinking about it a bit more, I think my preference is to land this after addressing your comments above and changing the error code to be more specific, and then adding the buffer size check separately, such that both conditions will have separate error codes. Is that okay? (Maybe I misunderstood your preference, feel free to correct me.)

@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Mar 29, 2019

Your plan sounds sensible to me.

tniessen added some commits Mar 29, 2019

@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Mar 29, 2019

@sam-github PTAL :)

@nodejs-github-bot

This comment has been minimized.

@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Mar 30, 2019

Landed in 2e2c015, thanks for reviewing thoroughly!

@tniessen tniessen closed this Mar 30, 2019

tniessen added a commit that referenced this pull request Mar 30, 2019

crypto: decode missing passphrase errors
When a user attempts to load an encrypted key without supplying a
passphrase, a cryptic OpenSSL error is thrown. This change intercepts
the OpenSSL error and throws a nice error code instead.

PR-URL: #25208
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

tniessen added a commit to tniessen/node that referenced this pull request Mar 30, 2019

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

Refs: nodejs#25208

@tniessen tniessen referenced this pull request Mar 30, 2019

Closed

crypto: fail early if passphrase is too long #27010

4 of 4 tasks complete

BridgeAR added a commit to BridgeAR/node that referenced this pull request Apr 4, 2019

crypto: fail early if passphrase is too long
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 <ruben@bridgewater.de>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

@tniessen tniessen referenced this pull request Apr 4, 2019

Closed

crypto: simplify missing passphrase detection #27089

2 of 2 tasks complete

BethGriggs added a commit that referenced this pull request Apr 5, 2019

crypto: decode missing passphrase errors
When a user attempts to load an encrypted key without supplying a
passphrase, a cryptic OpenSSL error is thrown. This change intercepts
the OpenSSL error and throws a nice error code instead.

PR-URL: #25208
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

@BethGriggs BethGriggs referenced this pull request Apr 18, 2019

Draft

v12.0.0 proposal #26930

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.