Skip to content

Golang : Add query to detect JWT signing vulnerabilities #9378

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.

Sign up for GitHub

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

Merged
smowton merged 11 commits into from Jun 2, 2022
Merged

Golang : Add query to detect JWT signing vulnerabilities #9378

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
bd1ddc1
Golang : Add query to detect JWT signing vulnerabilities
May 30, 2022
e0f74a5
Include suggested changes from review.
May 31, 2022
ae2bc1b
Include suggested changes from review.
May 31, 2022
1ef42a1
Include suggested changes from review.
Jun 2, 2022
361b703
Include suggested changes from review.
Jun 2, 2022
bfbc1d4
Simplify redundant sanitizer
smowton Jun 2, 2022
3155771
Rename empty-string sanitizer to reflect what it actually does.
smowton Jun 2, 2022
b48a07e
Tighten up CompareExprSanitizer
smowton Jun 2, 2022
602495d
Replace cases accidentally handled by CompareExprSanitizer with Retur…
smowton Jun 2, 2022
e54b29a
Autoformat
smowton Jun 2, 2022
d5ac719
Remove duplicate function
smowton Jun 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Rename empty-string sanitizer to reflect what it actually does.
  • Loading branch information
@smowton
smowton committed Jun 2, 2022
commit 3155771abe44cafaac642cb3321c9fb780f8536c
12 changes: 9 additions & 3 deletions go/ql/src/experimental/CWE-321/HardcodedKeysLib.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,9 +165,15 @@ module HardcodedKeys {
}
}
Comment on lines +160 to +171
Copy link
Copy Markdown
Contributor

@smowton smowton Jun 2, 2022

Choose a reason for hiding this comment

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

I tried removing this sanitizer, and it doesn't seem to be useful:

  • It handles a few cases where an empty string is returned alongside an error value, but this would be better handled by looking for the pattern of an empty string "" guarded by a test if x != nil, where x is of error type.
  • The other two cases were truly unreachable code: x := "hello"; if x != "hello" { ... }, which doesn't seem like a useful test (but perhaps you could change the test to better represent a real-world situation?)

Copy link
Copy Markdown
Author

@ghost ghost Jun 2, 2022

Choose a reason for hiding this comment

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

@smowton

It handles a few cases where an empty string is returned alongside an error value, but this would be better handled by looking for the pattern of an empty string "" guarded by a test if x != nil, where x is of error type.

This may not be ideal. There could be multiple instances where a error code may be returned without a test to nil, say for ex:

func t()(string, error){
    if config.hasKey(){
        key:= config.getKey()
        return key,nil
    } else {
    return getDefaultKey(), errors.New("no key")
}

To avoid this, I just check for cases where there the taint is compared with anything and mark that as sanitized. This handles the if x!=nil and if y !="" cases very well.

The other two cases were truly unreachable code: x := "hello"; if x != "hello" { ... }, which doesn't seem like a useful test (but perhaps you could change the test to better represent a real-world situation?)

I have added a better test case now. The test case for this sanitizer was earlier in main.go. I have moved that to sanitizer.go. Also, I have removed the test-case you mention.


/** Mark an empty string returned with an error as a sanitizer */
private class EmptyErrorSanitizer extends Sanitizer {
EmptyErrorSanitizer() {
/**
* Marks anything returned with an error as a sanitized.
*
* Typically this means contexts like `return "", errors.New("Oh no")`,
* where we can be reasonably confident downstream users won't mistake
* that empty string for a usable key.
*/
private class ReturnedAlongsideErrorSanitizer extends Sanitizer {
ReturnedAlongsideErrorSanitizer() {
exists(ReturnStmt r, DataFlow::CallNode c |
c.getTarget().hasQualifiedName("errors", "New") and
r.getNumChild() > 1 and
Expand Down