diff --git a/ql/src/Security/CWE-681/IncorrectIntegerConversion.ql b/ql/src/Security/CWE-681/IncorrectIntegerConversion.ql index 1be03e9ec93..5e83d19b94a 100644 --- a/ql/src/Security/CWE-681/IncorrectIntegerConversion.ql +++ b/ql/src/Security/CWE-681/IncorrectIntegerConversion.ql @@ -125,7 +125,7 @@ class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration { * a common pattern to serialise `byte(v)`, `byte(v >> 8)`, and so on. */ predicate isSink(DataFlow::TypeCastNode sink, int bitSize) { - exists(IntegerType integerType | sink.getType().getUnderlyingType() = integerType | + exists(IntegerType integerType | sink.getResultType().getUnderlyingType() = integerType | bitSize = integerType.getSize() or not exists(integerType.getSize()) and diff --git a/ql/src/experimental/Unsafe/WrongUsageOfUnsafe.ql b/ql/src/experimental/Unsafe/WrongUsageOfUnsafe.ql index d047817e04d..cafc719dd7a 100644 --- a/ql/src/experimental/Unsafe/WrongUsageOfUnsafe.ql +++ b/ql/src/experimental/Unsafe/WrongUsageOfUnsafe.ql @@ -34,7 +34,7 @@ Type getBaseType(Type typ) { /* A conversion to a `unsafe.Pointer` */ class ConversionToUnsafePointer extends DataFlow::TypeCastNode { - ConversionToUnsafePointer() { getFinalType(getType()) instanceof UnsafePointerType } + ConversionToUnsafePointer() { getFinalType(getResultType()) instanceof UnsafePointerType } } /* Type casting from a `unsafe.Pointer`.*/ @@ -68,7 +68,7 @@ predicate castShortArrayToLongerArray( cfg.hasFlowPath(source, sink) and cfg.isSource(source.getNode(), castLittle) and cfg.isSink(sink.getNode(), castBig) and - arrTo = getFinalType(castBig.getType()) and + arrTo = getFinalType(castBig.getResultType()) and ( // Array (whole) to array: // The `unsafe.Pointer` expression is on the array @@ -113,7 +113,7 @@ predicate castTypeToArray(DataFlow::PathNode source, DataFlow::PathNode sink, st cfg.hasFlowPath(source, sink) and cfg.isSource(source.getNode(), castLittle) and cfg.isSink(sink.getNode(), castBig) and - arrTo = getFinalType(castBig.getType()) and + arrTo = getFinalType(castBig.getResultType()) and not typeFrom.getUnderlyingType() instanceof ArrayType and not typeFrom instanceof PointerType and not castLittle @@ -143,7 +143,7 @@ predicate castDifferentBitSizeNumbers( cfg.hasFlowPath(source, sink) and cfg.isSource(source.getNode(), castLittle) and cfg.isSink(sink.getNode(), castBig) and - numTo = getFinalType(castBig.getType()) and + numTo = getFinalType(castBig.getResultType()) and numFrom = getFinalType(castLittle.getOperand().getType()) and // TODO: also consider cast from uint to int? getNumericTypeSize(numTo) != getNumericTypeSize(numFrom) and diff --git a/ql/src/semmle/go/controlflow/ControlFlowGraphImpl.qll b/ql/src/semmle/go/controlflow/ControlFlowGraphImpl.qll index 7c298c106d5..a512400f401 100644 --- a/ql/src/semmle/go/controlflow/ControlFlowGraphImpl.qll +++ b/ql/src/semmle/go/controlflow/ControlFlowGraphImpl.qll @@ -90,7 +90,7 @@ newtype TControlFlowNode = */ MkCompoundAssignRhsNode(CompoundAssignStmt assgn) or /** - * A control-flow node that represents the `i`th component of a tuple expression `base`. + * A control-flow node that represents the `i`th component of a tuple expression `s`. */ MkExtractNode(AstNode s, int i) { // in an assignment `x, y, z = tuple` diff --git a/ql/src/semmle/go/controlflow/IR.qll b/ql/src/semmle/go/controlflow/IR.qll index bd1d75a1a8b..1b1e7df9425 100644 --- a/ql/src/semmle/go/controlflow/IR.qll +++ b/ql/src/semmle/go/controlflow/IR.qll @@ -628,7 +628,8 @@ module IR { /** * An instruction selecting one of multiple values returned by a function, or either the key - * or the value of the iterator in a range loop. + * or the value of the iterator in a range loop, or the result or success value from a type + * assertion. */ class ExtractTupleElementInstruction extends Instruction, MkExtractNode { AstNode s; @@ -660,6 +661,10 @@ module IR { result = c.getTarget().getResultType(i) ) or + exists(TypeAssertExpr tae | getBase() = evalExprInstruction(tae) | + result = tae.getType().(TupleType).getComponentType(i) + ) + or exists(Type rangeType | rangeType = s.(RangeStmt).getDomain().getType().getUnderlyingType() | exists(Type baseType | baseType = rangeType.(ArrayType).getElementType() or diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll index 034e0f60910..21867a513c3 100644 --- a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll +++ b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll @@ -902,6 +902,16 @@ class TypeCastNode extends ExprNode { expr instanceof ConversionExpr } + /** + * Gets the type being converted to. Note this differs from `this.getType()` for + * `TypeAssertExpr`s that return a (result, ok) tuple. + */ + Type getResultType() { + if this.getType() instanceof TupleType + then result = this.getType().(TupleType).getComponentType(0) + else result = this.getType() + } + /** Gets the operand of the type cast. */ DataFlow::Node getOperand() { result.asExpr() = expr.(TypeAssertExpr).getExpr() @@ -969,7 +979,7 @@ SsaNode ssaNode(SsaVariable v) { result.getDefinition() = v.getDefinition() } /** * Gets the data-flow node corresponding to the `i`th element of tuple `t` (which is either a call - * with multiple results or an iterator in a range loop). + * with multiple results, an iterator in a range loop, or the result of a type assertion). */ Node extractTupleElement(Node t, int i) { exists(IR::Instruction insn | t = instructionNode(insn) | @@ -1005,13 +1015,23 @@ private predicate basicLocalFlowStep(Node nodeFrom, Node nodeTo) { // Instruction -> Instruction exists(Expr pred, Expr succ | succ.(LogicalBinaryExpr).getAnOperand() = pred or - succ.(ConversionExpr).getOperand() = pred or - succ.(TypeAssertExpr).getExpr() = pred + succ.(ConversionExpr).getOperand() = pred | nodeFrom = exprNode(pred) and nodeTo = exprNode(succ) ) or + // Type assertion: if in the context `checked, ok := e.(*Type)` (in which + // case tuple-extraction instructions exist), flow from `e` to `e.(*Type)[0]`; + // otherwise flow from `e` to `e.(*Type)`. + exists(IR::Instruction evalAssert, TypeAssertExpr assert | + nodeFrom.asExpr() = assert.getExpr() and + evalAssert = IR::evalExprInstruction(assert) and + if exists(IR::extractTupleElement(evalAssert, _)) + then nodeTo.asInstruction() = IR::extractTupleElement(evalAssert, 0) + else nodeTo.asInstruction() = evalAssert + ) + or // Instruction -> SSA exists(IR::Instruction pred, SsaExplicitDefinition succ | succ.getRhs() = pred and diff --git a/ql/test/library-tests/semmle/go/dataflow/FlowSteps/LocalFlowStep.expected b/ql/test/library-tests/semmle/go/dataflow/FlowSteps/LocalFlowStep.expected index e18b11179c3..97197b9975c 100644 --- a/ql/test/library-tests/semmle/go/dataflow/FlowSteps/LocalFlowStep.expected +++ b/ql/test/library-tests/semmle/go/dataflow/FlowSteps/LocalFlowStep.expected @@ -76,7 +76,7 @@ | main.go:26:2:26:17 | ... := ...[0] | main.go:26:2:26:2 | definition of n | | main.go:26:2:26:17 | ... := ...[1] | main.go:26:5:26:6 | definition of ok | | main.go:26:5:26:6 | definition of ok | main.go:27:5:27:6 | ok | -| main.go:26:11:26:11 | x | main.go:26:11:26:17 | type assertion | +| main.go:26:11:26:11 | x | main.go:26:2:26:17 | ... := ...[0] | | main.go:38:2:38:2 | definition of s | main.go:39:15:39:15 | s | | main.go:38:2:38:2 | definition of s | main.go:40:15:40:15 | s | | main.go:38:2:38:2 | definition of s | main.go:42:7:42:7 | s | diff --git a/ql/test/library-tests/semmle/go/dataflow/TypeAssertions/DataFlow.expected b/ql/test/library-tests/semmle/go/dataflow/TypeAssertions/DataFlow.expected new file mode 100644 index 00000000000..e69de29bb2d diff --git a/ql/test/library-tests/semmle/go/dataflow/TypeAssertions/DataFlow.ql b/ql/test/library-tests/semmle/go/dataflow/TypeAssertions/DataFlow.ql new file mode 100644 index 00000000000..a706f731134 --- /dev/null +++ b/ql/test/library-tests/semmle/go/dataflow/TypeAssertions/DataFlow.ql @@ -0,0 +1,29 @@ +import go +import TestUtilities.InlineExpectationsTest + +class Configuration extends DataFlow::Configuration { + Configuration() { this = "test-configuration" } + + override predicate isSource(DataFlow::Node source) { + source = any(DataFlow::CallNode c | c.getCalleeName() = "src").getResult(0) + } + + override predicate isSink(DataFlow::Node sink) { + sink = any(DataFlow::CallNode c | c.getCalleeName() = "sink").getArgument(0) + } +} + +class DataFlowTest extends InlineExpectationsTest { + DataFlowTest() { this = "DataFlowTest" } + + override string getARelevantTag() { result = "dataflow" } + + override predicate hasActualResult(string file, int line, string element, string tag, string value) { + tag = "dataflow" and + exists(DataFlow::Node sink | any(Configuration c).hasFlow(_, sink) | + element = sink.toString() and + value = sink.toString() and + sink.hasLocationInfo(file, line, _, _, _) + ) + } +} diff --git a/ql/test/library-tests/semmle/go/dataflow/TypeAssertions/test.go b/ql/test/library-tests/semmle/go/dataflow/TypeAssertions/test.go new file mode 100644 index 00000000000..c0e97e4387b --- /dev/null +++ b/ql/test/library-tests/semmle/go/dataflow/TypeAssertions/test.go @@ -0,0 +1,30 @@ +package main + +func src() interface{} { + return "hi" +} + +func sink(p interface{}) {} + +func test() (bool, *string) { + ptr := src() + sink(ptr) // $dataflow=ptr + cast := ptr.(*string) + sink(cast) // $dataflow=cast + cast2, ok := ptr.(*string) + if !ok { + return true, nil + } + sink(cast2) // $dataflow=cast2 + var cast3, ok2 = ptr.(*string) + if !ok2 { + return true, nil + } + sink(cast3) // $dataflow=cast3 + cast2, ok = ptr.(*string) + if !ok { + return true, nil + } + sink(cast2) // $dataflow=cast2 + return true, nil +}