From b7d3dd73025ea7d451d87cfd72d469eb4ee16217 Mon Sep 17 00:00:00 2001 From: Morgen Malinoski <51513125+morgenm@users.noreply.github.com> Date: Fri, 16 Jun 2023 15:14:32 -0500 Subject: [PATCH 01/12] Added slice bounds testing for slice expressions. --- rules/rulelist.go | 1 + rules/rules_test.go | 4 ++ rules/slice_bounds.go | 118 ++++++++++++++++++++++++++++++++++++++++++ testutils/source.go | 19 +++++++ 4 files changed, 142 insertions(+) create mode 100644 rules/slice_bounds.go diff --git a/rules/rulelist.go b/rules/rulelist.go index d856eccad2..6a195a94f5 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 out of bounds", NewSliceBoundCheck}, } ruleMap := make(map[string]RuleDefinition) diff --git a/rules/rules_test.go b/rules/rules_test.go index 7a214fcba3..9aa3971c03 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 0000000000..5e4ae7084b --- /dev/null +++ b/rules/slice_bounds.go @@ -0,0 +1,118 @@ +package rules + +import ( + "go/ast" + + "github.com/securego/gosec/v2" + "github.com/securego/gosec/v2/issue" +) + +type sliceOutOfBounds struct { + calls gosec.CallList + sliceSizes map[string]int64 + issue.MetaData +} + +func (s *sliceOutOfBounds) ID() string { + return s.MetaData.ID +} + +func (s *sliceOutOfBounds) Match(node ast.Node, ctx *gosec.Context) (*issue.Issue, error) { + switch node := node.(type) { + case *ast.AssignStmt: + return s.matchAssign(node, ctx) + case *ast.SliceExpr: + return s.matchSliceExpr(node, ctx) + } + return nil, nil +} + +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 { + funcCall, ok := i.(*ast.CallExpr) + if !ok { + return nil, nil + } + + _, funcName, err := gosec.GetCallInfo(i, ctx) + if err != nil || funcName != "make" { + return nil, nil + } + + if len(funcCall.Args) < 2 { + return nil, nil // No size passed + } + + // Check and get the size of the slice passed to make. It must be a literal value, since we aren't evaluating the expression. + sliceSizeLit, ok := funcCall.Args[1].(*ast.BasicLit) + if !ok { + return nil, nil + } + + sliceSize, err := gosec.GetInt(sliceSizeLit) + if err != nil { + return nil, nil + } + + // Get the slice name so we can associate the size with the slice in the map + sliceIdent, ok := node.Lhs[it].(*ast.Ident) + if !ok { + return nil, nil + } + + sliceName := sliceIdent.Name + if err != nil { + return nil, nil + } + + s.sliceSizes[sliceName] = sliceSize + } + return nil, nil +} + +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 size from the map to compare it against high and low + sliceSize, ok := s.sliceSizes[ident.Name] + if !ok { + return nil, nil // Slice is not present in map, so doing nothing + } + + // Get and check low value + highIdent, ok := node.High.(*ast.BasicLit) + if ok && highIdent != nil { + high, _ := gosec.GetInt(highIdent) + if high > sliceSize { + 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 > sliceSize { + return ctx.NewIssue(node, s.ID(), s.What, s.Severity, s.Confidence), nil + } + } + + return nil, nil +} + +func NewSliceBoundCheck(id string, _ gosec.Config) (gosec.Rule, []ast.Node) { + return &sliceOutOfBounds{ + sliceSizes: 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.AssignStmt)(nil), (*ast.ValueSpec)(nil), (*ast.BinaryExpr)(nil), (*ast.SliceExpr)(nil)} +} diff --git a/testutils/source.go b/testutils/source.go index 6f967ac629..1b68708155 100644 --- a/testutils/source.go +++ b/testutils/source.go @@ -3679,4 +3679,23 @@ func main() { C.printData(cData) } `}, 0, gosec.NewConfig()}} + + // SampleCodeG602 - Slice access out of bounds + SampleCodeG602 = []CodeSample{ + {[]string{ + ` +package main + +import "fmt" + +func main() { + + bb := make([]byte, 0) + + fmt.Println(bb[:3]) + +} +`, + }, 1, gosec.NewConfig()}, + } ) From e5f0e104a30e8842b40f2c9d5529847b7955460c Mon Sep 17 00:00:00 2001 From: Morgen Malinoski <51513125+morgenm@users.noreply.github.com> Date: Fri, 16 Jun 2023 15:38:21 -0500 Subject: [PATCH 02/12] Added checking slice index. --- rules/slice_bounds.go | 30 ++++++++++++++-- testutils/source.go | 83 +++++++++++++++++++++++++++++++++++++++---- 2 files changed, 104 insertions(+), 9 deletions(-) diff --git a/rules/slice_bounds.go b/rules/slice_bounds.go index 5e4ae7084b..d7ba586069 100644 --- a/rules/slice_bounds.go +++ b/rules/slice_bounds.go @@ -8,7 +8,6 @@ import ( ) type sliceOutOfBounds struct { - calls gosec.CallList sliceSizes map[string]int64 issue.MetaData } @@ -23,6 +22,8 @@ func (s *sliceOutOfBounds) Match(node ast.Node, ctx *gosec.Context) (*issue.Issu return s.matchAssign(node, ctx) case *ast.SliceExpr: return s.matchSliceExpr(node, ctx) + case *ast.IndexExpr: + return s.matchIndexExpr(node, ctx) } return nil, nil } @@ -105,6 +106,31 @@ func (s *sliceOutOfBounds) matchSliceExpr(node *ast.SliceExpr, ctx *gosec.Contex return nil, nil } +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 size from the map to compare it against high and low + sliceSize, ok := s.sliceSizes[ident.Name] + if !ok { + return nil, nil // Slice is not present in map, so doing nothing + } + + // Get the index literal + indexIdent, ok := node.Index.(*ast.BasicLit) + if ok && indexIdent != nil { + index, _ := gosec.GetInt(indexIdent) + if index >= sliceSize { + return ctx.NewIssue(node, s.ID(), s.What, s.Severity, s.Confidence), nil + } + } + + return nil, nil +} + func NewSliceBoundCheck(id string, _ gosec.Config) (gosec.Rule, []ast.Node) { return &sliceOutOfBounds{ sliceSizes: make(map[string]int64), @@ -114,5 +140,5 @@ func NewSliceBoundCheck(id string, _ gosec.Config) (gosec.Rule, []ast.Node) { Confidence: issue.Medium, What: "Potentially accessing slice out of bounds", }, - }, []ast.Node{(*ast.AssignStmt)(nil), (*ast.ValueSpec)(nil), (*ast.BinaryExpr)(nil), (*ast.SliceExpr)(nil)} + }, []ast.Node{(*ast.AssignStmt)(nil), (*ast.SliceExpr)(nil), (*ast.IndexExpr)(nil)} } diff --git a/testutils/source.go b/testutils/source.go index 1b68708155..2ffc4563f3 100644 --- a/testutils/source.go +++ b/testutils/source.go @@ -3682,20 +3682,89 @@ func main() { // SampleCodeG602 - Slice access out of bounds SampleCodeG602 = []CodeSample{ - {[]string{ - ` + {[]string{` package main import "fmt" func main() { - bb := make([]byte, 0) + s := make([]byte, 0) - fmt.Println(bb[:3]) + fmt.Println(s[:3]) -} -`, - }, 1, gosec.NewConfig()}, +}`}, 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()}, } ) From af7a2d90515b7c0c5c6ccaba88d4f533bbc98c5d Mon Sep 17 00:00:00 2001 From: Morgen Malinoski <51513125+morgenm@users.noreply.github.com> Date: Fri, 16 Jun 2023 15:44:09 -0500 Subject: [PATCH 03/12] Added test for reassigning slice. --- testutils/source.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/testutils/source.go b/testutils/source.go index 2ffc4563f3..b9bbb59342 100644 --- a/testutils/source.go +++ b/testutils/source.go @@ -3766,5 +3766,18 @@ func main() { 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()}, } ) From a968dc3f536f07789d06d489e4294c36b2ceae61 Mon Sep 17 00:00:00 2001 From: Morgen Malinoski <51513125+morgenm@users.noreply.github.com> Date: Fri, 16 Jun 2023 19:51:29 -0500 Subject: [PATCH 04/12] Store capacities on reslicing. --- rules/slice_bounds.go | 131 +++++++++++++++++++++++++++++------------- testutils/source.go | 55 ++++++++++++++++++ 2 files changed, 147 insertions(+), 39 deletions(-) diff --git a/rules/slice_bounds.go b/rules/slice_bounds.go index d7ba586069..72e2fb569a 100644 --- a/rules/slice_bounds.go +++ b/rules/slice_bounds.go @@ -2,13 +2,14 @@ package rules import ( "go/ast" + "log" "github.com/securego/gosec/v2" "github.com/securego/gosec/v2/issue" ) type sliceOutOfBounds struct { - sliceSizes map[string]int64 + sliceCaps map[string]int64 // Capacities of slices issue.MetaData } @@ -24,50 +25,99 @@ func (s *sliceOutOfBounds) Match(node ast.Node, ctx *gosec.Context) (*issue.Issu return s.matchSliceExpr(node, ctx) case *ast.IndexExpr: return s.matchIndexExpr(node, ctx) + //case *ast.CallExpr: + //return s.matchCallExpr(node, ctx) } return nil, nil } -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 { - funcCall, ok := i.(*ast.CallExpr) - if !ok { - return nil, nil - } +// 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 + } - _, funcName, err := gosec.GetCallInfo(i, ctx) - if err != nil || funcName != "make" { - return nil, nil - } + capacityArg := 1 + 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 + } - if len(funcCall.Args) < 2 { - return nil, nil // No size passed - } + // 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 + } - // Check and get the size of the slice passed to make. It must be a literal value, since we aren't evaluating the expression. - sliceSizeLit, ok := funcCall.Args[1].(*ast.BasicLit) - if !ok { - return nil, nil - } + sliceCap, err := gosec.GetInt(sliceCapLit) + if err != nil { + return nil, nil + } - sliceSize, err := gosec.GetInt(sliceSizeLit) - if err != nil { - return nil, nil - } + s.sliceCaps[sliceName] = sliceCap + return nil, nil +} - // Get the slice name so we can associate the size with the slice in the map +// 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, err + } + + // Now that the assignment is (presumably) successfully, we can calculate the capacity and add this new slice to the map + // Get ident to get name + ident, ok := node.X.(*ast.Ident) + if !ok { + return nil, nil + } + + // Get cap of old slice to calculate this new slice's cap + oldCap, ok := s.sliceCaps[ident.Name] + if !ok { + return nil, nil + } + log.Print(ident.Name, " OLD CAP--", oldCap) + + // Get and check low value + lowIdent, ok := node.Low.(*ast.BasicLit) + if ok && lowIdent != nil { + low, _ := gosec.GetInt(lowIdent) + + newCap := oldCap - low + log.Print(ident.Name, " NEW CAP--", newCap) + s.sliceCaps[sliceName] = newCap + } else if lowIdent == nil { // If no lower bound, capacity will be same + s.sliceCaps[sliceName] = oldCap + } + + log.Print(s.sliceCaps) + + return nil, nil +} + +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 - if err != nil { - return nil, nil - } - s.sliceSizes[sliceName] = sliceSize + 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 } @@ -79,17 +129,17 @@ func (s *sliceOutOfBounds) matchSliceExpr(node *ast.SliceExpr, ctx *gosec.Contex return nil, nil } - // Get slice size from the map to compare it against high and low - sliceSize, ok := s.sliceSizes[ident.Name] + // Get slice cap from the map to compare it against high and low + sliceCap, ok := s.sliceCaps[ident.Name] if !ok { return nil, nil // Slice is not present in map, so doing nothing } - // Get and check low value + // Get and check high value highIdent, ok := node.High.(*ast.BasicLit) if ok && highIdent != nil { high, _ := gosec.GetInt(highIdent) - if high > sliceSize { + if high > sliceCap { return ctx.NewIssue(node, s.ID(), s.What, s.Severity, s.Confidence), nil } } @@ -98,7 +148,7 @@ func (s *sliceOutOfBounds) matchSliceExpr(node *ast.SliceExpr, ctx *gosec.Contex lowIdent, ok := node.Low.(*ast.BasicLit) if ok && lowIdent != nil { low, _ := gosec.GetInt(lowIdent) - if low > sliceSize { + if low > sliceCap { return ctx.NewIssue(node, s.ID(), s.What, s.Severity, s.Confidence), nil } } @@ -113,8 +163,8 @@ func (s *sliceOutOfBounds) matchIndexExpr(node *ast.IndexExpr, ctx *gosec.Contex return nil, nil } - // Get slice size from the map to compare it against high and low - sliceSize, ok := s.sliceSizes[ident.Name] + // Get slice cap from the map to compare it against high and low + sliceSize, ok := s.sliceCaps[ident.Name] if !ok { return nil, nil // Slice is not present in map, so doing nothing } @@ -131,14 +181,17 @@ func (s *sliceOutOfBounds) matchIndexExpr(node *ast.IndexExpr, ctx *gosec.Contex return nil, nil } +//func (s *sliceOutOfBounds) matchAssign(node *ast.AssignStmt, ctx *gosec.Context) (*issue.Issue, error) { +//} + func NewSliceBoundCheck(id string, _ gosec.Config) (gosec.Rule, []ast.Node) { return &sliceOutOfBounds{ - sliceSizes: make(map[string]int64), + sliceCaps: 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.AssignStmt)(nil), (*ast.SliceExpr)(nil), (*ast.IndexExpr)(nil)} + }, []ast.Node{(*ast.AssignStmt)(nil), (*ast.SliceExpr)(nil), (*ast.IndexExpr)(nil), (*ast.CallExpr)(nil)} } diff --git a/testutils/source.go b/testutils/source.go index b9bbb59342..4174139eb8 100644 --- a/testutils/source.go +++ b/testutils/source.go @@ -3779,5 +3779,60 @@ func main() { 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) { + newSice := x[:10] + fmt.Println(newSlice) +}`}, 1, gosec.NewConfig()}, } ) From 0d54b197e158cfb53fb3df1d2cb7d6b8bfff406b Mon Sep 17 00:00:00 2001 From: Morgen Malinoski <51513125+morgenm@users.noreply.github.com> Date: Fri, 16 Jun 2023 20:39:08 -0500 Subject: [PATCH 05/12] Scope change clears map. Func name used to track slices. --- rules/slice_bounds.go | 32 ++++++++++++++++++++++---------- testutils/source.go | 2 +- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/rules/slice_bounds.go b/rules/slice_bounds.go index 72e2fb569a..02fbe60073 100644 --- a/rules/slice_bounds.go +++ b/rules/slice_bounds.go @@ -9,7 +9,9 @@ import ( ) type sliceOutOfBounds struct { - sliceCaps map[string]int64 // Capacities of slices + sliceCaps map[string]int64 // Capacities of slices + currentScope *ast.Scope // Current scope. Map is cleared when scope changes. + currentFuncName string // Current function. Slices are stored as funcName/sliceName. issue.MetaData } @@ -18,6 +20,13 @@ func (s *sliceOutOfBounds) ID() string { } func (s *sliceOutOfBounds) Match(node ast.Node, ctx *gosec.Context) (*issue.Issue, error) { + if s.currentScope == nil { + s.currentScope = ctx.Root.Scope + } else if s.currentScope != ctx.Root.Scope { + s.currentScope = ctx.Root.Scope + s.sliceCaps = make(map[string]int64) + } + switch node := node.(type) { case *ast.AssignStmt: return s.matchAssign(node, ctx) @@ -25,6 +34,8 @@ func (s *sliceOutOfBounds) Match(node ast.Node, ctx *gosec.Context) (*issue.Issu return s.matchSliceExpr(node, ctx) case *ast.IndexExpr: return s.matchIndexExpr(node, ctx) + case *ast.FuncDecl: + s.currentFuncName = node.Name.Name //case *ast.CallExpr: //return s.matchCallExpr(node, ctx) } @@ -60,7 +71,8 @@ func (s *sliceOutOfBounds) matchSliceMake(funcCall *ast.CallExpr, sliceName stri return nil, nil } - s.sliceCaps[sliceName] = sliceCap + s.sliceCaps[s.currentFuncName+"/"+sliceName] = sliceCap + log.Print(s.sliceCaps) return nil, nil } @@ -79,11 +91,10 @@ func (s *sliceOutOfBounds) matchSliceAssignment(node *ast.SliceExpr, sliceName s } // Get cap of old slice to calculate this new slice's cap - oldCap, ok := s.sliceCaps[ident.Name] + oldCap, ok := s.sliceCaps[s.currentFuncName+"/"+ident.Name] if !ok { return nil, nil } - log.Print(ident.Name, " OLD CAP--", oldCap) // Get and check low value lowIdent, ok := node.Low.(*ast.BasicLit) @@ -91,10 +102,9 @@ func (s *sliceOutOfBounds) matchSliceAssignment(node *ast.SliceExpr, sliceName s low, _ := gosec.GetInt(lowIdent) newCap := oldCap - low - log.Print(ident.Name, " NEW CAP--", newCap) - s.sliceCaps[sliceName] = newCap + s.sliceCaps[s.currentFuncName+"/"+sliceName] = newCap } else if lowIdent == nil { // If no lower bound, capacity will be same - s.sliceCaps[sliceName] = oldCap + s.sliceCaps[s.currentFuncName+"/"+sliceName] = oldCap } log.Print(s.sliceCaps) @@ -130,7 +140,7 @@ func (s *sliceOutOfBounds) matchSliceExpr(node *ast.SliceExpr, ctx *gosec.Contex } // Get slice cap from the map to compare it against high and low - sliceCap, ok := s.sliceCaps[ident.Name] + sliceCap, ok := s.sliceCaps[s.currentFuncName+"/"+ident.Name] if !ok { return nil, nil // Slice is not present in map, so doing nothing } @@ -153,6 +163,8 @@ func (s *sliceOutOfBounds) matchSliceExpr(node *ast.SliceExpr, ctx *gosec.Contex } } + log.Print(s.sliceCaps) + return nil, nil } @@ -164,7 +176,7 @@ func (s *sliceOutOfBounds) matchIndexExpr(node *ast.IndexExpr, ctx *gosec.Contex } // Get slice cap from the map to compare it against high and low - sliceSize, ok := s.sliceCaps[ident.Name] + sliceSize, ok := s.sliceCaps[s.currentFuncName+"/"+ident.Name] if !ok { return nil, nil // Slice is not present in map, so doing nothing } @@ -193,5 +205,5 @@ func NewSliceBoundCheck(id string, _ gosec.Config) (gosec.Rule, []ast.Node) { Confidence: issue.Medium, What: "Potentially accessing slice out of bounds", }, - }, []ast.Node{(*ast.AssignStmt)(nil), (*ast.SliceExpr)(nil), (*ast.IndexExpr)(nil), (*ast.CallExpr)(nil)} + }, []ast.Node{(*ast.AssignStmt)(nil), (*ast.SliceExpr)(nil), (*ast.IndexExpr)(nil), (*ast.CallExpr)(nil), (*ast.FuncDecl)(nil)} } diff --git a/testutils/source.go b/testutils/source.go index 4174139eb8..da8c3b290e 100644 --- a/testutils/source.go +++ b/testutils/source.go @@ -3831,7 +3831,7 @@ func main() { } func doStuff(x []int) { - newSice := x[:10] + newSlice := x[:10] fmt.Println(newSlice) }`}, 1, gosec.NewConfig()}, } From 3491cdbf4242b4e34b5b7f04000bf83f81fd4613 Mon Sep 17 00:00:00 2001 From: Morgen Malinoski <51513125+morgenm@users.noreply.github.com> Date: Sat, 17 Jun 2023 03:03:07 -0500 Subject: [PATCH 06/12] Map CallExpr to check bounds when passing to functions. --- rules/slice_bounds.go | 298 ++++++++++++++++++++++++++++++++---------- testutils/source.go | 22 ++++ 2 files changed, 252 insertions(+), 68 deletions(-) diff --git a/rules/slice_bounds.go b/rules/slice_bounds.go index 02fbe60073..5369e6109f 100644 --- a/rules/slice_bounds.go +++ b/rules/slice_bounds.go @@ -2,16 +2,17 @@ package rules import ( "go/ast" - "log" + "go/types" "github.com/securego/gosec/v2" "github.com/securego/gosec/v2/issue" ) type sliceOutOfBounds struct { - sliceCaps map[string]int64 // Capacities of slices - currentScope *ast.Scope // Current scope. Map is cleared when scope changes. - currentFuncName string // Current function. Slices are stored as funcName/sliceName. + 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 } @@ -21,10 +22,15 @@ func (s *sliceOutOfBounds) ID() string { func (s *sliceOutOfBounds) Match(node ast.Node, ctx *gosec.Context) (*issue.Issue, error) { if s.currentScope == nil { - s.currentScope = ctx.Root.Scope - } else if s.currentScope != ctx.Root.Scope { - s.currentScope = ctx.Root.Scope - s.sliceCaps = make(map[string]int64) + 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) { @@ -36,12 +42,152 @@ func (s *sliceOutOfBounds) Match(node ast.Node, ctx *gosec.Context) (*issue.Issu return s.matchIndexExpr(node, ctx) case *ast.FuncDecl: s.currentFuncName = node.Name.Name - //case *ast.CallExpr: - //return s.matchCallExpr(node, ctx) + s.loadArgCaps(node, ctx) + case *ast.CallExpr: + sliceMap := make(map[string]*int64) + s.sliceCaps[node] = sliceMap + s.setupCallArgCaps(node, ctx) } return nil, nil } +func (s *sliceOutOfBounds) updateSliceCaps(varName string, caps map[*ast.CallExpr]*int64) { + for callExpr, cap := range caps { + s.sliceCaps[callExpr][varName] = cap + } +} + +// Get all calls for the current 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 + _, funcName, err := gosec.GetCallInfo(callExpr, ctx) + if err != nil { + continue + } + + if funcName == s.currentFuncName { + calls = append(calls, callExpr) + } + } else { + calls = append(calls, callExpr) + } + } + return calls +} + +// Gets all the capacities for slice with given name that are stored for each call to the current function we are looking at +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 { + caps[call] = s.sliceCaps[call][varName] + } + + return caps +} + +// Evaluate and save 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 possibly 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 = nil + for _, cap := range caps { + if lowestCap == nil { + lowestCap = cap + } else if *lowestCap > *cap { + lowestCap = cap + } + } + + if lowestCap == 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(funcName, ident.Name, ctx) + + var lowestCap *int64 = nil + for _, cap := range caps { + if cap == nil { + continue + } + if lowestCap == nil { + lowestCap = cap + } else if *lowestCap > *cap { + lowestCap = cap + } + } + + if lowestCap == 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 +} + +// Load caps that were saved for a call to this function +func (s *sliceOutOfBounds) loadArgCaps(funcDecl *ast.FuncDecl, ctx *gosec.Context) { + sliceMap := make(map[string]*int64) + funcName := funcDecl.Name.Name + + argCaps, ok := s.funcCallArgs[funcName] + if !ok || len(argCaps) == 0 { + s.sliceCaps[nil] = sliceMap + return + } + + params := funcDecl.Type.Params.List + if len(params) > len(argCaps) { + return + } + + for it := range params { + cap := argCaps[it] + if cap == nil { + continue + } + + if len(params[it].Names) == 0 { + continue + } + + paramName := params[it].Names[0].Name + sliceMap[paramName] = cap + } + + s.sliceCaps[nil] = sliceMap +} + // 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) @@ -66,48 +212,58 @@ func (s *sliceOutOfBounds) matchSliceMake(funcCall *ast.CallExpr, sliceName stri return nil, nil } - sliceCap, err := gosec.GetInt(sliceCapLit) + cap, err := gosec.GetInt(sliceCapLit) if err != nil { return nil, nil } - s.sliceCaps[s.currentFuncName+"/"+sliceName] = sliceCap - log.Print(s.sliceCaps) + caps := s.getSliceCapsForFunc(s.currentFuncName, sliceName, ctx) + for callExpr, _ := range caps { + caps[callExpr] = &cap + } + + s.updateSliceCaps(sliceName, caps) return nil, nil } -// 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, err - } - - // Now that the assignment is (presumably) successfully, we can calculate the capacity and add this new slice to the map +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, nil + return nil } // Get cap of old slice to calculate this new slice's cap - oldCap, ok := s.sliceCaps[s.currentFuncName+"/"+ident.Name] - if !ok { - return nil, nil + 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 + } } - // Get and check low value - lowIdent, ok := node.Low.(*ast.BasicLit) - if ok && lowIdent != nil { - low, _ := gosec.GetInt(lowIdent) + return caps +} - newCap := oldCap - low - s.sliceCaps[s.currentFuncName+"/"+sliceName] = newCap - } else if lowIdent == nil { // If no lower bound, capacity will be same - s.sliceCaps[s.currentFuncName+"/"+sliceName] = oldCap +// 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, err } - log.Print(s.sliceCaps) + // 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 } @@ -140,30 +296,31 @@ func (s *sliceOutOfBounds) matchSliceExpr(node *ast.SliceExpr, ctx *gosec.Contex } // Get slice cap from the map to compare it against high and low - sliceCap, ok := s.sliceCaps[s.currentFuncName+"/"+ident.Name] - if !ok { - return nil, nil // Slice is not present in map, so doing nothing - } + caps := s.getSliceCapsForFunc(s.currentFuncName, ident.Name, ctx) - // Get and check high value - highIdent, ok := node.High.(*ast.BasicLit) - if ok && highIdent != nil { - high, _ := gosec.GetInt(highIdent) - if high > sliceCap { - return ctx.NewIssue(node, s.ID(), s.What, s.Severity, s.Confidence), nil + for _, cap := range caps { + if cap == nil { + continue } - } - // Get and check low value - lowIdent, ok := node.Low.(*ast.BasicLit) - if ok && lowIdent != nil { - low, _ := gosec.GetInt(lowIdent) - if low > sliceCap { - return ctx.NewIssue(node, s.ID(), s.What, s.Severity, s.Confidence), nil + // 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 + } } - } - log.Print(s.sliceCaps) + // 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 } @@ -176,34 +333,39 @@ func (s *sliceOutOfBounds) matchIndexExpr(node *ast.IndexExpr, ctx *gosec.Contex } // Get slice cap from the map to compare it against high and low - sliceSize, ok := s.sliceCaps[s.currentFuncName+"/"+ident.Name] - if !ok { - return nil, nil // Slice is not present in map, so doing nothing - } + caps := s.getSliceCapsForFunc(s.currentFuncName, ident.Name, ctx) - // Get the index literal - indexIdent, ok := node.Index.(*ast.BasicLit) - if ok && indexIdent != nil { - index, _ := gosec.GetInt(indexIdent) - if index >= sliceSize { - return ctx.NewIssue(node, s.ID(), s.What, s.Severity, s.Confidence), nil + 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 } -//func (s *sliceOutOfBounds) matchAssign(node *ast.AssignStmt, ctx *gosec.Context) (*issue.Issue, error) { -//} - func NewSliceBoundCheck(id string, _ gosec.Config) (gosec.Rule, []ast.Node) { + sliceMapNil := make(map[string]*int64) + sliceMap := make(map[*ast.CallExpr]map[string]*int64) + sliceMap[nil] = sliceMapNil + return &sliceOutOfBounds{ - sliceCaps: make(map[string]int64), + 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.AssignStmt)(nil), (*ast.SliceExpr)(nil), (*ast.IndexExpr)(nil), (*ast.CallExpr)(nil), (*ast.FuncDecl)(nil)} + }, []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 da8c3b290e..63e6e4c349 100644 --- a/testutils/source.go +++ b/testutils/source.go @@ -3834,5 +3834,27 @@ 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()}, } ) From 7915743b8d97bde55a9a0b14ab76e31313179e4d Mon Sep 17 00:00:00 2001 From: Morgen Malinoski <51513125+morgenm@users.noreply.github.com> Date: Sat, 17 Jun 2023 10:57:26 -0500 Subject: [PATCH 07/12] Fixed linter errors. --- rules/slice_bounds.go | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/rules/slice_bounds.go b/rules/slice_bounds.go index 5369e6109f..2235019061 100644 --- a/rules/slice_bounds.go +++ b/rules/slice_bounds.go @@ -42,7 +42,7 @@ func (s *sliceOutOfBounds) Match(node ast.Node, ctx *gosec.Context) (*issue.Issu return s.matchIndexExpr(node, ctx) case *ast.FuncDecl: s.currentFuncName = node.Name.Name - s.loadArgCaps(node, ctx) + s.loadArgCaps(node) case *ast.CallExpr: sliceMap := make(map[string]*int64) s.sliceCaps[node] = sliceMap @@ -61,15 +61,15 @@ func (s *sliceOutOfBounds) updateSliceCaps(varName string, caps map[*ast.CallExp func (s *sliceOutOfBounds) getAllCalls(funcName string, ctx *gosec.Context) []*ast.CallExpr { calls := []*ast.CallExpr{} - for callExpr, _ := range s.sliceCaps { + 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 - _, funcName, err := gosec.GetCallInfo(callExpr, ctx) + _, callFuncName, err := gosec.GetCallInfo(callExpr, ctx) if err != nil { continue } - if funcName == s.currentFuncName { + if callFuncName == funcName { calls = append(calls, callExpr) } } else { @@ -111,7 +111,7 @@ func (s *sliceOutOfBounds) setupCallArgCaps(callExpr *ast.CallExpr, ctx *gosec.C // 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 = nil + var lowestCap *int64 for _, cap := range caps { if lowestCap == nil { lowestCap = cap @@ -130,7 +130,7 @@ func (s *sliceOutOfBounds) setupCallArgCaps(callExpr *ast.CallExpr, ctx *gosec.C ident := arg.(*ast.Ident) caps := s.getSliceCapsForFunc(funcName, ident.Name, ctx) - var lowestCap *int64 = nil + var lowestCap *int64 for _, cap := range caps { if cap == nil { continue @@ -156,7 +156,7 @@ func (s *sliceOutOfBounds) setupCallArgCaps(callExpr *ast.CallExpr, ctx *gosec.C } // Load caps that were saved for a call to this function -func (s *sliceOutOfBounds) loadArgCaps(funcDecl *ast.FuncDecl, ctx *gosec.Context) { +func (s *sliceOutOfBounds) loadArgCaps(funcDecl *ast.FuncDecl) { sliceMap := make(map[string]*int64) funcName := funcDecl.Name.Name @@ -172,8 +172,8 @@ func (s *sliceOutOfBounds) loadArgCaps(funcDecl *ast.FuncDecl, ctx *gosec.Contex } for it := range params { - cap := argCaps[it] - if cap == nil { + capacity := argCaps[it] + if capacity == nil { continue } @@ -182,7 +182,7 @@ func (s *sliceOutOfBounds) loadArgCaps(funcDecl *ast.FuncDecl, ctx *gosec.Contex } paramName := params[it].Names[0].Name - sliceMap[paramName] = cap + sliceMap[paramName] = capacity } s.sliceCaps[nil] = sliceMap @@ -195,7 +195,7 @@ func (s *sliceOutOfBounds) matchSliceMake(funcCall *ast.CallExpr, sliceName stri return nil, nil } - capacityArg := 1 + var capacityArg int if len(funcCall.Args) < 2 { return nil, nil // No size passed } else if len(funcCall.Args) == 2 { @@ -212,14 +212,14 @@ func (s *sliceOutOfBounds) matchSliceMake(funcCall *ast.CallExpr, sliceName stri return nil, nil } - cap, err := gosec.GetInt(sliceCapLit) + capacity, err := gosec.GetInt(sliceCapLit) if err != nil { return nil, nil } caps := s.getSliceCapsForFunc(s.currentFuncName, sliceName, ctx) - for callExpr, _ := range caps { - caps[callExpr] = &cap + for callExpr := range caps { + caps[callExpr] = &capacity } s.updateSliceCaps(sliceName, caps) From d3b0fa2d6b226f21bdeb916b3f03c8c7191065bc Mon Sep 17 00:00:00 2001 From: Morgen Malinoski <51513125+morgenm@users.noreply.github.com> Date: Sat, 17 Jun 2023 11:15:35 -0500 Subject: [PATCH 08/12] Updated rulelist with CWE mapping. --- issue/issue.go | 1 + rules/rulelist.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/issue/issue.go b/issue/issue.go index 5bf00dec2d..db4d630fab 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 6a195a94f5..316691f614 100644 --- a/rules/rulelist.go +++ b/rules/rulelist.go @@ -107,7 +107,7 @@ func Generate(trackSuppressions bool, filters ...RuleFilter) RuleList { // memory safety {"G601", "Implicit memory aliasing in RangeStmt", NewImplicitAliasing}, - {"G602", "Slice out of bounds", NewSliceBoundCheck}, + {"G602", "Slice access out of bounds", NewSliceBoundCheck}, } ruleMap := make(map[string]RuleDefinition) From 7a3cc6005a579f4965820e791a85d8e79b26b732 Mon Sep 17 00:00:00 2001 From: Morgen Malinoski <51513125+morgenm@users.noreply.github.com> Date: Sat, 17 Jun 2023 11:33:52 -0500 Subject: [PATCH 09/12] Added comment for NewSliceBoundCheck. --- rules/slice_bounds.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/rules/slice_bounds.go b/rules/slice_bounds.go index 2235019061..a10746d84b 100644 --- a/rules/slice_bounds.go +++ b/rules/slice_bounds.go @@ -352,6 +352,8 @@ func (s *sliceOutOfBounds) matchIndexExpr(node *ast.IndexExpr, ctx *gosec.Contex return nil, nil } +// NewSliceBoundsCheck 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) { sliceMapNil := make(map[string]*int64) sliceMap := make(map[*ast.CallExpr]map[string]*int64) From 0f74b06cf737af26dac3353e0448608dea43cc91 Mon Sep 17 00:00:00 2001 From: Morgen Malinoski <51513125+morgenm@users.noreply.github.com> Date: Tue, 20 Jun 2023 07:35:35 -0500 Subject: [PATCH 10/12] Addressed nil cap runtime error. --- rules/slice_bounds.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/rules/slice_bounds.go b/rules/slice_bounds.go index a10746d84b..1d41908c2f 100644 --- a/rules/slice_bounds.go +++ b/rules/slice_bounds.go @@ -8,6 +8,9 @@ import ( "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. @@ -16,6 +19,7 @@ type sliceOutOfBounds struct { issue.MetaData } +// ID returns the rule ID for sliceOutOfBounds: G602. func (s *sliceOutOfBounds) ID() string { return s.MetaData.ID } @@ -107,12 +111,16 @@ func (s *sliceOutOfBounds) setupCallArgCaps(callExpr *ast.CallExpr, ctx *gosec.C case *ast.SliceExpr: caps := s.evaluateSliceExpr(node, ctx) - // Simplifying assumption: use the lowest capacity. Storing all possibly capacities for slices passed + // 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 { @@ -352,7 +360,7 @@ func (s *sliceOutOfBounds) matchIndexExpr(node *ast.IndexExpr, ctx *gosec.Contex return nil, nil } -// NewSliceBoundsCheck attempts to find any slices being accessed out of bounds +// 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) { sliceMapNil := make(map[string]*int64) From 6a86b77dce2c07422d7aae61b3b99afe75037740 Mon Sep 17 00:00:00 2001 From: Morgen Malinoski <51513125+morgenm@users.noreply.github.com> Date: Tue, 20 Jun 2023 10:48:11 -0500 Subject: [PATCH 11/12] Replaced usage of nil in call arg map with dummy callexprs. --- rules/slice_bounds.go | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/rules/slice_bounds.go b/rules/slice_bounds.go index 1d41908c2f..6aa41a9073 100644 --- a/rules/slice_bounds.go +++ b/rules/slice_bounds.go @@ -48,6 +48,12 @@ func (s *sliceOutOfBounds) Match(node ast.Node, ctx *gosec.Context) (*issue.Issu 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 + } + + // TODO: Dont do this for calls to make sliceMap := make(map[string]*int64) s.sliceCaps[node] = sliceMap s.setupCallArgCaps(node, ctx) @@ -76,8 +82,6 @@ func (s *sliceOutOfBounds) getAllCalls(funcName string, ctx *gosec.Context) []*a if callFuncName == funcName { calls = append(calls, callExpr) } - } else { - calls = append(calls, callExpr) } } return calls @@ -129,6 +133,7 @@ func (s *sliceOutOfBounds) setupCallArgCaps(callExpr *ast.CallExpr, ctx *gosec.C } if lowestCap == nil { + funcCallArgs = append(funcCallArgs, nil) continue } @@ -136,13 +141,14 @@ func (s *sliceOutOfBounds) setupCallArgCaps(callExpr *ast.CallExpr, ctx *gosec.C funcCallArgs = append(funcCallArgs, lowestCap) case *ast.Ident: ident := arg.(*ast.Ident) - caps := s.getSliceCapsForFunc(funcName, ident.Name, ctx) + 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 { @@ -151,6 +157,7 @@ func (s *sliceOutOfBounds) setupCallArgCaps(callExpr *ast.CallExpr, ctx *gosec.C } if lowestCap == nil { + funcCallArgs = append(funcCallArgs, nil) continue } @@ -168,15 +175,22 @@ 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[nil] = sliceMap + s.sliceCaps[&dummyCallExpr] = sliceMap return } params := funcDecl.Type.Params.List if len(params) > len(argCaps) { - return + return // Length of params and args doesn't match, so don't do anything with this. } for it := range params { @@ -193,7 +207,7 @@ func (s *sliceOutOfBounds) loadArgCaps(funcDecl *ast.FuncDecl) { sliceMap[paramName] = capacity } - s.sliceCaps[nil] = sliceMap + s.sliceCaps[&dummyCallExpr] = sliceMap } // Matches calls to make() and stores the capacity of the new slice in the map to compare against future slice usage @@ -247,6 +261,7 @@ func (s *sliceOutOfBounds) evaluateSliceExpr(node *ast.SliceExpr, ctx *gosec.Con if oldCap == nil { continue } + // Get and check low value lowIdent, ok := node.Low.(*ast.BasicLit) if ok && lowIdent != nil { @@ -363,9 +378,7 @@ func (s *sliceOutOfBounds) matchIndexExpr(node *ast.IndexExpr, ctx *gosec.Contex // 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) { - sliceMapNil := make(map[string]*int64) sliceMap := make(map[*ast.CallExpr]map[string]*int64) - sliceMap[nil] = sliceMapNil return &sliceOutOfBounds{ sliceCaps: sliceMap, From a7356c2bc8a2858d789ee0e50c75e37952fab4fa Mon Sep 17 00:00:00 2001 From: Morgen Malinoski <51513125+morgenm@users.noreply.github.com> Date: Tue, 20 Jun 2023 15:40:06 -0500 Subject: [PATCH 12/12] Updated comments, wrapped error return, addressed other review concerns. --- rules/slice_bounds.go | 41 ++++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/rules/slice_bounds.go b/rules/slice_bounds.go index 6aa41a9073..04811bb50f 100644 --- a/rules/slice_bounds.go +++ b/rules/slice_bounds.go @@ -1,6 +1,7 @@ package rules import ( + "fmt" "go/ast" "go/types" @@ -12,11 +13,11 @@ import ( // 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 + 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 + 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. @@ -53,7 +54,6 @@ func (s *sliceOutOfBounds) Match(node ast.Node, ctx *gosec.Context) (*issue.Issu break } - // TODO: Dont do this for calls to make sliceMap := make(map[string]*int64) s.sliceCaps[node] = sliceMap s.setupCallArgCaps(node, ctx) @@ -61,13 +61,15 @@ func (s *sliceOutOfBounds) Match(node ast.Node, ctx *gosec.Context) (*issue.Issu 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 } } -// Get all calls for the current function +// 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{} @@ -87,19 +89,21 @@ func (s *sliceOutOfBounds) getAllCalls(funcName string, ctx *gosec.Context) []*a return calls } -// Gets all the capacities for slice with given name that are stored for each call to the current function we are looking at +// 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 { - caps[call] = s.sliceCaps[call][varName] + if callCaps, ok := s.sliceCaps[call]; ok { + caps[call] = callCaps[varName] + } } return caps } -// Evaluate and save the caps for any slices in the args so they can be validated when the function is scanned +// 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{} @@ -170,7 +174,7 @@ func (s *sliceOutOfBounds) setupCallArgCaps(callExpr *ast.CallExpr, ctx *gosec.C s.funcCallArgs[funcName] = funcCallArgs } -// Load caps that were saved for a call to this function +// 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 @@ -203,14 +207,15 @@ func (s *sliceOutOfBounds) loadArgCaps(funcDecl *ast.FuncDecl) { continue } - paramName := params[it].Names[0].Name - sliceMap[paramName] = capacity + if paramName := params[it].Names[0]; paramName != nil { + sliceMap[paramName.Name] = capacity + } } s.sliceCaps[&dummyCallExpr] = sliceMap } -// Matches calls to make() and stores the capacity of the new slice in the map to compare against future slice usage +// 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" { @@ -248,6 +253,9 @@ func (s *sliceOutOfBounds) matchSliceMake(funcCall *ast.CallExpr, sliceName stri 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) @@ -277,11 +285,11 @@ func (s *sliceOutOfBounds) evaluateSliceExpr(node *ast.SliceExpr, ctx *gosec.Con return caps } -// Matches slice assignments, calculates capacity of slice if possible to store it in map +// 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, err + 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 @@ -291,6 +299,7 @@ func (s *sliceOutOfBounds) matchSliceAssignment(node *ast.SliceExpr, sliceName s 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 { @@ -311,6 +320,7 @@ func (s *sliceOutOfBounds) matchAssign(node *ast.AssignStmt, ctx *gosec.Context) 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) @@ -348,6 +358,7 @@ func (s *sliceOutOfBounds) matchSliceExpr(node *ast.SliceExpr, ctx *gosec.Contex 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)