From f08a5988217bf976e0b7b344aa3a696981349b98 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Fri, 12 Apr 2024 10:14:51 +0100 Subject: [PATCH 1/7] Add tests for FPs: type switches, type assertions --- .../CWE-681/IncorrectIntegerConversion.go | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) 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..2cf98b72c3ab 100644 --- a/go/ql/test/query-tests/Security/CWE-681/IncorrectIntegerConversion.go +++ b/go/ql/test/query-tests/Security/CWE-681/IncorrectIntegerConversion.go @@ -498,3 +498,55 @@ func dealWithArchSizeCorrectly(s string) uint { } 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)) // $ SPURIOUS: hasValueFlow="type assertion" + _ = int8(v.(int16)) // $ hasValueFlow="type assertion" + case int32: + _ = int32(v) // $ SPURIOUS: hasValueFlow="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)) // $ SPURIOUS: hasValueFlow="type assertion" + _ = int8(input.(int16)) // $ hasValueFlow="type assertion" + case int32: + _ = int32(input.(int32)) // $ SPURIOUS: hasValueFlow="type assertion" + _ = 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) // $ SPURIOUS: hasValueFlow="v" + _ = int8(v) // $ hasValueFlow="v" + } else if v, ok := input.(int32); ok { + _ = int16(v) // $ hasValueFlow="v" + } +} From 544660322f298106d66475b1a3293dd28c9d8068 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Tue, 16 Apr 2024 12:06:28 +0100 Subject: [PATCH 2/7] Refactor flow state transforming barriers --- .../IncorrectIntegerConversionLib.qll | 49 ++++++++++--------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll b/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll index df417397b138..6e541c994002 100644 --- a/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll +++ b/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll @@ -258,13 +258,13 @@ 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`. * * 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 +275,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,30 +385,15 @@ 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 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() - ) + override predicate barrierFor(int bitSize, int architectureBitSize) { + g.isBoundFor(bitSize, architectureBitSize) } } @@ -497,7 +495,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 +508,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, _) } } From 611f98bca49c829c63d621326d15fc014cb42183 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Tue, 16 Apr 2024 12:27:24 +0100 Subject: [PATCH 3/7] Make type assertions transform the flow state --- .../IncorrectIntegerConversionLib.qll | 33 +++++++++++++++++++ .../CWE-681/IncorrectIntegerConversion.go | 16 ++++----- 2 files changed, 41 insertions(+), 8 deletions(-) diff --git a/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll b/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll index 6e541c994002..103647c26a75 100644 --- a/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll +++ b/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll @@ -397,6 +397,39 @@ class UpperBoundCheck extends FlowStateTransformer { } } +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) + } +} + /** * Holds if `source` is the result of a call to `strconv.Atoi`, * `strconv.ParseInt`, or `strconv.ParseUint`, `bitSize` is the `bitSize` 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 2cf98b72c3ab..a6466af4b56b 100644 --- a/go/ql/test/query-tests/Security/CWE-681/IncorrectIntegerConversion.go +++ b/go/ql/test/query-tests/Security/CWE-681/IncorrectIntegerConversion.go @@ -507,8 +507,8 @@ func typeSwitch1(s string) { if _, ok := input.(string); ok { return } - _ = int16(v.(int16)) // $ SPURIOUS: hasValueFlow="type assertion" - _ = int8(v.(int16)) // $ hasValueFlow="type assertion" + _ = int16(v.(int16)) + _ = int8(v.(int16)) // $ hasValueFlow="type assertion" case int32: _ = int32(v) // $ SPURIOUS: hasValueFlow="v" _ = int8(v) // $ hasValueFlow="v" @@ -527,11 +527,11 @@ func typeSwitch2(s string) { if _, ok := input.(string); ok { return } - _ = int16(input.(int16)) // $ SPURIOUS: hasValueFlow="type assertion" - _ = int8(input.(int16)) // $ hasValueFlow="type assertion" + _ = int16(input.(int16)) + _ = int8(input.(int16)) // $ hasValueFlow="type assertion" case int32: - _ = int32(input.(int32)) // $ SPURIOUS: hasValueFlow="type assertion" - _ = int8(input.(int32)) // $ hasValueFlow="type assertion" + _ = int32(input.(int32)) + _ = int8(input.(int32)) // $ hasValueFlow="type assertion" case int64: _ = int8(input.(int64)) // $ hasValueFlow="type assertion" default: @@ -544,8 +544,8 @@ func checkedTypeAssertion(s string) { 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) // $ SPURIOUS: hasValueFlow="v" - _ = int8(v) // $ hasValueFlow="v" + _ = int16(v) + _ = int8(v) // $ hasValueFlow="v" } else if v, ok := input.(int32); ok { _ = int16(v) // $ hasValueFlow="v" } From 3ad2d90014e7d2e771e73e00d6bee8da6732a3b7 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Tue, 16 Apr 2024 12:32:35 +0100 Subject: [PATCH 4/7] Make type switches tranform flow state --- .../IncorrectIntegerConversionLib.qll | 20 +++++++++++++++++++ .../CWE-681/IncorrectIntegerConversion.go | 4 ++-- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll b/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll index 103647c26a75..3696b4475166 100644 --- a/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll +++ b/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll @@ -430,6 +430,26 @@ class TypeAssertionCheck extends DataFlow::ExprNode, FlowStateTransformer { } } +/** + * 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) + } +} + /** * Holds if `source` is the result of a call to `strconv.Atoi`, * `strconv.ParseInt`, or `strconv.ParseUint`, `bitSize` is the `bitSize` 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 a6466af4b56b..cb313765fa2a 100644 --- a/go/ql/test/query-tests/Security/CWE-681/IncorrectIntegerConversion.go +++ b/go/ql/test/query-tests/Security/CWE-681/IncorrectIntegerConversion.go @@ -510,8 +510,8 @@ func typeSwitch1(s string) { _ = int16(v.(int16)) _ = int8(v.(int16)) // $ hasValueFlow="type assertion" case int32: - _ = int32(v) // $ SPURIOUS: hasValueFlow="v" - _ = int8(v) // $ hasValueFlow="v" + _ = int32(v) + _ = int8(v) // $ hasValueFlow="v" case int64: _ = int8(v) // $ hasValueFlow="v" default: From 80c3993ddc32047958caf647af646c87cbd6a021 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Tue, 16 Apr 2024 12:44:03 +0100 Subject: [PATCH 5/7] Remove redundant test It was introduced in https://github.com/github/codeql-go/pull/718 in response to https://github.com/github/codeql-go/issues/717, to check that we don't have type assertions as sinks. We now have other tests covering type assertions. --- .../Security/CWE-681/IncorrectIntegerConversion.go | 10 ---------- 1 file changed, 10 deletions(-) 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 cb313765fa2a..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,16 +482,6 @@ 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) From 2f56ec7fe0cbfcfc08a3510fe39bf55c1811c12c Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Tue, 16 Apr 2024 22:16:43 +0100 Subject: [PATCH 6/7] Fix QLDoc --- go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll b/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll index 3696b4475166..a87d72b32f68 100644 --- a/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll +++ b/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll @@ -260,7 +260,8 @@ private class MaxValueState extends TMaxValueState { */ 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. */ From 212a0f27ff2ca6f608493a46673502bce420a9b5 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 17 Apr 2024 07:05:59 +0100 Subject: [PATCH 7/7] Add change note --- .../2024-04-17-incorrect-integer-conversion-barriers.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 go/ql/src/change-notes/2024-04-17-incorrect-integer-conversion-barriers.md 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.