diff --git a/rules/rulelist.go b/rules/rulelist.go index f387fa1..fdfd2e4 100644 --- a/rules/rulelist.go +++ b/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) diff --git a/rules/rules_test.go b/rules/rules_test.go index 5d7e4df..f3403ee 100644 --- a/rules/rules_test.go +++ b/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) }) diff --git a/rules/sdk/iterate_over_maps.go b/rules/sdk/iterate_over_maps.go new file mode 100644 index 0000000..1d46c8e --- /dev/null +++ b/rules/sdk/iterate_over_maps.go @@ -0,0 +1,211 @@ +// (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) + } + + // Expecting only an append to a slice. + stmt0 := rangeBody.List[0] + switch stmt := stmt0.(type) { + case *ast.ExprStmt: + call := stmt.X.(*ast.CallExpr) + name, ok := eitherAppendOrDeleteCall(call) + if ok { + // We got "delete", so this is safe to recognize + // as this is the fast map clearing idiom. + return nil, nil + } + return nil, fmt.Errorf("expecting only delete, got: %q", name) + } + + assignStmt, ok := stmt0.(*ast.AssignStmt) + if !ok { + return nil, fmt.Errorf("expecting only an append, got: %#v", stmt0.(*ast.ExprStmt).X) + } + + lhs, ok := assignStmt.Lhs[0].(*ast.Ident) + if !ok { + return nil, fmt.Errorf("expecting an identifier for an append call to a slice, got %T", assignStmt.Lhs[0]) + } + if _, ok := typeOf(lhs.Obj).(*ast.ArrayType); !ok { + return nil, fmt.Errorf("expecting an array/slice being used to retrieve keys, got %T", lhs.Obj) + } + + rhs, ok := assignStmt.Rhs[0].(*ast.CallExpr) + if !ok { + return nil, fmt.Errorf("expecting only an append, got: %#v", assignStmt.Rhs[0]) + } + // The Right Hand Side should only contain the "append". + if name, ok := eitherAppendOrDeleteCall(rhs); !ok { + return nil, fmt.Errorf(`got call %q want "append" or "delete"`, name) + } + return nil, nil +} + +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 +} diff --git a/testutils/source.go b/testutils/source.go index 47efc33..91e05c4 100644 --- a/testutils/source.go +++ b/testutils/source.go @@ -2399,4 +2399,28 @@ 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, + "e": 4, + "f": 5, + } + + for key := range m { + println(m[key]) + println("key", key) + } +}`}, 1, gosec.NewConfig(), + }, + } )