From c0cbb4a5329d5179861a03a935aa0666c1bd116b Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Tue, 8 Oct 2024 14:01:52 +0100 Subject: [PATCH] template/text.Template execution methods: support reading arbitrary content --- go/ql/lib/ext/text.template.model.yml | 4 +- .../dataflow/internal/TaintTrackingUtil.qll | 72 +++++++++++++++---- .../tainttracking1/TaintTrackingImpl.qll | 2 + .../go/frameworks/stdlib/TextTemplate.qll | 40 +++++++++++ .../go/frameworks/serialization/test.expected | 0 .../go/frameworks/serialization/test.ql | 9 +++ .../frameworks/serialization/texttemplate.go | 50 +++++++++++++ .../codeql/dataflow/TaintTracking.qll | 8 +++ 8 files changed, 169 insertions(+), 16 deletions(-) create mode 100644 go/ql/test/library-tests/semmle/go/frameworks/serialization/test.expected create mode 100644 go/ql/test/library-tests/semmle/go/frameworks/serialization/test.ql create mode 100644 go/ql/test/library-tests/semmle/go/frameworks/serialization/texttemplate.go diff --git a/go/ql/lib/ext/text.template.model.yml b/go/ql/lib/ext/text.template.model.yml index 669af3a8854fc..3ff0321a43c20 100644 --- a/go/ql/lib/ext/text.template.model.yml +++ b/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. diff --git a/go/ql/lib/semmle/go/dataflow/internal/TaintTrackingUtil.qll b/go/ql/lib/semmle/go/dataflow/internal/TaintTrackingUtil.qll index 1b521d89d98da..e09644a7ef1cc 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/TaintTrackingUtil.qll +++ b/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. * diff --git a/go/ql/lib/semmle/go/dataflow/internal/tainttracking1/TaintTrackingImpl.qll b/go/ql/lib/semmle/go/dataflow/internal/tainttracking1/TaintTrackingImpl.qll index 75e7856fd2613..1a33ee7cd0571 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/tainttracking1/TaintTrackingImpl.qll +++ b/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) } /** diff --git a/go/ql/lib/semmle/go/frameworks/stdlib/TextTemplate.qll b/go/ql/lib/semmle/go/frameworks/stdlib/TextTemplate.qll index 4ef4da0583959..1770d9da4a0af 100644 --- a/go/ql/lib/semmle/go/frameworks/stdlib/TextTemplate.qll +++ b/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 + } + } } diff --git a/go/ql/test/library-tests/semmle/go/frameworks/serialization/test.expected b/go/ql/test/library-tests/semmle/go/frameworks/serialization/test.expected new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/go/ql/test/library-tests/semmle/go/frameworks/serialization/test.ql b/go/ql/test/library-tests/semmle/go/frameworks/serialization/test.ql new file mode 100644 index 0000000000000..171f2a1181b7e --- /dev/null +++ b/go/ql/test/library-tests/semmle/go/frameworks/serialization/test.ql @@ -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 diff --git a/go/ql/test/library-tests/semmle/go/frameworks/serialization/texttemplate.go b/go/ql/test/library-tests/semmle/go/frameworks/serialization/texttemplate.go new file mode 100644 index 0000000000000..85a1372752ff8 --- /dev/null +++ b/go/ql/test/library-tests/semmle/go/frameworks/serialization/texttemplate.go @@ -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 + +} diff --git a/shared/dataflow/codeql/dataflow/TaintTracking.qll b/shared/dataflow/codeql/dataflow/TaintTracking.qll index 343f8be041f51..a35e0f89a350d 100644 --- a/shared/dataflow/codeql/dataflow/TaintTracking.qll +++ b/shared/dataflow/codeql/dataflow/TaintTracking.qll @@ -29,6 +29,12 @@ signature module InputSig 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) } }