Feature: G602 Slice Bound Checking #973

morgenm · 2023-06-17T16:29:49Z

This PR addresses this issue: #954

This adds a new rule which checks slice bounds access (reslicing and indexing) to ensure that slices are not accessed out of bounds. This only works for slice bounds defined by a make([]..., len) or make([]..., len, capacity) where len/capacity is defined as a literal integer. For example it works for:

s := make([]float, 10, 20)

But not for:

s := make([]float, 10*3)

Since we don't evaluate expressions. It also will calculate and validate slice capacities for new slices made by reslicing when possible, such as:

s := make([]float, 10)
x := s[5:]

The capacity for the new slice, x, will be validated in any subsequent usage. In addition, slices passed to functions are validated when possible by storing function-call-specific capacities. For example:

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

will cause an error on creation of newSlice and newSlice2, since when z is passed to it, it is accessed out of bounds.

rules/slice_bounds.go

houndci-bot · 2023-06-17T16:34:59Z

rules/slice_bounds.go

+	return nil, nil
+}
+
+// NewSliceBoundsCheck attempts to find any slices being accessed out of bounds


comment on exported function NewSliceBoundCheck should be of the form "NewSliceBoundCheck ..."

Could you please fix the function name in the comment?

codecov-commenter · 2023-06-17T16:35:28Z

Codecov Report

Patch coverage: 73.60% and project coverage change: +0.24 🎉

Comparison is base (abeab10) 71.84% compared to head (a7356c2) 72.08%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #973      +/-   ##
==========================================
+ Coverage   71.84%   72.08%   +0.24%     
==========================================
  Files          50       51       +1     
  Lines        3317     3586     +269     
==========================================
+ Hits         2383     2585     +202     
- Misses        868      911      +43     
- Partials       66       90      +24

Impacted Files	Coverage Δ
issue/issue.go	`0.00% <ø> (ø)`
rules/slice_bounds.go	`73.50% <73.50%> (ø)`
rules/rulelist.go	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

morgenm · 2023-06-17T21:26:35Z

Note: This solution is complex since it stores the constant capacities of all the slices made with make(). I am thinking of adding in a simple check for non-constant capacities (or capacities evaluated from expressions such as make([]int, 5*5)) by making sure the user adds in a bounds check such as if index < cap(slice). If it is better to simplify the whole check by only validating that the user has checked bounds, without storing the actual constant capacities in the map as I did in this PR, I can change the code.

Let me know your thoughts.

ccojocar

Thank you for this great contribution. I left a few comments. It would be great if you could fix them before we merge this rule. Thanks again!

rules/slice_bounds.go

ccojocar · 2023-06-19T07:41:02Z

rules/slice_bounds.go

+			continue
+		}
+
+		paramName := params[it].Names[0].Name


I would check Name[0] that is not nil before accessing the Name filed.

Okay, I added that check.

ccojocar · 2023-06-19T07:41:16Z

rules/slice_bounds.go

+	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


method name prefix in the comment

ccojocar · 2023-06-19T07:43:05Z

rules/slice_bounds.go

+	return caps
+}
+
+// Matches slice assignments, calculates capacity of slice if possible to store it in map


function name prefix missing in the comment

ccojocar · 2023-06-19T07:43:43Z

rules/slice_bounds.go

+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


I would wrap the err with some more context before returning.

Okay I wrapped it using fmt.Errorf.

ccojocar · 2023-06-19T07:46:08Z

rules/slice_bounds.go

+	return nil, nil
+}
+
+// NewSliceBoundsCheck attempts to find any slices being accessed out of bounds


Could you please fix the function name in the comment?

ccojocar · 2023-06-20T07:23:35Z

Note: This solution is complex since it stores the constant capacities of all the slices made with make(). I am thinking of adding in a simple check for non-constant capacities (or capacities evaluated from expressions such as make([]int, 5*5)) by making sure the user adds in a bounds check such as if index < cap(slice). If it is better to simplify the whole check by only validating that the user has checked bounds, without storing the actual constant capacities in the map as I did in this PR, I can change the code.

