Modify rule for integer overflow to have more acurate results (#434)

Signed-off-by: Hiroki Suezawa <suezawa@gmail.com>
This commit is contained in:
Hiroki Suezawa 2020-01-21 18:13:12 +09:00 committed by Cosmin Cojocar
parent a4d7b3628b
commit 459e2d3e91
2 changed files with 25 additions and 13 deletions

View file

@ -30,32 +30,29 @@ func (i *integerOverflowCheck) ID() string {
} }
func (i *integerOverflowCheck) Match(node ast.Node, ctx *gosec.Context) (*gosec.Issue, error) { func (i *integerOverflowCheck) Match(node ast.Node, ctx *gosec.Context) (*gosec.Issue, error) {
var atoiVarName map[string]ast.Node var atoiVarObj map[*ast.Object]ast.Node
// To check multiple lines, ctx.PassedValues is used to store temporary data. // To check multiple lines, ctx.PassedValues is used to store temporary data.
if _, ok := ctx.PassedValues[i.ID()]; !ok { if _, ok := ctx.PassedValues[i.ID()]; !ok {
atoiVarName = make(map[string]ast.Node) atoiVarObj = make(map[*ast.Object]ast.Node)
ctx.PassedValues[i.ID()] = atoiVarName ctx.PassedValues[i.ID()] = atoiVarObj
} else if pv, ok := ctx.PassedValues[i.ID()].(map[string]ast.Node); ok { } else if pv, ok := ctx.PassedValues[i.ID()].(map[*ast.Object]ast.Node); ok {
atoiVarName = pv atoiVarObj = pv
} else { } else {
return nil, fmt.Errorf("PassedValues[%s] of Context is not map[string]ast.Node, but %T", i.ID(), ctx.PassedValues[i.ID()]) return nil, fmt.Errorf("PassedValues[%s] of Context is not map[*ast.Object]ast.Node, but %T", i.ID(), ctx.PassedValues[i.ID()])
} }
// strconv.Atoi is a common function. // strconv.Atoi is a common function.
// To reduce false positives, This rule detects code which is converted to int32/int16 only. // To reduce false positives, This rule detects code which is converted to int32/int16 only.
switch n := node.(type) { switch n := node.(type) {
case *ast.FuncDecl:
// Clear atoiVarName by function
ctx.PassedValues[i.ID()] = make(map[string]ast.Node)
case *ast.AssignStmt: case *ast.AssignStmt:
for _, expr := range n.Rhs { for _, expr := range n.Rhs {
if callExpr, ok := expr.(*ast.CallExpr); ok && i.calls.ContainsCallExpr(callExpr, ctx, false) != nil { 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 != "_" { if idt, ok := n.Lhs[0].(*ast.Ident); ok && idt.Name != "_" {
// Example: // Example:
// v, _ := strconv.Atoi("1111") // v, _ := strconv.Atoi("1111")
// Add "v" to atoiVarName map // Add v's Obj to atoiVarObj map
atoiVarName[idt.Name] = n atoiVarObj[idt.Obj] = n
} }
} }
} }
@ -63,7 +60,7 @@ func (i *integerOverflowCheck) Match(node ast.Node, ctx *gosec.Context) (*gosec.
if fun, ok := n.Fun.(*ast.Ident); ok { if fun, ok := n.Fun.(*ast.Ident); ok {
if fun.Name == "int32" || fun.Name == "int16" { if fun.Name == "int32" || fun.Name == "int16" {
if idt, ok := n.Args[0].(*ast.Ident); ok { if idt, ok := n.Args[0].(*ast.Ident); ok {
if n, ok := atoiVarName[idt.Name]; ok { if n, ok := atoiVarObj[idt.Obj]; ok {
// Detect int32(v) and int16(v) // Detect int32(v) and int16(v)
return gosec.NewIssue(ctx, n, i.ID(), i.What, i.Severity, i.Confidence), nil return gosec.NewIssue(ctx, n, i.ID(), i.What, i.Severity, i.Confidence), nil
} }

View file

@ -525,7 +525,6 @@ func main() {
// SampleCodeG109 - Potential Integer OverFlow // SampleCodeG109 - Potential Integer OverFlow
SampleCodeG109 = []CodeSample{ SampleCodeG109 = []CodeSample{
// Bind to all networks explicitly
{[]string{` {[]string{`
package main package main
@ -592,6 +591,22 @@ func test() {
bigValue := 30 bigValue := 30
value := int32(bigValue) value := int32(bigValue)
fmt.Println(value) fmt.Println(value)
}`}, 0, gosec.NewConfig()}, {[]string{`
package main
import (
"fmt"
"strconv"
)
func main() {
value := 10
if value == 10 {
value, _ := strconv.Atoi("2147483648")
fmt.Println(value)
}
v := int32(value)
fmt.Println(v)
}`}, 0, gosec.NewConfig()}} }`}, 0, gosec.NewConfig()}}
// SampleCodeG110 - potential DoS vulnerability via decompression bomb // SampleCodeG110 - potential DoS vulnerability via decompression bomb