Refactor to support duplicate imports with different aliases #865

thaJeztah · 2022-09-05T11:28:28Z

The existing code assumed imports to be either imported, or imported with an
alias. Badly formatted files may have duplicate imports for a package, using
different aliases.

This patch refactors the code, and;

Introduces a new GetImportedNames function, which returns all name(s) and
aliase(s) for a package, which effectively combines GetAliasedName and
GetImportedName, but adding support for duplicate imports.

The old GetAliasedName and GetImportedName functions have been rewritten to
use the new function and marked deprecated, but could be removed if there are no
external consumers.

With this patch, the linter is able to detect issues in files such as;

package main

import (
    crand "crypto/rand"
    "math/big"
    "math/rand"
    rand2 "math/rand"
    rand3 "math/rand"
)

func main() {
    _, _ = crand.Int(crand.Reader, big.NewInt(int64(2))) // good

    _ = rand.Intn(2) // bad
    _ = rand2.Intn(2)  // bad
    _ = rand3.Intn(2)  // bad
}

Before this patch, only a single issue would be detected:

gosec --quiet .

[main.go:14] - G404 (CWE-338): Use of weak random number generator (math/rand instead of crypto/rand) (Confidence: MEDIUM, Severity: HIGH)
    13:
  > 14: 	_ = rand.Intn(2) // bad
    15: 	_ = rand2.Intn(2)  // bad

With this patch, all issues are identified:

gosec --quiet .

[main.go:16] - G404 (CWE-338): Use of weak random number generator (math/rand instead of crypto/rand) (Confidence: MEDIUM, Severity: HIGH)
    15: 	_ = rand2.Intn(2)  // bad
  > 16: 	_ = rand3.Intn(2)  // bad
    17: }

[main.go:15] - G404 (CWE-338): Use of weak random number generator (math/rand instead of crypto/rand) (Confidence: MEDIUM, Severity: HIGH)
    14: 	_ = rand.Intn(2) // bad
  > 15: 	_ = rand2.Intn(2)  // bad
    16: 	_ = rand3.Intn(2)  // bad

[main.go:14] - G404 (CWE-338): Use of weak random number generator (math/rand instead of crypto/rand) (Confidence: MEDIUM, Severity: HIGH)
    13:
  > 14: 	_ = rand.Intn(2) // bad
    15: 	_ = rand2.Intn(2)  // bad

thaJeztah · 2022-09-05T11:29:56Z

import_tracker.go

-	Imported map[string]string
-	Aliased  map[string]string
+	// Imported is a map of imports with their associated names/aliases.
+	Imported map[string][]string
 	InitOnly map[string]bool


I should mention that this change is not backward compatible if there's external users of the ImportTracker type; not sure if there's users, and if that's a concern (if it is, we can probably add back the old fields, and introduce a new field (which could be non-exported); https://grep.app/search?q=%22github.com/securego/gosec/v2

I think is fine, this is not supposed to be used outside of the project. We didn't do a good job to isolate this as internal.

thaJeztah · 2022-09-05T11:30:47Z

helpers.go

+// Deprecated: use GetImportedNames
+func GetImportedName(path string, ctx *Context) (string, bool) {


I deprecated the old ones, but not sure if there's external consumers (if we don't expect there to be, these could be removed).

I think this can be removed, it is not supposed to be used outside of the project.

thaJeztah · 2022-09-05T11:43:19Z

Alright, this needs some work; have a false positive on

package main

import (
	"crypto/rand"
	mrand "math/rand"
)

func main() {
	good, _ := rand.Read(nil)
	println(good)
	bad := mrand.Int31()
	println(bad)
}

ccojocar

Thanks for working on this. It looks good so far.

ccojocar · 2022-09-12T07:37:59Z

helpers.go

