From ee3146e637167cd156ccfc0310f4983f1bc76566 Mon Sep 17 00:00:00 2001 From: Caccavale Date: Thu, 19 Dec 2019 12:39:33 -0500 Subject: [PATCH] Rule which detects aliasing of values in RangeStmt --- README.md | 1 + rules/implicit_aliasing.go | 116 +++++++++++++++++++++++++++++++++++++ rules/rulelist.go | 3 + rules/rules_test.go | 5 ++ testutils/source.go | 36 ++++++++++++ 5 files changed, 161 insertions(+) create mode 100644 rules/implicit_aliasing.go diff --git a/README.md b/README.md index d6f5311..52be734 100644 --- a/README.md +++ b/README.md @@ -119,6 +119,7 @@ directory you can supply `./...` as the input argument. - G503: Import blacklist: crypto/rc4 - G504: Import blacklist: net/http/cgi - G505: Import blacklist: crypto/sha1 +- G601: Implicit memory aliasing of items from a range statement ### Retired rules diff --git a/rules/implicit_aliasing.go b/rules/implicit_aliasing.go new file mode 100644 index 0000000..65c7ae3 --- /dev/null +++ b/rules/implicit_aliasing.go @@ -0,0 +1,116 @@ +package rules + +import ( + "fmt" + "github.com/securego/gosec/v2" + "go/ast" + "go/token" +) + +type implicitAliasing struct { + gosec.MetaData + aliases map[*ast.Object]struct{} + rightBrace token.Pos + acceptableAlias []*ast.UnaryExpr +} + +func (r *implicitAliasing) ID() string { + return r.MetaData.ID +} + +func containsUnary(exprs []*ast.UnaryExpr, expr *ast.UnaryExpr) bool { + for _, e := range exprs { + if e == expr { + return true + } + } + return false +} + +func (r *implicitAliasing) Match(n ast.Node, c *gosec.Context) (*gosec.Issue, error) { + switch node := n.(type) { + case *ast.RangeStmt: + // When presented with a range statement, get the underlying Object bound to + // by assignment and add it to our set (r.aliases) of objects to check for. + if key, ok := node.Value.(*ast.Ident); ok { + if assignment, ok := key.Obj.Decl.(*ast.AssignStmt); ok { + if len(assignment.Lhs) < 2 { + return nil, nil + } + + if object, ok := assignment.Lhs[1].(*ast.Ident); ok { + r.aliases[object.Obj] = struct{}{} + + if r.rightBrace < node.Body.Rbrace { + r.rightBrace = node.Body.Rbrace + } + } + } + } + case *ast.UnaryExpr: + // If this unary expression is outside of the last range statement we were looking at + // then clear the list of objects we're concerned about because they're no longer in + // scope + if node.Pos() > r.rightBrace { + r.aliases = make(map[*ast.Object]struct{}) + r.acceptableAlias = make([]*ast.UnaryExpr, 0) + } + + // Short circuit logic to skip checking aliases if we have nothing to check against. + if len(r.aliases) == 0 { + return nil, nil + } + + // If this unary is at the top level of a return statement then it is okay-- + // see *ast.ReturnStmt comment below. + if containsUnary(r.acceptableAlias, node) { + return nil, nil + } + + // If we find a unary op of & (reference) of an object within r.aliases, complain. + if ident, ok := node.X.(*ast.Ident); ok && node.Op.String() == "&" { + if _, contains := r.aliases[ident.Obj]; contains { + return gosec.NewIssue(c, n, r.ID(), r.What, r.Severity, r.Confidence), nil + } + } + case *ast.ReturnStmt: + // Returning a rangeStmt yielded value is acceptable since only one value will be returned + for _, item := range node.Results { + if unary, ok := item.(*ast.UnaryExpr); ok && unary.Op.String() == "&" { + r.acceptableAlias = append(r.acceptableAlias, unary) + } + } + } + + return nil, nil +} + +// NewImplicitAliasing detects implicit memory aliasing of type: for blah := SomeCall() {... SomeOtherCall(&blah) ...} +func NewImplicitAliasing(id string, conf gosec.Config) (gosec.Rule, []ast.Node) { + return &implicitAliasing{ + aliases: make(map[*ast.Object]struct{}), + rightBrace: token.NoPos, + acceptableAlias: make([]*ast.UnaryExpr, 0), + MetaData: gosec.MetaData{ + ID: id, + Severity: gosec.Medium, + Confidence: gosec.Medium, + What: fmt.Sprintf("Implicit memory aliasing in for loop."), + }, + }, []ast.Node{(*ast.RangeStmt)(nil), (*ast.UnaryExpr)(nil), (*ast.ReturnStmt)(nil)} +} + +/* +This rule is prone to flag false positives. + +Within GoSec, the rule is just an AST match-- there are a handful of other +implementation strategies which might lend more nuance to the rule at the +cost of allowing false negatives. + +From a tooling side, I'd rather have this rule flag false positives than +potentially have some false negatives-- especially if the sentiment of this +rule (as I understand it, and Go) is that referencing a rangeStmt-yielded +value is kinda strange and does not have a strongly justified use case. + +Which is to say-- a false positive _should_ just be changed. +*/ diff --git a/rules/rulelist.go b/rules/rulelist.go index fd03068..06e1dfb 100644 --- a/rules/rulelist.go +++ b/rules/rulelist.go @@ -96,6 +96,9 @@ func Generate(filters ...RuleFilter) RuleList { {"G503", "Import blacklist: crypto/rc4", NewBlacklistedImportRC4}, {"G504", "Import blacklist: net/http/cgi", NewBlacklistedImportCGI}, {"G505", "Import blacklist: crypto/sha1", NewBlacklistedImportSHA1}, + + // memory safety + {"G601", "Implicit memory aliasing in RangeStmt", NewImplicitAliasing}, } ruleMap := make(map[string]RuleDefinition) diff --git a/rules/rules_test.go b/rules/rules_test.go index 7060c4b..9c18979 100644 --- a/rules/rules_test.go +++ b/rules/rules_test.go @@ -170,10 +170,15 @@ var _ = Describe("gosec rules", func() { It("should detect blacklisted imports - CGI (httpoxy)", func() { runner("G504", testutils.SampleCodeG504) }) + It("should detect blacklisted imports - SHA1", func() { runner("G505", testutils.SampleCodeG505) }) + It("should detect implicit aliasing in ForRange", func() { + runner("G601", testutils.SampleCodeG601) + }) + }) }) diff --git a/testutils/source.go b/testutils/source.go index 069771c..2f0a2bc 100644 --- a/testutils/source.go +++ b/testutils/source.go @@ -1848,6 +1848,42 @@ func main() { fmt.Printf("%x - %s\n", sha1.Sum([]byte(arg)), arg) } }`}, 1, gosec.NewConfig()}} + + // SampleCodeG601 - Implicit ForRange aliasing + SampleCodeG601 = []CodeSample{{[]string{`package main + +import "fmt" + +var vector []*string + +func appendVector(s *string) { + vector = append(vector, s) +} + +func printVector() { + for _, item := range vector { + fmt.Printf("%s", *item) + } + fmt.Println() +} + +func foo() (int, *string, string) { + for _, item := range vector { + return 0, &item, item + } +} + +func main() { + for _, item := range []string{"A", "B", "C"} { + appendVector(&item) + } + + printVector() + + zero, c_star, c := foo() + fmt.Printf("%d %v %s", zero, c_start, c) +}`}, 1, gosec.NewConfig()}} + // SampleCode601 - Go build tags SampleCode601 = []CodeSample{{[]string{` // +build tag