From cdab4792b07ea4fbb54d517f3c071cac3aa1b7cd Mon Sep 17 00:00:00 2001 From: Emmanuel T Odeke Date: Mon, 22 Nov 2021 18:35:16 +0100 Subject: [PATCH] rules/sdk: add pass to reject time.Now() time.Now() uses local clocks that unfortunately when used in distributed consensus introduce lots of skew and can be exploited in vulnerabilities. Instead there is consensus aware clock whose timestamp is embedded in the current Block. This pass rejects usages as per https://forum.cosmos.network/t/cosmos-sdk-vulnerability-retrospective-security-advisory-jackfruit-october-12-2021/5349 Updates #1 --- Makefile | 4 +- rules/rulelist.go | 1 + rules/rules_test.go | 4 ++ rules/sdk/iterate_over_maps.go | 38 +++++++++++++-- rules/sdk/time_now.go | 84 ++++++++++++++++++++++++++++++++++ testutils/source.go | 19 ++++++++ 6 files changed, 145 insertions(+), 5 deletions(-) create mode 100644 rules/sdk/time_now.go diff --git a/Makefile b/Makefile index c720619..ea42cfc 100644 --- a/Makefile +++ b/Makefile @@ -5,7 +5,7 @@ IMAGE_REPO = securego BUILDFLAGS := '-w -s' CGO_ENABLED = 0 GO := GO111MODULE=on go -GO_NOMOD :=GO111MODULE=off go +GO_NOMOD :=GO111MODULE=on go GOPATH ?= $(shell $(GO) env GOPATH) GOBIN ?= $(GOPATH)/bin GOLINT ?= $(GOBIN)/golint @@ -21,7 +21,7 @@ install-test-deps: $(GO_NOMOD) get -u golang.org/x/crypto/ssh $(GO_NOMOD) get -u github.com/lib/pq -test: install-test-deps build fmt lint sec +test: install-test-deps build fmt sec # lint -- disabled for minimal builds $(GINKGO) -r -v fmt: diff --git a/rules/rulelist.go b/rules/rulelist.go index fdfd2e4..ebcb556 100644 --- a/rules/rulelist.go +++ b/rules/rulelist.go @@ -109,6 +109,7 @@ func Generate(filters ...RuleFilter) RuleList { {"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}, + {"G706", "Non-consensus aware time.Now() usage", sdk.NewTimeNowRefusal}, } ruleMap := make(map[string]RuleDefinition) diff --git a/rules/rules_test.go b/rules/rules_test.go index f3403ee..02adb9a 100644 --- a/rules/rules_test.go +++ b/rules/rules_test.go @@ -95,6 +95,10 @@ var _ = Describe("gosec rules", func() { runner("G705", testutils.SampleCodeMapRangingNonDeterministic) }) + It("should detect non-consensus aware time.Now() usage", func() { + runner("G706", testutils.SampleCodeTimeNowNonCensusAware) + }) + 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 index 62627df..1b77e90 100644 --- a/rules/sdk/iterate_over_maps.go +++ b/rules/sdk/iterate_over_maps.go @@ -35,6 +35,22 @@ func (mr *mapRanging) ID() string { return mr.MetaData.ID } +func extractIdent(call ast.Expr) *ast.Ident { + switch n := call.(type) { + case *ast.Ident: + return n + + case *ast.SelectorExpr: + if ident, ok := n.X.(*ast.Ident); ok { + return ident + } + return extractIdent(n.X) + + default: + panic(fmt.Sprintf("Unhandled type: %T", call)) + } +} + func (mr *mapRanging) Match(node ast.Node, ctx *gosec.Context) (*gosec.Issue, error) { rangeStmt, ok := node.(*ast.RangeStmt) if !ok { @@ -61,7 +77,7 @@ func (mr *mapRanging) Match(node ast.Node, ctx *gosec.Context) (*gosec.Issue, er 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 + idecl := extractIdent(rangeRHS.Fun).Obj.Decl switch idecl := idecl.(type) { case *ast.FuncDecl: decl = idecl.Type @@ -83,8 +99,7 @@ func (mr *mapRanging) Match(node ast.Node, ctx *gosec.Context) (*gosec.Issue, er } case *ast.AssignStmt: - rhs0 := decl.Rhs[0].(*ast.CompositeLit) - if _, ok := rhs0.Type.(*ast.MapType); !ok { + if skip := mapHandleAssignStmt(decl); skip { return nil, nil } @@ -154,6 +169,23 @@ func (mr *mapRanging) Match(node ast.Node, ctx *gosec.Context) (*gosec.Issue, er } } +func mapHandleAssignStmt(decl *ast.AssignStmt) (skip bool) { + switch rhs0 := decl.Rhs[0].(type) { + case *ast.CompositeLit: + if _, ok := rhs0.Type.(*ast.MapType); !ok { + return true + } + return false + + case *ast.CallExpr: + return true + + default: + // TODO: handle other types. + return true + } +} + func eitherAppendOrDeleteCall(callExpr *ast.CallExpr) (string, bool) { fn, ok := callExpr.Fun.(*ast.Ident) if !ok { diff --git a/rules/sdk/time_now.go b/rules/sdk/time_now.go new file mode 100644 index 0000000..c174ea7 --- /dev/null +++ b/rules/sdk/time_now.go @@ -0,0 +1,84 @@ +// (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" + "go/ast" + + "github.com/securego/gosec/v2" +) + +// This static analyzer discourages the use of time.Now() as it was discovered that +// its usage caused local non-determinism as reported and detailed at +// https://forum.cosmos.network/t/cosmos-sdk-vulnerability-retrospective-security-advisory-jackfruit-october-12-2021/5349 + +type timeNowCheck struct { + gosec.MetaData + calls gosec.CallList +} + +func (tmc *timeNowCheck) ID() string { return tmc.MetaData.ID } + +func (tmc *timeNowCheck) Match(node ast.Node, ctx *gosec.Context) (*gosec.Issue, error) { + // We want to catch all function invocations as well as assignments of any of the form: + // .Value = time.Now().* + // fn := time.Now + callExpr, ok := node.(*ast.CallExpr) + if !ok { + return nil, nil + } + + sel, ok := callExpr.Fun.(*ast.SelectorExpr) + if !ok { + return nil, nil + } + + if sel.Sel.Name != "Now" { + return nil, nil + } + + switch x := sel.X.(type) { + case *ast.Ident: + if x.Name != "time" { + return nil, nil + } + + case *ast.SelectorExpr: + if x.Sel.Name != "time" { + return nil, nil + } + } + + // By this point issue the error. + return nil, errors.New("time.Now() is non-deterministic for distributed consensus, you should use the current Block's timestamp") +} + +func NewTimeNowRefusal(id string, config gosec.Config) (rule gosec.Rule, nodes []ast.Node) { + calls := gosec.NewCallList() + + tnc := &timeNowCheck{ + MetaData: gosec.MetaData{ + ID: id, + Severity: gosec.High, + Confidence: gosec.High, + What: "Non-determinism from using non-consensus aware time.Now()", + }, + calls: calls, + } + + nodes = append(nodes, (*ast.CallExpr)(nil)) + return tnc, nodes +} diff --git a/testutils/source.go b/testutils/source.go index cd49d07..f09a6dc 100644 --- a/testutils/source.go +++ b/testutils/source.go @@ -2456,4 +2456,23 @@ func do() map[string]string { return nil } `}, 3, gosec.NewConfig(), }, } + + SampleCodeTimeNowNonCensusAware = []CodeSample{ + { + []string{` +package main + +func main() { + _ = time.Now() +}`}, 3, gosec.NewConfig(), + }, + { + []string{` +package main + +func main() { + _ = time.Now().Unix() +}`}, 3, gosec.NewConfig(), + }, + } )