Add support for html.Render method.

This entails generalising Http::ResponseBody to account for any modelled function writing to a ResponseWriter.

Author: smowton

change-notes/2020-10-12-x-net-html.md

@@ -1,2 +1,3 @@
 lgtm,codescanning
 * Added partial support for the `golang.org/x/net/html` package, modeling tainted data flow from a retrieved HTML document to its attributes and other data.
+* Modeled more ways of writing data to an `net/http.ResponseWriter`. This may produce more results from queries such as `go/reflected-xss` which look for data flowing to an HTTP response.

ql/src/semmle/go/frameworks/XNetHtml.qll

@@ -2,8 +2,9 @@
  * Provides classes modeling security-relevant aspects of the `golang.org/x/net/html` subpackage.
  *
  * Currently we support the unmarshalling aspect of this package, conducting taint from an untrusted
- * reader to an untrusted `Node` tree or `Tokenizer` instance. We do not yet model adding a child
- * `Node` to a tree then calling `Render` yielding an untrustworthy string.
+ * reader to an untrusted `Node` tree or `Tokenizer` instance, as well as simple remarshalling of `Node`s
+ * that were already untrusted. We do not yet model adding a child `Node` to a tree then calling `Render`
+ * yielding an untrustworthy string.
  */
 
 import go
