diff --git a/change-notes/2020-10-12-x-net-html.md b/change-notes/2020-10-12-x-net-html.md
index d0ffca3c9fa..4644ecf9e09 100644
--- a/change-notes/2020-10-12-x-net-html.md
+++ b/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.
diff --git a/ql/src/semmle/go/frameworks/XNetHtml.qll b/ql/src/semmle/go/frameworks/XNetHtml.qll
index 096b3e847e2..81fcce45428 100644
--- a/ql/src/semmle/go/frameworks/XNetHtml.qll
+++ b/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
diff --git a/ql/src/semmle/go/frameworks/stdlib/NetHttp.qll b/ql/src/semmle/go/frameworks/stdlib/NetHttp.qll
index 22dbcfe473f..bd406400cb4 100644
--- a/ql/src/semmle/go/frameworks/stdlib/NetHttp.qll
+++ b/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 {
diff --git a/ql/test/library-tests/semmle/go/frameworks/XNetHtml/ReflectedXss.expected b/ql/test/library-tests/semmle/go/frameworks/XNetHtml/ReflectedXss.expected
index 96ec36de14c..9b02118770b 100644
--- a/ql/test/library-tests/semmle/go/frameworks/XNetHtml/ReflectedXss.expected
+++ b/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 |
diff --git a/ql/test/library-tests/semmle/go/frameworks/XNetHtml/test.go b/ql/test/library-tests/semmle/go/frameworks/XNetHtml/test.go
index de5832559c2..466eba3cc2d 100644
--- a/ql/test/library-tests/semmle/go/frameworks/XNetHtml/test.go
+++ b/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
diff --git a/ql/test/library-tests/semmle/go/frameworks/XNetHtml/vendor/golang.org/x/net/html/stub.go b/ql/test/library-tests/semmle/go/frameworks/XNetHtml/vendor/golang.org/x/net/html/stub.go
index 3eaf4cd2a33..02d4a4fa55b 100644
--- a/ql/test/library-tests/semmle/go/frameworks/XNetHtml/vendor/golang.org/x/net/html/stub.go
+++ b/ql/test/library-tests/semmle/go/frameworks/XNetHtml/vendor/golang.org/x/net/html/stub.go
@@ -2,7 +2,7 @@
// This is a simple stub for golang.org/x/net/html, strictly for use in testing.
// See the LICENSE file for information about the licensing of the original library.
-// Source: golang.org/x/net/html (exports: Node,Token,Tokenizer; functions: EscapeString,UnescapeString,Parse,ParseWithOptions,ParseFragment,ParseFragmentWithOptions,NewTokenizer)
+// Source: golang.org/x/net/html (exports: Node,Token,Tokenizer; functions: EscapeString,UnescapeString,Parse,ParseWithOptions,ParseFragment,ParseFragmentWithOptions,NewTokenizer,Render)
// Package html is a stub of golang.org/x/net/html, generated by depstubber.
package html
@@ -124,3 +124,7 @@ func UnescapeString(_ string) string {
func NewTokenizer(r io.Reader) *Tokenizer {
return nil
}
+
+func Render(w io.Writer, n *Node) error {
+ return nil
+}