diff --git a/README.md b/README.md index ca4eb15..7e6e12e 100644 --- a/README.md +++ b/README.md @@ -71,6 +71,7 @@ directory you can supply `./...` as the input argument. - G106: Audit the use of ssh.InsecureIgnoreHostKey - G107: Url provided to HTTP request as taint input - G108: Profiling endpoint automatically exposed on /debug/pprof +- G109: Potential Integer overflow made by strconv.Atoi result conversion to int16/32 - G201: SQL query construction using format string - G202: SQL query construction using string concatenation - G203: Use of unescaped data in HTML templates diff --git a/analyzer.go b/analyzer.go index 7cc788e..52fa750 100644 --- a/analyzer.go +++ b/analyzer.go @@ -47,15 +47,16 @@ const LoadMode = packages.NeedName | // It is passed through to all rule functions as they are called. Rules may use // this data in conjunction withe the encountered AST node. type Context struct { - FileSet *token.FileSet - Comments ast.CommentMap - Info *types.Info - Pkg *types.Package - PkgFiles []*ast.File - Root *ast.File - Config Config - Imports *ImportTracker - Ignores []map[string]bool + FileSet *token.FileSet + Comments ast.CommentMap + Info *types.Info + Pkg *types.Package + PkgFiles []*ast.File + Root *ast.File + Config Config + Imports *ImportTracker + Ignores []map[string]bool + PassedValues map[string]interface{} } // Metrics used when reporting information about a scanning run. @@ -204,6 +205,7 @@ func (gosec *Analyzer) Check(pkg *packages.Package) { gosec.context.PkgFiles = pkg.Syntax gosec.context.Imports = NewImportTracker() gosec.context.Imports.TrackFile(file) + gosec.context.PassedValues = make(map[string]interface{}) ast.Walk(gosec, file) gosec.stats.NumFiles++ gosec.stats.NumLines += pkg.Fset.File(file.Pos()).LineCount() diff --git a/issue.go b/issue.go index 297030c..62a4859 100644 --- a/issue.go +++ b/issue.go @@ -53,6 +53,7 @@ var IssueToCWE = map[string]Cwe{ "G104": GetCwe("703"), "G106": GetCwe("322"), "G107": GetCwe("88"), + "G109": GetCwe("190"), "G201": GetCwe("89"), "G202": GetCwe("89"), "G203": GetCwe("79"), diff --git a/output/formatter_test.go b/output/formatter_test.go index 6aeb64a..41a851f 100644 --- a/output/formatter_test.go +++ b/output/formatter_test.go @@ -250,9 +250,9 @@ var _ = Describe("Formatter", func() { Context("When using different report formats", func() { grules := []string{"G101", "G102", "G103", "G104", "G106", - "G107", "G201", "G202", "G203", "G204", "G301", - "G302", "G303", "G304", "G305", "G401", "G402", - "G403", "G404", "G501", "G502", "G503", "G504", "G505"} + "G107", "G109", "G201", "G202", "G203", "G204", + "G301", "G302", "G303", "G304", "G305", "G401", + "G402", "G403", "G404", "G501", "G502", "G503", "G504", "G505"} It("csv formatted report should contain the CWE mapping", func() { for _, rule := range grules { diff --git a/rules/integer_overflow.go b/rules/integer_overflow.go new file mode 100644 index 0000000..702c02e --- /dev/null +++ b/rules/integer_overflow.go @@ -0,0 +1,91 @@ +// (c) Copyright 2016 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 rules + +import ( + "fmt" + "github.com/securego/gosec" + "go/ast" +) + +type integerOverflowCheck struct { + gosec.MetaData + calls gosec.CallList +} + +func (i *integerOverflowCheck) ID() string { + return i.MetaData.ID +} + +func (i *integerOverflowCheck) Match(node ast.Node, ctx *gosec.Context) (*gosec.Issue, error) { + var atoiVarName map[string]ast.Node + + // To check multiple lines, ctx.PassedValues is used to store temporary data. + if _, ok := ctx.PassedValues[i.ID()]; !ok { + atoiVarName = make(map[string]ast.Node) + ctx.PassedValues[i.ID()] = atoiVarName + } else if pv, ok := ctx.PassedValues[i.ID()].(map[string]ast.Node); ok { + atoiVarName = pv + } else { + return nil, fmt.Errorf("PassedValues[%s] of Context is not map[string]ast.Node, but %T", i.ID(), ctx.PassedValues[i.ID()]) + } + + // strconv.Atoi is a common function. + // To reduce false positives, This rule detects code which is converted to int32/int16 only. + switch n := node.(type) { + case *ast.FuncDecl: + // Clear atoiVarName by function + ctx.PassedValues[i.ID()] = make(map[string]ast.Node) + case *ast.AssignStmt: + for _, expr := range n.Rhs { + if callExpr, ok := expr.(*ast.CallExpr); ok && i.calls.ContainsCallExpr(callExpr, ctx, false) != nil { + if idt, ok := n.Lhs[0].(*ast.Ident); ok && idt.Name != "_" { + // Example: + // v, _ := strconv.Atoi("1111") + // Add "v" to atoiVarName map + atoiVarName[idt.Name] = n + } + } + } + case *ast.CallExpr: + if fun, ok := n.Fun.(*ast.Ident); ok { + if fun.Name == "int32" || fun.Name == "int16" { + if idt, ok := n.Args[0].(*ast.Ident); ok { + if n, ok := atoiVarName[idt.Name]; ok { + // Detect int32(v) and int16(v) + return gosec.NewIssue(ctx, n, i.ID(), i.What, i.Severity, i.Confidence), nil + } + } + } + } + } + + return nil, nil +} + +// NewIntegerOverflowCheck detects if there is potential Integer OverFlow +func NewIntegerOverflowCheck(id string, conf gosec.Config) (gosec.Rule, []ast.Node) { + calls := gosec.NewCallList() + calls.Add("strconv", "Atoi") + return &integerOverflowCheck{ + MetaData: gosec.MetaData{ + ID: id, + Severity: gosec.High, + Confidence: gosec.Medium, + What: "Potential Integer overflow made by strconv.Atoi result conversion to int16/32", + }, + calls: calls, + }, []ast.Node{(*ast.FuncDecl)(nil), (*ast.AssignStmt)(nil), (*ast.CallExpr)(nil)} +} diff --git a/rules/rulelist.go b/rules/rulelist.go index 97d262a..d04a3bc 100644 --- a/rules/rulelist.go +++ b/rules/rulelist.go @@ -66,6 +66,7 @@ func Generate(filters ...RuleFilter) RuleList { {"G106", "Audit the use of ssh.InsecureIgnoreHostKey function", NewSSHHostKey}, {"G107", "Url provided to HTTP request as taint input", NewSSRFCheck}, {"G108", "Profiling endpoint is automatically exposed", NewPprofCheck}, + {"G109", "Converting strconv.Atoi result to int32/int16", NewIntegerOverflowCheck}, // injection {"G201", "SQL query construction using format string", NewSQLStrFormat}, diff --git a/rules/rules_test.go b/rules/rules_test.go index 4efdfbe..6a4cd70 100644 --- a/rules/rules_test.go +++ b/rules/rules_test.go @@ -83,6 +83,10 @@ var _ = Describe("gosec rules", func() { runner("G108", testutils.SampleCodeG108) }) + It("should detect integer overflow", func() { + runner("G109", testutils.SampleCodeG109) + }) + It("should detect sql injection via format strings", func() { runner("G201", testutils.SampleCodeG201) }) diff --git a/testutils/pkg.go b/testutils/pkg.go index 65bca97..f733039 100644 --- a/testutils/pkg.go +++ b/testutils/pkg.go @@ -109,12 +109,13 @@ func (p *TestPackage) CreateContext(filename string) *gosec.Context { pkgFile = strings.TrimPrefix(pkgFile, strip) if pkgFile == filename { ctx := &gosec.Context{ - FileSet: pkg.Fset, - Root: file, - Config: gosec.NewConfig(), - Info: pkg.TypesInfo, - Pkg: pkg.Types, - Imports: gosec.NewImportTracker(), + FileSet: pkg.Fset, + Root: file, + Config: gosec.NewConfig(), + Info: pkg.TypesInfo, + Pkg: pkg.Types, + Imports: gosec.NewImportTracker(), + PassedValues: make(map[string]interface{}), } ctx.Imports.TrackPackages(ctx.Pkg.Imports()...) return ctx diff --git a/testutils/source.go b/testutils/source.go index 8cb16a3..13e91ef 100644 --- a/testutils/source.go +++ b/testutils/source.go @@ -522,6 +522,78 @@ func main() { }) log.Fatal(http.ListenAndServe(":8080", nil)) }`}, 0, gosec.NewConfig()}} + + // SampleCodeG109 - Potential Integer OverFlow + SampleCodeG109 = []CodeSample{ + // Bind to all networks explicitly + {[]string{` +package main + +import ( + "fmt" + "strconv" +) + +func main() { + bigValue, err := strconv.Atoi("2147483648") + if err != nil { + panic(err) + } + value := int32(bigValue) + fmt.Println(value) +}`}, 1, gosec.NewConfig()}, {[]string{` +package main + +import ( + "fmt" + "strconv" +) + +func main() { + bigValue, err := strconv.Atoi("32768") + if err != nil { + panic(err) + } + if int16(bigValue) < 0 { + fmt.Println(bigValue) + } +}`}, 1, gosec.NewConfig()}, {[]string{` +package main + +import ( + "fmt" + "strconv" +) + +func main() { + bigValue, err := strconv.Atoi("2147483648") + if err != nil { + panic(err) + } + fmt.Println(bigValue) +}`}, 0, gosec.NewConfig()}, {[]string{` +package main + +import ( + "fmt" + "strconv" +) + +func main() { + bigValue, err := strconv.Atoi("2147483648") + if err != nil { + panic(err) + } + fmt.Println(bigValue) + test() +} + +func test() { + bigValue := 30 + value := int32(bigValue) + fmt.Println(value) +}`}, 0, gosec.NewConfig()}} + // SampleCodeG201 - SQL injection via format string SampleCodeG201 = []CodeSample{ {[]string{`