@@ -34,12 +35,18 @@ module XNetHtml {
       getName() = ["AppendChild", "InsertBefore"] and
       input.isParameter(0) and
       output.isReceiver()
+      or
+      getName() = "Render" and
+      input.isParameter(1) and
+      output.isParameter(0)
     }
   }
 
   private class TokenizerMethodModels extends Method, TaintTracking::FunctionModel {
     TokenizerMethodModels() { this.hasQualifiedName(packagePath(), "Tokenizer", _) }
 
+    // Note that `TagName` and the key part of `TagAttr` are not sources by default under the assumption
+    // that their character-set restrictions usually rule them out as useful attack routes.
     override predicate hasTaintFlow(DataFlow::FunctionInput input, DataFlow::FunctionOutput output) {
       getName() = ["Buffered", "Raw", "Text", "Token"] and input.isReceiver() and output.isResult(0)
       or

ql/src/semmle/go/frameworks/stdlib/NetHttp.qll

@@ -135,33 +135,31 @@ module NetHttp {
   }
 
   private class ResponseBody extends HTTP::ResponseBody::Range, DataFlow::ArgumentNode {
-    int arg;
+    DataFlow::Node responseWriter;
 
     ResponseBody() {
       exists(DataFlow::CallNode call |
+        // A direct call to ResponseWriter.Write, conveying taint from the argument to the receiver
         call.getTarget().(Method).implements("net/http", "ResponseWriter", "Write") and
-        arg = 0
-        or
-        (
-          call.getTarget().hasQualifiedName("fmt", "Fprintf")
-          or
-          call.getTarget().hasQualifiedName("io", "WriteString")
-        ) and
-        call.getArgument(0).getType().hasQualifiedName("net/http", "ResponseWriter") and
-        arg >= 1
+        this = call.getArgument(0) and
+        responseWriter = call.(DataFlow::MethodCallNode).getReceiver()
+      )
+      or
+      exists(
+        TaintTracking::FunctionModel model, FunctionOutput modelOutput, FunctionInput modelInput,
+        DataFlow::CallNode call
       |
-        this = call.getArgument(arg)
+        // A modelled function conveying taint from some input to the response writer,
+        // e.g. `io.Copy(responseWriter, someTaintedReader)`
+        call = model.getACall() and
+        model.hasTaintFlow(modelInput, modelOutput) and
+        this = modelInput.getNode(call) and
+        responseWriter = modelOutput.getNode(call).(DataFlow::PostUpdateNode).getPreUpdateNode() and
+        responseWriter.getType().implements("net/http", "ResponseWriter")
       )
     }
 
-    override HTTP::ResponseWriter getResponseWriter() {
-      // the response writer is the receiver of this call
-      result.getANode() = this.getCall().(DataFlow::MethodCallNode).getReceiver()
-      or
-      // the response writer is an argument to Fprintf or WriteString
-      arg >= 1 and
-      result.getANode() = this.getCall().getArgument(0)
-    }
+    override HTTP::ResponseWriter getResponseWriter() { result.getANode() = responseWriter }
   }
 
   private class RedirectCall extends HTTP::Redirect::Range, DataFlow::CallNode {

ql/test/library-tests/semmle/go/frameworks/XNetHtml/ReflectedXss.expected

@@ -5,8 +5,10 @@ edges
 | test.go:14:42:14:47 | implicit dereference : Cookie | test.go:14:42:14:47 | implicit dereference : Cookie |
 | test.go:16:24:16:35 | selection of Body : ReadCloser | test.go:17:15:17:31 | type conversion |
 | test.go:16:24:16:35 | selection of Body : ReadCloser | test.go:17:22:17:25 | implicit dereference : Node |
+| test.go:16:24:16:35 | selection of Body : ReadCloser | test.go:28:22:28:25 | node |
 | test.go:17:22:17:25 | implicit dereference : Node | test.go:17:15:17:31 | type conversion |
 | test.go:17:22:17:25 | implicit dereference : Node | test.go:17:22:17:25 | implicit dereference : Node |
+| test.go:17:22:17:25 | implicit dereference : Node | test.go:28:22:28:25 | node |
 | test.go:19:36:19:47 | selection of Body : ReadCloser | test.go:20:15:20:32 | type conversion |
 | test.go:19:36:19:47 | selection of Body : ReadCloser | test.go:20:22:20:26 | implicit dereference : Node |
 | test.go:20:22:20:26 | implicit dereference : Node | test.go:20:15:20:32 | type conversion |
@@ -19,13 +21,13 @@ edges
 | test.go:25:45:25:56 | selection of Body : ReadCloser | test.go:26:22:26:30 | implicit dereference : Node |
 | test.go:26:22:26:30 | implicit dereference : Node | test.go:26:15:26:36 | type conversion |
 | test.go:26:22:26:30 | implicit dereference : Node | test.go:26:22:26:30 | implicit dereference : Node |
-| test.go:28:33:28:44 | selection of Body : ReadCloser | test.go:29:15:29:34 | call to Buffered |
-| test.go:28:33:28:44 | selection of Body : ReadCloser | test.go:30:15:30:29 | call to Raw |
-| test.go:28:33:28:44 | selection of Body : ReadCloser | test.go:32:15:32:19 | value |
-| test.go:28:33:28:44 | selection of Body : ReadCloser | test.go:33:15:33:30 | call to Text |
-| test.go:28:33:28:44 | selection of Body : ReadCloser | test.go:34:15:34:44 | type conversion |
-| test.go:28:33:28:44 | selection of Body : ReadCloser | test.go:34:22:34:38 | call to Token : Token |
-| test.go:34:22:34:38 | call to Token : Token | test.go:34:15:34:44 | type conversion |
+| test.go:30:33:30:44 | selection of Body : ReadCloser | test.go:31:15:31:34 | call to Buffered |
+| test.go:30:33:30:44 | selection of Body : ReadCloser | test.go:32:15:32:29 | call to Raw |
+| test.go:30:33:30:44 | selection of Body : ReadCloser | test.go:34:15:34:19 | value |
+| test.go:30:33:30:44 | selection of Body : ReadCloser | test.go:35:15:35:30 | call to Text |
+| test.go:30:33:30:44 | selection of Body : ReadCloser | test.go:36:15:36:44 | type conversion |
+| test.go:30:33:30:44 | selection of Body : ReadCloser | test.go:36:22:36:38 | call to Token : Token |
+| test.go:36:22:36:38 | call to Token : Token | test.go:36:15:36:44 | type conversion |
 nodes
 | test.go:10:15:10:42 | call to Cookie : tuple type | semmle.label | call to Cookie : tuple type |
 | test.go:14:15:14:55 | type conversion | semmle.label | type conversion |
@@ -42,21 +44,23 @@ nodes
 | test.go:25:45:25:56 | selection of Body : ReadCloser | semmle.label | selection of Body : ReadCloser |
 | test.go:26:15:26:36 | type conversion | semmle.label | type conversion |
 | test.go:26:22:26:30 | implicit dereference : Node | semmle.label | implicit dereference : Node |
-| test.go:28:33:28:44 | selection of Body : ReadCloser | semmle.label | selection of Body : ReadCloser |
-| test.go:29:15:29:34 | call to Buffered | semmle.label | call to Buffered |
-| test.go:30:15:30:29 | call to Raw | semmle.label | call to Raw |
-| test.go:32:15:32:19 | value | semmle.label | value |
-| test.go:33:15:33:30 | call to Text | semmle.label | call to Text |
-| test.go:34:15:34:44 | type conversion | semmle.label | type conversion |
-| test.go:34:22:34:38 | call to Token : Token | semmle.label | call to Token : Token |
+| test.go:28:22:28:25 | node | semmle.label | node |
+| test.go:30:33:30:44 | selection of Body : ReadCloser | semmle.label | selection of Body : ReadCloser |
+| test.go:31:15:31:34 | call to Buffered | semmle.label | call to Buffered |
+| test.go:32:15:32:29 | call to Raw | semmle.label | call to Raw |
+| test.go:34:15:34:19 | value | semmle.label | value |
+| test.go:35:15:35:30 | call to Text | semmle.label | call to Text |
+| test.go:36:15:36:44 | type conversion | semmle.label | type conversion |
+| test.go:36:22:36:38 | call to Token : Token | semmle.label | call to Token : Token |
 #select
 | test.go:14:15:14:55 | type conversion | test.go:10:15:10:42 | call to Cookie : tuple type | test.go:14:15:14:55 | type conversion | Cross-site scripting vulnerability due to $@. | test.go:10:15:10:42 | call to Cookie | user-provided value |
 | test.go:17:15:17:31 | type conversion | test.go:16:24:16:35 | selection of Body : ReadCloser | test.go:17:15:17:31 | type conversion | Cross-site scripting vulnerability due to $@. | test.go:16:24:16:35 | selection of Body | user-provided value |
 | test.go:20:15:20:32 | type conversion | test.go:19:36:19:47 | selection of Body : ReadCloser | test.go:20:15:20:32 | type conversion | Cross-site scripting vulnerability due to $@. | test.go:19:36:19:47 | selection of Body | user-provided value |
 | test.go:23:15:23:35 | type conversion | test.go:22:33:22:44 | selection of Body : ReadCloser | test.go:23:15:23:35 | type conversion | Cross-site scripting vulnerability due to $@. | test.go:22:33:22:44 | selection of Body | user-provided value |
 | test.go:26:15:26:36 | type conversion | test.go:25:45:25:56 | selection of Body : ReadCloser | test.go:26:15:26:36 | type conversion | Cross-site scripting vulnerability due to $@. | test.go:25:45:25:56 | selection of Body | user-provided value |
-| test.go:29:15:29:34 | call to Buffered | test.go:28:33:28:44 | selection of Body : ReadCloser | test.go:29:15:29:34 | call to Buffered | Cross-site scripting vulnerability due to $@. | test.go:28:33:28:44 | selection of Body | user-provided value |
-| test.go:30:15:30:29 | call to Raw | test.go:28:33:28:44 | selection of Body : ReadCloser | test.go:30:15:30:29 | call to Raw | Cross-site scripting vulnerability due to $@. | test.go:28:33:28:44 | selection of Body | user-provided value |
-| test.go:32:15:32:19 | value | test.go:28:33:28:44 | selection of Body : ReadCloser | test.go:32:15:32:19 | value | Cross-site scripting vulnerability due to $@. | test.go:28:33:28:44 | selection of Body | user-provided value |
-| test.go:33:15:33:30 | call to Text | test.go:28:33:28:44 | selection of Body : ReadCloser | test.go:33:15:33:30 | call to Text | Cross-site scripting vulnerability due to $@. | test.go:28:33:28:44 | selection of Body | user-provided value |
-| test.go:34:15:34:44 | type conversion | test.go:28:33:28:44 | selection of Body : ReadCloser | test.go:34:15:34:44 | type conversion | Cross-site scripting vulnerability due to $@. | test.go:28:33:28:44 | selection of Body | user-provided value |
+| test.go:28:22:28:25 | node | test.go:16:24:16:35 | selection of Body : ReadCloser | test.go:28:22:28:25 | node | Cross-site scripting vulnerability due to $@. | test.go:16:24:16:35 | selection of Body | user-provided value |
+| test.go:31:15:31:34 | call to Buffered | test.go:30:33:30:44 | selection of Body : ReadCloser | test.go:31:15:31:34 | call to Buffered | Cross-site scripting vulnerability due to $@. | test.go:30:33:30:44 | selection of Body | user-provided value |
+| test.go:32:15:32:29 | call to Raw | test.go:30:33:30:44 | selection of Body : ReadCloser | test.go:32:15:32:29 | call to Raw | Cross-site scripting vulnerability due to $@. | test.go:30:33:30:44 | selection of Body | user-provided value |
+| test.go:34:15:34:19 | value | test.go:30:33:30:44 | selection of Body : ReadCloser | test.go:34:15:34:19 | value | Cross-site scripting vulnerability due to $@. | test.go:30:33:30:44 | selection of Body | user-provided value |
+| test.go:35:15:35:30 | call to Text | test.go:30:33:30:44 | selection of Body : ReadCloser | test.go:35:15:35:30 | call to Text | Cross-site scripting vulnerability due to $@. | test.go:30:33:30:44 | selection of Body | user-provided value |
+| test.go:36:15:36:44 | type conversion | test.go:30:33:30:44 | selection of Body : ReadCloser | test.go:36:15:36:44 | type conversion | Cross-site scripting vulnerability due to $@. | test.go:30:33:30:44 | selection of Body | user-provided value |

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

@@ -25,6 +25,8 @@ func test(request *http.Request, writer http.ResponseWriter) {
 	nodes2, _ := html.ParseFragmentWithOptions(request.Body, nil)
 	writer.Write([]byte(nodes2[0].Data)) // BAD: writing unescaped HTML data
 
+	html.Render(writer, node) // BAD: rendering untrusted HTML to `writer`
+
 	tokenizer := html.NewTokenizer(request.Body)
 	writer.Write(tokenizer.Buffered()) // BAD: writing unescaped HTML data
 	writer.Write(tokenizer.Raw())      // BAD: writing unescaped HTML data