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: don't crash on unknown keyObject.asymmetricKeyType #26786

Closed
wants to merge 7 commits into from
Closed

crypto: don't crash on unknown keyObject.asymmetricKeyType #26786

wants to merge 7 commits into from

Conversation

panva
Copy link
Member

@panva panva commented Mar 19, 2019

Fixes #26775

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

@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 19, 2019
@panva
Copy link
Member Author

panva commented Mar 19, 2019

I'd welcome hints and key material other than (ed/x25519 or ed/x448) for adding tests to this.

doc/api/crypto.md Outdated Show resolved Hide resolved
@sam-github
Copy link
Contributor

Maybe take an unencrypted X488 key and damage the curve OID so it will always be invalid? Or perhaps that will refuse to import?

@mscdex
Copy link
Contributor

mscdex commented Mar 19, 2019

I'm not sure if we should use 'unknown' or 'unsupported', I'd probably lean towards the latter since the type must be known (otherwise OpenSSL would probably throw an error), but node does not support/know about it yet.

Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

I guess flat-out removing the asymetricKeyType isn't an option? We are returning this string, but it doesn't appear to have any purpose in our API, am I missing something?

doc/api/crypto.md Outdated Show resolved Hide resolved
@mscdex
Copy link
Contributor

mscdex commented Mar 19, 2019

Having the key type is useful, I don't think we should get rid of it.

@panva
Copy link
Member Author

panva commented Mar 19, 2019

I guess flat-out removing the asyme,tricKeyType isn't an option?

No. The information we have about a key is scarse as-is.

So, 'unsupported' or 'unknown'? I'd also be fine with undefined.

@sam-github
Copy link
Contributor

definitely not 'undefined', its not. 'unsupported' was a good choice if we keep it.

@mscdex What use is the asymetricKeyType? Its not accepted as input by any API, it can't be switched on to decide what API to pass a key too. Not too push too hard, but the numeric equivalent in openssl can actually be used (must be used) to do things, not so in our API. I know we hate to take things out, but this is the time to pull it if its problematic, while its still only in 11.x.

@panva I'm super sympathetic to the lack of key data, and support that data being made available, but keeping this around doesn't get you that. You still don't know what kind of DH or DSA key it is, what the EC curve is, etc, etc.

@panva
Copy link
Member Author

panva commented Mar 19, 2019

@sam-github the API is already in use by libraries today. To begin with, it's a decent way to ignore keys that are not useful in any context, e.g. jose library having no use for DSA keys, keys that do not have asn1.js implementation yet, etc.

Its not accepted as input by any API, it can't be switched on to decide what API to pass a key too.

Indeed it is. ed25519 will get passed to oneshot for EdDSA while rsa will not. ec will be used with createECDH while rsa will be used for RSA-OAEP to wrap/derive a CEK.

If this got removed we must consider a stable replacement. E.g. #26709, we didn't land that one because the information it provided was already delivered by asymmetricKeyType so i'm quite surprised for that to be on the cutting room floor now.

@mscdex
Copy link
Contributor

mscdex commented Mar 19, 2019

What use is the asymetricKeyType? Its not accepted as input by any API, it can't be switched on to decide what API to pass a key too.

The key type may not be utilized within node core, but it's certainly helpful in userland when you're accepting arbitrary keys in your application/module.

@panva
Copy link
Member Author

panva commented Mar 20, 2019

Maybe take an unencrypted X488 key and damage the curve OID so it will always be invalid? Or perhaps that will refuse to import?

It'll refuse to import.

Error: error:0609E09C:digital envelope routines:pkey_set_type:unsupported algorithm
    at createPrivateKey (internal/crypto/keys.js:323:10)

Edit: tests added.

@sam-github
Copy link
Contributor

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

I am working on support for RSA-PSS so testing this might become a bit trickier soon.

@panva
Copy link
Member Author

panva commented Mar 22, 2019

I am working on support for RSA-PSS so testing this might become a bit trickier soon.

I was hoping you'd miss that :trollface:

@tniessen
Copy link
Member

I am afraid a rebase is necessary.

@panva
Copy link
Member Author

panva commented Mar 25, 2019

@tniessen done

@tniessen
Copy link
Member

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

Is there a reason why we do not just throw an error while running createPrivateKey()? That would be my expectation as a user.

@tniessen
Copy link
Member

That's debatable, but certainly also a valid approach. The only upside I see of throwing is that people will notify us when we forget to add a key type. However, even if asymmetricKeyType returns 'unsupported', people might be able to use the key since OpenSSL supports it, node just doesn't recognize it. Ideally, this value is never returned, but we aren't quite there yet.