I think the current solution which checks the constant bounds looks fine to me. It is non intrusive comparing with a bound check which is more enforced on user.

My impression is, that one will really check the bounds at runtime if she wants to be defensive but most of people won't do it. This will cause a lot of warnings which most likely will get ignored.

morgenm · 2023-06-20T20:52:41Z

I believe I addressed all the reviews.

I think the current solution which checks the constant bounds looks fine to me. It is non intrusive comparing with a bound check which is more enforced on user.

My impression is, that one will really check the bounds at runtime if she wants to be defensive but most of people won't do it. This will cause a lot of warnings which most likely will get ignored.

Yes, thinking on it some more I agree with you.

razor-1 · 2023-08-21T19:16:41Z

It would be nice if this didn't fire when it was protected by something like if len(slice) >= x

franchb · 2023-08-22T11:04:52Z

Yes, this rule fires a lot of false positives on code protected with something like if len(slice) >= x

morgenm added 8 commits June 16, 2023 15:14

Added slice bounds testing for slice expressions.

b7d3dd7

Added checking slice index.

e5f0e10

Added test for reassigning slice.

af7a2d9

Store capacities on reslicing.

a968dc3

Scope change clears map. Func name used to track slices.

0d54b19

Map CallExpr to check bounds when passing to functions.

3491cdb

Fixed linter errors.

7915743

Updated rulelist with CWE mapping.

d3b0fa2

houndci-bot reviewed Jun 17, 2023

View reviewed changes

rules/slice_bounds.go Show resolved Hide resolved

morgenm mentioned this pull request Jun 17, 2023

No issue reported for slice bounds out of range which causes panic #954

Closed

Added comment for NewSliceBoundCheck.

7a3cc60

houndci-bot reviewed Jun 17, 2023

View reviewed changes

ccojocar reviewed Jun 19, 2023

View reviewed changes

morgenm added 3 commits June 20, 2023 07:35

Addressed nil cap runtime error.

0f74b06

Replaced usage of nil in call arg map with dummy callexprs.

6a86b77

Updated comments, wrapped error return, addressed other review concerns.

a7356c2

ccojocar approved these changes Jun 21, 2023

View reviewed changes

ccojocar merged commit a018cf0 into securego:master Jun 21, 2023
6 checks passed

tobias-kuendig mentioned this pull request Aug 24, 2023

Performance issue with gosec on v1.54.2 golangci/golangci-lint#4039

Closed

5 tasks

zak-pawel mentioned this pull request Jan 29, 2024

Linter: gosec, Rule: G602 - Slice access out of bounds. Should we enable it? influxdata/telegraf#14633

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: G602 Slice Bound Checking #973

Feature: G602 Slice Bound Checking #973

morgenm commented Jun 17, 2023 •

edited

Loading

houndci-bot Jun 17, 2023

ccojocar Jun 19, 2023

codecov-commenter commented Jun 17, 2023 •

edited

Loading

morgenm commented Jun 17, 2023

ccojocar left a comment

ccojocar Jun 19, 2023

morgenm Jun 20, 2023

ccojocar Jun 19, 2023

ccojocar Jun 19, 2023

ccojocar Jun 19, 2023

morgenm Jun 20, 2023

ccojocar Jun 19, 2023

ccojocar commented Jun 20, 2023

morgenm commented Jun 20, 2023

razor-1 commented Aug 21, 2023

franchb commented Aug 22, 2023

Feature: G602 Slice Bound Checking #973

Feature: G602 Slice Bound Checking #973

Conversation

morgenm commented Jun 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 17, 2023 • edited Loading

Codecov Report

morgenm commented Jun 17, 2023

ccojocar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccojocar commented Jun 20, 2023

morgenm commented Jun 20, 2023

razor-1 commented Aug 21, 2023

franchb commented Aug 22, 2023

morgenm commented Jun 17, 2023 •

edited

Loading

codecov-commenter commented Jun 17, 2023 •

edited

Loading