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

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

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)

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

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

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