Rule which detects aliasing of values in RangeStmt

Author: caccavale

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
 

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.
+*/

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)

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)
+		})
+
 	})
 
 })

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