mirror of
https://github.com/securego/gosec.git
synced 2025-07-03 00:27:05 +02:00
Improvement the int conversion overflow logic to handle bound checks (#1194)
* add test cases Signed-off-by: czechbol <adamludes@gmail.com> * fix bounds check logic Signed-off-by: czechbol <adamludes@gmail.com> * tweak test cases Signed-off-by: czechbol <adamludes@gmail.com> * fix codestyle Signed-off-by: czechbol <adamludes@gmail.com> * improve bounds check logic Signed-off-by: czechbol <adamludes@gmail.com> * max recursion depth Signed-off-by: czechbol <adamludes@gmail.com> * add test case for len function Signed-off-by: czechbol <adamludes@gmail.com> * relax len function bounds checks Co-authored-by: Ben Krieger <ben.krieger@intel.com> * handle cases when convert instruction is after the if blocks Signed-off-by: czechbol <adamludes@gmail.com> * improve range check discovery, add tests Signed-off-by: czechbol <adamludes@gmail.com> * refactor for readability Signed-off-by: czechbol <adamludes@gmail.com> * add cap function test Signed-off-by: czechbol <adamludes@gmail.com> * calculate signed min without throwing overflow warnings Signed-off-by: czechbol <adamludes@gmail.com> * perform bounds checks int size calculations Signed-off-by: czechbol <adamludes@gmail.com> * basic equal operator logic Signed-off-by: czechbol <adamludes@gmail.com> * uintptr -> unsafe.Pointer test case Signed-off-by: czechbol <adamludes@gmail.com> * fix review comments Signed-off-by: czechbol <adamludes@gmail.com> * Rebase and fix go module Change-Id: I8da6495eaaf25b1739389aa98492bd7df338085b Signed-off-by: Cosmin Cojocar <ccojocar@google.com> * fix false positive for negated value Signed-off-by: czechbol <adamludes@gmail.com> * fix range conditions Signed-off-by: czechbol <adamludes@gmail.com> * Ignore the golangci/gosec G115 warning Change-Id: I0db56cb0a5f9ab6e815e2480ec0b66d7061b23d3 Signed-off-by: Cosmin Cojocar <ccojocar@google.com> --------- Signed-off-by: czechbol <adamludes@gmail.com> Signed-off-by: Cosmin Cojocar <ccojocar@google.com> Co-authored-by: Ben Krieger <ben.krieger@intel.com> Co-authored-by: Cosmin Cojocar <ccojocar@google.com>
This commit is contained in:
czechbol
GitHub
@ -17,9 +17,12 @@ package analyzers
|
||||
import (
|
||||
"fmt"
|
||||
"go/token"
|
||||
"math"
|
||||
"regexp"
|
||||
"strconv"
|
||||
"strings"
|
||||
|
||||
"golang.org/x/exp/constraints"
|
||||
"golang.org/x/tools/go/analysis"
|
||||
"golang.org/x/tools/go/analysis/passes/buildssa"
|
||||
"golang.org/x/tools/go/ssa"
|
||||
@ -27,6 +30,30 @@ import (
|
||||
"github.com/securego/gosec/v2/issue"
|
||||
)
|
||||
|
||||
type integer struct {
|
||||
signed bool
|
||||
size int
|
||||
min int
|
||||
max uint
|
||||
}
|
||||
|
||||
type rangeResult struct {
|
||||
minValue int
|
||||
maxValue uint
|
||||
explixitPositiveVals []uint
|
||||
explicitNegativeVals []int
|
||||
isRangeCheck bool
|
||||
convertFound bool
|
||||
}
|
||||
|
||||
type branchResults struct {
|
||||
minValue *int
|
||||
maxValue *uint
|
||||
explixitPositiveVals []uint
|
||||
explicitNegativeVals []int
|
||||
convertFound bool
|
||||
}
|
||||
|
||||
func newConversionOverflowAnalyzer(id string, description string) *analysis.Analyzer {
|
||||
return &analysis.Analyzer{
|
||||
Name: id,
|
||||
@ -50,10 +77,10 @@ func runConversionOverflow(pass *analysis.Pass) (interface{}, error) {
|
||||
case *ssa.Convert:
|
||||
src := instr.X.Type().Underlying().String()
|
||||
dst := instr.Type().Underlying().String()
|
||||
if isSafeConversion(instr) {
|
||||
continue
|
||||
}
|
||||
if isIntOverflow(src, dst) {
|
||||
if isSafeConversion(instr) {
|
||||
continue
|
||||
}
|
||||
issue := newIssue(pass.Analyzer.Name,
|
||||
fmt.Sprintf("integer overflow conversion %s -> %s", src, dst),
|
||||
pass.Fset,
|
||||
@ -74,23 +101,90 @@ func runConversionOverflow(pass *analysis.Pass) (interface{}, error) {
|
||||
return nil, 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
|
||||
}
|
||||
|
||||
return srcInt.min < dstInt.min || srcInt.max > dstInt.max
|
||||
}
|
||||
|
||||
func parseIntType(intType string) (integer, error) {
|
||||
re := regexp.MustCompile(`^(?P<type>u?int)(?P<size>\d{1,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 := it == "int"
|
||||
|
||||
// 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)
|
||||
}
|
||||
}
|
||||
|
||||
if intSize != 8 && intSize != 16 && intSize != 32 && intSize != 64 && is != "" {
|
||||
return integer{}, fmt.Errorf("invalid bit size: %d", intSize)
|
||||
}
|
||||
|
||||
var min int
|
||||
var max uint
|
||||
|
||||
if signed {
|
||||
shiftAmount := intSize - 1
|
||||
|
||||
// Perform a bounds check.
|
||||
if shiftAmount < 0 {
|
||||
return integer{}, fmt.Errorf("invalid shift amount: %d", shiftAmount)
|
||||
}
|
||||
|
||||
max = (1 << uint(shiftAmount)) - 1
|
||||
min = -1 << (intSize - 1)
|
||||
|
||||
} else {
|
||||
max = (1 << uint(intSize)) - 1
|
||||
min = 0
|
||||
}
|
||||
|
||||
return integer{
|
||||
signed: signed,
|
||||
size: intSize,
|
||||
min: min,
|
||||
max: max,
|
||||
}, nil
|
||||
}
|
||||
|
||||
func isSafeConversion(instr *ssa.Convert) bool {
|
||||
dstType := instr.Type().Underlying().String()
|
||||
|
||||
// Check for constant conversions
|
||||
// Check for constant conversions.
|
||||
if constVal, ok := instr.X.(*ssa.Const); ok {
|
||||
if isConstantInRange(constVal, dstType) {
|
||||
return true
|
||||
}
|
||||
}
|
||||
|
||||
// Check for explicit range checks
|
||||
if hasExplicitRangeCheck(instr) {
|
||||
// Check for string to integer conversions with specified bit size.
|
||||
if isStringToIntConversion(instr, dstType) {
|
||||
return true
|
||||
}
|
||||
|
||||
// Check for string to integer conversions with specified bit size
|
||||
if isStringToIntConversion(instr, dstType) {
|
||||
// Check for explicit range checks.
|
||||
if hasExplicitRangeCheck(instr, dstType) {
|
||||
return true
|
||||
}
|
||||
|
||||
@ -114,22 +208,8 @@ func isConstantInRange(constVal *ssa.Const, dstType string) bool {
|
||||
return value >= 0 && value <= (1<<dstInt.size)-1
|
||||
}
|
||||
|
||||
func hasExplicitRangeCheck(instr *ssa.Convert) bool {
|
||||
block := instr.Block()
|
||||
for _, i := range block.Instrs {
|
||||
if binOp, ok := i.(*ssa.BinOp); ok {
|
||||
// Check if either operand of the BinOp is the result of the Convert instruction
|
||||
if (binOp.X == instr || binOp.Y == instr) &&
|
||||
(binOp.Op == token.LSS || binOp.Op == token.LEQ || binOp.Op == token.GTR || binOp.Op == token.GEQ) {
|
||||
return true
|
||||
}
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
func isStringToIntConversion(instr *ssa.Convert, dstType string) bool {
|
||||
// Traverse the SSA instructions to find the original variable
|
||||
// Traverse the SSA instructions to find the original variable.
|
||||
original := instr.X
|
||||
for {
|
||||
switch v := original.(type) {
|
||||
@ -162,66 +242,285 @@ func isStringToIntConversion(instr *ssa.Convert, dstType string) bool {
|
||||
}
|
||||
}
|
||||
|
||||
type integer struct {
|
||||
signed bool
|
||||
size int
|
||||
func hasExplicitRangeCheck(instr *ssa.Convert, dstType string) bool {
|
||||
dstInt, err := parseIntType(dstType)
|
||||
if err != nil {
|
||||
return false
|
||||
}
|
||||
|
||||
srcInt, err := parseIntType(instr.X.Type().String())
|
||||
if err != nil {
|
||||
return false
|
||||
}
|
||||
|
||||
minValue := srcInt.min
|
||||
maxValue := srcInt.max
|
||||
explicitPositiveVals := []uint{}
|
||||
explicitNegativeVals := []int{}
|
||||
|
||||
if minValue > dstInt.min && maxValue < dstInt.max {
|
||||
return true
|
||||
}
|
||||
|
||||
visitedIfs := make(map[*ssa.If]bool)
|
||||
for _, block := range instr.Parent().Blocks {
|
||||
for _, blockInstr := range block.Instrs {
|
||||
switch v := blockInstr.(type) {
|
||||
case *ssa.If:
|
||||
result := getResultRange(v, instr, visitedIfs)
|
||||
if result.isRangeCheck {
|
||||
minValue = max(minValue, &result.minValue)
|
||||
maxValue = min(maxValue, &result.maxValue)
|
||||
explicitPositiveVals = append(explicitPositiveVals, result.explixitPositiveVals...)
|
||||
explicitNegativeVals = append(explicitNegativeVals, result.explicitNegativeVals...)
|
||||
}
|
||||
case *ssa.Call:
|
||||
// These function return an int of a guaranteed size.
|
||||
if v != instr.X {
|
||||
continue
|
||||
}
|
||||
if fn, isBuiltin := v.Call.Value.(*ssa.Builtin); isBuiltin {
|
||||
switch fn.Name() {
|
||||
case "len", "cap":
|
||||
minValue = 0
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if explicitValsInRange(explicitPositiveVals, explicitNegativeVals, dstInt) {
|
||||
return true
|
||||
} else if minValue >= dstInt.min && maxValue <= dstInt.max {
|
||||
return true
|
||||
}
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
func parseIntType(intType string) (integer, error) {
|
||||
re := regexp.MustCompile(`(?P<type>u?int)(?P<size>\d{1,2})?`)
|
||||
matches := re.FindStringSubmatch(intType)
|
||||
if matches == nil {
|
||||
return integer{}, fmt.Errorf("no integer type match found for %s", intType)
|
||||
// getResultRange is a recursive function that walks the branches of the if statement to find the range of the variable.
|
||||
func getResultRange(ifInstr *ssa.If, instr *ssa.Convert, visitedIfs map[*ssa.If]bool) rangeResult {
|
||||
if visitedIfs[ifInstr] {
|
||||
return rangeResult{minValue: math.MinInt, maxValue: math.MaxUint}
|
||||
}
|
||||
visitedIfs[ifInstr] = true
|
||||
|
||||
cond := ifInstr.Cond
|
||||
binOp, ok := cond.(*ssa.BinOp)
|
||||
if !ok || !isRangeCheck(binOp, instr.X) {
|
||||
return rangeResult{minValue: math.MinInt, maxValue: math.MaxUint}
|
||||
}
|
||||
|
||||
it := matches[re.SubexpIndex("type")]
|
||||
is := matches[re.SubexpIndex("size")]
|
||||
|
||||
signed := false
|
||||
if it == "int" {
|
||||
signed = true
|
||||
result := rangeResult{
|
||||
minValue: math.MinInt,
|
||||
maxValue: math.MaxUint,
|
||||
isRangeCheck: 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)
|
||||
thenBounds := walkBranchForConvert(ifInstr.Block().Succs[0], instr, visitedIfs)
|
||||
elseBounds := walkBranchForConvert(ifInstr.Block().Succs[1], instr, visitedIfs)
|
||||
|
||||
updateResultFromBinOp(&result, binOp, instr, thenBounds.convertFound)
|
||||
|
||||
if thenBounds.convertFound {
|
||||
result.convertFound = true
|
||||
result.minValue = max(result.minValue, thenBounds.minValue)
|
||||
result.maxValue = min(result.maxValue, thenBounds.maxValue)
|
||||
result.explixitPositiveVals = append(result.explixitPositiveVals, thenBounds.explixitPositiveVals...)
|
||||
result.explicitNegativeVals = append(result.explicitNegativeVals, thenBounds.explicitNegativeVals...)
|
||||
} else if elseBounds.convertFound {
|
||||
result.convertFound = true
|
||||
result.minValue = max(result.minValue, elseBounds.minValue)
|
||||
result.maxValue = min(result.maxValue, elseBounds.maxValue)
|
||||
result.explixitPositiveVals = append(result.explixitPositiveVals, elseBounds.explixitPositiveVals...)
|
||||
result.explicitNegativeVals = append(result.explicitNegativeVals, elseBounds.explicitNegativeVals...)
|
||||
}
|
||||
|
||||
return result
|
||||
}
|
||||
|
||||
// updateResultFromBinOp updates the rangeResult based on the BinOp instruction and the location of the Convert instruction.
|
||||
func updateResultFromBinOp(result *rangeResult, binOp *ssa.BinOp, instr *ssa.Convert, successPathConvert bool) {
|
||||
x, y := binOp.X, binOp.Y
|
||||
operandsFlipped := false
|
||||
|
||||
compareVal, op := getRealValueFromOperation(instr.X)
|
||||
if x != compareVal {
|
||||
y, operandsFlipped = x, true
|
||||
}
|
||||
|
||||
constVal, ok := y.(*ssa.Const)
|
||||
if !ok {
|
||||
return
|
||||
}
|
||||
|
||||
switch binOp.Op {
|
||||
case token.LEQ, token.LSS:
|
||||
updateMinMaxForLessOrEqual(result, constVal, binOp.Op, operandsFlipped, successPathConvert)
|
||||
case token.GEQ, token.GTR:
|
||||
updateMinMaxForGreaterOrEqual(result, constVal, binOp.Op, operandsFlipped, successPathConvert)
|
||||
case token.EQL:
|
||||
if !successPathConvert {
|
||||
break
|
||||
}
|
||||
|
||||
// Determine if the constant value is positive or negative.
|
||||
if strings.Contains(constVal.String(), "-") {
|
||||
result.explicitNegativeVals = append(result.explicitNegativeVals, int(constVal.Int64()))
|
||||
} else {
|
||||
result.explixitPositiveVals = append(result.explixitPositiveVals, uint(constVal.Uint64()))
|
||||
}
|
||||
|
||||
case token.NEQ:
|
||||
if successPathConvert {
|
||||
break
|
||||
}
|
||||
|
||||
// Determine if the constant value is positive or negative.
|
||||
if strings.Contains(constVal.String(), "-") {
|
||||
result.explicitNegativeVals = append(result.explicitNegativeVals, int(constVal.Int64()))
|
||||
} else {
|
||||
result.explixitPositiveVals = append(result.explixitPositiveVals, uint(constVal.Uint64()))
|
||||
}
|
||||
}
|
||||
|
||||
return integer{signed: signed, size: intSize}, nil
|
||||
if op == "neg" {
|
||||
min := result.minValue
|
||||
max := result.maxValue
|
||||
|
||||
if min >= 0 {
|
||||
result.maxValue = uint(min)
|
||||
}
|
||||
if max <= math.MaxInt {
|
||||
result.minValue = int(max) //nolint:gosec
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func isIntOverflow(src string, dst string) bool {
|
||||
srcInt, err := parseIntType(src)
|
||||
if err != nil {
|
||||
return false
|
||||
func updateMinMaxForLessOrEqual(result *rangeResult, constVal *ssa.Const, op token.Token, operandsFlipped bool, successPathConvert bool) {
|
||||
// If the success path has a conversion and the operands are not flipped, then the constant value is the maximum value.
|
||||
if successPathConvert && !operandsFlipped {
|
||||
result.maxValue = uint(constVal.Uint64())
|
||||
if op == token.LEQ {
|
||||
result.maxValue--
|
||||
}
|
||||
} else {
|
||||
result.minValue = int(constVal.Int64())
|
||||
if op == token.GTR {
|
||||
result.minValue++
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func updateMinMaxForGreaterOrEqual(result *rangeResult, constVal *ssa.Const, op token.Token, operandsFlipped bool, successPathConvert bool) {
|
||||
// If the success path has a conversion and the operands are not flipped, then the constant value is the minimum value.
|
||||
if successPathConvert && !operandsFlipped {
|
||||
result.minValue = int(constVal.Int64())
|
||||
if op == token.GEQ {
|
||||
result.minValue++
|
||||
}
|
||||
} else {
|
||||
result.maxValue = uint(constVal.Uint64())
|
||||
if op == token.LSS {
|
||||
result.maxValue--
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// walkBranchForConvert walks the branch of the if statement to find the range of the variable and where the conversion is.
|
||||
func walkBranchForConvert(block *ssa.BasicBlock, instr *ssa.Convert, visitedIfs map[*ssa.If]bool) branchResults {
|
||||
bounds := branchResults{}
|
||||
|
||||
for _, blockInstr := range block.Instrs {
|
||||
switch v := blockInstr.(type) {
|
||||
case *ssa.If:
|
||||
result := getResultRange(v, instr, visitedIfs)
|
||||
bounds.convertFound = bounds.convertFound || result.convertFound
|
||||
|
||||
if result.isRangeCheck {
|
||||
bounds.minValue = toPtr(max(result.minValue, bounds.minValue))
|
||||
bounds.maxValue = toPtr(min(result.maxValue, bounds.maxValue))
|
||||
}
|
||||
case *ssa.Call:
|
||||
if v == instr.X {
|
||||
if fn, isBuiltin := v.Call.Value.(*ssa.Builtin); isBuiltin && (fn.Name() == "len" || fn.Name() == "cap") {
|
||||
bounds.minValue = toPtr(0)
|
||||
}
|
||||
}
|
||||
case *ssa.Convert:
|
||||
if v == instr {
|
||||
bounds.convertFound = true
|
||||
return bounds
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
dstInt, err := parseIntType(dst)
|
||||
if err != nil {
|
||||
return false
|
||||
}
|
||||
return bounds
|
||||
}
|
||||
|
||||
// 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
|
||||
}
|
||||
func isRangeCheck(v ssa.Value, x ssa.Value) bool {
|
||||
compareVal, _ := getRealValueFromOperation(x)
|
||||
|
||||
switch op := v.(type) {
|
||||
case *ssa.BinOp:
|
||||
switch op.Op {
|
||||
case token.LSS, token.LEQ, token.GTR, token.GEQ,
|
||||
token.EQL, token.NEQ:
|
||||
return op.X == compareVal || op.Y == compareVal
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
func getRealValueFromOperation(v ssa.Value) (ssa.Value, string) {
|
||||
switch v := v.(type) {
|
||||
case *ssa.UnOp:
|
||||
if v.Op == token.SUB {
|
||||
return v.X, "neg"
|
||||
}
|
||||
}
|
||||
return v, ""
|
||||
}
|
||||
|
||||
func explicitValsInRange(explicitPosVals []uint, explicitNegVals []int, dstInt integer) bool {
|
||||
if len(explicitPosVals) == 0 && len(explicitNegVals) == 0 {
|
||||
return false
|
||||
}
|
||||
|
||||
for _, val := range explicitPosVals {
|
||||
if val > dstInt.max {
|
||||
return false
|
||||
}
|
||||
}
|
||||
|
||||
for _, val := range explicitNegVals {
|
||||
if val < dstInt.min {
|
||||
return false
|
||||
}
|
||||
}
|
||||
|
||||
return true
|
||||
}
|
||||
|
||||
func min[T constraints.Integer](a T, b *T) T {
|
||||
if b == nil {
|
||||
return a
|
||||
}
|
||||
if a < *b {
|
||||
return a
|
||||
}
|
||||
return *b
|
||||
}
|
||||
|
||||
func max[T constraints.Integer](a T, b *T) T {
|
||||
if b == nil {
|
||||
return a
|
||||
}
|
||||
if a > *b {
|
||||
return a
|
||||
}
|
||||
return *b
|
||||
}
|
||||
|
||||
func toPtr[T any](a T) *T {
|
||||
return &a
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user