Add a rule which detects when there is potential integer overflow (#422)

* Add G109(Potential Integer OverFlow Detection)

Signed-off-by: Hiroki Suezawa <suezawa@gmail.com>

* add CWE to G109(Potential Integer Overflow)

Signed-off-by: Hiroki Suezawa <suezawa@gmail.com>

* Modify G109 to use gosec.Context

Signed-off-by: Hiroki Suezawa <suezawa@gmail.com>
This commit is contained in:
Hiroki Suezawa 2020-01-06 17:55:52 +09:00 committed by Cosmin Cojocar
parent f43a957359
commit 9cb83e10af
9 changed files with 191 additions and 18 deletions

View file

@ -71,6 +71,7 @@ directory you can supply `./...` as the input argument.
- G106: Audit the use of ssh.InsecureIgnoreHostKey - G106: Audit the use of ssh.InsecureIgnoreHostKey
- G107: Url provided to HTTP request as taint input - G107: Url provided to HTTP request as taint input
- G108: Profiling endpoint automatically exposed on /debug/pprof - 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 - G201: SQL query construction using format string
- G202: SQL query construction using string concatenation - G202: SQL query construction using string concatenation
- G203: Use of unescaped data in HTML templates - G203: Use of unescaped data in HTML templates

View file

@ -47,15 +47,16 @@ const LoadMode = packages.NeedName |
// It is passed through to all rule functions as they are called. Rules may use // It is passed through to all rule functions as they are called. Rules may use
// this data in conjunction withe the encountered AST node. // this data in conjunction withe the encountered AST node.
type Context struct { type Context struct {
FileSet *token.FileSet FileSet *token.FileSet
Comments ast.CommentMap Comments ast.CommentMap
Info *types.Info Info *types.Info
Pkg *types.Package Pkg *types.Package
PkgFiles []*ast.File PkgFiles []*ast.File
Root *ast.File Root *ast.File
Config Config Config Config
Imports *ImportTracker Imports *ImportTracker
Ignores []map[string]bool Ignores []map[string]bool
PassedValues map[string]interface{}
} }
// Metrics used when reporting information about a scanning run. // 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.PkgFiles = pkg.Syntax
gosec.context.Imports = NewImportTracker() gosec.context.Imports = NewImportTracker()
gosec.context.Imports.TrackFile(file) gosec.context.Imports.TrackFile(file)
gosec.context.PassedValues = make(map[string]interface{})
ast.Walk(gosec, file) ast.Walk(gosec, file)
gosec.stats.NumFiles++ gosec.stats.NumFiles++
gosec.stats.NumLines += pkg.Fset.File(file.Pos()).LineCount() gosec.stats.NumLines += pkg.Fset.File(file.Pos()).LineCount()

View file

@ -53,6 +53,7 @@ var IssueToCWE = map[string]Cwe{
"G104": GetCwe("703"), "G104": GetCwe("703"),
"G106": GetCwe("322"), "G106": GetCwe("322"),
"G107": GetCwe("88"), "G107": GetCwe("88"),
"G109": GetCwe("190"),
"G201": GetCwe("89"), "G201": GetCwe("89"),
"G202": GetCwe("89"), "G202": GetCwe("89"),
"G203": GetCwe("79"), "G203": GetCwe("79"),

View file

@ -250,9 +250,9 @@ var _ = Describe("Formatter", func() {
Context("When using different report formats", func() { Context("When using different report formats", func() {
grules := []string{"G101", "G102", "G103", "G104", "G106", grules := []string{"G101", "G102", "G103", "G104", "G106",
"G107", "G201", "G202", "G203", "G204", "G301", "G107", "G109", "G201", "G202", "G203", "G204",
"G302", "G303", "G304", "G305", "G401", "G402", "G301", "G302", "G303", "G304", "G305", "G401",
"G403", "G404", "G501", "G502", "G503", "G504", "G505"} "G402", "G403", "G404", "G501", "G502", "G503", "G504", "G505"}
It("csv formatted report should contain the CWE mapping", func() { It("csv formatted report should contain the CWE mapping", func() {
for _, rule := range grules { for _, rule := range grules {

91
rules/integer_overflow.go Normal file
View file

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

View file

@ -66,6 +66,7 @@ func Generate(filters ...RuleFilter) RuleList {
{"G106", "Audit the use of ssh.InsecureIgnoreHostKey function", NewSSHHostKey}, {"G106", "Audit the use of ssh.InsecureIgnoreHostKey function", NewSSHHostKey},
{"G107", "Url provided to HTTP request as taint input", NewSSRFCheck}, {"G107", "Url provided to HTTP request as taint input", NewSSRFCheck},
{"G108", "Profiling endpoint is automatically exposed", NewPprofCheck}, {"G108", "Profiling endpoint is automatically exposed", NewPprofCheck},
{"G109", "Converting strconv.Atoi result to int32/int16", NewIntegerOverflowCheck},
// injection // injection
{"G201", "SQL query construction using format string", NewSQLStrFormat}, {"G201", "SQL query construction using format string", NewSQLStrFormat},

View file

@ -83,6 +83,10 @@ var _ = Describe("gosec rules", func() {
runner("G108", testutils.SampleCodeG108) runner("G108", testutils.SampleCodeG108)
}) })
It("should detect integer overflow", func() {
runner("G109", testutils.SampleCodeG109)
})
It("should detect sql injection via format strings", func() { It("should detect sql injection via format strings", func() {
runner("G201", testutils.SampleCodeG201) runner("G201", testutils.SampleCodeG201)
}) })

View file

@ -109,12 +109,13 @@ func (p *TestPackage) CreateContext(filename string) *gosec.Context {
pkgFile = strings.TrimPrefix(pkgFile, strip) pkgFile = strings.TrimPrefix(pkgFile, strip)
if pkgFile == filename { if pkgFile == filename {
ctx := &gosec.Context{ ctx := &gosec.Context{
FileSet: pkg.Fset, FileSet: pkg.Fset,
Root: file, Root: file,
Config: gosec.NewConfig(), Config: gosec.NewConfig(),
Info: pkg.TypesInfo, Info: pkg.TypesInfo,
Pkg: pkg.Types, Pkg: pkg.Types,
Imports: gosec.NewImportTracker(), Imports: gosec.NewImportTracker(),
PassedValues: make(map[string]interface{}),
} }
ctx.Imports.TrackPackages(ctx.Pkg.Imports()...) ctx.Imports.TrackPackages(ctx.Pkg.Imports()...)
return ctx return ctx

View file

@ -522,6 +522,78 @@ func main() {
}) })
log.Fatal(http.ListenAndServe(":8080", nil)) log.Fatal(http.ListenAndServe(":8080", nil))
}`}, 0, gosec.NewConfig()}} }`}, 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 - SQL injection via format string
SampleCodeG201 = []CodeSample{ SampleCodeG201 = []CodeSample{
{[]string{` {[]string{`