Merge pull request #69 from max-schaefer/issue-72

Track taint through element writes.

ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll

@@ -396,7 +396,9 @@ class PostUpdateNode extends Node {
       or
       preupd = any(PointerDereferenceNode deref).getOperand()
       or
-      exists(Write w, DataFlow::Node base | w.writesField(base, _, _) |
+      exists(Write w, DataFlow::Node base |
+        w.writesField(base, _, _) or w.writesElement(base, _, _)
+      |
         preupd = base
         or
         preupd = base.(PointerDereferenceNode).getOperand()

ql/src/semmle/go/dataflow/internal/TaintTrackingUtil.qll

@@ -50,6 +50,7 @@ class AdditionalTaintStep extends Unit {
  */
 predicate localAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
   referenceStep(pred, succ) or
+  elementWriteStep(pred, succ) or
   fieldReadStep(pred, succ) or
   arrayStep(pred, succ) or
   tupleStep(pred, succ) or
@@ -87,6 +88,14 @@ predicate referenceStep(DataFlow::Node pred, DataFlow::Node succ) {
   )
 }
 
+/**
+ * Holds if there is an assignment of the form `succ[idx] = pred`, meaning that `pred` may taint
+ * `succ`.
+ */
+predicate elementWriteStep(DataFlow::Node pred, DataFlow::Node succ) {
+  any(DataFlow::Write w).writesElement(succ.(DataFlow::PostUpdateNode).getPreUpdateNode(), _, pred)
+}
+
 /** Holds if taint flows from `pred` to `succ` via a field read. */
 predicate fieldReadStep(DataFlow::Node pred, DataFlow::Node succ) {
   succ.(DataFlow::FieldReadNode).getBase() = pred
@@ -105,7 +114,8 @@ predicate tupleStep(DataFlow::Node pred, DataFlow::Node succ) {
 /** Holds if taint flows from `pred` to `succ` via string concatenation. */
 predicate stringConcatStep(DataFlow::Node pred, DataFlow::Node succ) {
   exists(DataFlow::BinaryOperationNode conc |
-    conc.getOperator() = "+" and conc.getType() instanceof StringType |
+    conc.getOperator() = "+" and conc.getType() instanceof StringType
+  |
     succ = conc and conc.getAnOperand() = pred
   )
 }

ql/test/library-tests/semmle/go/dataflow/FlowSteps/LocalTaintStep.expected

@@ -1,5 +1,8 @@
 | main.go:26:11:26:17 | type assertion | main.go:26:2:26:17 | ... := ...[0] |
 | main.go:26:11:26:17 | type assertion | main.go:26:2:26:17 | ... := ...[1] |
+| main.go:38:13:38:13 | 1 | main.go:38:7:38:20 | composite literal |
+| main.go:38:16:38:16 | 2 | main.go:38:7:38:20 | composite literal |
+| main.go:38:19:38:19 | 3 | main.go:38:7:38:20 | composite literal |
 | main.go:39:15:39:15 | s | main.go:39:8:39:25 | call to append |
 | main.go:39:18:39:18 | 4 | main.go:39:8:39:25 | call to append |
 | main.go:39:21:39:21 | 5 | main.go:39:8:39:25 | call to append |

ql/test/library-tests/semmle/go/frameworks/TaintSteps/TaintStep.expected

@@ -11,6 +11,13 @@
 | main.go:12:14:12:52 | call to MarshalIndent | main.go:12:2:12:52 | ... := ...[0] |
 | main.go:12:14:12:52 | call to MarshalIndent | main.go:12:2:12:52 | ... := ...[1] |
 | main.go:12:33:12:33 | v | main.go:12:2:12:52 | ... := ...[0] |
+| main.go:13:25:13:25 | b | main.go:13:9:13:41 | composite literal |
+| main.go:13:28:13:30 | err | main.go:13:9:13:41 | composite literal |
+| main.go:13:33:13:34 | b2 | main.go:13:9:13:41 | composite literal |
+| main.go:13:37:13:40 | err2 | main.go:13:9:13:41 | composite literal |
 | main.go:18:18:18:42 | call to DecodeString | main.go:18:2:18:42 | ... := ...[0] |
 | main.go:18:18:18:42 | call to DecodeString | main.go:18:2:18:42 | ... := ...[1] |
 | main.go:18:35:18:41 | encoded | main.go:18:2:18:42 | ... := ...[0] |
+| main.go:22:25:22:31 | decoded | main.go:22:9:22:48 | composite literal |
+| main.go:22:34:22:36 | err | main.go:22:9:22:48 | composite literal |
+| main.go:22:39:22:47 | reEncoded | main.go:22:9:22:48 | composite literal |

ql/test/query-tests/Security/CWE-079/ReflectedXss.expected

@@ -2,6 +2,7 @@ edges
 | ReflectedXss.go:11:15:11:20 | selection of Form : Values | ReflectedXss.go:14:44:14:51 | username |
 | contenttype.go:11:11:11:16 | selection of Form : Values | contenttype.go:17:11:17:22 | type conversion |
 | contenttype.go:49:11:49:16 | selection of Form : Values | contenttype.go:53:34:53:37 | data |
+| tst.go:11:15:11:20 | selection of Form : Values | tst.go:15:12:15:39 | type conversion |
 nodes
 | ReflectedXss.go:11:15:11:20 | selection of Form : Values | semmle.label | selection of Form : Values |
 | ReflectedXss.go:14:44:14:51 | username | semmle.label | username |
@@ -9,7 +10,10 @@ nodes
 | contenttype.go:17:11:17:22 | type conversion | semmle.label | type conversion |
 | contenttype.go:49:11:49:16 | selection of Form : Values | semmle.label | selection of Form : Values |
 | contenttype.go:53:34:53:37 | data | semmle.label | data |
+| tst.go:11:15:11:20 | selection of Form : Values | semmle.label | selection of Form : Values |
+| tst.go:15:12:15:39 | type conversion | semmle.label | type conversion |
 #select
 | ReflectedXss.go:14:44:14:51 | username | ReflectedXss.go:11:15:11:20 | selection of Form : Values | ReflectedXss.go:14:44:14:51 | username | Cross-site scripting vulnerability due to $@. | ReflectedXss.go:11:15:11:20 | selection of Form | user-provided value |
 | contenttype.go:17:11:17:22 | type conversion | contenttype.go:11:11:11:16 | selection of Form : Values | contenttype.go:17:11:17:22 | type conversion | Cross-site scripting vulnerability due to $@. | contenttype.go:11:11:11:16 | selection of Form | user-provided value |
 | contenttype.go:53:34:53:37 | data | contenttype.go:49:11:49:16 | selection of Form : Values | contenttype.go:53:34:53:37 | data | Cross-site scripting vulnerability due to $@. | contenttype.go:49:11:49:16 | selection of Form | user-provided value |
+| tst.go:15:12:15:39 | type conversion | tst.go:11:15:11:20 | selection of Form : Values | tst.go:15:12:15:39 | type conversion | Cross-site scripting vulnerability due to $@. | tst.go:11:15:11:20 | selection of Form | user-provided value |

ql/test/query-tests/Security/CWE-079/tst.go

@@ -0,0 +1,21 @@
+package main
+
+import (
+	"net/http"
+	"strings"
+)
+
+func serve6() {
+	http.HandleFunc("/user", func(w http.ResponseWriter, r *http.Request) {
+		r.ParseForm()
+		username := r.Form.Get("username")
+		if !isValidUsername(username) {
+			// BAD: a request parameter is incorporated without validation into the response
+			a := []string{username, "is", "an", "unknown", "user"}
+			w.Write([]byte(strings.Join(a, " ")))
+		} else {
+			// TODO: do something exciting
+		}
+	})
+	http.ListenAndServe(":80", nil)
+}