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

Conversation

Projects
None yet
8 participants
@panva
Copy link
Contributor

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
@panva

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

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

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

This comment has been minimized.

Copy link
Member

commented Mar 19, 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?

@mscdex

This comment has been minimized.

Copy link
Contributor

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.

@sam-github
Copy link
Member

left a comment

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?

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

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

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

@panva

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

@tniessen
Copy link
Member

left a comment

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

@panva

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

I am afraid a rebase is necessary.

@panva panva force-pushed the panva:crypto-fix-key-type-crash branch from 84ef879 to 22aa3f3 Mar 25, 2019

@panva

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

@tniessen done

@tniessen

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

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

@tniessen

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

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.

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.

@tniessen

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

Ref #26996

@panva panva force-pushed the panva:crypto-fix-key-type-crash branch from 22aa3f3 to 8c5edf1 Mar 29, 2019

@panva panva force-pushed the panva:crypto-fix-key-type-crash branch from 8c5edf1 to 7f386e9 Mar 29, 2019

@tniessen
Copy link
Member

left a comment

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@tniessen tniessen referenced this pull request Mar 31, 2019

Closed

crypto: add support for RSA-PSS keys #26960

4 of 4 tasks complete
@tniessen

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

Landed in 7c1fc93.

@tniessen tniessen closed this Apr 1, 2019

tniessen added a commit that referenced this pull request Apr 1, 2019

crypto: don't crash on unknown asymmetricKeyType
PR-URL: #26786
Fixes: #26775
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

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

crypto: don't crash on unknown asymmetricKeyType
PR-URL: #26786
Fixes: #26775
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

@sam-github sam-github referenced this pull request Apr 10, 2019

Open

crypto: add OID constants for EVP_PKEY_ types #27181

0 of 4 tasks complete
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.