template/text.Template execution methods: support reading arbitrary content

Author: smowton

go/ql/lib/ext/text.template.model.yml

@@ -7,5 +7,5 @@ extensions:
       - ["text/template", "", False, "HTMLEscapeString", "", "", "Argument[0]", "ReturnValue", "taint", "manual"]
       - ["text/template", "", False, "JSEscape", "", "", "Argument[1]", "Argument[0]", "taint", "manual"]
       - ["text/template", "", False, "JSEscapeString", "", "", "Argument[0]", "ReturnValue", "taint", "manual"]
-      - ["text/template", "Template", True, "Execute", "", "", "Argument[1]", "Argument[0]", "taint", "manual"]
-      - ["text/template", "Template", True, "ExecuteTemplate", "", "", "Argument[2]", "Argument[0]", "taint", "manual"]
+#      - ["text/template", "Template", True, "Execute", "", "", "Argument[1]", "Argument[0]", "taint", "manual"] # Implemented in QL to provide an arbitrary content read from the input.
+#      - ["text/template", "Template", True, "ExecuteTemplate", "", "", "Argument[2]", "Argument[0]", "taint", "manual"] # Implemented in QL to provide an arbitrary content read from the input.

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

@@ -39,7 +39,33 @@ private Type getElementType(Type containerType) {
   result = containerType.(SliceType).getElementType() or
   result = containerType.(ChanType).getElementType() or
   result = containerType.(MapType).getValueType() or
-  result = containerType.(PointerType).getPointerType()
+  result = containerType.(PointerType).getPointerType() or
+  result = containerType.(NamedType).getUnderlyingType()
+}
+
+private Type getElementTypeIncludingFields(Type containerType) {
+  result = getElementType(containerType) or
+  result = containerType.(StructType).getField(_).getType()
+}
+
+private DataFlow::ContentSet getContentForType(Type containerType) {
+  containerType instanceof ArrayType and
+  result instanceof DataFlow::ArrayContent
+  or
+  containerType instanceof SliceType and
+  result instanceof DataFlow::ArrayContent
+  or
+  containerType instanceof ChanType and
+  result instanceof DataFlow::CollectionContent
+  or
+  containerType instanceof MapType and
+  result instanceof DataFlow::MapValueContent
+  or
+  result.(DataFlow::PointerContent).getPointerType() = containerType
+  or
+  exists(Field f | f = containerType.(StructType).getField(_) |
+    result.(DataFlow::FieldContent).getField() = f
+  )
 }
 
 /**
@@ -51,23 +77,41 @@ predicate defaultImplicitTaintRead(DataFlow::Node node, DataFlow::ContentSet c)
   exists(Type containerType |
     node instanceof DataFlow::ArgumentNode and
     getElementType*(node.getType()) = containerType
-  |
-    containerType instanceof ArrayType and
-    c instanceof DataFlow::ArrayContent
-    or
-    containerType instanceof SliceType and
-    c instanceof DataFlow::ArrayContent
     or
-    containerType instanceof ChanType and
-    c instanceof DataFlow::CollectionContent
-    or
-    containerType instanceof MapType and
-    c instanceof DataFlow::MapValueContent
-    or
-    c.(DataFlow::PointerContent).getPointerType() = containerType
+    any(ImplicitFieldReadNode ifrn).shouldImplicitlyReadAllFields(node) and
+    getElementTypeIncludingFields*(node.getType()) = containerType
+  |
+    c = getContentForType(containerType)
   )
 }
 
+/**
+ * Holds if default `TaintTracking::Configuration`s should allow implicit reads
+ * of `c` at `node` in any context.
+ */
+bindingset[node]
+predicate defaultImplicitTaintReadGlobal(DataFlow::Node node, DataFlow::ContentSet c) {
+  exists(Type containerType |
+    any(ImplicitFieldReadNode ifrn).shouldImplicitlyReadAllFields(node) and
+    getElementTypeIncludingFields*(node.getType()) = containerType
+  |
+    c = getContentForType(containerType)
+  )
+}
+
+/**
+ * A unit class for adding nodes that should implicitly read from all nested content
+ * in a taint-tracking context.
+ *
+ * For example, this might be appopriate for the argument to a method that serializes a struct.
+ */
+class ImplicitFieldReadNode extends Unit {
+  /**
+   * Holds if the node `n` should implicitly read from all nested content in a taint-tracking context.
+   */
+  abstract predicate shouldImplicitlyReadAllFields(DataFlow::Node n);
+}
+
 /**
  * A unit class for adding additional taint steps.
  *

go/ql/lib/semmle/go/dataflow/internal/tainttracking1/TaintTrackingImpl.qll

@@ -156,6 +156,8 @@ abstract deprecated class Configuration extends DataFlow::Configuration {
       this.isAdditionalTaintStep(node, _, _, _)
     ) and
     defaultImplicitTaintRead(node, c)
+    or
+    defaultImplicitTaintReadGlobal(node, c)
   }
 
   /**

go/ql/lib/semmle/go/frameworks/stdlib/TextTemplate.qll

@@ -67,4 +67,44 @@ module TextTemplate {
       input = inp and output = outp
     }
   }
+
+  private class ExecuteTemplateMethod extends Method {
+    string name;
+    int inputArg;
+
+    ExecuteTemplateMethod() {
+      this.hasQualifiedName("text/template", "Template", name) and
+      (
+        name = "Execute" and inputArg = 1
+        or
+        name = "ExecuteTemplate" and inputArg = 2
+      )
+    }
+
+    int getInputArgIdx() { result = inputArg }
+  }
+
+  private class ExecuteTemplateFieldReader extends TaintTracking::ImplicitFieldReadNode {
+    override predicate shouldImplicitlyReadAllFields(DataFlow::Node n) {
+      exists(ExecuteTemplateMethod m, DataFlow::MethodCallNode cn |
+        cn.getACalleeIncludingExternals().asFunction() = m and
+        n = cn.getArgument(m.getInputArgIdx())
+      )
+    }
+  }
+
+  private class ExecuteTemplateFunctionModels extends TaintTracking::FunctionModel,
+    ExecuteTemplateMethod
+  {
+    FunctionInput inp;
+    FunctionOutput outp;
+
+    ExecuteTemplateFunctionModels() {
+      inp.isParameter(this.getInputArgIdx()) and outp.isParameter(0)
+    }
+
+    override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
+      input = inp and output = outp
+    }
+  }
 }

go/ql/test/library-tests/semmle/go/frameworks/serialization/test.expected

@@ -0,0 +1,9 @@
+import go
+import TestUtilities.InlineFlowTest
+
+string getArgString(DataFlow::Node src, DataFlow::Node sink) {
+  exists(sink) and
+  result = src.(DataFlow::CallNode).getArgument(0).getExactValue()
+}
+
+import TaintFlowTestArgString<DefaultFlowConfig, getArgString/2>

go/ql/test/library-tests/semmle/go/frameworks/serialization/test.ql

@@ -0,0 +1,50 @@
+package xyz
+
+import (
+	"bytes"
+	"text/template"
+)
+
+type Inner1 struct {
+	Data string
+}
+
+type Inner2 struct {
+	Data string
+}
+
+type Inner3 struct {
+	Data string
+}
+
+type Outer struct {
+	SliceField []Inner1
+	PtrField   *Inner2
+	MapField   map[string]Inner3
+}
+
+func source(n int) string { return "dummy" }
+func sink(arg any)        {}
+
+func test() {
+
+	source1 := source(1)
+	source2 := source(2)
+	source3 := source(3)
+
+	toSerialize := Outer{[]Inner1{{source1}}, &Inner2{source2}, map[string]Inner3{"key": {source3}}}
+	buff1 := new(bytes.Buffer)
+	buff2 := new(bytes.Buffer)
+	bytes1 := make([]byte, 10)
+	bytes2 := make([]byte, 10)
+
+	tmpl, _ := template.New("test").Parse("Template text goes here (irrelevant for test)")
+	tmpl.ExecuteTemplate(buff1, "test", toSerialize)
+	buff1.Read(bytes1)
+	sink(bytes1) // $ hasTaintFlow=1 hasTaintFlow=2 hasTaintFlow=3
+
+	tmpl.Execute(buff2, toSerialize)
+	buff2.Read(bytes2)
+	sink(bytes2) // $ hasTaintFlow=1 hasTaintFlow=2 hasTaintFlow=3
+
+}

go/ql/test/library-tests/semmle/go/frameworks/serialization/texttemplate.go

@@ -29,6 +29,12 @@ signature module InputSig<LocationSig Location, DF::InputSig<Location> Lang> {
    */
   bindingset[node]
   predicate defaultImplicitTaintRead(Lang::Node node, Lang::ContentSet c);
+
+  /**
+   * Holds if taint flow configurations should allow implicit reads of `c` in any context.
+   */
+  bindingset[node]
+  predicate defaultImplicitTaintReadGlobal(Lang::Node node, Lang::ContentSet c);
 }
 
 /**
@@ -66,6 +72,8 @@ module TaintFlowMake<
         Config::isAdditionalFlowStep(node, _, _, _)
       ) and
       defaultImplicitTaintRead(node, c)
+      or
+      defaultImplicitTaintReadGlobal(node, c)
     }
   }