From 1d850873f373025683e73af7679c47121e25aeff Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Thu, 12 Nov 2020 18:38:55 +0000 Subject: [PATCH 1/2] Add data-flow edge `from -> to` in the context `to, ok := from.(*Type)` --- .../go/controlflow/ControlFlowGraphImpl.qll | 2 +- ql/src/semmle/go/controlflow/IR.qll | 7 ++++- .../go/dataflow/internal/DataFlowUtil.qll | 16 ++++++++-- .../dataflow/FlowSteps/LocalFlowStep.expected | 2 +- .../dataflow/TypeAssertions/DataFlow.expected | 0 .../go/dataflow/TypeAssertions/DataFlow.ql | 29 ++++++++++++++++++ .../semmle/go/dataflow/TypeAssertions/test.go | 30 +++++++++++++++++++ 7 files changed, 80 insertions(+), 6 deletions(-) create mode 100644 ql/test/library-tests/semmle/go/dataflow/TypeAssertions/DataFlow.expected create mode 100644 ql/test/library-tests/semmle/go/dataflow/TypeAssertions/DataFlow.ql create mode 100644 ql/test/library-tests/semmle/go/dataflow/TypeAssertions/test.go 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 da941b3efa4..857bdaabe5b 100644 --- a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll +++ b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll @@ -945,7 +945,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) | @@ -981,13 +981,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 +} From 3d8470e1e2b1f7d50912fb70141988a7f885c4c5 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Tue, 17 Nov 2020 16:03:33 +0000 Subject: [PATCH 2/2] Add and use `TypeCastNode::getResultType` This can differ from `getType` when a `TypeAssertExpr` returns a (result, ok) pair. --- ql/src/Security/CWE-681/IncorrectIntegerConversion.ql | 2 +- ql/src/experimental/Unsafe/WrongUsageOfUnsafe.ql | 8 ++++---- ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll | 10 ++++++++++ 3 files changed, 15 insertions(+), 5 deletions(-) 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/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll index 857bdaabe5b..b5ea22de0bc 100644 --- a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll +++ b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll @@ -878,6 +878,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()