From 3817ae80e5eb2186946116815fd78984fb47cd36 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 21 Oct 2020 15:27:43 +0100 Subject: [PATCH] Add support for html.Render method. This entails generalising Http::ResponseBody to account for any modelled function writing to a ResponseWriter. --- change-notes/2020-10-12-x-net-html.md | 1 + ql/src/semmle/go/frameworks/XNetHtml.qll | 11 ++++- .../semmle/go/frameworks/stdlib/NetHttp.qll | 36 ++++++++-------- .../frameworks/XNetHtml/ReflectedXss.expected | 42 ++++++++++--------- .../semmle/go/frameworks/XNetHtml/test.go | 2 + .../vendor/golang.org/x/net/html/stub.go | 6 ++- 6 files changed, 57 insertions(+), 41 deletions(-) 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 +}