From ce3007395f5385659b3c8996c7a452cdde87d1db Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Wed, 24 Jun 2020 07:00:41 +0100 Subject: [PATCH 1/4] Rename `arrayStep` to `elementStep`, which is more accurate. --- .../semmle/go/dataflow/internal/TaintTrackingUtil.qll | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/ql/src/semmle/go/dataflow/internal/TaintTrackingUtil.qll b/ql/src/semmle/go/dataflow/internal/TaintTrackingUtil.qll index e1ad7eaacb5..f6554f78e5c 100644 --- a/ql/src/semmle/go/dataflow/internal/TaintTrackingUtil.qll +++ b/ql/src/semmle/go/dataflow/internal/TaintTrackingUtil.qll @@ -56,7 +56,7 @@ predicate localAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { referenceStep(pred, succ) or elementWriteStep(pred, succ) or fieldReadStep(pred, succ) or - arrayStep(pred, succ) or + elementStep(pred, succ) or tupleStep(pred, succ) or stringConcatStep(pred, succ) or sliceStep(pred, succ) or @@ -105,11 +105,16 @@ predicate fieldReadStep(DataFlow::Node pred, DataFlow::Node succ) { succ.(DataFlow::FieldReadNode).getBase() = pred } -/** Holds if taint flows from `pred` to `succ` via an array index operation. */ -predicate arrayStep(DataFlow::Node pred, DataFlow::Node succ) { +/** + * Holds if taint flows from `pred` to `succ` via an array, map, slice, or string + * index operation. + */ +predicate elementStep(DataFlow::Node pred, DataFlow::Node succ) { succ.(DataFlow::ElementReadNode).getBase() = pred } +deprecated predicate arrayStep = elementStep/2; + /** Holds if taint flows from `pred` to `succ` via an extract tuple operation. */ predicate tupleStep(DataFlow::Node pred, DataFlow::Node succ) { succ = DataFlow::extractTupleElement(pred, _) From 258a2762428ec8ca96943138c0c17578cebeb4a8 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Wed, 24 Jun 2020 07:08:00 +0100 Subject: [PATCH 2/4] Propagate taint through range loops. --- ql/src/semmle/go/controlflow/IR.qll | 5 +++++ .../go/dataflow/internal/TaintTrackingUtil.qll | 6 ++++++ .../dataflow/FlowSteps/LocalFlowStep.expected | 18 ++++++++++++++++++ .../dataflow/FlowSteps/LocalTaintStep.expected | 3 +++ .../semmle/go/dataflow/FlowSteps/main.go | 8 ++++++++ 5 files changed, 40 insertions(+) diff --git a/ql/src/semmle/go/controlflow/IR.qll b/ql/src/semmle/go/controlflow/IR.qll index e980153f18a..3ae817f66a9 100644 --- a/ql/src/semmle/go/controlflow/IR.qll +++ b/ql/src/semmle/go/controlflow/IR.qll @@ -1089,6 +1089,11 @@ module IR { GetNextEntryInstruction() { this = MkNextNode(rs) } + /** + * Gets the instruction computing the value whose key-value pairs this instruction reads. + */ + Instruction getDomain() { result = evalExprInstruction(rs.getDomain()) } + override ControlFlow::Root getRoot() { result.isRootOf(rs) } override string toString() { result = "next key-value pair in range" } diff --git a/ql/src/semmle/go/dataflow/internal/TaintTrackingUtil.qll b/ql/src/semmle/go/dataflow/internal/TaintTrackingUtil.qll index f6554f78e5c..46383212826 100644 --- a/ql/src/semmle/go/dataflow/internal/TaintTrackingUtil.qll +++ b/ql/src/semmle/go/dataflow/internal/TaintTrackingUtil.qll @@ -111,6 +111,12 @@ predicate fieldReadStep(DataFlow::Node pred, DataFlow::Node succ) { */ predicate elementStep(DataFlow::Node pred, DataFlow::Node succ) { succ.(DataFlow::ElementReadNode).getBase() = pred + or + exists(IR::GetNextEntryInstruction nextEntry | + pred.asInstruction() = nextEntry.getDomain() and + // only step into the value, not the index + succ.asInstruction() = IR::extractTupleElement(nextEntry, 1) + ) } deprecated predicate arrayStep = elementStep/2; 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 730e38eb357..e18b11179c3 100644 --- a/ql/test/library-tests/semmle/go/dataflow/FlowSteps/LocalFlowStep.expected +++ b/ql/test/library-tests/semmle/go/dataflow/FlowSteps/LocalFlowStep.expected @@ -87,6 +87,24 @@ | main.go:40:8:40:23 | call to append | main.go:40:2:40:3 | definition of s2 | | main.go:41:2:41:3 | definition of s4 | main.go:42:10:42:11 | s4 | | main.go:41:8:41:21 | call to make | main.go:41:2:41:3 | definition of s4 | +| main.go:46:13:46:14 | argument corresponding to xs | main.go:46:13:46:14 | definition of xs | +| main.go:46:13:46:14 | definition of xs | main.go:47:20:47:21 | xs | +| main.go:46:24:46:27 | definition of keys | main.go:47:20:47:20 | keys = phi(def@46:24, def@49:3) | +| main.go:46:24:46:27 | zero value for keys | main.go:46:24:46:27 | definition of keys | +| main.go:46:34:46:37 | definition of vals | main.go:47:20:47:20 | vals = phi(def@46:34, def@48:3) | +| main.go:46:34:46:37 | zero value for vals | main.go:46:34:46:37 | definition of vals | +| main.go:47:2:50:2 | range statement[0] | main.go:47:6:47:6 | definition of k | +| main.go:47:2:50:2 | range statement[1] | main.go:47:9:47:9 | definition of v | +| main.go:47:6:47:6 | definition of k | main.go:49:11:49:11 | k | +| main.go:47:9:47:9 | definition of v | main.go:48:11:48:11 | v | +| main.go:47:20:47:20 | keys = phi(def@46:24, def@49:3) | main.go:46:24:46:27 | implicit read of keys | +| main.go:47:20:47:20 | keys = phi(def@46:24, def@49:3) | main.go:49:3:49:6 | keys | +| main.go:47:20:47:20 | vals = phi(def@46:34, def@48:3) | main.go:46:34:46:37 | implicit read of vals | +| main.go:47:20:47:20 | vals = phi(def@46:34, def@48:3) | main.go:48:3:48:6 | vals | +| main.go:48:3:48:6 | definition of vals | main.go:47:20:47:20 | vals = phi(def@46:34, def@48:3) | +| main.go:48:3:48:11 | ... += ... | main.go:48:3:48:6 | definition of vals | +| main.go:49:3:49:6 | definition of keys | main.go:47:20:47:20 | keys = phi(def@46:24, def@49:3) | +| main.go:49:3:49:11 | ... += ... | main.go:49:3:49:6 | definition of keys | | strings.go:8:12:8:12 | argument corresponding to s | strings.go:8:12:8:12 | definition of s | | strings.go:8:12:8:12 | definition of s | strings.go:9:24:9:24 | s | | strings.go:8:12:8:12 | definition of s | strings.go:10:27:10:27 | s | diff --git a/ql/test/library-tests/semmle/go/dataflow/FlowSteps/LocalTaintStep.expected b/ql/test/library-tests/semmle/go/dataflow/FlowSteps/LocalTaintStep.expected index d19150d5e7c..30f26fbef39 100644 --- a/ql/test/library-tests/semmle/go/dataflow/FlowSteps/LocalTaintStep.expected +++ b/ql/test/library-tests/semmle/go/dataflow/FlowSteps/LocalTaintStep.expected @@ -10,6 +10,9 @@ | main.go:40:15:40:15 | s | main.go:40:8:40:23 | call to append | | main.go:40:18:40:19 | s1 | main.go:40:8:40:23 | call to append | | main.go:42:10:42:11 | s4 | main.go:38:2:38:2 | definition of s | +| main.go:47:20:47:21 | next key-value pair in range | main.go:47:2:50:2 | range statement[0] | +| main.go:47:20:47:21 | next key-value pair in range | main.go:47:2:50:2 | range statement[1] | +| main.go:47:20:47:21 | xs | main.go:47:2:50:2 | range statement[1] | | strings.go:9:24:9:24 | s | strings.go:9:8:9:38 | call to Replace | | strings.go:9:32:9:34 | "_" | strings.go:9:8:9:38 | call to Replace | | strings.go:10:27:10:27 | s | strings.go:10:8:10:42 | call to ReplaceAll | diff --git a/ql/test/library-tests/semmle/go/dataflow/FlowSteps/main.go b/ql/test/library-tests/semmle/go/dataflow/FlowSteps/main.go index c6191a73a55..d47942ee9e8 100644 --- a/ql/test/library-tests/semmle/go/dataflow/FlowSteps/main.go +++ b/ql/test/library-tests/semmle/go/dataflow/FlowSteps/main.go @@ -42,3 +42,11 @@ func test9() []int { copy(s, s4) return s2 } + +func test10(xs []int) (keys int, vals int) { + for k, v := range xs { + vals += v // taint from `xs` + keys += k // no taint from `xs` + } + return +} From 66ec160f64e800c52af390f7822ce18cc065223e Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Fri, 26 Jun 2020 11:19:58 +0100 Subject: [PATCH 3/4] Add change note. --- change-notes/2020-06-26-taint-through-range.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 change-notes/2020-06-26-taint-through-range.md diff --git a/change-notes/2020-06-26-taint-through-range.md b/change-notes/2020-06-26-taint-through-range.md new file mode 100644 index 00000000000..55d870d1c88 --- /dev/null +++ b/change-notes/2020-06-26-taint-through-range.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* Taint tracking through `range` statements has been improved, which may cause more results from the security queries. From 57f8b08568ff0ab714e6671b155aa0a603a58e2e Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Fri, 26 Jun 2020 11:30:26 +0100 Subject: [PATCH 4/4] Update expected test output. The tests for `UnsafeTLS` now work as expected. --- .../experimental/CWE-327/UnsafeTLS.expected | 39 +++++++++++++++++++ ql/test/experimental/CWE-327/UnsafeTLS.go | 4 +- 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/ql/test/experimental/CWE-327/UnsafeTLS.expected b/ql/test/experimental/CWE-327/UnsafeTLS.expected index 64cf3c6b019..810bc9d08c1 100644 --- a/ql/test/experimental/CWE-327/UnsafeTLS.expected +++ b/ql/test/experimental/CWE-327/UnsafeTLS.expected @@ -25,6 +25,28 @@ edges | UnsafeTLS.go:171:32:171:50 | selection of CipherSuites : slice type | UnsafeTLS.go:171:25:171:94 | call to append : slice type | | UnsafeTLS.go:171:53:171:93 | selection of TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 : uint16 | UnsafeTLS.go:171:25:171:94 | call to append | | UnsafeTLS.go:171:53:171:93 | selection of TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 : uint16 | UnsafeTLS.go:171:25:171:94 | call to append : slice type | +| UnsafeTLS.go:174:3:174:8 | definition of config [pointer, CipherSuites] | UnsafeTLS.go:175:3:175:8 | config [pointer, CipherSuites] | +| UnsafeTLS.go:174:3:174:8 | definition of config [pointer, CipherSuites] | UnsafeTLS.go:178:4:178:9 | config [pointer, CipherSuites] | +| UnsafeTLS.go:174:3:174:8 | definition of config [pointer, CipherSuites] | UnsafeTLS.go:178:33:178:38 | config [pointer, CipherSuites] | +| UnsafeTLS.go:175:3:175:8 | config [pointer, CipherSuites] | UnsafeTLS.go:175:3:175:8 | implicit dereference [CipherSuites] : slice type | +| UnsafeTLS.go:175:3:175:8 | implicit dereference [CipherSuites] : slice type | UnsafeTLS.go:174:3:174:8 | definition of config [pointer, CipherSuites] | +| UnsafeTLS.go:176:21:176:46 | call to InsecureCipherSuites : slice type | UnsafeTLS.go:178:26:178:58 | call to append | +| UnsafeTLS.go:176:21:176:46 | call to InsecureCipherSuites : slice type | UnsafeTLS.go:178:26:178:58 | call to append : slice type | +| UnsafeTLS.go:176:21:176:46 | call to InsecureCipherSuites : slice type | UnsafeTLS.go:178:54:178:54 | implicit dereference : CipherSuite | +| UnsafeTLS.go:178:4:178:9 | config [pointer, CipherSuites] | UnsafeTLS.go:178:4:178:9 | implicit dereference [CipherSuites] : slice type | +| UnsafeTLS.go:178:4:178:9 | implicit dereference [CipherSuites] : slice type | UnsafeTLS.go:174:3:174:8 | definition of config [pointer, CipherSuites] | +| UnsafeTLS.go:178:26:178:58 | call to append : slice type | UnsafeTLS.go:178:4:178:9 | implicit dereference [CipherSuites] : slice type | +| UnsafeTLS.go:178:33:178:38 | config [pointer, CipherSuites] | UnsafeTLS.go:178:33:178:38 | implicit dereference [CipherSuites] : slice type | +| UnsafeTLS.go:178:33:178:38 | implicit dereference [CipherSuites] : slice type | UnsafeTLS.go:178:33:178:51 | selection of CipherSuites : slice type | +| UnsafeTLS.go:178:33:178:51 | selection of CipherSuites : slice type | UnsafeTLS.go:178:26:178:58 | call to append | +| UnsafeTLS.go:178:33:178:51 | selection of CipherSuites : slice type | UnsafeTLS.go:178:26:178:58 | call to append : slice type | +| UnsafeTLS.go:178:54:178:54 | implicit dereference : CipherSuite | UnsafeTLS.go:178:26:178:58 | call to append | +| UnsafeTLS.go:178:54:178:54 | implicit dereference : CipherSuite | UnsafeTLS.go:178:26:178:58 | call to append : slice type | +| UnsafeTLS.go:178:54:178:54 | implicit dereference : CipherSuite | UnsafeTLS.go:178:54:178:54 | implicit dereference : CipherSuite | +| UnsafeTLS.go:184:21:184:46 | call to InsecureCipherSuites : slice type | UnsafeTLS.go:186:40:186:40 | implicit dereference : CipherSuite | +| UnsafeTLS.go:184:21:184:46 | call to InsecureCipherSuites : slice type | UnsafeTLS.go:188:25:188:36 | cipherSuites | +| UnsafeTLS.go:186:40:186:40 | implicit dereference : CipherSuite | UnsafeTLS.go:186:40:186:40 | implicit dereference : CipherSuite | +| UnsafeTLS.go:186:40:186:40 | implicit dereference : CipherSuite | UnsafeTLS.go:188:25:188:36 | cipherSuites | | UnsafeTLS.go:193:21:193:46 | call to InsecureCipherSuites : slice type | UnsafeTLS.go:195:40:195:56 | implicit dereference : CipherSuite | | UnsafeTLS.go:193:21:193:46 | call to InsecureCipherSuites : slice type | UnsafeTLS.go:197:25:197:36 | cipherSuites | | UnsafeTLS.go:195:40:195:56 | implicit dereference : CipherSuite | UnsafeTLS.go:195:40:195:56 | implicit dereference : CipherSuite | @@ -74,6 +96,21 @@ nodes | UnsafeTLS.go:171:32:171:37 | implicit dereference [CipherSuites] : slice type | semmle.label | implicit dereference [CipherSuites] : slice type | | UnsafeTLS.go:171:32:171:50 | selection of CipherSuites : slice type | semmle.label | selection of CipherSuites : slice type | | UnsafeTLS.go:171:53:171:93 | selection of TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 : uint16 | semmle.label | selection of TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 : uint16 | +| UnsafeTLS.go:174:3:174:8 | definition of config [pointer, CipherSuites] | semmle.label | definition of config [pointer, CipherSuites] | +| UnsafeTLS.go:175:3:175:8 | config [pointer, CipherSuites] | semmle.label | config [pointer, CipherSuites] | +| UnsafeTLS.go:175:3:175:8 | implicit dereference [CipherSuites] : slice type | semmle.label | implicit dereference [CipherSuites] : slice type | +| UnsafeTLS.go:176:21:176:46 | call to InsecureCipherSuites : slice type | semmle.label | call to InsecureCipherSuites : slice type | +| UnsafeTLS.go:178:4:178:9 | config [pointer, CipherSuites] | semmle.label | config [pointer, CipherSuites] | +| UnsafeTLS.go:178:4:178:9 | implicit dereference [CipherSuites] : slice type | semmle.label | implicit dereference [CipherSuites] : slice type | +| UnsafeTLS.go:178:26:178:58 | call to append | semmle.label | call to append | +| UnsafeTLS.go:178:26:178:58 | call to append : slice type | semmle.label | call to append : slice type | +| UnsafeTLS.go:178:33:178:38 | config [pointer, CipherSuites] | semmle.label | config [pointer, CipherSuites] | +| UnsafeTLS.go:178:33:178:38 | implicit dereference [CipherSuites] : slice type | semmle.label | implicit dereference [CipherSuites] : slice type | +| UnsafeTLS.go:178:33:178:51 | selection of CipherSuites : slice type | semmle.label | selection of CipherSuites : slice type | +| UnsafeTLS.go:178:54:178:54 | implicit dereference : CipherSuite | semmle.label | implicit dereference : CipherSuite | +| UnsafeTLS.go:184:21:184:46 | call to InsecureCipherSuites : slice type | semmle.label | call to InsecureCipherSuites : slice type | +| UnsafeTLS.go:186:40:186:40 | implicit dereference : CipherSuite | semmle.label | implicit dereference : CipherSuite | +| UnsafeTLS.go:188:25:188:36 | cipherSuites | semmle.label | cipherSuites | | UnsafeTLS.go:193:21:193:46 | call to InsecureCipherSuites : slice type | semmle.label | call to InsecureCipherSuites : slice type | | UnsafeTLS.go:195:40:195:56 | implicit dereference : CipherSuite | semmle.label | implicit dereference : CipherSuite | | UnsafeTLS.go:197:25:197:36 | cipherSuites | semmle.label | cipherSuites | @@ -103,4 +140,6 @@ nodes | UnsafeTLS.go:146:18:148:4 | slice literal | UnsafeTLS.go:147:5:147:47 | selection of TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256 : uint16 | UnsafeTLS.go:146:18:148:4 | slice literal | Use of an insecure cipher suite: TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256. | | UnsafeTLS.go:154:18:156:4 | slice literal | UnsafeTLS.go:155:5:155:45 | selection of TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 : uint16 | UnsafeTLS.go:154:18:156:4 | slice literal | Use of an insecure cipher suite: TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256. | | UnsafeTLS.go:171:25:171:94 | call to append | UnsafeTLS.go:171:53:171:93 | selection of TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 : uint16 | UnsafeTLS.go:171:25:171:94 | call to append | Use of an insecure cipher suite: TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256. | +| UnsafeTLS.go:178:26:178:58 | call to append | UnsafeTLS.go:176:21:176:46 | call to InsecureCipherSuites : slice type | UnsafeTLS.go:178:26:178:58 | call to append | Use of an insecure cipher suite from InsecureCipherSuites(). | +| UnsafeTLS.go:188:25:188:36 | cipherSuites | UnsafeTLS.go:184:21:184:46 | call to InsecureCipherSuites : slice type | UnsafeTLS.go:188:25:188:36 | cipherSuites | Use of an insecure cipher suite from InsecureCipherSuites(). | | UnsafeTLS.go:197:25:197:36 | cipherSuites | UnsafeTLS.go:193:21:193:46 | call to InsecureCipherSuites : slice type | UnsafeTLS.go:197:25:197:36 | cipherSuites | Use of an insecure cipher suite from InsecureCipherSuites(). | diff --git a/ql/test/experimental/CWE-327/UnsafeTLS.go b/ql/test/experimental/CWE-327/UnsafeTLS.go index accbf04fa26..3a60ffc9543 100644 --- a/ql/test/experimental/CWE-327/UnsafeTLS.go +++ b/ql/test/experimental/CWE-327/UnsafeTLS.go @@ -175,7 +175,7 @@ func cipherSuites() { config.CipherSuites = make([]uint16, 0) insecureSuites := tls.InsecureCipherSuites() for _, v := range insecureSuites { - config.CipherSuites = append(config.CipherSuites, v.ID) // TODO: should be flagged as BAD. + config.CipherSuites = append(config.CipherSuites, v.ID) // BAD } } { @@ -185,7 +185,7 @@ func cipherSuites() { for _, v := range insecureSuites { cipherSuites = append(cipherSuites, v.ID) } - config.CipherSuites = cipherSuites // TODO: should be flagged as BAD. + config.CipherSuites = cipherSuites // BAD } { config := &tls.Config{}