Skip to content

Conversation

toufik-airane
Copy link
Contributor

@toufik-airane toufik-airane commented Jun 20, 2020

Outline

The featured CodeQL query warns using of none algorithm in verify() functions imported from jsonwebtoken package developed by the auth0 organization.

Details

JavaScript applications handling JWT could be affected by a well-known misconfiguration due to the misuse of the none algorithm.

Indeed, the jsonwebtoken package developed by the Auth0 company specifies that the none algorithm doesn't enforce digital signature and It's a feature. In that scenario, providing the value false instead of a secret or a key will decode the JWT without signature verification.

A secure coding framework requires auditable and automated security measures to ensure good practices in codebases.
By running the featured CodeQL query, you ensure that calling verify() functions always take a secret or a key to decode JWT payloads.

Resource

Add an experimental CodeQL query.
Add a test file for CWE-347.
The HS256 algorithm is safe, but the none algorithm is unsafe.
@toufik-airane
Copy link
Contributor Author

The status of the pull request is still in progress.
Thanks for your patience.

- Add help documentation
- Empty qll file
- rename examples
@toufik-airane toufik-airane marked this pull request as ready for review June 22, 2020 11:27
@toufik-airane toufik-airane requested a review from a team as a code owner June 22, 2020 11:27
@erik-krogh erik-krogh self-requested a review June 22, 2020 13:15
@erik-krogh erik-krogh self-assigned this Jun 22, 2020
Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

This is a very simple query, but it flags something important 👍

Your CodeQL is good for a first-time contributor 🎉 just a small comment there.
The documentation is mostly fine, but I got a few more comments for that.
(Documentation is hard, it also took me quite a while to learn).

Note: jwt.verify() is not a function, it is a function call to the verify function.

You don't have to accept the suggestions verbatim. Treat them as what they are: suggestions.

toufik-airane and others added 2 commits June 22, 2020 19:27
Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>
Copy link
Contributor Author

@toufik-airane toufik-airane left a comment

Choose a reason for hiding this comment

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

@erik-krogh Thank you for your feedback.
It was very insightful and valuable.

@toufik-airane toufik-airane requested a review from erik-krogh June 22, 2020 18:07
@toufik-airane toufik-airane changed the title JWT Missing Secret Or Public Key Verification [javascript] CWE-347: JWT Missing Secret Or Public Key Verification Jun 22, 2020
- add valuable text to assess the query results
- add an example of the output
Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

Looks better now 👍

Great to see that you took my suggestions, and used them to write some documentation that is better than my suggestions 👍

We are getting close to merge, just a few more suggestions.

toufik-airane and others added 2 commits June 23, 2020 12:28
…etOrPublicKeyVerification.ql

Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>
Copy link
Contributor Author

@toufik-airane toufik-airane left a comment

Choose a reason for hiding this comment

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

@erik-krogh Thanks again for your guidance. 🙌

For now, the query is in the experimental folder.
What's the requirement to move in with other Security queries?

@toufik-airane toufik-airane requested a review from erik-krogh June 23, 2020 10:33
@erik-krogh
Copy link
Contributor

@erik-krogh Thanks again for your guidance.

For now, the query is in the experimental folder.
What's the requirement to move in with other Security queries?

In the end it is the JavaScript team that decides what ends up as non-experimental JavaScript queries.

Among others it requires an evaluation of results to evaluate true and false positives.
Showing existing results in real projects can help with that (and it's even better if the existing result has a CVE assigned).

One thing I usually do is find a bunch of projects on GitHub that are flagged by the query (GitHub.com/search is sometimes helpful for that), and I also check that there isn't a whole bunch of false positives (false positives should not be an issue for this query, but otherwise I often run the query using this query console with 200+ projects).

I don't think you should worry about whether this particular query ends up in the non-experimental folder.
We will likely look at this query later and see if we can move it into non-experimental in some form.

@toufik-airane
Copy link
Contributor Author

A list of pertinent projects to run the query is exactly what I was looking for days.
I will try it and make you aware of the results. 👍

@erik-krogh
Copy link
Contributor

erik-krogh commented Jun 23, 2020

A list of pertinent projects to run the query is exactly what I was looking for days.
I will try it and make you aware of the results.

You don't need to spend too much time on it.
When I sign off on this, then we will likely run your query across 100.000+ projects to find some results.

(Right now I'm waiting for the CI to finish, then I'll hopefully take one last look at your code before I press accept).

Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

🎉

@toufik-airane
Copy link
Contributor Author

toufik-airane commented Jun 23, 2020

Could this contribution be eligible for the All For One program on HackerOne?
Thanks.

@erik-krogh @xcorail

@intrigus-lgtm
Copy link
Contributor

Could this contribution be eligible for the All For One program on HackerOne?
Thanks.

@erik-krogh @xcorail

If you find

at least one useful result on some revision of a real project

and follow the instructions here i.e. open an additional issue using the All For One template, then this contribution should be eligible :)

@toufik-airane
Copy link
Contributor Author

toufik-airane commented Jun 24, 2020

Hello, are we ready to merge? 😇

@asgerf asgerf merged commit 090a685 into github:master Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants