rules/sdk: implement pass to only permit key iteration over maps, not…

… value

This pass exists to curtail non-determinism in the cosmos-sdk
which stemmed from iterating over maps during upgrades and that
caused a chaotic debug for weeks. With this change, we'll now
enforce and report failed iterations, with the rule being that
a map in a range should involve ONLY one of these 2 operations:
* for k := range m { delete(m, k) } for fast map clearing
* for k := range m { keys = append(keys, k) } to retrieve keys & sort

thus we shall get this report:
```shell
[gosec] 2021/11/09 03:18:57 Rule error: *sdk.mapRanging => the value in the range statement should be nil: want: for key := range m (main.go:19)
[gosec] 2021/11/09 03:18:57 Rule error: *sdk.mapRanging => the value in the range statement should be nil: want: for key := range m (main.go:27)
```

from the code below:

```go
package main

func main() {
	m := map[string]int{
		"a": 0,
		"b": 1,
		"c": 2,
		"d": 3,
	}

	makeMap := func() map[string]string { return nil }

	keys := make([]string, 0, len(m))
	for k := range m {
		keys = append(keys, k)
	}

	values := make([]int, 0, len(m))
	for _, value := range m {
		values = append(values, value)
	}

	type kv struct {
		k, v interface{}
	}
	kvL := make([]*kv, 0, len(m))
	for k, v := range m {
		kvL = append(kvL, &kv{k, v})
	}

	for k := range m {
		delete(m, k)
	}

	for k := range makeMap() {
		delete(m, k)
	}

	for k := range do() {
		delete(m, k)
	}
}

func do() map[string]string { return nil }
```

Updates cosmos/cosmos-sdk#10190

rules/rulelist.go

@@ -108,6 +108,7 @@ func Generate(filters ...RuleFilter) RuleList {
 		{"G702", "Import blocklist for SDK modules", sdk.NewUnsafeImport},
 		{"G703", "Errors that don't result in rollback", sdk.NewErrorNotPropagated},
 		{"G704", "Strconv invalid bitSize and cast", sdk.NewStrconvIntBitSizeOverflow},
+		{"G705", "Iterating over maps undeterministically", sdk.NewMapRangingCheck},
 	}
 
 	ruleMap := make(map[string]RuleDefinition)

rules/rules_test.go

@@ -91,6 +91,10 @@ var _ = Describe("gosec rules", func() {
 			runner("G704", testutils.SampleCodeStrconvBitsize)
 		})
 
+		It("should detect non-deterministic map ranging", func() {
+			runner("G705", testutils.SampleCodeMapRangingNonDeterministic)
+		})
+
 		It("should detect DoS vulnerability via decompression bomb", func() {
 			runner("G110", testutils.SampleCodeG110)
 		})

rules/sdk/iterate_over_maps.go

@@ -0,0 +1,210 @@
+// (c) Copyright 2021 Hewlett Packard Enterprise Development LP
+//
+// 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 sdk
+
+import (
+	"errors"
+	"fmt"
+	"go/ast"
+
+	"github.com/securego/gosec/v2"
+)
+
+// This pass enforces ONLY key retrieval from maps. It resolves a problem that was
+// discovered in the Cosmos-SDK in which maps were being iterated on by key and value
+// and that produced non-determinism in upgrades.
+
+type mapRanging struct {
+	gosec.MetaData
+	calls gosec.CallList
+}
+
+func (mr *mapRanging) ID() string {
+	return mr.MetaData.ID
+}
+
+func (mr *mapRanging) Match(node ast.Node, ctx *gosec.Context) (*gosec.Issue, error) {
+	rangeStmt, ok := node.(*ast.RangeStmt)
+	if !ok {
+		return nil, nil
+	}
+
+	// Algorithm:
+	// 1. Ensure that right hand side's eventual type is a map.
+	// 2. Ensure that only the form:
+	//          for k := range m
+	// is allowed, and NOT:
+	//          for k, v := range m
+	//    NOR
+	//          for _, v := range m
+	// 3. Ensure that only keys are appended
+	// 4. The only exception is if we have the map clearing idiom.
+
+	// 1. Ensure that the type of right hand side of the range is eventually a map.
+	var decl interface{}
+	switch rangeRHS := rangeStmt.X.(type) {
+	case *ast.Ident:
+		decl = rangeRHS.Obj.Decl
+
+	case *ast.CallExpr:
+		// Synthesize the declaration to be an *ast.FuncType from
+		// either function declarations or function literals.
+		idecl := rangeRHS.Fun.(*ast.Ident).Obj.Decl
+		switch idecl := idecl.(type) {
+		case *ast.FuncDecl:
+			decl = idecl.Type
+
+		case *ast.AssignStmt:
+			decl = idecl.Rhs[0].(*ast.FuncLit).Type
+		}
+	}
+
+	switch decl := decl.(type) {
+	case *ast.FuncType:
+		returns := decl.Results
+		if g, w := len(returns.List), 1; g != w {
+			return nil, fmt.Errorf("returns %d arguments, want %d", g, w)
+		}
+		returnType := returns.List[0].Type
+		if _, ok := returnType.(*ast.MapType); !ok {
+			return nil, nil
+		}
+
+	case *ast.AssignStmt:
+		rhs0 := decl.Rhs[0].(*ast.CompositeLit)
+		if _, ok := rhs0.Type.(*ast.MapType); !ok {
+			return nil, nil
+		}
+
+	case *ast.ValueSpec:
+		if _, ok := decl.Type.(*ast.MapType); !ok {
+			return nil, nil
+		}
+
+	default:
+		return nil, fmt.Errorf("unhandled type of declaration: %T", decl)
+	}
+
+	// 2. Let's be pedantic to only permit the keys to be iterated upon:
+	// Allow only:
+	//     for key := range m {
+	// AND NOT:
+	//     for _, value := range m {
+	// NOR
+	//     for key, value := range m {
+	if rangeStmt.Key == nil {
+		return nil, errors.New("the key in the range statement should be non-nil: want: for key := range m")
+	}
+	if rangeStmt.Value != nil {
+		return nil, errors.New("the value in the range statement should be nil: want: for key := range m")
+	}
+
+	// Now ensure that only either "append" or "delete" statement is present in the range.
+	rangeBody := rangeStmt.Body
+
+	if n := len(rangeBody.List); n > 1 {
+		return nil, fmt.Errorf("got %d statements, yet expecting exactly 1 statement (either append or delete) in a range with a map", n)
+	}
+
+	stmt0 := rangeBody.List[0]
+	switch stmt := stmt0.(type) {
+	case *ast.ExprStmt:
+		call := stmt.X.(*ast.CallExpr)
+		name, ok := eitherAppendOrDeleteCall(call)
+		if !ok {
+			return nil, fmt.Errorf("expecting only delete, got: %q", name)
+		}
+		// We got "delete", so this is safe to recognize
+		// as this is the fast map clearing idiom.
+		return nil, nil
+
+	case *ast.AssignStmt:
+		lhs0, ok := stmt.Lhs[0].(*ast.Ident)
+		if !ok {
+			return nil, fmt.Errorf("expecting an identifier for an append call to a slice, got %T", stmt.Lhs[0])
+		}
+		if _, ok := typeOf(lhs0.Obj).(*ast.ArrayType); !ok {
+			return nil, fmt.Errorf("expecting an array/slice being used to retrieve keys, got %T", lhs0.Obj)
+		}
+
+		rhs0, ok := stmt.Rhs[0].(*ast.CallExpr)
+		if !ok {
+			return nil, fmt.Errorf("expecting only an append, got: %#v", stmt.Rhs[0])
+		}
+		// The Right Hand Side should only contain the "append".
+		if name, ok := eitherAppendOrDeleteCall(rhs0); !ok {
+			return nil, fmt.Errorf(`got call %q want "append" or "delete"`, name)
+		}
+		return nil, nil
+
+	default:
+		return nil, fmt.Errorf("got %T; expecting exactly 1 statement (either append or delete) in a range with a map", stmt)
+	}
+}
+
+func eitherAppendOrDeleteCall(callExpr *ast.CallExpr) (string, bool) {
+	fn, ok := callExpr.Fun.(*ast.Ident)
+	if !ok {
+		return "", false
+	}
+	switch fn.Name {
+	case "append", "delete":
+		return fn.Name, true
+	default:
+		return fn.Name, false
+	}
+}
+
+func typeOf(value interface{}) ast.Node {
+	switch typ := value.(type) {
+	case *ast.Object:
+		return typeOf(typ.Decl)
+
+	case *ast.AssignStmt:
+		decl := typ
+		rhs := decl.Rhs[0]
+		if _, ok := rhs.(*ast.CallExpr); ok {
+			return typeOf(rhs)
+		}
+
+	case *ast.CallExpr:
+		decl := typ
+		fn := decl.Fun.(*ast.Ident)
+		if fn.Name == "make" {
+			// We can infer the type from the first argument.
+			return decl.Args[0]
+		}
+		return typeOf(decl.Args[0])
+	}
+
+	panic(fmt.Sprintf("Unexpected type: %T", value))
+}
+
+func NewMapRangingCheck(id string, config gosec.Config) (rule gosec.Rule, nodes []ast.Node) {
+	calls := gosec.NewCallList()
+
+	mr := &mapRanging{
+		MetaData: gosec.MetaData{
+			ID:         id,
+			Severity:   gosec.High,
+			Confidence: gosec.Medium,
+			What:       "Non-determinism from ranging over maps",
+		},
+		calls: calls,
+	}
+
+	nodes = append(nodes, (*ast.RangeStmt)(nil))
+	return mr, nodes
+}

testutils/source.go

@@ -2399,4 +2399,61 @@ func main() {
 	v := int32(value)
 	fmt.Println(v)
 }`}, 0, gosec.NewConfig()}}
+
+	// SampleCodeMapRangingNonDeterministic - Detect potential non-determinism
+	SampleCodeMapRangingNonDeterministic = []CodeSample{
+		{
+			[]string{`
+package main
+
+func main() {
+	m := map[string]int{
+		"a": 0,
+		"b": 1,
+		"c": 2,
+		"d": 3,
+	}
+
+	makeMap := func() map[string]string { return nil }
+
+	keys := make([]string, 0, len(m))
+	for k := range m {
+		keys = append(keys, k)
+	}
+
+	values := make([]int, 0, len(m))
+	for _, value := range m {
+		values = append(values, value)
+	}
+
+	type kv struct {
+		k, v interface{}
+	}
+	kvL := make([]*kv, 0, len(m))
+	for k, v := range m {
+		kvL = append(kvL, &kv{k, v})
+	}
+
+	for k := range m {
+		delete(m, k)
+	}
+
+	for k := range makeMap() {
+		delete(m, k)
+	}
+
+	for k := range do() {
+		delete(m, k)
+	}
+
+	for k, v := range do() {
+                println(v)
+		delete(m, k)
+	}
+}
+
+func do() map[string]string { return nil }
+`}, 3, gosec.NewConfig(),
+		},
+	}
 )