@BridgeAR
Copy link
Member

@tniessen thanks for clarifying this. I guess in that case both approaches seem legit. What about adding a warning in case 'unsupported' is returned somewhere? That way we could tell users to notify us but the code itself will still work.

I personally have a slight preference for 'unknown' over 'unsupported'. If I would stumble upon this and would read unsupported, I'd have to look up in what way it's unsupported while unknown would be a indication that Node.js can not tell what type it is without having to look up anything.

@panva
Copy link
Member Author

panva commented Mar 27, 2019

@BridgeAR, we went back and forth on the appropriate value to use. As this crash makes the use of KeyObjects impossible with third-party inputs, could we move forward with it as-is? The ✅ are there.

@tniessen
Copy link
Member

Personally, I prefer 'unsupported'. I don't think we should warn or throw. If we don't recognize the key type, the user might be linking against a different crypto lib or OpenSSL version, we should try to be flexible there IMO.

@sam-github
Copy link
Contributor

sam-github commented Mar 29, 2019

The thing about 'unsupported' is that I think it is supported, in the the key can be re-encoded as PEM, and used (possibly) in sign/verify, etc. Isn't that the case?

How about we just encode the numeric key id as a string? default: .asymetricKeyType = sprintf("key-type-%d', nid) (obviously pseudo-code). So, then the nid is known to the user, and if their code depends on the specific key type, and something isn't working, we'll get a useful bug report, and they will have an idea what is wrong.

Which argues for a .nid property on keys.... and maybe a .oid, if its guaranteed by the openssl API that every EVP_KEY_X has a nid and OID.

@panva
Copy link
Member Author

panva commented Mar 29, 2019

Here’s the thing. If we return a constructed string it might look intentional and someone might start using it thinking it’s final.

But then we add a string for the type later and it’ll be breaking for the developer.

@tniessen
Copy link
Member

Here’s the thing. If we return a constructed string it might look intentional and someone might start using it thinking it’s final.

I share this concern. We could choose something inbetween such as ${oid}-unsupported.

Which argues for a .nid property on keys.... and maybe a .oid, if its guaranteed by the openssl API that every EVP_KEY_X has a nid and OID.

Ref #26709. I think we should avoid using NIDs since they are specific to OpenSSL IIRC. Each key type must have an associated OID, though, since OIDs are used to identify key types when encoded as PKCS#8 / DER etc.

@sam-github
Copy link
Contributor

sam-github commented Mar 29, 2019

I share this concern. We could choose something inbetween such as ${oid}-unsupported.

Its in-between, but has the exact same problem as key-id-$(nid).

Anything, other than an actual string specific to the key id will eventually change, and be semver-major when we start to support the numeric type. This argues strongly against returning any string for the .asymetricKeyType if we don't recognize the numeric key type. Perhaps the property should be defined, but with an actual value of undefined? Changing that to a string when we add support for a new EVP key ID should not be semver-major.

(EDIT: I rewrote the above slightly, sorry, I think the old text was confusing).

I can think of a couple solutions:

  1. add a property that has an OID valud (or nid, it doesn't matter to me that its an openssl API artifact, we only support openssl), because they will always exist, and in order to not have people be typing OIDs into their code, add crypto.constants.KEY_ID_RSA, crypto.constants.KEY_ID_EC, etc., that have an OID as a value, and tell people to start using those constants to check key types, not .asymetricKeyType. Maybe .asymetricKeyType can be deprecated.

  2. Start throwing errors. If someone gets the error thrown, they can make a feature request for support.

  3. Add every one of the existing key ids, along with a node.js string equivalent, to the switch statement of key types, and stop calling them "supported key types" in our docs, just "key types". Do this along with 2. (which should no longer happen, unless there is an openssl update that adds a key type and we didn't notice).

@sam-github
Copy link
Contributor

May I change this PR to undefined instead then?, as a followup you’ll be able to add all switch statements.

Sounds good to me, and also to @tniessen , so yes, please.

@tniessen
Copy link
Member

Ref #26996

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

Ping @nodejs/crypto

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@tniessen
Copy link
Member

tniessen commented Apr 1, 2019

Landed in 7c1fc93.

@tniessen tniessen closed this Apr 1, 2019
tniessen pushed a commit that referenced this pull request Apr 1, 2019
PR-URL: #26786
Fixes: #26775
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
BethGriggs pushed a commit that referenced this pull request Apr 5, 2019
PR-URL: #26786
Fixes: #26775
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants