JS: decoding JWT without signature verification#14088
Conversation
… results now, ConstantString is suggested as a better alternative for finding constant sources
|
No-verification query should be under CWE-347. |
… pkg, create a path query for jsonwebtoken package which is not work correctly
am0o0
changed the title
…th-problem, update jsonwebtoken source and sinks
|
@JarLob I created one query file for each JWT package, but these all can be merged in some cases where the JWT is decoded in an unsafe way by package1 and then the signature has been validated by package2. it is easy to merge all these together because all have the same template ( only two |
|
Hi @JarLob |
|
@am0o0 Could you please change the pull request from draft to ready for review, so someone from JavaScript CodeQL team can take a look? |
|
First you need to move everything into the I'll do a review after you've done that. |
|
QHelp previews: javascript/ql/src/experimental/Security/CWE-347/decodeJwtWithoutVerification.qhelpJWT missing secret or public key verificationA JSON Web Token (JWT) is used for authenticating and managing users in an application. Only Decoding JWTs without checking if they have a valid signature or not can lead to security vulnerabilities. RecommendationDon't use methods that only decode JWT, Instead use methods that verify the signature of JWT. ExampleIn the following code, you can see the proper usage of the most popular JWT libraries. const express = require('express')
const app = express()
const jwtJsonwebtoken = require('jsonwebtoken');
const jwt_decode = require('jwt-decode');
const jwt_simple = require('jwt-simple');
const jose = require('jose')
const port = 3000
function getSecret() {
return "A Safe generated random key"
}
app.get('/jose1', async (req, res) => {
const UserToken = req.headers.authorization;
// GOOD: with signature verification
await jose.jwtVerify(UserToken, new TextEncoder().encode(getSecret()))
})
app.get('/jose2', async (req, res) => {
const UserToken = req.headers.authorization;
// GOOD: first without signature verification then with signature verification for same UserToken
jose.decodeJwt(UserToken)
await jose.jwtVerify(UserToken, new TextEncoder().encode(getSecret()))
})
app.get('/jwtSimple1', (req, res) => {
const UserToken = req.headers.authorization;
// GOOD: first without signature verification then with signature verification for same UserToken
jwt_simple.decode(UserToken, getSecret(), false);
jwt_simple.decode(UserToken, getSecret());
})
app.get('/jwtSimple2', (req, res) => {
const UserToken = req.headers.authorization;
// GOOD: with signature verification
jwt_simple.decode(UserToken, getSecret(), true);
jwt_simple.decode(UserToken, getSecret());
})
app.get('/jwtJsonwebtoken1', (req, res) => {
const UserToken = req.headers.authorization;
// GOOD: with signature verification
jwtJsonwebtoken.verify(UserToken, getSecret())
})
app.get('/jwtJsonwebtoken2', (req, res) => {
const UserToken = req.headers.authorization;
// GOOD: first without signature verification then with signature verification for same UserToken
jwtJsonwebtoken.decode(UserToken)
jwtJsonwebtoken.verify(UserToken, getSecret())
})
app.listen(port, () => {
console.log(`Example app listening on port ${port}`)
})In the following code, you can see the improper usage of the most popular JWT libraries. const express = require('express')
const app = express()
const jwtJsonwebtoken = require('jsonwebtoken');
const jwt_decode = require('jwt-decode');
const jwt_simple = require('jwt-simple');
const jose = require('jose')
const port = 3000
function getSecret() {
return "A Safe generated random key"
}
app.get('/jose', (req, res) => {
const UserToken = req.headers.authorization;
// BAD: no signature verification
jose.decodeJwt(UserToken)
})
app.get('/jwtDecode', (req, res) => {
const UserToken = req.headers.authorization;
// BAD: no signature verification
jwt_decode(UserToken)
})
app.get('/jwtSimple', (req, res) => {
const UserToken = req.headers.authorization;
// jwt.decode(token, key, noVerify, algorithm)
// BAD: no signature verification
jwt_simple.decode(UserToken, getSecret(), true);
})
app.get('/jwtJsonwebtoken', (req, res) => {
const UserToken = req.headers.authorization;
// BAD: no signature verification
jwtJsonwebtoken.decode(UserToken)
})
app.listen(port, () => {
console.log(`Example app listening on port ${port}`)
})References
|
| @@ -0,0 +1,66 @@ | |||
| /** | |||
| * @name This query is for seeing if we can have two taint config within on query file | |||
There was a problem hiding this comment.
This does not look like a file you meant to include?
This look like code you used for testing.
There was a problem hiding this comment.
I wrote this file for more clarification about what I wanted to do but I failed to express what I needed to get my query to the right point which I failed. this is what I want:
the same source has a Path to two different sinks, one is a dangerous sink, and the other is a guard sink. if one source has a path to both sinks then the source code doesn't contain any vulnerability, but if the this source only has a path to the dangerous sink then the source code contains a vulnerability and should detect it.
There was a problem hiding this comment.
Oh, I think I know why.
The DataFlow::PathNode includes which DataFlow::Configuration it came from.
So you need to check that the DataFlow::Node that you get from calling .getNode() on the source is the same between the two "sources", you can't use the DataFlow::PathNode from one configuration directly in another.
|
@erik-krogh sorry this PR was one of my first PRs and I didn't apply what I learnt from you and your colleagues to this PR. I hope the changes are acceptable now. |
|
The |
|
I forgot to update this branch, the tests are still OK. are you sure about failing? |
Yeah, they're passing now 🤷 The I think changing the from-where-select to the below might fix your issue (it's the fix I described further up): from ConfigurationUnverifiedDecode cfg, DataFlow::PathNode source, DataFlow::PathNode sink
where
cfg.hasFlowPath(source, sink) and
not exists(ConfigurationVerifiedDecode cfg2 |
cfg2.hasFlowPath(any(DataFlow::PathNode p | p.getNode() = source.getNode()), _)
)
select source.getNode(), source, sink, "Decoding JWT $@.", sink.getNode(),
"without signature verification"If that works you can use the new approach in your queries. |
…s and separate the local and remote query tests
|
Thanks. Everything looks good for a merge into Thanks again for the contribution. |
3 participants
No description provided.