+// Deprecated: use GetImportedNames
+func GetImportedName(path string, ctx *Context) (string, bool) {


I think this can be removed, it is not supposed to be used outside of the project.

ccojocar · 2022-09-12T07:39:12Z

import_tracker.go

-	Imported map[string]string
-	Aliased  map[string]string
+	// Imported is a map of imports with their associated names/aliases.
+	Imported map[string][]string
 	InitOnly map[string]bool


I think is fine, this is not supposed to be used outside of the project. We didn't do a good job to isolate this as internal.

mauriciocm9 · 2022-10-05T00:54:12Z

any update?

ccojocar · 2022-10-16T19:46:55Z

Hi @thaJeztah, Do you still plan to finish this pull request? Thanks

thaJeztah · 2022-10-16T23:16:52Z

@ccojocar I think I fixed it, but looks like CI is failing due to older Go versions being installed, and an outdated module; I opened #880 to fix / work around that, and temporarily rebased this PR on top of that (will rebase once the other PR is merged)

codecov-commenter · 2022-10-16T23:19:22Z

Codecov Report

Base: 73.89% // Head: 74.19% // Increases project coverage by +0.30% 🎉

Coverage data is based on head (98158f4) compared to base (6cd9e62).
Patch coverage: 88.88% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #865      +/-   ##
==========================================
+ Coverage   73.89%   74.19%   +0.30%     
==========================================
  Files          51       51              
  Lines        3195     3186       -9     
==========================================
+ Hits         2361     2364       +3     
+ Misses        763      752      -11     
+ Partials       71       70       -1

Impacted Files	Coverage Δ
analyzer.go	`89.40% <ø> (-0.04%)`	⬇️
import_tracker.go	`75.60% <82.60%> (+9.98%)`	⬆️
helpers.go	`43.18% <100.00%> (+0.42%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

thaJeztah · 2022-10-16T23:18:11Z

import_tracker.go

-		path := strings.Trim(imp.Path.Value, `"`)
-		parts := strings.Split(path, "/")
-		if len(parts) > 0 {
-			name := parts[len(parts)-1]
-			t.Imported[path] = name
+		importPath := strings.Trim(imp.Path.Value, `"`)


This was part of the problem; the old code needed to manually track the file, to get the "un-aliased" import, and then used the ImportTracker.TrackImport to get the aliased versions. But the TrackFile would ignore aliases.

thaJeztah · 2022-10-16T23:21:52Z

import_tracker_test.go

-			Expect(tracker.Imported).Should(Equal(map[string]string{"fmt": "fmt"}))
+			Expect(tracker.Imported).Should(Equal(map[string][]string{"fmt": {"fm"}}))


Which can be seen here (the file tested has fm as alias for fmt, but TrackFile() doesn't detect that.

thaJeztah · 2022-10-16T23:24:06Z

analyzer.go

-		gosec.context.Imports.TrackFile(file)
 		gosec.context.PassedValues = make(map[string]interface{})
 		ast.Walk(gosec, file)


...and because Analyzer.Check() called both TrackFile() and ast.Walk(), the incorrect import name would be added (so a non-aliased import would be added, even though the package used an alias), which broke detection.

With the new code, CheckImports() finds both, so calling TrackFile is no longer needed.

I wasn't sure if that function could be removed altogether (it's slightly easier to use than starting an Analyzer, so perhaps it's useful as a utility still).

I wasn't sure if that function could be removed altogether (it's slightly easier to use than starting an Analyzer, so perhaps it's useful as a utility still).

Actually, perhaps it should be the reverse; use TrackFile once, instead of during Visit() (we only need it once per file). Could perhaps be as part as Visit() though.

thaJeztah · 2022-10-16T23:27:06Z

helpers.go

-	if !imported {
-		return "", false
-	}
-
-	if _, initonly := ctx.Imports.InitOnly[path]; initonly {
-		return "", false
-	}


The initOnly seemed redundant; all tests seem to pass without it, so I removed it, but maybe there was a reason it was there 😅

ccojocar

lgtm, please could you please rebase it now? Is this still a draft?

thaJeztah · 2022-10-17T08:36:20Z

Working on some last changes; perhaps it's better after all to handle imports at the File level, otherwise we're calling it for every Node; let me push those changes 👍

The existing code assumed imports to be either imported, or imported with an alias. Badly formatted files may have duplicate imports for a package, using different aliases. This patch refactors the code, and; Introduces a new `GetImportedNames` function, which returns all name(s) and aliase(s) for a package, which effectively combines `GetAliasedName` and `GetImportedName`, but adding support for duplicate imports. The old `GetAliasedName` and `GetImportedName` functions have been rewritten to use the new function and marked deprecated, but could be removed if there are no external consumers. With this patch, the linter is able to detect issues in files such as; package main import ( crand "crypto/rand" "math/big" "math/rand" rand2 "math/rand" rand3 "math/rand" ) func main() { _, _ = crand.Int(crand.Reader, big.NewInt(int64(2))) // good _ = rand.Intn(2) // bad _ = rand2.Intn(2) // bad _ = rand3.Intn(2) // bad } Before this patch, only a single issue would be detected: gosec --quiet . [main.go:14] - G404 (CWE-338): Use of weak random number generator (math/rand instead of crypto/rand) (Confidence: MEDIUM, Severity: HIGH) 13: > 14: _ = rand.Intn(2) // bad 15: _ = rand2.Intn(2) // bad With this patch, all issues are identified: gosec --quiet . [main.go:16] - G404 (CWE-338): Use of weak random number generator (math/rand instead of crypto/rand) (Confidence: MEDIUM, Severity: HIGH) 15: _ = rand2.Intn(2) // bad > 16: _ = rand3.Intn(2) // bad 17: } [main.go:15] - G404 (CWE-338): Use of weak random number generator (math/rand instead of crypto/rand) (Confidence: MEDIUM, Severity: HIGH) 14: _ = rand.Intn(2) // bad > 15: _ = rand2.Intn(2) // bad 16: _ = rand3.Intn(2) // bad [main.go:14] - G404 (CWE-338): Use of weak random number generator (math/rand instead of crypto/rand) (Confidence: MEDIUM, Severity: HIGH) 13: > 14: _ = rand.Intn(2) // bad 15: _ = rand2.Intn(2) // bad While working on this change, I noticed that ImportTracker.TrackFile() was not able to find import aliases; Analyser.Check() called both ImportTracker.TrackFile() and ast.Walk(), which (with the updated ImportTracker) resulted in importes to be in- correctly included multiple times (once with the correct alias, once with the default). I updated ImportTracker.TrackFile() to fix this, but with the updated ImportTracker, Analyser.Check() no longer has to call ImportTracker.TrackFile() separately, as ast.Walk() already handles the file, and will find all imports. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2022-10-17T08:58:25Z

@ccojocar should be good now; PTAL 🤗

ccojocar

Perfect! Thanks!

thaJeztah · 2022-10-17T09:00:18Z

YW! Was fun digging a bit into how the ast Parsing works 😄

thaJeztah commented Sep 5, 2022

View reviewed changes

ccojocar reviewed Sep 12, 2022

View reviewed changes

thaJeztah force-pushed the improve_import_matching branch 3 times, most recently from d52d68c to 98158f4 Compare October 16, 2022 23:15

thaJeztah commented Oct 16, 2022

View reviewed changes

ccojocar approved these changes Oct 17, 2022

View reviewed changes

thaJeztah force-pushed the improve_import_matching branch from 98158f4 to 61bbb83 Compare October 17, 2022 08:46

thaJeztah force-pushed the improve_import_matching branch from 61bbb83 to 473f548 Compare October 17, 2022 08:52

thaJeztah marked this pull request as ready for review October 17, 2022 08:58

ccojocar approved these changes Oct 17, 2022

View reviewed changes

ccojocar merged commit 0ae0174 into securego:master Oct 17, 2022

thaJeztah deleted the improve_import_matching branch October 17, 2022 08:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor to support duplicate imports with different aliases #865

Refactor to support duplicate imports with different aliases #865

thaJeztah commented Sep 5, 2022

thaJeztah Sep 5, 2022

ccojocar Sep 12, 2022

thaJeztah Sep 5, 2022

ccojocar Sep 12, 2022

thaJeztah commented Sep 5, 2022

ccojocar left a comment

ccojocar Sep 12, 2022

ccojocar Sep 12, 2022

mauriciocm9 commented Oct 5, 2022

ccojocar commented Oct 16, 2022

thaJeztah commented Oct 16, 2022

codecov-commenter commented Oct 16, 2022

thaJeztah Oct 16, 2022

thaJeztah Oct 16, 2022

thaJeztah Oct 16, 2022

thaJeztah Oct 17, 2022

thaJeztah Oct 16, 2022

ccojocar left a comment

thaJeztah commented Oct 17, 2022

thaJeztah commented Oct 17, 2022

ccojocar left a comment

thaJeztah commented Oct 17, 2022

		// Deprecated: use GetImportedNames
		func GetImportedName(path string, ctx *Context) (string, bool) {

		Expect(tracker.Imported).Should(Equal(map[string]string{"fmt": "fmt"}))
		Expect(tracker.Imported).Should(Equal(map[string][]string{"fmt": {"fm"}}))

Refactor to support duplicate imports with different aliases #865

Refactor to support duplicate imports with different aliases #865

Conversation

thaJeztah commented Sep 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Sep 5, 2022

ccojocar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciocm9 commented Oct 5, 2022

ccojocar commented Oct 16, 2022

thaJeztah commented Oct 16, 2022

codecov-commenter commented Oct 16, 2022

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccojocar left a comment

Choose a reason for hiding this comment

thaJeztah commented Oct 17, 2022

thaJeztah commented Oct 17, 2022

ccojocar left a comment

Choose a reason for hiding this comment

thaJeztah commented Oct 17, 2022