Skip to content
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

Refactor to support duplicate imports with different aliases #865

Merged
merged 1 commit into from
Oct 17, 2022
Merged

Refactor to support duplicate imports with different aliases #865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
Refactor to support duplicate imports with different aliases 
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>
  • Loading branch information
@thaJeztah
thaJeztah committed Oct 17, 2022
commit 473f5489f1631ef88a91acaed476c669d409550b
14 changes: 8 additions & 6 deletions analyzer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,9 +172,9 @@ func (gosec *Analyzer) Process(buildTags []string, packagePaths ...string) error
for {
select {
case s := <-j:
packages, err := gosec.load(s, config)
pkgs, err := gosec.load(s, config)
select {
case r <- result{pkgPath: s, pkgs: packages, err: err}:
case r <- result{pkgPath: s, pkgs: pkgs, err: err}:
case <-quit:
// we've been told to stop, probably an error while
// processing a previous result.
Expand Down Expand Up @@ -296,7 +296,6 @@ func (gosec *Analyzer) Check(pkg *packages.Package) {
gosec.context.Pkg = pkg.Types
gosec.context.PkgFiles = pkg.Syntax
gosec.context.Imports = NewImportTracker()
gosec.context.Imports.TrackFile(file)
gosec.context.PassedValues = make(map[string]interface{})
ast.Walk(gosec, file)
Comment on lines -299 to 300
Copy link
Contributor Author

Choose a reason for hiding this comment

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

...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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

gosec.stats.NumFiles++
Expand Down Expand Up @@ -434,6 +433,12 @@ func (gosec *Analyzer) Visit(n ast.Node) ast.Visitor {
}
return gosec
}
switch i := n.(type) {
case *ast.File:
// Using ast.File instead of ast.ImportSpec, so that we can track
// all imports at once.
gosec.context.Imports.TrackFile(i)
}

// Get any new rule exclusions.
ignoredRules := gosec.ignore(n)
Expand All @@ -453,9 +458,6 @@ func (gosec *Analyzer) Visit(n ast.Node) ast.Visitor {
// Push the new set onto the stack.
gosec.context.Ignores = append([]map[string][]SuppressionInfo{ignores}, gosec.context.Ignores...)

// Track aliased and initialization imports
gosec.context.Imports.TrackImport(n)

for _, rule := range gosec.ruleset.RegisteredFor(n) {
// Check if all rules are ignored.
generalSuppressions, generalIgnored := ignores[aliasOfAllRules]
Expand Down
59 changes: 17 additions & 42 deletions helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,20 +37,20 @@ import (
//
// node, matched := MatchCallByPackage(n, ctx, "math/rand", "Read")
func MatchCallByPackage(n ast.Node, c *Context, pkg string, names ...string) (*ast.CallExpr, bool) {
importedName, found := GetAliasedName(pkg, c)
importedNames, found := GetImportedNames(pkg, c)
if !found {
importedName, found = GetImportedName(pkg, c)
if !found {
return nil, false
}
return nil, false
}

if callExpr, ok := n.(*ast.CallExpr); ok {
packageName, callName, err := GetCallInfo(callExpr, c)
if err != nil {
return nil, false
}
if packageName == importedName {
for _, in := range importedNames {
if packageName != in {
continue
}
for _, name := range names {
if callName == name {
return callExpr, true
Expand Down Expand Up @@ -247,48 +247,23 @@ func GetBinaryExprOperands(be *ast.BinaryExpr) []ast.Node {
return result
}

// 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) {
importName, imported := ctx.Imports.Imported[path]
if !imported {
return "", false
}

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

return importName, true
}

// GetAliasedName returns the aliased name used for the package within the
// code. It will ignore initialization only imports.
func GetAliasedName(path string, ctx *Context) (string, bool) {
importName, imported := ctx.Imports.Aliased[path]
if !imported {
return "", false
}

if _, initonly := ctx.Imports.InitOnly[path]; initonly {
return "", false
}
Comment on lines -269 to -275
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


return importName, true
// GetImportedNames returns the name(s)/alias(es) used for the package within
// the code. It ignores initialization-only imports.
func GetImportedNames(path string, ctx *Context) (names []string, found bool) {
importNames, imported := ctx.Imports.Imported[path]
return importNames, imported
}

// GetImportPath resolves the full import path of an identifier based on
// the imports in the current context(including aliases).
func GetImportPath(name string, ctx *Context) (string, bool) {
for path := range ctx.Imports.Imported {
if imported, ok := GetImportedName(path, ctx); ok && imported == name {
return path, true
}
}

for path := range ctx.Imports.Aliased {
if imported, ok := GetAliasedName(path, ctx); ok && imported == name {
return path, true
if imported, ok := GetImportedNames(path, ctx); ok {
for _, n := range imported {
if n == name {
return path, true
}
}
}
}

Expand Down
53 changes: 25 additions & 28 deletions import_tracker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,54 +22,51 @@ import (
// by a source file. It is able to differentiate between plain imports, aliased
// imports and init only imports.
type ImportTracker struct {
Imported map[string]string
Aliased map[string]string
InitOnly map[string]bool
// Imported is a map of Imported with their associated names/aliases.
Imported map[string][]string
}

// NewImportTracker creates an empty Import tracker instance
func NewImportTracker() *ImportTracker {
return &ImportTracker{
make(map[string]string),
make(map[string]string),
make(map[string]bool),
Imported: make(map[string][]string),
}
}

// TrackFile track all the imports used by the supplied file
func (t *ImportTracker) TrackFile(file *ast.File) {
for _, imp := range file.Imports {
path := strings.Trim(imp.Path.Value, `"`)
parts := strings.Split(path, "/")
if len(parts) > 0 {
name := parts[len(parts)-1]
t.Imported[path] = name
}
t.TrackImport(imp)
}
}

// TrackPackages tracks all the imports used by the supplied packages
func (t *ImportTracker) TrackPackages(pkgs ...*types.Package) {
for _, pkg := range pkgs {
t.Imported[pkg.Path()] = pkg.Name()
t.Imported[pkg.Path()] = []string{pkg.Name()}
}
}

// TrackImport tracks imports and handles the 'unsafe' import
func (t *ImportTracker) TrackImport(n ast.Node) {
if imported, ok := n.(*ast.ImportSpec); ok {
path := strings.Trim(imported.Path.Value, `"`)
if imported.Name != nil {
if imported.Name.Name == "_" {
// Initialization only import
t.InitOnly[path] = true
} else {
// Aliased import
t.Aliased[path] = imported.Name.Name
}
}
if path == "unsafe" {
t.Imported[path] = path
// TrackImport tracks imports.
func (t *ImportTracker) TrackImport(imported *ast.ImportSpec) {
importPath := strings.Trim(imported.Path.Value, `"`)
if imported.Name != nil {
if imported.Name.Name == "_" {
// Initialization only import
} else {
// Aliased import
t.Imported[importPath] = append(t.Imported[importPath], imported.Name.String())
}
} else {
t.Imported[importPath] = append(t.Imported[importPath], importName(importPath))
}
}

func importName(importPath string) string {
parts := strings.Split(importPath, "/")
name := importPath
if len(parts) > 0 {
name = parts[len(parts)-1]
}
return name
}
4 changes: 2 additions & 2 deletions import_tracker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ var _ = Describe("Import Tracker", func() {
files := pkgs[0].Syntax
Expect(files).Should(HaveLen(1))
tracker.TrackFile(files[0])
Expect(tracker.Imported).Should(Equal(map[string]string{"fmt": "fmt"}))
Expect(tracker.Imported).Should(Equal(map[string][]string{"fmt": {"fmt"}}))
})
It("should parse the named imports from file", func() {
tracker := gosec.NewImportTracker()
Expand All @@ -47,7 +47,7 @@ var _ = Describe("Import Tracker", func() {
files := pkgs[0].Syntax
Expect(files).Should(HaveLen(1))
tracker.TrackFile(files[0])
Expect(tracker.Imported).Should(Equal(map[string]string{"fmt": "fmt"}))
Expect(tracker.Imported).Should(Equal(map[string][]string{"fmt": {"fm"}}))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

})
})
})
19 changes: 19 additions & 0 deletions testutils/source.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3196,6 +3196,25 @@ func main() {
println(bad)
}
`}, 1, gosec.NewConfig()},
{[]string{`
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
}
`}, 3, gosec.NewConfig()},
}

// SampleCodeG501 - Blocklisted import MD5
Expand Down