-
-
Notifications
You must be signed in to change notification settings - Fork 675
Fix false positives for G404 with aliased packages #863
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.
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
Conversation
|
@timonomsk @ccojocar ptal |
It appears that `GetImportedName` returns _both_ aliased and non-aliased imports.
As a result, a file having both crypto/rand and math/rand (but aliased) would
trigger false positives on G404. Given the following file;
```go
package main
import (
"crypto/rand"
"math/big"
rnd "math/rand"
)
func main() {
_, _ = rand.Int(rand.Reader, big.NewInt(int64(2)))
_ = rnd.Intn(2)
}
```
And patching for debugging;
```patch
diff --git a/helpers.go b/helpers.go
index 437d032..80f4233 100644
--- a/helpers.go
+++ b/helpers.go
@@ -250,6 +250,8 @@ func GetBinaryExprOperands(be *ast.BinaryExpr) []ast.Node {
// GetImportedName returns the name used for the package within the
// code. It will ignore initialization only imports.
func GetImportedName(path string, ctx *Context) (string, bool) {
+ fmt.Printf("%+v", ctx.Imports.Imported)
+ os.Exit(1)
importName, imported := ctx.Imports.Imported[path]
if !imported {
return "", false
```
Would show that `math/rand` was included in the list, using it's non-aliased
name (`:rand`).
gosec -quiet .
map[crypto/rand:rand math/big:big math/rand:rand]
This patch works around this problem by reversing the order in which imports
are resolved in `MatchCallByPackage()`. Aliased packages are tried first, after
which non-aliased imports are tried.
Given the example application mentioned above:
Before this patch:
```bash
gosec -quiet .
Results:
[/Users/sebastiaan/Projects/test/gosec-issue/main.go:10] - G404 (CWE-338): Use of weak random number generator (math/rand instead of crypto/rand) (Confidence: MEDIUM, Severity: HIGH)
9: func main() {
> 10: _, _ = rand.Int(rand.Reader, big.NewInt(int64(2)))
11: _ = rnd.Intn(2)
```
With this patch applied:
```bash
gosec --quiet .
Results:
[/Users/sebastiaan/Projects/test/gosec-issue/main.go:11] - G404 (CWE-338): Use of weak random number generator (math/rand instead of crypto/rand) (Confidence: MEDIUM, Severity: HIGH)
10: _, _ = rand.Int(rand.Reader, big.NewInt(int64(2)))
> 11: _ = rnd.Intn(2)
12: }
```
Signed-off-by: Sebastiaan van Stijn <[email protected]>
6a715d2 to
fa4eeea
Compare
|
I have a strong feeling that there is a case that can break that logic too. But I've failed to find it :) So, this PR looks good for me 👍 |
ccojocar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgmt, thanks for fixing this issue!
Yes, I'll be the first to admit that it felt like a "workaround", but also tried to come up with issues in this approach, and couldn't think of any, so kept it simple. I think there will be issues if a file would have duplicate imports, such as; import(
"math/rand"
foo "math/rand"
bar "math/rand"
baz "math/rand"
)Perhaps we need a
Thanks for reviewing (both!). I hadn't coded on linters before, and expected it to be complicated, but after looking a bit at the code involved in this part, it turned out to be not too difficult, so thought I might as well open a PR and fix it 😄 |
It appears that
GetImportedNamereturns both aliased and non-aliased imports.As a result, a file having both crypto/rand and math/rand (but aliased) would
trigger false positives on G404. Given the following file;
And patching for debugging;
Would show that
math/randwas included in the list, using it's non-aliasedname (
:rand).This patch works around this problem by reversing the order in which imports
are resolved in
MatchCallByPackage(). Aliased packages are tried first, afterwhich non-aliased imports are tried.
Given the example application mentioned above:
Before this patch:
With this patch applied: