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

RFC confrontation on "none" algo #711

Closed
frankli0324 opened this issue Apr 20, 2020 · 6 comments
Closed

RFC confrontation on "none" algo #711

frankli0324 opened this issue Apr 20, 2020 · 6 comments

Comments

@frankli0324
Copy link

frankli0324 commented Apr 20, 2020

Description

According to https://tools.ietf.org/html/rfc7518#section-3.6, jws s "MUST NOT accept such objects as valid unless the application specifies that it is acceptable for a specific object to not be integrity protected"

Reproduction

> var token = jwt.sign({'a':1}, 'asdf', { algorithm: 'none'})
undefined
> jwt.verify(token)
{ a: 1, iat: 1587359376 }

maybe we should add some kind of switch options?

Expected output:

> var token = jwt.sign({'a':1}, 'asdf', { algorithm: 'none'})
undefined
> jwt.verify(token)
throws error
> jwt.verify(token, '', { algorithm: 'none'})
{ a: 1, iat: 1587359376 }
@frankli0324 frankli0324 changed the title RFC confrontation RFC confrontation on "none" algo Apr 20, 2020
@frankli0324
Copy link
Author

node-jsonwebtoken/verify.js

Lines 109 to 111 in 5f10bf9

if (!hasSignature && !options.algorithms) {
options.algorithms = ['none'];
}

so it's intentional?

@panva
Copy link
Contributor

panva commented Apr 20, 2020

@frankli0324 none won't be automatically accepted unless the developer provided no keyOrSecret. The three lines in isolation aren't the whole story.

node-jsonwebtoken/verify.js

Lines 101 to 118 in 5f10bf9

if (!hasSignature && secretOrPublicKey){
return done(new JsonWebTokenError('jwt signature is required'));
}
if (hasSignature && !secretOrPublicKey) {
return done(new JsonWebTokenError('secret or public key must be provided'));
}
if (!hasSignature && !options.algorithms) {
options.algorithms = ['none'];
}
if (!options.algorithms) {
options.algorithms = secretOrPublicKey.toString().includes('BEGIN CERTIFICATE') ||
secretOrPublicKey.toString().includes('BEGIN PUBLIC KEY') ? PUB_KEY_ALGS :
secretOrPublicKey.toString().includes('BEGIN RSA PUBLIC KEY') ? RSA_KEY_ALGS : HS_ALGS;
}

@frankli0324
Copy link
Author

from my point of view, providing no secret doesn't mean that one explicitly "specifies that it is acceptable for a specific object to not be integrity protected", more like a mistake one could jump into

@panva
Copy link
Contributor

panva commented Apr 20, 2020

We'll consider making the algorithms option required when accepting unsigned JWTs for the next major.

@frankli0324
Copy link
Author

seems that this issue is forgotten?

@jakelacey2012
Copy link
Contributor

This issue was fixed in #851, please do re-open if you have any questions.

nickhobbs94 referenced this issue Jan 3, 2023
* Check if node version supports asymmetricKeyDetails

* Validate algorithms for ec key type

* Rename variable

* Rename function

* Add early return for symmetric keys

* Validate algorithm for RSA key type

* Validate algorithm for RSA-PSS key type

* Check key types for EdDSA algorithm

* Rename function

* Move validateKey function to module

* Convert arrow to function notation

* Validate key in verify function

* Simplify if

* Convert if to switch..case

* Guard against empty key in validation

* Remove empty line

* Add lib to check modulus length

* Add modulus length checks

* Validate mgf1HashAlgorithm and saltLength

* Check node version before using key details API

* Use built-in modulus length getter

* Fix Node version validations

* Remove duplicate validateKey

* Add periods to error messages

* Fix validation in verify function

* Make asymmetric key validation the latest validation step

* Change key curve validation

* Remove support for ES256K

* Fix old test that was using wrong key types to sign tokens

* Enable RSA-PSS for old Node versions

* Add specific RSA-PSS validations on Node 16 LTS+

* Improve error message

* Simplify key validation code

* Fix typo

* Improve error message

* Change var to const in test

* Change const to let to avoid reassigning problem

* Improve error message

* Test incorrect private key type

* Rename invalid to unsupported

* Test verifying of jwt token with unsupported key

* Test invalid private key type

* Change order of object parameters

* Move validation test to separate file

* Move all validation tests to separate file

* Add prime256v1 ec key

* Remove modulus length check

* WIP: Add EC key validation tests

* Fix node version checks

* Fix error message check on test

* Add successful tests for EC curve check

* Remove only from describe

* Remove `only`

* Remove duplicate block of code

* Move variable to a different scope and make it const

* Convert allowed curves to object for faster lookup

* Rename variable

* Change variable assignment order

* Remove unused object properties

* Test RSA-PSS happy path and wrong length

* Add missing tests

* Pass validation if no algorithm has been provided

* Test validation of invalid salt length

* Test error when signing token with invalid key

* Change var to const/let in verify tests

* Test verifying token with invalid key

* Improve test error messages

* Add parameter to skip private key validation

* Replace DSA key with a 4096 bit long key

* Test allowInvalidPrivateKeys in key signing

* Improve test message

* Rename variable

* Add key validation flag tests

* Fix variable name in Readme

* Change private to public dsa key in verify

* Rename flag

* Run EC validation tests conditionally

* Fix tests in old node versions

* Ignore block of code from test coverage

* Separate EC validations tests into two different ones

* Add comment

* Wrap switch in if instead of having an early return

* Remove unsupported algorithms from asymmetric key validation

* Rename option to allowInvalidAsymmetricKeyTypes and improve Readme

* 9.0.0

* adding migration notes to readme

* adding changelog for version 9.0.0

Co-authored-by: julienwoll <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants