Disallow None algorithm by default
p-v-d-Veeken opened this issue · 6 comments
At my company we just discovered that the financial services application (NestJS backend) we are building has a critical security vulnerability because our JWT passport strategy accepted claims signed with the None algorithm. In the first place this is our fault of course, because we should've been more thorough with the implementation, also we weren't live for customers yet so really no harm done.
That being said, I think it's a bad design choice that using passport-jwt with the defaults unchanged results in an extremely unsafe authentication method. We were fortunate enough to find this vulnerability before any real harm was done, but I suspect there are a lot companies/projects/applications out there that unknowingly have this landmine in their codebase.
My suggestion is to disable the None algorithm (and any other unsafe algorithms) by default and optionally allow users to explicitly enable it.
Thoughts?
How would you propose explicitly disabling the "None" algorithm? jsonwebtoken jumps through some hoops to guess the correct algorithm based on the provided secret, whether the token is signed etc. See here. Based on this, simply passing a list of all of the "blessed" algorithms probably won't work and I'd rather not duplicate all of that logic.
jsonwebtoken doesn't appear to have an option to explicitly disable a single algorithm, only selectively enable them.
The best approach seems like it might be to force users of passport-jwt to set algorithms
to a list or explicitly set another frighteningly named option like INSECUREallowAllAlgorithms: true
to get the default guessing behavior of the jsonwebtoken library with no algorithms specified.
Just out of curiosity i tried to reproduce the issue of accepting unsinged tokens with the default configuration. But could not reproduce it with the default configuration. @p-v-d-Veeken maybe i'm overseeing something and you could provide a small example server and a token, how to reproduce your issue?
Findings
- if neither
secretOrKey
norsecretOrKeyProvider
are provided or ifsecretOrKey
is set to a non truthy value "TypeError: JwtStrategy requires a secret or key" is thrown by passport-jwt - as soon as a truthy value is set for
secretOrKey
(either bysecretOrKey
itself or viasecretOrKeyProvider
), jsonwebtoken module requires the token to have a signature. - only if
secretOrKeyProvider
is set to a function that sets a non truthy value forsecretOrKey
(eg(req, token, cb) => cb()
), I could get unsigned tokens to be accepted as valid (see example below). This only works because the jsonwebtoken module falls back tonone
algorithm if:
"no secret or key is provided" && "the token has no signature" && "options.algorithms is not set"
Conclusion
1.) To me it seems that passport-jwt already does a great job by requiring a secret or a key (and therefore disabling none
algorithm by default). It seems the consumer actively needs to misconfigure the strategy in order to allow unsigned tokens. Might be that nestjs under some circumstances does force that misconfiguration?
2.) It's questionable if automatically falling back to none
algorithm in jsonwebtoken is a great choice. But that would be an issue for jsonwebtoken module.
Possible improvements for passport-jwt
1.) Adding a check for non truthy secretOrKey
returned by secretOrKeyProvider and throwing an error if secretOrKey
is not set, might be an improvement (eventually a breaking change?).
2.) I think @mikenicholson's proposal makeing algorithms
a required option and throw when it is not set, seems to be a great – tho breaking – change to me.
3.) Akward configs like INSECUREallowAllAlgorithms: true
seem to be a really bad choice. none
should never be allowed in a auth environment anyway.
Example server
This is the server i used to test, where an unsigned token will be accepted:
const express = require('express');
const passport = require('passport');
const app = express();
const { ExtractJwt, Strategy: JwtStrategy} = require('passport-jwt');
const opts = {
jwtFromRequest: ExtractJwt.fromAuthHeaderAsBearerToken(),
secretOrKeyProvider: (req, token, cb) => cb()
}
passport.use('jwt', new JwtStrategy(opts, (jwt_payload, done) => {
console.log('Got a valid jwt payload', jwt_payload);
done(null, jwt_payload);
}));
app.get('/',
passport.authenticate('jwt', {session: false}),
function(req, res){
res.send('welcome in');
}
);
app.listen(4000);
Tested with this request
curl http://localhost:4000/ -H "Authorization: Bearer eyJhbGciOiJub25lIiwidHlwIjoiSldUIn0.eyJmb28iOiJiYXIiLCJpYXQiOjE2NDY0OTAzMzN9."
Used modules
"dependencies": {
"express": "4.17.3",
"passport": "0.5.2",
"passport-jwt": "4.0.0"
}
thanks for your analysis this has helped me, the simplest migration seems to me a standard algorithm (other than none
)
only if secretOrKeyProvider is set to a function that sets a non truthy value for secretOrKey (eg (req, token, cb) => cb()), I could get unsigned tokens to be accepted as valid
passport-jwt@4.0.1 fixed that issue with the update to jsonwebtoken@9.0.0. Looks like this issue can be closed.
only if secretOrKeyProvider is set to a function that sets a non truthy value for secretOrKey (eg (req, token, cb) => cb()), I >>could get unsigned tokens to be accepted as valid
passport-jwt@4.0.1 fixed that issue with the update to jsonwebtoken@9.0.0. Looks like this issue can be closed.
Does this mean that a secret is now always required? I have a use case to use a JWT that is unsigned (None algorithm). Is there any way to accept an unsigned JWT?
Does this mean that a secret is now always required? I have a use case to use a JWT that is unsigned (None algorithm). Is there any way to accept an unsigned JWT?
jsonwebtoken@9.0.0
and with that passport-jwt
do still allow unsigned tokens (alg: none) if they are explicitly enabled with algorithms: ['none']
.
But you should reconsider your requirements. If you accept unsigned tokens, you basically do not have any authorization, as everyone can create valid unsigned tokens.