Golang : Add query to detect JWT signing vulnerabilities #705
Conversation
ghost
self-requested a review
as a 
ghost
mentioned this pull request
|
Thanks for this contribution. I haven't looked at it closely yet, but my first comment is that it would benefit from some tests and a qhelp file. |
|
@owen-mc added QHelp along with an example. PTAL |
| exists(ConversionExpr ce, StringLit str | ce.getOperand() = str and this.asExpr() = ce | | ||
| ce.getTypeExpr().getType() instanceof ByteSliceType | ||
| ) |
There was a problem hiding this comment.
Seems like this clause is generating some FPs such as:
- https://lgtm.com/projects/g/heroiclabs/nakama/snapshot/200456c75c5831cd65d6871a22af131c0363755d/files/server/runtime_lua_nakama.go?sort=name&dir=ASC&mode=heatmap#L988
- https://lgtm.com/projects/g/kiali/kiali/snapshot/b0e3ea7038053b44bc2c6576332c17f560714416/files/config/token.go?sort=name&dir=ASC&mode=heatmap#L29
It doesnt seem like there is a StringLiteral involved in these (any many other) cases
There was a problem hiding this comment.
@pwntester I thought about handling the FP in the first case. Having a empty string "" as a secret is a valid detection as it would the JWT is not signed at all. Hence, excluding all empty string from taint flow would lead to a lot of false negatives. However, I think considering any kind of comparison operation as a sanitizer would work here. So I have added a new sanitizer to detect any kind of binary op such as == or !=. I don't go about reasoning the correctness of the operation here.
In the second case, the project has a default insecure hard-coded key which it uses in the absence of a user supplied key. The alert is for the default key flow. Since, the default configuration is insecure, shouldn't we be flagging it? Ideally, the project should generate and use a random key if a user does not supply a custom key. Hence, I have kept this as is.
Can you point me to the empty key for the first case or the default key on the second case? Im not sure CodeQL is picking those
|
@pwntester I thought about handling the FP in the first case. Having a empty string In the second case, the project has a default insecure hard-coded key which it uses in the absence of a user supplied key. The alert is for the default key flow. Since, the default configuration is insecure, shouldn't we be flagging it? Ideally, the project should generate and use a random key if a user does not supply a custom key. Hence, I have kept this as is. As for the clause you mentioned earlier, I think it a QL bug. See my other code comment for details. |
|
@pwntester RE : #705 (comment) In the firstcase, I was talking about the general case where there is no key used to sign a JWT. Since there is no secret, there is no encryption here. As for the second case, can't you find |
|
Hey, Please don't run this on LGTM until this issue is resolved. |
|
Test failure:
which means that it thinks there's something wrong with the go code in |
Using a hardcoded secret for signing JWT tokens in an open source project can cause security vulnerabilities. There are multiple detections with this query. One of the publicly disclosed vulnerabilities includes [CVE-2022-0664](https://www.huntr.dev/bounties/29898a42-fd4f-4b5b-a8e3-ab573cb87eac/) found in [gravitl/netmaker](https://github.com/gravitl/netmaker). A run of this query against a vulnerable version of the project [gravitl/netmaker](https://github.com/gravitl/netmaker) can be found [here](https://lgtm.com/query/8221520154596097031/)
|
@owen-mc I don't understand why the tests are failing. The build successfully completes on my PC. |
|
I think I was missing a main function. The tests are now cleared. @owen-mc I think this is now ready for a review. PTAL. |
|
@owen-mc Any updates here? |
|
hi @porcupineyhairs We are waiting for the CodeQL fixes to be pushed to LGTM to run the query again. In the meantime, I run the query on a smaller set of projects and found a FP worth addressing |
|
@owen-mc A quick update: I have seen your comments and plan on applying them. |
|
@smowton will take over reviewing this PR. |
ghost
mentioned this pull request
|
I am closing this PR and opening a new one github/codeql#9378 |
ghost
closed this
3 participants
Using a hard-coded secret for signing JWT tokens in an open source project can cause security vulnerabilities.
This PR includes a query to find instances where a hard-coded string is used to sign a token.