From 0dd7676bd8f20715067b7567c200acbb9714e9b1 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Thu, 28 May 2020 15:29:06 +0100 Subject: [PATCH 1/2] Add another function-model test. --- .../FunctionInput_getEntryNode.expected | 2 ++ .../FunctionInput_getExitNode.expected | 1 + .../FunctionModelStep.ql | 10 +++++++++- .../FunctionOutput_getEntryNode.expected | 1 + .../FunctionOutput_getExitNode.expected | 1 + .../dataflow/FunctionInputsAndOutputs/reset.go | 16 ++++++++++++++++ 6 files changed, 30 insertions(+), 1 deletion(-) create mode 100644 ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/reset.go diff --git a/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionInput_getEntryNode.expected b/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionInput_getEntryNode.expected index 649f5fd6273..7a80ea0b808 100644 --- a/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionInput_getEntryNode.expected +++ b/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionInput_getEntryNode.expected @@ -2,6 +2,7 @@ | parameter 0 | main.go:53:2:53:22 | call to op2 | main.go:53:6:53:8 | "-" | | parameter 0 | main.go:55:2:55:27 | call to Printf | main.go:55:13:55:20 | "%d, %d" | | parameter 0 | main.go:57:2:57:27 | call to Printf | main.go:57:13:57:20 | "%d, %d" | +| parameter 0 | reset.go:12:2:12:21 | call to Reset | reset.go:12:15:12:20 | source | | parameter 0 | tst.go:10:2:10:29 | call to ReadFrom | tst.go:10:23:10:28 | reader | | parameter 1 | main.go:51:2:51:14 | call to op | main.go:51:10:51:10 | 1 | | parameter 1 | main.go:53:2:53:22 | call to op2 | main.go:53:11:53:11 | 2 | @@ -12,5 +13,6 @@ | parameter 2 | main.go:55:2:55:27 | call to Printf | main.go:55:26:55:26 | y | | parameter 2 | main.go:57:2:57:27 | call to Printf | main.go:57:26:57:26 | y | | receiver | main.go:53:14:53:21 | call to bump | main.go:53:14:53:14 | c | +| receiver | reset.go:12:2:12:21 | call to Reset | reset.go:12:2:12:7 | reader | | receiver | tst.go:10:2:10:29 | call to ReadFrom | tst.go:10:2:10:12 | bytesBuffer | | result | tst.go:9:17:9:33 | call to new | tst.go:9:2:9:12 | definition of bytesBuffer | diff --git a/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionInput_getExitNode.expected b/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionInput_getExitNode.expected index f13c3f16bd1..3f4fdfccbc2 100644 --- a/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionInput_getExitNode.expected +++ b/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionInput_getExitNode.expected @@ -1,6 +1,7 @@ | parameter 0 | main.go:5:1:11:1 | function declaration | main.go:5:9:5:10 | definition of op | | parameter 0 | main.go:13:1:20:1 | function declaration | main.go:13:10:13:11 | definition of op | | parameter 0 | main.go:40:1:48:1 | function declaration | main.go:40:12:40:12 | definition of b | +| parameter 0 | reset.go:8:1:16:1 | function declaration | reset.go:8:27:8:27 | definition of r | | parameter 0 | tst.go:8:1:11:1 | function declaration | tst.go:8:12:8:17 | definition of reader | | parameter 1 | main.go:5:1:11:1 | function declaration | main.go:5:20:5:20 | definition of x | | parameter 1 | main.go:13:1:20:1 | function declaration | main.go:13:21:13:21 | definition of x | diff --git a/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionModelStep.ql b/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionModelStep.ql index d21a051038d..276239350d7 100644 --- a/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionModelStep.ql +++ b/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionModelStep.ql @@ -1,7 +1,15 @@ import go class BytesReadFrom extends TaintTracking::FunctionModel, Method { - BytesReadFrom() { this.(Method).hasQualifiedName("bytes", "Buffer", "ReadFrom") } + BytesReadFrom() { this.hasQualifiedName("bytes", "Buffer", "ReadFrom") } + + override predicate hasTaintFlow(FunctionInput inp, FunctionOutput outp) { + inp.isParameter(0) and outp.isReceiver() + } +} + +class ReaderReset extends TaintTracking::FunctionModel, Method { + ReaderReset() { this.hasQualifiedName("bufio", "Reader", "Reset") } override predicate hasTaintFlow(FunctionInput inp, FunctionOutput outp) { inp.isParameter(0) and outp.isReceiver() diff --git a/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionOutput_getEntryNode.expected b/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionOutput_getEntryNode.expected index a9bde481fa3..1745841994e 100644 --- a/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionOutput_getEntryNode.expected +++ b/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionOutput_getEntryNode.expected @@ -4,6 +4,7 @@ | result | main.go:13:1:20:1 | function declaration | main.go:15:9:15:13 | ...+... | | result | main.go:13:1:20:1 | function declaration | main.go:17:10:17:14 | ...-... | | result | main.go:26:1:29:1 | function declaration | main.go:28:9:28:15 | selection of count | +| result | reset.go:8:1:16:1 | function declaration | reset.go:15:9:15:12 | sink | | result 0 | main.go:31:1:33:1 | function declaration | main.go:32:9:32:10 | 23 | | result 0 | main.go:35:1:38:1 | function declaration | main.go:35:15:35:15 | zero value for x | | result 0 | main.go:35:1:38:1 | function declaration | main.go:36:13:36:14 | 23 | diff --git a/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionOutput_getExitNode.expected b/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionOutput_getExitNode.expected index cf0fe4752b4..4e41f942007 100644 --- a/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionOutput_getExitNode.expected +++ b/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionOutput_getExitNode.expected @@ -1,3 +1,4 @@ +| parameter 0 | reset.go:12:2:12:21 | call to Reset | reset.go:9:2:9:7 | definition of source | | parameter 0 | tst.go:10:2:10:29 | call to ReadFrom | tst.go:8:12:8:17 | definition of reader | | receiver | tst.go:10:2:10:29 | call to ReadFrom | tst.go:9:2:9:12 | definition of bytesBuffer | | result | main.go:51:2:51:14 | call to op | main.go:51:2:51:14 | call to op | diff --git a/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/reset.go b/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/reset.go new file mode 100644 index 00000000000..d135d77d51a --- /dev/null +++ b/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/reset.go @@ -0,0 +1,16 @@ +package main + +import ( + "bufio" + "io" +) + +func bufioReaderResetTest(r io.Reader) bufio.Reader { + source := r + + var reader bufio.Reader + reader.Reset(source) + sink := reader + + return sink +} From e3501ddb447f53456ab98021cc6e5f8c8e6e6daf Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Thu, 28 May 2020 15:33:09 +0100 Subject: [PATCH 2/2] Introduce more post-update nodes. To model (taint) flow through functions, we introduce post-update nodes for arguments (including receivers), but only if that argument is mutable. However, previously our criterion for determining whether an argument is mutable was a little too restrictive. In particular, we would not consider a struct-typed argument as mutable, since structs are passed by value. While this is reasonable for data flow, it is unnecessarily restrictive for taint, since it makes perfect sense to track deep taint through structs. So instead we now turn things round and instead consider _all_ types to be mutable except for primitive types (booleans, numbers, and strings). --- ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll | 9 ++++----- .../FunctionInputsAndOutputs/FunctionModelStep.expected | 1 + .../FunctionOutput_getExitNode.expected | 2 ++ 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll index d3d9fd928f9..0f9d67c287e 100644 --- a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll +++ b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll @@ -464,11 +464,10 @@ class ArgumentNode extends Node { */ predicate mutableType(Type tp) { exists(Type underlying | underlying = tp.getUnderlyingType() | - underlying instanceof ArrayType or - underlying instanceof SliceType or - underlying instanceof MapType or - underlying instanceof PointerType or - underlying instanceof InterfaceType + not underlying instanceof BoolType and + not underlying instanceof NumericType and + not underlying instanceof StringType and + not underlying instanceof LiteralType ) } diff --git a/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionModelStep.expected b/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionModelStep.expected index 0c64f6cc4b4..be1c46b3437 100644 --- a/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionModelStep.expected +++ b/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionModelStep.expected @@ -1 +1,2 @@ | file://:0:0:0:0 | ReadFrom | tst.go:10:23:10:28 | reader | tst.go:9:2:9:12 | definition of bytesBuffer | +| file://:0:0:0:0 | Reset | reset.go:12:15:12:20 | source | reset.go:11:6:11:11 | definition of reader | diff --git a/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionOutput_getExitNode.expected b/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionOutput_getExitNode.expected index 4e41f942007..12d67fbc8e5 100644 --- a/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionOutput_getExitNode.expected +++ b/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionOutput_getExitNode.expected @@ -1,5 +1,7 @@ | parameter 0 | reset.go:12:2:12:21 | call to Reset | reset.go:9:2:9:7 | definition of source | | parameter 0 | tst.go:10:2:10:29 | call to ReadFrom | tst.go:8:12:8:17 | definition of reader | +| receiver | main.go:53:14:53:21 | call to bump | main.go:52:2:52:2 | definition of c | +| receiver | reset.go:12:2:12:21 | call to Reset | reset.go:11:6:11:11 | definition of reader | | receiver | tst.go:10:2:10:29 | call to ReadFrom | tst.go:9:2:9:12 | definition of bytesBuffer | | result | main.go:51:2:51:14 | call to op | main.go:51:2:51:14 | call to op | | result | main.go:53:2:53:22 | call to op2 | main.go:53:2:53:22 | call to op2 |