diff --git a/README.md b/README.md index 6c6d298..3cd4020 100644 --- a/README.md +++ b/README.md @@ -1,7 +1,7 @@ # gosec - Golang Security Checker -Inspects source code for security problems by scanning the Go AST. +Inspects source code for security problems by scanning the Go AST and SSA code representation. diff --git a/analyzer.go b/analyzer.go index 023514b..50fd4b5 100644 --- a/analyzer.go +++ b/analyzer.go @@ -231,9 +231,7 @@ func (gosec *Analyzer) Process(buildTags []string, packagePaths ...string) error return fmt.Errorf("parsing errors in pkg %q: %w", pkg.Name, err) } gosec.CheckRules(pkg) - if on, err := gosec.config.IsGlobalEnabled(SSA); err == nil && on { - gosec.CheckAnalyzers(pkg) - } + gosec.CheckAnalyzers(pkg) } } } @@ -377,8 +375,10 @@ func (gosec *Analyzer) CheckAnalyzers(pkg *packages.Package) { continue } if result != nil { - if aissue, ok := result.(*issue.Issue); ok { - gosec.updateIssues(aissue, false, []issue.SuppressionInfo{}) + if passIssues, ok := result.([]*issue.Issue); ok { + for _, iss := range passIssues { + gosec.updateIssues(iss, false, []issue.SuppressionInfo{}) + } } } } diff --git a/analyzers/slice_bounds.go b/analyzers/slice_bounds.go new file mode 100644 index 0000000..98b3c2a --- /dev/null +++ b/analyzers/slice_bounds.go @@ -0,0 +1,376 @@ +// (c) Copyright gosec's authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package analyzers + +import ( + "errors" + "fmt" + "go/token" + "regexp" + "strconv" + "strings" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/buildssa" + "golang.org/x/tools/go/ssa" + + "github.com/securego/gosec/v2/issue" +) + +type bound int + +const ( + lowerUnbounded bound = iota + upperUnbounded + unbounded + upperBounded +) + +func newSliceBoundsAnalyzer(id string, description string) *analysis.Analyzer { + return &analysis.Analyzer{ + Name: id, + Doc: description, + Run: runSliceBounds, + Requires: []*analysis.Analyzer{buildssa.Analyzer}, + } +} + +func runSliceBounds(pass *analysis.Pass) (interface{}, error) { + ssaResult, err := getSSAResult(pass) + if err != nil { + return nil, err + } + + issues := map[ssa.Instruction]*issue.Issue{} + ifs := map[ssa.If]*ssa.BinOp{} + for _, mcall := range ssaResult.SSA.SrcFuncs { + for _, block := range mcall.DomPreorder() { + for _, instr := range block.Instrs { + switch instr := instr.(type) { + case *ssa.Alloc: + cap, err := extractSliceCapFromAlloc(instr.String()) + if err != nil { + break + } + allocRefs := instr.Referrers() + if allocRefs == nil { + break + } + for _, instr := range *allocRefs { + if slice, ok := instr.(*ssa.Slice); ok { + if _, ok := slice.X.(*ssa.Alloc); ok { + if slice.Parent() != nil { + l, h := extractSliceBounds(slice) + newCap := computeSliceNewCap(l, h, cap) + violations := []ssa.Instruction{} + trackSliceBounds(newCap, slice, &violations, ifs) + for _, s := range violations { + switch s := s.(type) { + case *ssa.Slice: + issue := newIssue( + pass.Analyzer.Name, + "slice bounds out of range", + pass.Fset, + s.Pos(), + issue.Low, + issue.High) + issues[s] = issue + case *ssa.IndexAddr: + issue := newIssue( + pass.Analyzer.Name, + "slice index out of range", + pass.Fset, + s.Pos(), + issue.Low, + issue.High) + issues[s] = issue + } + } + } + } + } + } + } + } + } + } + + for ifref, binop := range ifs { + bound, value, err := extractBinOpBound(binop) + if err != nil { + continue + } + for i, block := range ifref.Block().Succs { + if i == 1 { + bound = invBound(bound) + } + for _, instr := range block.Instrs { + if _, ok := issues[instr]; ok { + switch bound { + case lowerUnbounded: + break + case upperUnbounded, unbounded: + delete(issues, instr) + case upperBounded: + switch tinstr := instr.(type) { + case *ssa.Slice: + lower, upper := extractSliceBounds(tinstr) + if isSliceInsideBounds(0, value, lower, upper) { + delete(issues, instr) + } + case *ssa.IndexAddr: + indexValue, err := extractIntValue(tinstr.Index.String()) + if err != nil { + break + } + if isSliceIndexInsideBounds(0, value, indexValue) { + delete(issues, instr) + } + } + } + } + } + } + } + + foundIssues := []*issue.Issue{} + for _, issue := range issues { + foundIssues = append(foundIssues, issue) + } + if len(foundIssues) > 0 { + return foundIssues, nil + } + return nil, nil +} + +func trackSliceBounds(cap int, slice ssa.Node, violations *[]ssa.Instruction, ifs map[ssa.If]*ssa.BinOp) { + if violations == nil { + violations = &[]ssa.Instruction{} + } + referrers := slice.Referrers() + if referrers != nil { + for _, refinstr := range *referrers { + switch refinstr := refinstr.(type) { + case *ssa.Slice: + checkAllSlicesBounds(cap, refinstr, violations, ifs) + switch refinstr.X.(type) { + case *ssa.Alloc, *ssa.Parameter: + l, h := extractSliceBounds(refinstr) + newCap := computeSliceNewCap(l, h, cap) + trackSliceBounds(newCap, refinstr, violations, ifs) + } + case *ssa.IndexAddr: + indexValue, err := extractIntValue(refinstr.Index.String()) + if err == nil && !isSliceIndexInsideBounds(0, cap, indexValue) { + *violations = append(*violations, refinstr) + } + case *ssa.Call: + if ifref, cond := extractSliceIfLenCondition(refinstr); ifref != nil && cond != nil { + ifs[*ifref] = cond + } else { + parPos := -1 + for pos, arg := range refinstr.Call.Args { + if a, ok := arg.(*ssa.Slice); ok && a == slice { + parPos = pos + } + } + if fn, ok := refinstr.Call.Value.(*ssa.Function); ok { + if len(fn.Params) > parPos && parPos > -1 { + param := fn.Params[parPos] + trackSliceBounds(cap, param, violations, ifs) + } + } + } + } + } + } +} + +func checkAllSlicesBounds(cap int, slice *ssa.Slice, violations *[]ssa.Instruction, ifs map[ssa.If]*ssa.BinOp) { + if violations == nil { + violations = &[]ssa.Instruction{} + } + sliceLow, sliceHigh := extractSliceBounds(slice) + if !isSliceInsideBounds(0, cap, sliceLow, sliceHigh) { + *violations = append(*violations, slice) + } + switch slice.X.(type) { + case *ssa.Alloc, *ssa.Parameter, *ssa.Slice: + l, h := extractSliceBounds(slice) + newCap := computeSliceNewCap(l, h, cap) + trackSliceBounds(newCap, slice, violations, ifs) + } + + references := slice.Referrers() + if references == nil { + return + } + for _, ref := range *references { + switch s := ref.(type) { + case *ssa.Slice: + checkAllSlicesBounds(cap, s, violations, ifs) + switch s.X.(type) { + case *ssa.Alloc, *ssa.Parameter: + l, h := extractSliceBounds(s) + newCap := computeSliceNewCap(l, h, cap) + trackSliceBounds(newCap, s, violations, ifs) + } + } + } +} + +func extractSliceIfLenCondition(call *ssa.Call) (*ssa.If, *ssa.BinOp) { + if builtInLen, ok := call.Call.Value.(*ssa.Builtin); ok { + if builtInLen.Name() == "len" { + refs := call.Referrers() + if refs != nil { + for _, ref := range *refs { + if binop, ok := ref.(*ssa.BinOp); ok { + binoprefs := binop.Referrers() + for _, ref := range *binoprefs { + if ifref, ok := ref.(*ssa.If); ok { + return ifref, binop + } + } + } + } + } + } + } + return nil, nil +} + +func computeSliceNewCap(l, h, cap int) int { + if l == 0 && h == 0 { + return cap + } + if l > 0 && h == 0 { + return cap - l + } + if l == 0 && h > 0 { + return h + } + return h - l +} + +func invBound(bound bound) bound { + switch bound { + case lowerUnbounded: + return upperUnbounded + case upperUnbounded: + return lowerUnbounded + case upperBounded: + return unbounded + case unbounded: + return upperBounded + default: + return unbounded + } +} + +func extractBinOpBound(binop *ssa.BinOp) (bound, int, error) { + if binop.X != nil { + if x, ok := binop.X.(*ssa.Const); ok { + value, err := strconv.Atoi(x.Value.String()) + if err != nil { + return lowerUnbounded, value, err + } + switch binop.Op { + case token.LSS, token.LEQ: + return upperUnbounded, value, nil + case token.GTR, token.GEQ: + return lowerUnbounded, value, nil + case token.EQL: + return upperBounded, value, nil + case token.NEQ: + return unbounded, value, nil + } + } + } + if binop.Y != nil { + if y, ok := binop.Y.(*ssa.Const); ok { + value, err := strconv.Atoi(y.Value.String()) + if err != nil { + return lowerUnbounded, value, err + } + switch binop.Op { + case token.LSS, token.LEQ: + return lowerUnbounded, value, nil + case token.GTR, token.GEQ: + return upperUnbounded, value, nil + case token.EQL: + return upperBounded, value, nil + case token.NEQ: + return unbounded, value, nil + } + } + } + return lowerUnbounded, 0, fmt.Errorf("unable to extract constant from binop") +} + +func isSliceIndexInsideBounds(l, h int, index int) bool { + return (l <= index && index < h) +} + +func isSliceInsideBounds(l, h int, cl, ch int) bool { + return (l <= cl && h >= ch) && (l <= ch && h >= cl) +} + +func extractSliceBounds(slice *ssa.Slice) (int, int) { + var low int + if slice.Low != nil { + l, err := extractIntValue(slice.Low.String()) + if err == nil { + low = l + } + } + var high int + if slice.High != nil { + h, err := extractIntValue(slice.High.String()) + if err == nil { + high = h + } + } + return low, high +} + +func extractIntValue(value string) (int, error) { + parts := strings.Split(value, ":") + if len(parts) != 2 { + return 0, fmt.Errorf("invalid value: %s", value) + } + if parts[1] != "int" { + return 0, fmt.Errorf("invalid value: %s", value) + } + return strconv.Atoi(parts[0]) +} + +func extractSliceCapFromAlloc(instr string) (int, error) { + re := regexp.MustCompile(`new \[(\d+)\]*`) + var cap int + matches := re.FindAllStringSubmatch(instr, -1) + if matches == nil { + return cap, errors.New("no slice cap found") + } + + if len(matches) > 0 { + m := matches[0] + if len(m) > 1 { + return strconv.Atoi(m[1]) + } + } + + return 0, errors.New("no slice cap found") +} diff --git a/analyzers/ssrf.go b/analyzers/ssrf.go deleted file mode 100644 index 70e0211..0000000 --- a/analyzers/ssrf.go +++ /dev/null @@ -1,57 +0,0 @@ -// (c) Copyright gosec's authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package analyzers - -import ( - "golang.org/x/tools/go/analysis" - "golang.org/x/tools/go/analysis/passes/buildssa" - "golang.org/x/tools/go/ssa" - - "github.com/securego/gosec/v2/issue" -) - -func newSSRFAnalyzer(id string, description string) *analysis.Analyzer { - return &analysis.Analyzer{ - Name: id, - Doc: description, - Run: runSSRF, - Requires: []*analysis.Analyzer{buildssa.Analyzer}, - } -} - -func runSSRF(pass *analysis.Pass) (interface{}, error) { - ssaResult, err := getSSAResult(pass) - if err != nil { - return nil, err - } - // TODO: implement the analysis - for _, fn := range ssaResult.SSA.SrcFuncs { - for _, block := range fn.DomPreorder() { - for _, instr := range block.Instrs { - switch instr := instr.(type) { - case *ssa.Call: - callee := instr.Call.StaticCallee() - if callee != nil { - ssaResult.Logger.Printf("callee: %s\n", callee) - return newIssue(pass.Analyzer.Name, - "not implemented", - pass.Fset, instr.Call.Pos(), issue.Low, issue.High), nil - } - } - } - } - } - return nil, nil -} diff --git a/analyzers/util.go b/analyzers/util.go index f1bd867..5941184 100644 --- a/analyzers/util.go +++ b/analyzers/util.go @@ -38,7 +38,7 @@ type SSAAnalyzerResult struct { // BuildDefaultAnalyzers returns the default list of analyzers func BuildDefaultAnalyzers() []*analysis.Analyzer { return []*analysis.Analyzer{ - newSSRFAnalyzer("G107", "URL provided to HTTP request as taint input"), + newSliceBoundsAnalyzer("G602", "Possible slice bounds out of range"), } } diff --git a/rules/rulelist.go b/rules/rulelist.go index 316691f..d856ecc 100644 --- a/rules/rulelist.go +++ b/rules/rulelist.go @@ -107,7 +107,6 @@ 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/slice_bounds.go b/rules/slice_bounds.go deleted file mode 100644 index d74672b..0000000 --- a/rules/slice_bounds.go +++ /dev/null @@ -1,410 +0,0 @@ -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 if the type of the slice is a map, since they should no be checked. - if _, ok := funcCall.Args[0].(*ast.MapType); ok { - return nil, nil - } - - // 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 981cf3e..59b8c9e 100644 --- a/testutils/source.go +++ b/testutils/source.go @@ -3861,55 +3861,55 @@ func main() { }`}, 2, gosec.NewConfig()}, {[]string{` -package main + package main -import "fmt" + import "fmt" -func main() { + func main() { - s := make([]byte, 0, 4) - x := s[:2] - y := x[:10] - fmt.Println(y) -}`}, 1, gosec.NewConfig()}, + s := make([]byte, 0, 4) + x := s[:2] + y := x[:10] + fmt.Println(y) + }`}, 1, gosec.NewConfig()}, {[]string{` -package main + package main -import "fmt" + import "fmt" -func main() { + func main() { - s := make([]int, 0, 4) - doStuff(s) -} + s := make([]int, 0, 4) + doStuff(s) + } -func doStuff(x []int) { - newSlice := x[:10] - fmt.Println(newSlice) -}`}, 1, gosec.NewConfig()}, + func doStuff(x []int) { + newSlice := x[:10] + fmt.Println(newSlice) + }`}, 1, gosec.NewConfig()}, {[]string{` -package main + package main -import "fmt" + import "fmt" -func main() { + func main() { - s := make([]int, 0, 30) - doStuff(s) - x := make([]int, 20) - y := x[10:] - doStuff(y) - z := y[5:] - doStuff(z) -} + 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()}, + func doStuff(x []int) { + newSlice := x[:10] + fmt.Println(newSlice) + newSlice2 := x[:6] + fmt.Println(newSlice2) + }`}, 2, gosec.NewConfig()}, {[]string{` package main @@ -3923,6 +3923,49 @@ func main() { }, } fmt.Println(testMap) +}`}, 0, gosec.NewConfig()}, + {[]string{` +package main + +import "fmt" + +func main() { + s := make([]byte, 0) + if len(s) > 0 { + fmt.Println(s[0]) + } +}`}, 0, gosec.NewConfig()}, + {[]string{` +package main + +import "fmt" + +func main() { + s := make([]byte, 0) + if len(s) > 0 { + fmt.Println("fake test") + } + fmt.Println(s[0]) +}`}, 1, gosec.NewConfig()}, + {[]string{` +package main + +import "fmt" + +func main() { + s := make([]int, 16) + for i := 0; i < 17; i++ { + s = append(s, i) + } + if len(s) < 16 { + fmt.Println(s[10:16]) + } else { + fmt.Println(s[3:18]) + } + fmt.Println(s[0]) + for i := range s { + fmt.Println(s[i]) + } }`}, 0, gosec.NewConfig()}, } )