Rule which detects aliasing of values in RangeStmt

This commit is contained in:
Caccavale 2019-12-19 12:39:33 -05:00 committed by Cosmin Cojocar
parent 8662624e28
commit ee3146e637
5 changed files with 161 additions and 0 deletions

View file

@ -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

116
rules/implicit_aliasing.go Normal file
View file

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

View file

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

View file

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

View file

@ -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