Skip to content

Conversation

@thaJeztah
Copy link
Contributor

@thaJeztah thaJeztah commented Sep 4, 2022

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;

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;

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:

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:

    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: }

@thaJeztah
Copy link
Contributor Author

@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]>
@thaJeztah thaJeztah changed the title Fix false positives for GO404 with aliased packages Fix false positives for G404 with aliased packages Sep 4, 2022
@timonomsk
Copy link
Contributor

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 👍

Copy link
Member

@ccojocar ccojocar left a 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!

@ccojocar ccojocar merged commit dfde579 into securego:master Sep 5, 2022
@thaJeztah thaJeztah deleted the fix_import_matching branch September 5, 2022 09:03
@thaJeztah
Copy link
Contributor Author

I have a strong feeling that there is a case that can break that logic too.

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 GetImportedNames (plural) instead that returns a slice of import names (non-aliased and aliased combined). That said; having duplicate imports isn't great, and should already make other linters fail.

thanks for fixing this issue!

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 😄

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.

False positive for G404 if both math/rand and crypto/rand are imported

3 participants