From 4bf5667f6673c43d356235086ecfe41f5bb5ca7b Mon Sep 17 00:00:00 2001 From: Cosmin Cojocar Date: Mon, 27 May 2024 11:46:36 +0100 Subject: [PATCH] Add a new rule to detect integer overflow on integer types conversion Signed-off-by: Cosmin Cojocar --- analyzers/conversion_overflow.go | 134 +++++++++++++++++++++++++++++++ analyzers/util.go | 1 + rules/rules_test.go | 4 + testutils/g103_samples.go | 2 +- testutils/g109_samples.go | 10 +-- testutils/g115_samples.go | 118 +++++++++++++++++++++++++++ 6 files changed, 263 insertions(+), 6 deletions(-) create mode 100644 analyzers/conversion_overflow.go create mode 100644 testutils/g115_samples.go diff --git a/analyzers/conversion_overflow.go b/analyzers/conversion_overflow.go new file mode 100644 index 0000000..a8db9f5 --- /dev/null +++ b/analyzers/conversion_overflow.go @@ -0,0 +1,134 @@ +// (c) Copyright gosec's authors +// +// 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 analyzers + +import ( + "fmt" + "regexp" + "strconv" + + "github.com/securego/gosec/v2/issue" + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/buildssa" + "golang.org/x/tools/go/ssa" +) + +func newConversionOverflowAnalyzer(id string, description string) *analysis.Analyzer { + return &analysis.Analyzer{ + Name: id, + Doc: description, + Run: runConversionOverflow, + Requires: []*analysis.Analyzer{buildssa.Analyzer}, + } +} + +func runConversionOverflow(pass *analysis.Pass) (interface{}, error) { + ssaResult, err := getSSAResult(pass) + if err != nil { + return nil, fmt.Errorf("building ssa representation: %w", err) + } + + issues := []*issue.Issue{} + for _, mcall := range ssaResult.SSA.SrcFuncs { + for _, block := range mcall.DomPreorder() { + for _, instr := range block.Instrs { + switch instr := instr.(type) { + case *ssa.Convert: + src := instr.X.Type().String() + dst := instr.Type().String() + if isIntOverflow(src, dst) { + issue := newIssue(pass.Analyzer.Name, + fmt.Sprintf("integer overflow conversion %s -> %s", src, dst), + pass.Fset, + instr.Pos(), + issue.High, + issue.Medium, + ) + issues = append(issues, issue) + } + } + } + } + } + + if len(issues) > 0 { + return issues, nil + } + return nil, nil +} + +type integer struct { + signed bool + size int +} + +func parseIntType(intType string) (integer, error) { + re := regexp.MustCompile(`(?Pu?int)(?P\d{2})?`) + matches := re.FindStringSubmatch(intType) + if matches == nil { + return integer{}, fmt.Errorf("no integer type match found for %s", intType) + } + + it := matches[re.SubexpIndex("type")] + is := matches[re.SubexpIndex("size")] + + signed := false + if it == "int" { + signed = true + } + + // use default system int type in case size is not present in the type + intSize := strconv.IntSize + if is != "" { + var err error + intSize, err = strconv.Atoi(is) + if err != nil { + return integer{}, fmt.Errorf("failed to parse the integer type size: %w", err) + } + } + + return integer{signed: signed, size: intSize}, nil +} + +func isIntOverflow(src string, dst string) bool { + srcInt, err := parseIntType(src) + if err != nil { + return false + } + + dstInt, err := parseIntType(dst) + if err != nil { + return false + } + + // converting uint to int of the same size or smaller might lead to overflow + if !srcInt.signed && dstInt.signed && dstInt.size <= srcInt.size { + return true + } + // converting uint to unit of a smaller size might lead to overflow + if !srcInt.signed && !dstInt.signed && dstInt.size < srcInt.size { + return true + } + // converting int to int of a smaller size might lead to overflow + if srcInt.signed && dstInt.signed && dstInt.size < srcInt.size { + return true + } + // converting int to uint of a smaller size might lead to overflow + if srcInt.signed && !dstInt.signed && dstInt.size < srcInt.size && srcInt.size-dstInt.size > 8 { + return true + } + + return false +} diff --git a/analyzers/util.go b/analyzers/util.go index 5941184..49e49e6 100644 --- a/analyzers/util.go +++ b/analyzers/util.go @@ -38,6 +38,7 @@ type SSAAnalyzerResult struct { // BuildDefaultAnalyzers returns the default list of analyzers func BuildDefaultAnalyzers() []*analysis.Analyzer { return []*analysis.Analyzer{ + newConversionOverflowAnalyzer("G115", "Type conversion which leads to integer overflow"), newSliceBoundsAnalyzer("G602", "Possible slice bounds out of range"), } } diff --git a/rules/rules_test.go b/rules/rules_test.go index 074e5c4..aa2e2a4 100644 --- a/rules/rules_test.go +++ b/rules/rules_test.go @@ -111,6 +111,10 @@ var _ = Describe("gosec rules", func() { runner("G114", testutils.SampleCodeG114) }) + It("should detect integer conversion overflow", func() { + runner("G115", testutils.SampleCodeG115) + }) + It("should detect sql injection via format strings", func() { runner("G201", testutils.SampleCodeG201) }) diff --git a/testutils/g103_samples.go b/testutils/g103_samples.go index feeb6b6..d1cb38d 100644 --- a/testutils/g103_samples.go +++ b/testutils/g103_samples.go @@ -27,7 +27,7 @@ func main() { intPtr = (*int)(unsafe.Pointer(addressHolder)) fmt.Printf("\nintPtr=%p, *intPtr=%d.\n\n", intPtr, *intPtr) } -`}, 2, gosec.NewConfig()}, +`}, 3, gosec.NewConfig()}, {[]string{` package main diff --git a/testutils/g109_samples.go b/testutils/g109_samples.go index a374355..e05a2bd 100644 --- a/testutils/g109_samples.go +++ b/testutils/g109_samples.go @@ -20,7 +20,7 @@ func main() { value := int32(bigValue) fmt.Println(value) } -`}, 1, gosec.NewConfig()}, +`}, 2, gosec.NewConfig()}, {[]string{` package main @@ -38,7 +38,7 @@ func main() { fmt.Println(bigValue) } } -`}, 1, gosec.NewConfig()}, +`}, 2, gosec.NewConfig()}, {[]string{` package main @@ -74,7 +74,7 @@ func main() { func test() { bigValue := 30 - value := int32(bigValue) + value := int64(bigValue) fmt.Println(value) } `}, 0, gosec.NewConfig()}, @@ -92,7 +92,7 @@ func main() { value, _ := strconv.Atoi("2147483648") fmt.Println(value) } - v := int32(value) + v := int64(value) fmt.Println(v) } `}, 0, gosec.NewConfig()}, @@ -105,7 +105,7 @@ import ( ) func main() { a, err := strconv.Atoi("a") - b := int32(a) //#nosec G109 + b := int64(a) //#nosec G109 fmt.Println(b, err) } `}, 0, gosec.NewConfig()}, diff --git a/testutils/g115_samples.go b/testutils/g115_samples.go new file mode 100644 index 0000000..4e075c6 --- /dev/null +++ b/testutils/g115_samples.go @@ -0,0 +1,118 @@ +package testutils + +import "github.com/securego/gosec/v2" + +var SampleCodeG115 = []CodeSample{ + {[]string{` +package main + +import ( + "fmt" + "math" +) + +func main() { + var a uint32 = math.MaxUint32 + b := int32(a) + fmt.Println(b) +} + `}, 1, gosec.NewConfig()}, + {[]string{` +package main + +import ( + "fmt" + "math" +) + +func main() { + var a uint16 = math.MaxUint16 + b := int32(a) + fmt.Println(b) +} + `}, 0, gosec.NewConfig()}, + {[]string{` +package main + +import ( + "fmt" + "math" +) + +func main() { + var a uint32 = math.MaxUint32 + b := uint16(a) + fmt.Println(b) +} + `}, 1, gosec.NewConfig()}, + {[]string{` +package main + +import ( + "fmt" + "math" +) + +func main() { + var a int32 = math.MaxInt32 + b := int16(a) + fmt.Println(b) +} + `}, 1, gosec.NewConfig()}, + {[]string{` +package main + +import ( + "fmt" + "math" +) + +func main() { + var a int16 = math.MaxInt16 + b := int32(a) + fmt.Println(b) +} + `}, 0, gosec.NewConfig()}, + {[]string{` +package main + +import ( + "fmt" + "math" +) + +func main() { + var a int32 = math.MaxInt32 + b := uint32(a) + fmt.Println(b) +} + `}, 0, gosec.NewConfig()}, + {[]string{` +package main + +import ( + "fmt" + "math" +) + +func main() { + var a uint = math.MaxUint + b := int16(a) + fmt.Println(b) +} + `}, 1, gosec.NewConfig()}, + {[]string{` +package main + +import ( + "fmt" + "math" +) + +func main() { + var a uint = math.MaxUint + b := int64(a) + fmt.Println(b) +} + `}, 1, gosec.NewConfig()}, +}