diff --git a/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll b/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll index df417397b138..a87d72b32f68 100644 --- a/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll +++ b/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll @@ -258,13 +258,14 @@ private class MaxValueState extends TMaxValueState { * A node that blocks some flow states and transforms some others as they flow * through it. */ -abstract class BarrierFlowStateTransformer extends DataFlow::Node { +abstract class FlowStateTransformer extends DataFlow::Node { /** - * Holds if this should be a barrier for `flowstate`. + * Holds if this should be a barrier for a flow state with bit size `bitSize` + * and architecture bit size `architectureBitSize`. * * This includes flow states which are transformed into other flow states. */ - abstract predicate barrierFor(MaxValueState flowstate); + abstract predicate barrierFor(int bitSize, int architectureBitSize); /** * Gets the flow state that `flowstate` is transformed into. @@ -275,7 +276,20 @@ abstract class BarrierFlowStateTransformer extends DataFlow::Node { * transform(transform(x)) = transform(x) * ``` */ - abstract MaxValueState transform(MaxValueState flowstate); + MaxValueState transform(MaxValueState state) { + this.barrierFor(state.getBitSize(), state.getSinkBitSize()) and + result.getBitSize() = + max(int bitsize | + bitsize = validBitSize() and + bitsize < state.getBitSize() and + not this.barrierFor(bitsize, state.getSinkBitSize()) + ) and + ( + result.getArchitectureBitSize() = state.getArchitectureBitSize() + or + state.architectureBitSizeUnknown() and result.architectureBitSizeUnknown() + ) + } } private predicate upperBoundCheckGuard(DataFlow::Node g, Expr e, boolean branch) { @@ -372,31 +386,69 @@ class UpperBoundCheckGuard extends DataFlow::RelationalComparisonNode { * for all flow states and would not transform any flow states, thus * effectively blocking them. */ -class UpperBoundCheck extends BarrierFlowStateTransformer { +class UpperBoundCheck extends FlowStateTransformer { UpperBoundCheckGuard g; UpperBoundCheck() { this = DataFlow::BarrierGuard::getABarrierNodeForGuard(g) } - override predicate barrierFor(MaxValueState flowstate) { - g.isBoundFor(flowstate.getBitSize(), flowstate.getSinkBitSize()) + override predicate barrierFor(int bitSize, int architectureBitSize) { + g.isBoundFor(bitSize, architectureBitSize) } +} - override MaxValueState transform(MaxValueState state) { - this.barrierFor(state) and - result.getBitSize() = - max(int bitsize | - bitsize = validBitSize() and - bitsize < state.getBitSize() and - not g.isBoundFor(bitsize, state.getSinkBitSize()) - ) and - ( - result.getArchitectureBitSize() = state.getArchitectureBitSize() - or - state.architectureBitSizeUnknown() and result.architectureBitSizeUnknown() +private predicate integerTypeBound(IntegerType it, int bitSize, int architectureBitSize) { + bitSize = validBitSize() and + architectureBitSize = [32, 64] and + exists(int offset | if it instanceof SignedIntegerType then offset = 1 else offset = 0 | + if it instanceof IntType or it instanceof UintType + then bitSize >= architectureBitSize - offset + else bitSize >= it.getSize() - offset + ) +} + +/** + * An expression which a type assertion guarantees will have a particular + * integer type. + * + * If this is a checked type expression then this value will only be used if + * the type assertion succeeded. If it is not checked then there will be a + * run-time panic if the type assertion fails, so we can assume it succeeded. + */ +class TypeAssertionCheck extends DataFlow::ExprNode, FlowStateTransformer { + IntegerType it; + + TypeAssertionCheck() { + exists(TypeAssertExpr tae | + this = DataFlow::exprNode(tae.getExpr()) and + it = tae.getTypeExpr().getType() + ) + } + + override predicate barrierFor(int bitSize, int architectureBitSize) { + integerTypeBound(it, bitSize, architectureBitSize) + } +} + +/** + * The implicit definition of a variable with integer type for a case clause of + * a type switch statement which declares a variable in its guard, which has + * effectively had a checked type assertion. + */ +class TypeSwitchVarFlowStateTransformer extends DataFlow::SsaNode, FlowStateTransformer { + IntegerType it; + + TypeSwitchVarFlowStateTransformer() { + exists(IR::TypeSwitchImplicitVariableInstruction insn, LocalVariable lv | insn.writes(lv, _) | + this.getSourceVariable() = lv and + it = lv.getType() ) } + + override predicate barrierFor(int bitSize, int architectureBitSize) { + integerTypeBound(it, bitSize, architectureBitSize) + } } /** @@ -497,7 +549,10 @@ private module ConversionWithoutBoundsCheckConfig implements DataFlow::StateConf predicate isBarrier(DataFlow::Node node, FlowState state) { // Safely guarded by a barrier guard. - exists(BarrierFlowStateTransformer bfst | node = bfst and bfst.barrierFor(state)) + exists(FlowStateTransformer fst | + node = fst and + fst.barrierFor(state.getBitSize(), state.getSinkBitSize()) + ) or // When there is a flow from a source to a sink, do not allow the flow to // continue to a further sink. @@ -507,8 +562,8 @@ private module ConversionWithoutBoundsCheckConfig implements DataFlow::StateConf predicate isAdditionalFlowStep( DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2 ) { - // Create additional flow steps for `BarrierFlowStateTransformer`s - state2 = node2.(BarrierFlowStateTransformer).transform(state1) and + // Create additional flow steps for `FlowStateTransformer`s + state2 = node2.(FlowStateTransformer).transform(state1) and DataFlow::simpleLocalFlowStep(node1, node2, _) } } diff --git a/go/ql/src/change-notes/2024-04-17-incorrect-integer-conversion-barriers.md b/go/ql/src/change-notes/2024-04-17-incorrect-integer-conversion-barriers.md new file mode 100644 index 000000000000..7453f2bef5c5 --- /dev/null +++ b/go/ql/src/change-notes/2024-04-17-incorrect-integer-conversion-barriers.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Added some more barriers to flow for `go/incorrect-integer-conversion` to reduce false positives, especially around type switches. diff --git a/go/ql/test/query-tests/Security/CWE-681/IncorrectIntegerConversion.go b/go/ql/test/query-tests/Security/CWE-681/IncorrectIntegerConversion.go index 138058866e11..7927a5fe252c 100644 --- a/go/ql/test/query-tests/Security/CWE-681/IncorrectIntegerConversion.go +++ b/go/ql/test/query-tests/Security/CWE-681/IncorrectIntegerConversion.go @@ -482,19 +482,61 @@ func parsePositiveInt2(value string) (int, error) { return int(i64), nil } -func typeAssertion(s string) { - n, err := strconv.ParseInt(s, 10, 0) - if err == nil { - var itf interface{} = n - i32 := itf.(int32) - println(i32) - } - -} - func dealWithArchSizeCorrectly(s string) uint { if i, err := strconv.ParseUint(s, 10, 64); err == nil && i < math.MaxUint { return uint(i) } return 0 } + +func typeSwitch1(s string) { + i64, _ := strconv.ParseInt(s, 10, 64) + var input any = i64 + switch v := input.(type) { + case int16, string: + if _, ok := input.(string); ok { + return + } + _ = int16(v.(int16)) + _ = int8(v.(int16)) // $ hasValueFlow="type assertion" + case int32: + _ = int32(v) + _ = int8(v) // $ hasValueFlow="v" + case int64: + _ = int8(v) // $ hasValueFlow="v" + default: + _ = int8(v.(int64)) // $ hasValueFlow="type assertion" + } +} + +func typeSwitch2(s string) { + i64, _ := strconv.ParseInt(s, 10, 64) + var input any = i64 + switch input.(type) { + case int16, string: + if _, ok := input.(string); ok { + return + } + _ = int16(input.(int16)) + _ = int8(input.(int16)) // $ hasValueFlow="type assertion" + case int32: + _ = int32(input.(int32)) + _ = int8(input.(int32)) // $ hasValueFlow="type assertion" + case int64: + _ = int8(input.(int64)) // $ hasValueFlow="type assertion" + default: + _ = int8(input.(int64)) // $ hasValueFlow="type assertion" + } +} + +func checkedTypeAssertion(s string) { + i64, _ := strconv.ParseInt(s, 10, 64) + var input any = i64 + if v, ok := input.(int16); ok { + // Need to account for the fact that within this case clause, v is an int16 + _ = int16(v) + _ = int8(v) // $ hasValueFlow="v" + } else if v, ok := input.(int32); ok { + _ = int16(v) // $ hasValueFlow="v" + } +}