From a018cf0fbb0f7746f26cb3411d1bf79024191daf Mon Sep 17 00:00:00 2001 From: Morgen Malinoski <51513125+morgenm@users.noreply.github.com> Date: Wed, 21 Jun 2023 02:56:36 -0500 Subject: [PATCH] Feature: G602 Slice Bound Checking (#973) * Added slice bounds testing for slice expressions. * Added checking slice index. * Added test for reassigning slice. * Store capacities on reslicing. * Scope change clears map. Func name used to track slices. * Map CallExpr to check bounds when passing to functions. * Fixed linter errors. * Updated rulelist with CWE mapping. * Added comment for NewSliceBoundCheck. * Addressed nil cap runtime error. * Replaced usage of nil in call arg map with dummy callexprs. * Updated comments, wrapped error return, addressed other review concerns. --- issue/issue.go | 1 + rules/rulelist.go | 1 + rules/rules_test.go | 4 + rules/slice_bounds.go | 405 ++++++++++++++++++++++++++++++++++++++++++ testutils/source.go | 178 +++++++++++++++++++ 5 files changed, 589 insertions(+) create mode 100644 rules/slice_bounds.go diff --git a/issue/issue.go b/issue/issue.go index 5bf00de..db4d630 100644 --- a/issue/issue.go +++ b/issue/issue.go @@ -87,6 +87,7 @@ var ruleToCWE = map[string]string{ "G504": "327", "G505": "327", "G601": "118", + "G602": "118", } // Issue is returned by a gosec rule if it discovers an issue with the scanned code. diff --git a/rules/rulelist.go b/rules/rulelist.go index d856ecc..316691f 100644 --- a/rules/rulelist.go +++ b/rules/rulelist.go @@ -107,6 +107,7 @@ func Generate(trackSuppressions bool, filters ...RuleFilter) RuleList { // memory safety {"G601", "Implicit memory aliasing in RangeStmt", NewImplicitAliasing}, + {"G602", "Slice access out of bounds", NewSliceBoundCheck}, } ruleMap := make(map[string]RuleDefinition) diff --git a/rules/rules_test.go b/rules/rules_test.go index 7a214fc..9aa3971 100644 --- a/rules/rules_test.go +++ b/rules/rules_test.go @@ -194,5 +194,9 @@ var _ = Describe("gosec rules", func() { It("should detect implicit aliasing in ForRange", func() { runner("G601", testutils.SampleCodeG601) }) + + It("should detect out of bounds slice access", func() { + runner("G602", testutils.SampleCodeG602) + }) }) }) diff --git a/rules/slice_bounds.go b/rules/slice_bounds.go new file mode 100644 index 0000000..04811bb --- /dev/null +++ b/rules/slice_bounds.go @@ -0,0 +1,405 @@ +package rules + +import ( + "fmt" + "go/ast" + "go/types" + + "github.com/securego/gosec/v2" + "github.com/securego/gosec/v2/issue" +) + +// sliceOutOfBounds is a rule which checks for slices which are accessed outside their capacity, +// either through indexing it out of bounds or through slice expressions whose low or high index +// are out of bounds. +type sliceOutOfBounds struct { + sliceCaps map[*ast.CallExpr]map[string]*int64 // Capacities of slices. Maps function call -> var name -> value. + currentScope *types.Scope // Current scope. Map is cleared when scope changes. + currentFuncName string // Current function. + funcCallArgs map[string][]*int64 // Caps to load once a func declaration is scanned. + issue.MetaData // Metadata for this rule. +} + +// ID returns the rule ID for sliceOutOfBounds: G602. +func (s *sliceOutOfBounds) ID() string { + return s.MetaData.ID +} + +func (s *sliceOutOfBounds) Match(node ast.Node, ctx *gosec.Context) (*issue.Issue, error) { + if s.currentScope == nil { + s.currentScope = ctx.Pkg.Scope() + } else if s.currentScope != ctx.Pkg.Scope() { + s.currentScope = ctx.Pkg.Scope() + + // Clear slice map, since we are in a new scope + sliceMapNil := make(map[string]*int64) + sliceCaps := make(map[*ast.CallExpr]map[string]*int64) + sliceCaps[nil] = sliceMapNil + s.sliceCaps = sliceCaps + } + + switch node := node.(type) { + case *ast.AssignStmt: + return s.matchAssign(node, ctx) + case *ast.SliceExpr: + return s.matchSliceExpr(node, ctx) + case *ast.IndexExpr: + return s.matchIndexExpr(node, ctx) + case *ast.FuncDecl: + s.currentFuncName = node.Name.Name + s.loadArgCaps(node) + case *ast.CallExpr: + if _, ok := node.Fun.(*ast.FuncLit); ok { + // Do nothing with func literals for now. + break + } + + sliceMap := make(map[string]*int64) + s.sliceCaps[node] = sliceMap + s.setupCallArgCaps(node, ctx) + } + return nil, nil +} + +// updateSliceCaps takes in a variable name and a map of calls we are updating the variables for to the updated values +// and will add it to the sliceCaps map. +func (s *sliceOutOfBounds) updateSliceCaps(varName string, caps map[*ast.CallExpr]*int64) { + for callExpr, cap := range caps { + s.sliceCaps[callExpr][varName] = cap + } +} + +// getAllCalls returns all CallExprs that are calls to the given function. +func (s *sliceOutOfBounds) getAllCalls(funcName string, ctx *gosec.Context) []*ast.CallExpr { + calls := []*ast.CallExpr{} + + for callExpr := range s.sliceCaps { + if callExpr != nil { + // Compare the names of the function the code is scanning with the current call we are iterating over + _, callFuncName, err := gosec.GetCallInfo(callExpr, ctx) + if err != nil { + continue + } + + if callFuncName == funcName { + calls = append(calls, callExpr) + } + } + } + return calls +} + +// getSliceCapsForFunc gets all the capacities for slice with given name that are stored for each call to the passed function. +func (s *sliceOutOfBounds) getSliceCapsForFunc(funcName string, varName string, ctx *gosec.Context) map[*ast.CallExpr]*int64 { + caps := make(map[*ast.CallExpr]*int64) + + calls := s.getAllCalls(funcName, ctx) + for _, call := range calls { + if callCaps, ok := s.sliceCaps[call]; ok { + caps[call] = callCaps[varName] + } + } + + return caps +} + +// setupCallArgCaps evaluates and saves the caps for any slices in the args so they can be validated when the function is scanned. +func (s *sliceOutOfBounds) setupCallArgCaps(callExpr *ast.CallExpr, ctx *gosec.Context) { + // Array of caps to be loaded once the function declaration is scanned + funcCallArgs := []*int64{} + + // Get function name + _, funcName, err := gosec.GetCallInfo(callExpr, ctx) + if err != nil { + return + } + + for _, arg := range callExpr.Args { + switch node := arg.(type) { + case *ast.SliceExpr: + caps := s.evaluateSliceExpr(node, ctx) + + // Simplifying assumption: use the lowest capacity. Storing all possible capacities for slices passed + // to a function call would catch the most issues, but would require a data structure like a stack and a + // reworking of the code for scanning itself. Use the lowest capacity, as this would be more likely to + // raise an issue for being out of bounds. + var lowestCap *int64 + for _, cap := range caps { + if cap == nil { + continue + } + + if lowestCap == nil { + lowestCap = cap + } else if *lowestCap > *cap { + lowestCap = cap + } + } + + if lowestCap == nil { + funcCallArgs = append(funcCallArgs, nil) + continue + } + + // Now create a map of just this value to add it to the sliceCaps + funcCallArgs = append(funcCallArgs, lowestCap) + case *ast.Ident: + ident := arg.(*ast.Ident) + caps := s.getSliceCapsForFunc(s.currentFuncName, ident.Name, ctx) + + var lowestCap *int64 + for _, cap := range caps { + if cap == nil { + continue + } + + if lowestCap == nil { + lowestCap = cap + } else if *lowestCap > *cap { + lowestCap = cap + } + } + + if lowestCap == nil { + funcCallArgs = append(funcCallArgs, nil) + continue + } + + // Now create a map of just this value to add it to the sliceCaps + funcCallArgs = append(funcCallArgs, lowestCap) + default: + funcCallArgs = append(funcCallArgs, nil) + } + } + s.funcCallArgs[funcName] = funcCallArgs +} + +// loadArgCaps loads caps that were saved for a call to this function. +func (s *sliceOutOfBounds) loadArgCaps(funcDecl *ast.FuncDecl) { + sliceMap := make(map[string]*int64) + funcName := funcDecl.Name.Name + + // Create a dummmy call expr for the new function. This is so we can still store args for + // functions which are not explicitly called in the code by other functions (specifically, main). + ident := ast.NewIdent(funcName) + dummyCallExpr := ast.CallExpr{ + Fun: ident, + } + + argCaps, ok := s.funcCallArgs[funcName] + if !ok || len(argCaps) == 0 { + s.sliceCaps[&dummyCallExpr] = sliceMap + return + } + + params := funcDecl.Type.Params.List + if len(params) > len(argCaps) { + return // Length of params and args doesn't match, so don't do anything with this. + } + + for it := range params { + capacity := argCaps[it] + if capacity == nil { + continue + } + + if len(params[it].Names) == 0 { + continue + } + + if paramName := params[it].Names[0]; paramName != nil { + sliceMap[paramName.Name] = capacity + } + } + + s.sliceCaps[&dummyCallExpr] = sliceMap +} + +// matchSliceMake matches calls to make() and stores the capacity of the new slice in the map to compare against future slice usage. +func (s *sliceOutOfBounds) matchSliceMake(funcCall *ast.CallExpr, sliceName string, ctx *gosec.Context) (*issue.Issue, error) { + _, funcName, err := gosec.GetCallInfo(funcCall, ctx) + if err != nil || funcName != "make" { + return nil, nil + } + + var capacityArg int + if len(funcCall.Args) < 2 { + return nil, nil // No size passed + } else if len(funcCall.Args) == 2 { + capacityArg = 1 + } else if len(funcCall.Args) == 3 { + capacityArg = 2 + } else { + return nil, nil // Unexpected, args should always be 2 or 3 + } + + // Check and get the capacity of the slice passed to make. It must be a literal value, since we aren't evaluating the expression. + sliceCapLit, ok := funcCall.Args[capacityArg].(*ast.BasicLit) + if !ok { + return nil, nil + } + + capacity, err := gosec.GetInt(sliceCapLit) + if err != nil { + return nil, nil + } + + caps := s.getSliceCapsForFunc(s.currentFuncName, sliceName, ctx) + for callExpr := range caps { + caps[callExpr] = &capacity + } + + s.updateSliceCaps(sliceName, caps) + return nil, nil +} + +// evaluateSliceExpr takes a slice expression and evaluates what the capacity of said slice is for each of the +// calls to the current function. Returns map of the call expressions of each call to the current function to +// the evaluated capacities. +func (s *sliceOutOfBounds) evaluateSliceExpr(node *ast.SliceExpr, ctx *gosec.Context) map[*ast.CallExpr]*int64 { + // Get ident to get name + ident, ok := node.X.(*ast.Ident) + if !ok { + return nil + } + + // Get cap of old slice to calculate this new slice's cap + caps := s.getSliceCapsForFunc(s.currentFuncName, ident.Name, ctx) + for callExpr, oldCap := range caps { + if oldCap == nil { + continue + } + + // Get and check low value + lowIdent, ok := node.Low.(*ast.BasicLit) + if ok && lowIdent != nil { + low, _ := gosec.GetInt(lowIdent) + + newCap := *oldCap - low + caps[callExpr] = &newCap + } else if lowIdent == nil { // If no lower bound, capacity will be same + continue + } + } + + return caps +} + +// matchSliceAssignment matches slice assignments, calculates capacity of slice if possible to store it in map. +func (s *sliceOutOfBounds) matchSliceAssignment(node *ast.SliceExpr, sliceName string, ctx *gosec.Context) (*issue.Issue, error) { + // First do the normal match that verifies the slice expr is not out of bounds + if i, err := s.matchSliceExpr(node, ctx); err != nil { + return i, fmt.Errorf("There was an error while matching a slice expression to check slice bounds for %s: %w", sliceName, err) + } + + // Now that the assignment is (presumably) successfully, we can calculate the capacity and add this new slice to the map + caps := s.evaluateSliceExpr(node, ctx) + s.updateSliceCaps(sliceName, caps) + + return nil, nil +} + +// matchAssign matches checks if an assignment statement is making a slice, or if it is assigning a slice. +func (s *sliceOutOfBounds) matchAssign(node *ast.AssignStmt, ctx *gosec.Context) (*issue.Issue, error) { + // Check RHS for calls to make() so we can get the actual size of the slice + for it, i := range node.Rhs { + // Get the slice name so we can associate the cap with the slice in the map + sliceIdent, ok := node.Lhs[it].(*ast.Ident) + if !ok { + return nil, nil + } + sliceName := sliceIdent.Name + + switch expr := i.(type) { + case *ast.CallExpr: // Check for and handle call to make() + return s.matchSliceMake(expr, sliceName, ctx) + case *ast.SliceExpr: // Handle assignments to a slice + return s.matchSliceAssignment(expr, sliceName, ctx) + } + } + return nil, nil +} + +// matchSliceExpr validates that a given slice expression (eg, slice[10:30]) is not out of bounds. +func (s *sliceOutOfBounds) matchSliceExpr(node *ast.SliceExpr, ctx *gosec.Context) (*issue.Issue, error) { + // First get the slice name so we can check the size in our map + ident, ok := node.X.(*ast.Ident) + if !ok { + return nil, nil + } + + // Get slice cap from the map to compare it against high and low + caps := s.getSliceCapsForFunc(s.currentFuncName, ident.Name, ctx) + + for _, cap := range caps { + if cap == nil { + continue + } + + // Get and check high value + highIdent, ok := node.High.(*ast.BasicLit) + if ok && highIdent != nil { + high, _ := gosec.GetInt(highIdent) + if high > *cap { + return ctx.NewIssue(node, s.ID(), s.What, s.Severity, s.Confidence), nil + } + } + + // Get and check low value + lowIdent, ok := node.Low.(*ast.BasicLit) + if ok && lowIdent != nil { + low, _ := gosec.GetInt(lowIdent) + if low > *cap { + return ctx.NewIssue(node, s.ID(), s.What, s.Severity, s.Confidence), nil + } + } + } + + return nil, nil +} + +// matchIndexExpr validates that an index into a slice is not out of bounds. +func (s *sliceOutOfBounds) matchIndexExpr(node *ast.IndexExpr, ctx *gosec.Context) (*issue.Issue, error) { + // First get the slice name so we can check the size in our map + ident, ok := node.X.(*ast.Ident) + if !ok { + return nil, nil + } + + // Get slice cap from the map to compare it against high and low + caps := s.getSliceCapsForFunc(s.currentFuncName, ident.Name, ctx) + + for _, cap := range caps { + if cap == nil { + continue + } + // Get the index literal + indexIdent, ok := node.Index.(*ast.BasicLit) + if ok && indexIdent != nil { + index, _ := gosec.GetInt(indexIdent) + if index >= *cap { + return ctx.NewIssue(node, s.ID(), s.What, s.Severity, s.Confidence), nil + } + } + } + + return nil, nil +} + +// NewSliceBoundCheck attempts to find any slices being accessed out of bounds +// by reslicing or by being indexed. +func NewSliceBoundCheck(id string, _ gosec.Config) (gosec.Rule, []ast.Node) { + sliceMap := make(map[*ast.CallExpr]map[string]*int64) + + return &sliceOutOfBounds{ + sliceCaps: sliceMap, + currentFuncName: "", + funcCallArgs: make(map[string][]*int64), + MetaData: issue.MetaData{ + ID: id, + Severity: issue.Medium, + Confidence: issue.Medium, + What: "Potentially accessing slice out of bounds", + }, + }, []ast.Node{(*ast.CallExpr)(nil), (*ast.FuncDecl)(nil), (*ast.AssignStmt)(nil), (*ast.SliceExpr)(nil), (*ast.IndexExpr)(nil)} +} diff --git a/testutils/source.go b/testutils/source.go index 6f967ac..63e6e4c 100644 --- a/testutils/source.go +++ b/testutils/source.go @@ -3679,4 +3679,182 @@ func main() { C.printData(cData) } `}, 0, gosec.NewConfig()}} + + // SampleCodeG602 - Slice access out of bounds + SampleCodeG602 = []CodeSample{ + {[]string{` +package main + +import "fmt" + +func main() { + + s := make([]byte, 0) + + fmt.Println(s[:3]) + +}`}, 1, gosec.NewConfig()}, + {[]string{` +package main + +import "fmt" + +func main() { + + s := make([]byte, 0) + + fmt.Println(s[3:]) + +}`}, 1, gosec.NewConfig()}, + {[]string{` +package main + +import "fmt" + +func main() { + + s := make([]byte, 16) + + fmt.Println(s[:17]) + +}`}, 1, gosec.NewConfig()}, + {[]string{` +package main + +import "fmt" + +func main() { + + s := make([]byte, 16) + + fmt.Println(s[:16]) + +}`}, 0, gosec.NewConfig()}, + {[]string{` +package main + +import "fmt" + +func main() { + + s := make([]byte, 16) + + fmt.Println(s[5:17]) + +}`}, 1, gosec.NewConfig()}, + {[]string{` +package main + +import "fmt" + +func main() { + + s := make([]byte, 4) + + fmt.Println(s[3]) + +}`}, 0, gosec.NewConfig()}, + {[]string{` +package main + +import "fmt" + +func main() { + + s := make([]byte, 4) + + fmt.Println(s[5]) + +}`}, 1, gosec.NewConfig()}, + {[]string{` +package main + +import "fmt" + +func main() { + + s := make([]byte, 0) + s = make([]byte, 3) + + fmt.Println(s[:3]) + +}`}, 0, gosec.NewConfig()}, + + {[]string{` +package main + +import "fmt" + +func main() { + + s := make([]byte, 0, 4) + + fmt.Println(s[:3]) + fmt.Println(s[3]) + +}`}, 0, gosec.NewConfig()}, + {[]string{` +package main + +import "fmt" + +func main() { + + s := make([]byte, 0, 4) + + fmt.Println(s[:5]) + fmt.Println(s[7]) + +}`}, 2, gosec.NewConfig()}, + {[]string{` +package main + +import "fmt" + +func main() { + + s := make([]byte, 0, 4) + x := s[:2] + y := x[:10] + fmt.Println(y) +}`}, 1, gosec.NewConfig()}, + + {[]string{` +package main + +import "fmt" + +func main() { + + s := make([]int, 0, 4) + doStuff(s) +} + +func doStuff(x []int) { + newSlice := x[:10] + fmt.Println(newSlice) +}`}, 1, gosec.NewConfig()}, + {[]string{` +package main + +import "fmt" + +func main() { + + s := make([]int, 0, 30) + doStuff(s) + x := make([]int, 20) + y := x[10:] + doStuff(y) + z := y[5:] + doStuff(z) +} + +func doStuff(x []int) { + newSlice := x[:10] + fmt.Println(newSlice) + newSlice2 := x[:6] + fmt.Println(newSlice2) +}`}, 2, gosec.NewConfig()}, + } )