From 5068b8b195a98d2fdfa52185e9fced3fb53106e9 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Tue, 8 Sep 2020 16:28:02 +0100 Subject: [PATCH 1/2] Add PostUpdateNodes for nested structs and arrays This creates a PostUpdateNode for x in the contexts `x.field[element]`, `x.field.otherfield`, `x[element].field` and so on. Most uses of PostUpdateNode implicitly assume its old definition, but our protobuf model benefits. --- .../go/dataflow/internal/DataFlowUtil.qll | 17 ++++-- .../frameworks/Protobuf/TaintFlows.expected | 2 + .../OpenUrlRedirect/OpenUrlRedirect.expected | 60 +++++++++++++++++++ 3 files changed, 74 insertions(+), 5 deletions(-) diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll index 81caf563fb1..f5c147085e3 100644 --- a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll +++ b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll @@ -416,6 +416,15 @@ class ReceiverNode extends ParameterNode { predicate isReceiverOf(MethodDecl m) { parm.isReceiverOf(m) } } +private Node getADirectlyWrittenNode() { + exists(Write w | w.writesField(result, _, _) or w.writesElement(result, _, _)) +} + +private Node getAWrittenNode() { + result = getADirectlyWrittenNode() or + result = getADirectlyWrittenNode().(ComponentReadNode).getBase+() +} + /** * A node associated with an object after an operation that might have * changed its state. @@ -439,12 +448,10 @@ class PostUpdateNode extends Node { or preupd = any(PointerDereferenceNode deref).getOperand() or - exists(Write w, DataFlow::Node base | - w.writesField(base, _, _) or w.writesElement(base, _, _) - | - preupd = base + exists(Node written | written = getAWrittenNode() | + preupd = written or - preupd = base.(PointerDereferenceNode).getOperand() + preupd = written.(PointerDereferenceNode).getOperand() ) or preupd instanceof ArgumentNode and diff --git a/ql/test/library-tests/semmle/go/frameworks/Protobuf/TaintFlows.expected b/ql/test/library-tests/semmle/go/frameworks/Protobuf/TaintFlows.expected index 136839b7b9f..b8d9451ba37 100644 --- a/ql/test/library-tests/semmle/go/frameworks/Protobuf/TaintFlows.expected +++ b/ql/test/library-tests/semmle/go/frameworks/Protobuf/TaintFlows.expected @@ -12,6 +12,7 @@ | testDeprecatedApi.go:143:20:143:39 | call to getUntrustedString : string | testDeprecatedApi.go:148:12:148:21 | serialized | | testDeprecatedApi.go:152:25:152:43 | call to getUntrustedBytes : slice type | testDeprecatedApi.go:157:13:157:36 | index expression | | testDeprecatedApi.go:161:25:161:43 | call to getUntrustedBytes : slice type | testDeprecatedApi.go:168:13:168:25 | index expression | +| testDeprecatedApi.go:176:24:176:43 | call to getUntrustedString : string | testDeprecatedApi.go:180:12:180:21 | serialized | | testModernApi.go:11:22:11:41 | call to getUntrustedString : string | testModernApi.go:15:12:15:21 | serialized | | testModernApi.go:20:22:20:41 | call to getUntrustedString : string | testModernApi.go:26:12:26:21 | serialized | | testModernApi.go:30:25:30:43 | call to getUntrustedBytes : slice type | testModernApi.go:34:13:34:29 | selection of Description | @@ -29,3 +30,4 @@ | testModernApi.go:200:20:200:39 | call to getUntrustedString : string | testModernApi.go:205:12:205:21 | serialized | | testModernApi.go:209:25:209:43 | call to getUntrustedBytes : slice type | testModernApi.go:214:13:214:36 | index expression | | testModernApi.go:218:25:218:43 | call to getUntrustedBytes : slice type | testModernApi.go:225:13:225:25 | index expression | +| testModernApi.go:233:24:233:43 | call to getUntrustedString : string | testModernApi.go:237:12:237:21 | serialized | diff --git a/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/OpenUrlRedirect.expected b/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/OpenUrlRedirect.expected index cd1dcbd3a52..9d3538c9080 100644 --- a/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/OpenUrlRedirect.expected +++ b/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/OpenUrlRedirect.expected @@ -6,6 +6,44 @@ edges | stdlib.go:44:13:44:18 | selection of Form : Values | stdlib.go:46:23:46:28 | target | | stdlib.go:64:13:64:18 | selection of Form : Values | stdlib.go:67:23:67:40 | ...+... | | stdlib.go:89:13:89:18 | selection of Form : Values | stdlib.go:92:23:92:28 | target | +| stdlib.go:107:54:107:54 | definition of r [pointer, URL, ... (3)] | stdlib.go:112:4:112:4 | r [pointer, URL, ... (3)] | +| stdlib.go:107:54:107:54 | definition of r [pointer, URL, ... (3)] | stdlib.go:112:4:112:4 | r [pointer, URL, ... (3)] | +| stdlib.go:107:54:107:54 | definition of r [pointer, URL] | stdlib.go:112:4:112:4 | r [pointer, URL] | +| stdlib.go:107:54:107:54 | definition of r [pointer, URL] | stdlib.go:112:4:112:4 | r [pointer, URL] | +| stdlib.go:107:54:107:54 | definition of r [pointer, URL] | stdlib.go:113:24:113:24 | r [pointer, URL] | +| stdlib.go:107:54:107:54 | definition of r [pointer, URL] | stdlib.go:113:24:113:24 | r [pointer, URL] | +| stdlib.go:112:4:112:4 | implicit dereference [URL, pointer] | stdlib.go:107:54:107:54 | definition of r [pointer, URL, ... (3)] | +| stdlib.go:112:4:112:4 | implicit dereference [URL, pointer] | stdlib.go:107:54:107:54 | definition of r [pointer, URL, ... (3)] | +| stdlib.go:112:4:112:4 | implicit dereference [URL, pointer] | stdlib.go:112:4:112:8 | selection of URL [pointer] : URL | +| stdlib.go:112:4:112:4 | implicit dereference [URL, pointer] | stdlib.go:112:4:112:8 | selection of URL [pointer] : URL | +| stdlib.go:112:4:112:4 | implicit dereference [URL] : pointer type | stdlib.go:107:54:107:54 | definition of r [pointer, URL] | +| stdlib.go:112:4:112:4 | implicit dereference [URL] : pointer type | stdlib.go:107:54:107:54 | definition of r [pointer, URL] | +| stdlib.go:112:4:112:4 | implicit dereference [URL] : pointer type | stdlib.go:112:4:112:8 | selection of URL : pointer type | +| stdlib.go:112:4:112:4 | implicit dereference [URL] : pointer type | stdlib.go:112:4:112:8 | selection of URL : pointer type | +| stdlib.go:112:4:112:4 | r [pointer, URL, ... (3)] | stdlib.go:112:4:112:4 | implicit dereference [URL, pointer] | +| stdlib.go:112:4:112:4 | r [pointer, URL, ... (3)] | stdlib.go:112:4:112:4 | implicit dereference [URL, pointer] | +| stdlib.go:112:4:112:4 | r [pointer, URL] | stdlib.go:112:4:112:4 | implicit dereference [URL] : pointer type | +| stdlib.go:112:4:112:4 | r [pointer, URL] | stdlib.go:112:4:112:4 | implicit dereference [URL] : pointer type | +| stdlib.go:112:4:112:8 | implicit dereference : URL | stdlib.go:112:4:112:8 | implicit dereference : URL | +| stdlib.go:112:4:112:8 | implicit dereference : URL | stdlib.go:112:4:112:8 | implicit dereference : URL | +| stdlib.go:112:4:112:8 | implicit dereference : URL | stdlib.go:112:4:112:8 | selection of URL : pointer type | +| stdlib.go:112:4:112:8 | implicit dereference : URL | stdlib.go:112:4:112:8 | selection of URL : pointer type | +| stdlib.go:112:4:112:8 | implicit dereference : URL | stdlib.go:112:4:112:8 | selection of URL [pointer] : URL | +| stdlib.go:112:4:112:8 | implicit dereference : URL | stdlib.go:112:4:112:8 | selection of URL [pointer] : URL | +| stdlib.go:112:4:112:8 | selection of URL : pointer type | stdlib.go:112:4:112:4 | implicit dereference [URL] : pointer type | +| stdlib.go:112:4:112:8 | selection of URL : pointer type | stdlib.go:112:4:112:4 | implicit dereference [URL] : pointer type | +| stdlib.go:112:4:112:8 | selection of URL : pointer type | stdlib.go:112:4:112:8 | implicit dereference : URL | +| stdlib.go:112:4:112:8 | selection of URL : pointer type | stdlib.go:112:4:112:8 | implicit dereference : URL | +| stdlib.go:112:4:112:8 | selection of URL : pointer type | stdlib.go:112:4:112:8 | selection of URL : pointer type | +| stdlib.go:112:4:112:8 | selection of URL : pointer type | stdlib.go:112:4:112:8 | selection of URL : pointer type | +| stdlib.go:112:4:112:8 | selection of URL [pointer] : URL | stdlib.go:112:4:112:4 | implicit dereference [URL, pointer] | +| stdlib.go:112:4:112:8 | selection of URL [pointer] : URL | stdlib.go:112:4:112:4 | implicit dereference [URL, pointer] | +| stdlib.go:112:4:112:8 | selection of URL [pointer] : URL | stdlib.go:112:4:112:8 | implicit dereference : URL | +| stdlib.go:112:4:112:8 | selection of URL [pointer] : URL | stdlib.go:112:4:112:8 | implicit dereference : URL | +| stdlib.go:113:24:113:24 | implicit dereference [URL] : pointer type | stdlib.go:113:24:113:28 | selection of URL : pointer type | +| stdlib.go:113:24:113:24 | implicit dereference [URL] : pointer type | stdlib.go:113:24:113:28 | selection of URL : pointer type | +| stdlib.go:113:24:113:24 | r [pointer, URL] | stdlib.go:113:24:113:24 | implicit dereference [URL] : pointer type | +| stdlib.go:113:24:113:24 | r [pointer, URL] | stdlib.go:113:24:113:24 | implicit dereference [URL] : pointer type | | stdlib.go:113:24:113:28 | selection of URL : pointer type | stdlib.go:113:24:113:37 | call to String | | stdlib.go:113:24:113:28 | selection of URL : pointer type | stdlib.go:113:24:113:37 | call to String | | stdlib.go:146:13:146:18 | selection of Form : Values | stdlib.go:152:23:152:28 | target | @@ -37,6 +75,28 @@ nodes | stdlib.go:67:23:67:40 | ...+... | semmle.label | ...+... | | stdlib.go:89:13:89:18 | selection of Form : Values | semmle.label | selection of Form : Values | | stdlib.go:92:23:92:28 | target | semmle.label | target | +| stdlib.go:107:54:107:54 | definition of r [pointer, URL, ... (3)] | semmle.label | definition of r [pointer, URL, ... (3)] | +| stdlib.go:107:54:107:54 | definition of r [pointer, URL, ... (3)] | semmle.label | definition of r [pointer, URL, ... (3)] | +| stdlib.go:107:54:107:54 | definition of r [pointer, URL] | semmle.label | definition of r [pointer, URL] | +| stdlib.go:107:54:107:54 | definition of r [pointer, URL] | semmle.label | definition of r [pointer, URL] | +| stdlib.go:112:4:112:4 | implicit dereference [URL, pointer] | semmle.label | implicit dereference [URL, pointer] | +| stdlib.go:112:4:112:4 | implicit dereference [URL, pointer] | semmle.label | implicit dereference [URL, pointer] | +| stdlib.go:112:4:112:4 | implicit dereference [URL] : pointer type | semmle.label | implicit dereference [URL] : pointer type | +| stdlib.go:112:4:112:4 | implicit dereference [URL] : pointer type | semmle.label | implicit dereference [URL] : pointer type | +| stdlib.go:112:4:112:4 | r [pointer, URL, ... (3)] | semmle.label | r [pointer, URL, ... (3)] | +| stdlib.go:112:4:112:4 | r [pointer, URL, ... (3)] | semmle.label | r [pointer, URL, ... (3)] | +| stdlib.go:112:4:112:4 | r [pointer, URL] | semmle.label | r [pointer, URL] | +| stdlib.go:112:4:112:4 | r [pointer, URL] | semmle.label | r [pointer, URL] | +| stdlib.go:112:4:112:8 | implicit dereference : URL | semmle.label | implicit dereference : URL | +| stdlib.go:112:4:112:8 | implicit dereference : URL | semmle.label | implicit dereference : URL | +| stdlib.go:112:4:112:8 | selection of URL : pointer type | semmle.label | selection of URL : pointer type | +| stdlib.go:112:4:112:8 | selection of URL : pointer type | semmle.label | selection of URL : pointer type | +| stdlib.go:112:4:112:8 | selection of URL [pointer] : URL | semmle.label | selection of URL [pointer] : URL | +| stdlib.go:112:4:112:8 | selection of URL [pointer] : URL | semmle.label | selection of URL [pointer] : URL | +| stdlib.go:113:24:113:24 | implicit dereference [URL] : pointer type | semmle.label | implicit dereference [URL] : pointer type | +| stdlib.go:113:24:113:24 | implicit dereference [URL] : pointer type | semmle.label | implicit dereference [URL] : pointer type | +| stdlib.go:113:24:113:24 | r [pointer, URL] | semmle.label | r [pointer, URL] | +| stdlib.go:113:24:113:24 | r [pointer, URL] | semmle.label | r [pointer, URL] | | stdlib.go:113:24:113:28 | selection of URL : pointer type | semmle.label | selection of URL : pointer type | | stdlib.go:113:24:113:28 | selection of URL : pointer type | semmle.label | selection of URL : pointer type | | stdlib.go:113:24:113:37 | call to String | semmle.label | call to String | From 650bc1d38f63f1f841ec8356ab996b1863616b45 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 9 Sep 2020 15:42:52 +0100 Subject: [PATCH 2/2] Add PostUpdateNodes for derferenced expressions on an access path to a field- or element-write --- .../go/dataflow/internal/DataFlowUtil.qll | 15 ++++++----- .../go/dataflow/PostUpdateNodes/test.expected | 15 +++++++++++ .../go/dataflow/PostUpdateNodes/test.go | 25 +++++++++++++++++++ .../go/dataflow/PostUpdateNodes/test.ql | 4 +++ 4 files changed, 51 insertions(+), 8 deletions(-) create mode 100644 ql/test/library-tests/semmle/go/dataflow/PostUpdateNodes/test.expected create mode 100644 ql/test/library-tests/semmle/go/dataflow/PostUpdateNodes/test.go create mode 100644 ql/test/library-tests/semmle/go/dataflow/PostUpdateNodes/test.ql diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll index f5c147085e3..30755f93bc3 100644 --- a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll +++ b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll @@ -420,11 +420,14 @@ private Node getADirectlyWrittenNode() { exists(Write w | w.writesField(result, _, _) or w.writesElement(result, _, _)) } -private Node getAWrittenNode() { - result = getADirectlyWrittenNode() or - result = getADirectlyWrittenNode().(ComponentReadNode).getBase+() +private DataFlow::Node getAccessPathPredecessor(DataFlow::Node node) { + result = node.(PointerDereferenceNode).getOperand() + or + result = node.(ComponentReadNode).getBase() } +private Node getAWrittenNode() { result = getAccessPathPredecessor*(getADirectlyWrittenNode()) } + /** * A node associated with an object after an operation that might have * changed its state. @@ -448,11 +451,7 @@ class PostUpdateNode extends Node { or preupd = any(PointerDereferenceNode deref).getOperand() or - exists(Node written | written = getAWrittenNode() | - preupd = written - or - preupd = written.(PointerDereferenceNode).getOperand() - ) + preupd = getAWrittenNode() or preupd instanceof ArgumentNode and mutableType(preupd.getType()) diff --git a/ql/test/library-tests/semmle/go/dataflow/PostUpdateNodes/test.expected b/ql/test/library-tests/semmle/go/dataflow/PostUpdateNodes/test.expected new file mode 100644 index 00000000000..5ee74a7cde4 --- /dev/null +++ b/ql/test/library-tests/semmle/go/dataflow/PostUpdateNodes/test.expected @@ -0,0 +1,15 @@ +| test.go:19:2:19:2 | definition of a | +| test.go:20:11:20:14 | &... | +| test.go:20:12:20:14 | selection of b | +| test.go:21:2:21:5 | selection of bs | +| test.go:21:2:21:8 | index expression | +| test.go:21:17:21:20 | &... | +| test.go:21:18:21:20 | struct literal | +| test.go:22:2:22:5 | selection of bs | +| test.go:22:2:22:8 | index expression | +| test.go:22:2:22:13 | implicit dereference | +| test.go:22:2:22:13 | selection of cptr | +| test.go:23:2:23:7 | implicit dereference | +| test.go:23:2:23:7 | selection of bptr | +| test.go:23:2:23:12 | implicit dereference | +| test.go:23:2:23:12 | selection of cptr | diff --git a/ql/test/library-tests/semmle/go/dataflow/PostUpdateNodes/test.go b/ql/test/library-tests/semmle/go/dataflow/PostUpdateNodes/test.go new file mode 100644 index 00000000000..f9d4b3d8e65 --- /dev/null +++ b/ql/test/library-tests/semmle/go/dataflow/PostUpdateNodes/test.go @@ -0,0 +1,25 @@ +package a + +type C struct { + field int +} + +type B struct { + cptr *C +} + +type A struct { + b B + bptr *B + bs [5]B +} + +func f() { + + a := A{} + a.bptr = &a.b + a.bs[3].cptr = &C{} + a.bs[3].cptr.field = 100 + a.bptr.cptr.field = 101 + +} diff --git a/ql/test/library-tests/semmle/go/dataflow/PostUpdateNodes/test.ql b/ql/test/library-tests/semmle/go/dataflow/PostUpdateNodes/test.ql new file mode 100644 index 00000000000..384b3346431 --- /dev/null +++ b/ql/test/library-tests/semmle/go/dataflow/PostUpdateNodes/test.ql @@ -0,0 +1,4 @@ +import go + +from DataFlow::PostUpdateNode pun +select pun