Skip to content
This repository was archived by the owner on Jan 5, 2023. It is now read-only.

Commit 0938437

Browse files
smowtonsmowton
authored
Merge pull request #373 from smowton/smowton/feature/golang-x-net-html
Add models for the read side of golang.org/x/net/html
2 parents e4aa252 + a78c35b commit 0938437

File tree

11 files changed

+327
-22
lines changed

11 files changed

+327
-22
lines changed

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
lgtm,codescanning
2+
* 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.
3+
* 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/go.qll

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,5 +47,6 @@ import semmle.go.frameworks.Stdlib
4747
import semmle.go.frameworks.SystemCommandExecutors
4848
import semmle.go.frameworks.Testing
4949
import semmle.go.frameworks.WebSocket
50+
import semmle.go.frameworks.XNetHtml
5051
import semmle.go.frameworks.XPath
5152
import semmle.go.security.FlowSources

‎ql/src/semmle/go/Concepts.qll

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -366,7 +366,12 @@ module HTTP {
366366
* extend `HTTP::ResponseWriter` instead.
367367
*/
368368
abstract class Range extends Variable {
369-
/** Gets a data-flow node that is a use of this response writer. */
369+
/**
370+
* Gets a data-flow node that is a use of this response writer.
371+
*
372+
* Note that `PostUpdateNode`s for nodes that this predicate gets do not need to be
373+
* included, as they are handled by the concrete `ResponseWriter`'s `getANode`.
374+
*/
370375
abstract DataFlow::Node getANode();
371376
}
372377
}
@@ -392,7 +397,10 @@ module HTTP {
392397
Redirect getARedirect() { result.getResponseWriter() = this }
393398

394399
/** Gets a data-flow node that is a use of this response writer. */
395-
DataFlow::Node getANode() { result = self.getANode() }
400+
DataFlow::Node getANode() {
401+
result = self.getANode() or
402+
result.(DataFlow::PostUpdateNode).getPreUpdateNode() = self.getANode()
403+
}
396404
}
397405

398406
/** Provides a class for modeling new HTTP header-write APIs. */

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

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
/**
2+
* Provides classes modeling security-relevant aspects of the `golang.org/x/net/html` subpackage.
3+
*
4+
* Currently we support the unmarshalling aspect of this package, conducting taint from an untrusted
5+
* reader to an untrusted `Node` tree or `Tokenizer` instance, as well as simple remarshalling of `Node`s
6+
* that were already untrusted. We do not yet model adding a child `Node` to a tree then calling `Render`
7+
* yielding an untrustworthy string.
8+
*/
9+
10+
import go
11+
12+
/** Provides models of commonly used functions in the `golang.org/x/net/html` subpackage. */
13+
module XNetHtml {
14+
/** Gets the package name `golang.org/x/net/html`. */
15+
string packagePath() { result = "golang.org/x/net/html" }
16+
17+
private class EscapeString extends HtmlEscapeFunction, TaintTracking::FunctionModel {
18+
EscapeString() { this.hasQualifiedName(packagePath(), "EscapeString") }
19+
20+
override predicate hasTaintFlow(DataFlow::FunctionInput input, DataFlow::FunctionOutput output) {
21+
input.isParameter(0) and output.isResult()
22+
}
23+
}
24+
25+
private class FunctionModels extends TaintTracking::FunctionModel {
26+
FunctionModels() { hasQualifiedName(packagePath(), _) }
27+
28+
override predicate hasTaintFlow(DataFlow::FunctionInput input, DataFlow::FunctionOutput output) {
29+
getName() =
30+
["UnescapeString", "Parse", "ParseFragment", "ParseFragmentWithOptions", "ParseWithOptions",
31+
"NewTokenizer", "NewTokenizerFragment"] and
32+
input.isParameter(0) and
33+
output.isResult(0)
34+
or
35+
getName() = ["AppendChild", "InsertBefore"] and
36+
input.isParameter(0) and
37+
output.isReceiver()
38+
or
39+
getName() = "Render" and
40+
input.isParameter(1) and
41+
output.isParameter(0)
42+
}
43+
}
44+
45+
private class TokenizerMethodModels extends Method, TaintTracking::FunctionModel {
46+
TokenizerMethodModels() { this.hasQualifiedName(packagePath(), "Tokenizer", _) }
47+
48+
// Note that `TagName` and the key part of `TagAttr` are not sources by default under the assumption
49+
// that their character-set restrictions usually rule them out as useful attack routes.
50+
override predicate hasTaintFlow(DataFlow::FunctionInput input, DataFlow::FunctionOutput output) {
51+
getName() = ["Buffered", "Raw", "Text", "Token"] and input.isReceiver() and output.isResult(0)
52+
or
53+
getName() = "TagAttr" and input.isReceiver() and output.isResult(1)
54+
}
55+
}
56+
}

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

Lines changed: 12 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -135,33 +135,25 @@ module NetHttp {
135135
}
136136

137137
private class ResponseBody extends HTTP::ResponseBody::Range, DataFlow::ArgumentNode {
138-
int arg;
138+
DataFlow::Node responseWriter;
139139

140140
ResponseBody() {
141141
exists(DataFlow::CallNode call |
142+
// A direct call to ResponseWriter.Write, conveying taint from the argument to the receiver
142143
call.getTarget().(Method).implements("net/http", "ResponseWriter", "Write") and
143-
arg = 0
144-
or
145-
(
146-
call.getTarget().hasQualifiedName("fmt", "Fprintf")
147-
or
148-
call.getTarget().hasQualifiedName("io", "WriteString")
149-
) and
150-
call.getArgument(0).getType().hasQualifiedName("net/http", "ResponseWriter") and
151-
arg >= 1
152-
|
153-
this = call.getArgument(arg)
144+
this = call.getArgument(0) and
145+
responseWriter = call.(DataFlow::MethodCallNode).getReceiver()
154146
)
155-
}
156-
157-
override HTTP::ResponseWriter getResponseWriter() {
158-
// the response writer is the receiver of this call
159-
result.getANode() = this.getCall().(DataFlow::MethodCallNode).getReceiver()
160147
or
161-
// the response writer is an argument to Fprintf or WriteString
162-
arg >= 1 and
163-
result.getANode() = this.getCall().getArgument(0)
148+
exists(TaintTracking::FunctionModel model |
149+
// A modelled function conveying taint from some input to the response writer,
150+
// e.g. `io.Copy(responseWriter, someTaintedReader)`
151+
model.taintStep(this, responseWriter) and
152+
responseWriter.getType().implements("net/http", "ResponseWriter")
153+
)
164154
}
155+
156+
override HTTP::ResponseWriter getResponseWriter() { result.getANode() = responseWriter }
165157
}
166158

167159
private class RedirectCall extends HTTP::Redirect::Range, DataFlow::CallNode {

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

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
edges
2+
| test.go:10:15:10:42 | call to Cookie : tuple type | test.go:14:15:14:55 | type conversion |
3+
| test.go:10:15:10:42 | call to Cookie : tuple type | test.go:14:42:14:47 | implicit dereference : Cookie |
4+
| test.go:14:42:14:47 | implicit dereference : Cookie | test.go:14:15:14:55 | type conversion |
5+
| test.go:14:42:14:47 | implicit dereference : Cookie | test.go:14:42:14:47 | implicit dereference : Cookie |
6+
| test.go:16:24:16:35 | selection of Body : ReadCloser | test.go:17:15:17:31 | type conversion |
7+
| test.go:16:24:16:35 | selection of Body : ReadCloser | test.go:17:22:17:25 | implicit dereference : Node |
8+
| test.go:16:24:16:35 | selection of Body : ReadCloser | test.go:28:22:28:25 | node |
9+
| test.go:17:22:17:25 | implicit dereference : Node | test.go:17:15:17:31 | type conversion |
10+
| test.go:17:22:17:25 | implicit dereference : Node | test.go:17:22:17:25 | implicit dereference : Node |
11+
| test.go:17:22:17:25 | implicit dereference : Node | test.go:28:22:28:25 | node |
12+
| test.go:19:36:19:47 | selection of Body : ReadCloser | test.go:20:15:20:32 | type conversion |
13+
| test.go:19:36:19:47 | selection of Body : ReadCloser | test.go:20:22:20:26 | implicit dereference : Node |
14+
| test.go:20:22:20:26 | implicit dereference : Node | test.go:20:15:20:32 | type conversion |
15+
| test.go:20:22:20:26 | implicit dereference : Node | test.go:20:22:20:26 | implicit dereference : Node |
16+
| test.go:22:33:22:44 | selection of Body : ReadCloser | test.go:23:15:23:35 | type conversion |
17+
| test.go:22:33:22:44 | selection of Body : ReadCloser | test.go:23:22:23:29 | implicit dereference : Node |
18+
| test.go:23:22:23:29 | implicit dereference : Node | test.go:23:15:23:35 | type conversion |
19+
| test.go:23:22:23:29 | implicit dereference : Node | test.go:23:22:23:29 | implicit dereference : Node |
20+
| test.go:25:45:25:56 | selection of Body : ReadCloser | test.go:26:15:26:36 | type conversion |
21+
| test.go:25:45:25:56 | selection of Body : ReadCloser | test.go:26:22:26:30 | implicit dereference : Node |
22+
| test.go:26:22:26:30 | implicit dereference : Node | test.go:26:15:26:36 | type conversion |
23+
| test.go:26:22:26:30 | implicit dereference : Node | test.go:26:22:26:30 | implicit dereference : Node |
24+
| test.go:30:33:30:44 | selection of Body : ReadCloser | test.go:31:15:31:34 | call to Buffered |
25+
| test.go:30:33:30:44 | selection of Body : ReadCloser | test.go:32:15:32:29 | call to Raw |
26+
| test.go:30:33:30:44 | selection of Body : ReadCloser | test.go:34:15:34:19 | value |
27+
| test.go:30:33:30:44 | selection of Body : ReadCloser | test.go:35:15:35:30 | call to Text |
28+
| test.go:30:33:30:44 | selection of Body : ReadCloser | test.go:36:15:36:44 | type conversion |
29+
| test.go:30:33:30:44 | selection of Body : ReadCloser | test.go:36:22:36:38 | call to Token : Token |
30+
| test.go:36:22:36:38 | call to Token : Token | test.go:36:15:36:44 | type conversion |
31+
nodes
32+
| test.go:10:15:10:42 | call to Cookie : tuple type | semmle.label | call to Cookie : tuple type |
33+
| test.go:14:15:14:55 | type conversion | semmle.label | type conversion |
34+
| test.go:14:42:14:47 | implicit dereference : Cookie | semmle.label | implicit dereference : Cookie |
35+
| test.go:16:24:16:35 | selection of Body : ReadCloser | semmle.label | selection of Body : ReadCloser |
36+
| test.go:17:15:17:31 | type conversion | semmle.label | type conversion |
37+
| test.go:17:22:17:25 | implicit dereference : Node | semmle.label | implicit dereference : Node |
38+
| test.go:19:36:19:47 | selection of Body : ReadCloser | semmle.label | selection of Body : ReadCloser |
39+
| test.go:20:15:20:32 | type conversion | semmle.label | type conversion |
40+
| test.go:20:22:20:26 | implicit dereference : Node | semmle.label | implicit dereference : Node |
41+
| test.go:22:33:22:44 | selection of Body : ReadCloser | semmle.label | selection of Body : ReadCloser |
42+
| test.go:23:15:23:35 | type conversion | semmle.label | type conversion |
43+
| test.go:23:22:23:29 | implicit dereference : Node | semmle.label | implicit dereference : Node |
44+
| test.go:25:45:25:56 | selection of Body : ReadCloser | semmle.label | selection of Body : ReadCloser |
45+
| test.go:26:15:26:36 | type conversion | semmle.label | type conversion |
46+
| test.go:26:22:26:30 | implicit dereference : Node | semmle.label | implicit dereference : Node |
47+
| test.go:28:22:28:25 | node | semmle.label | node |
48+
| test.go:30:33:30:44 | selection of Body : ReadCloser | semmle.label | selection of Body : ReadCloser |
49+
| test.go:31:15:31:34 | call to Buffered | semmle.label | call to Buffered |
50+
| test.go:32:15:32:29 | call to Raw | semmle.label | call to Raw |
51+
| test.go:34:15:34:19 | value | semmle.label | value |
52+
| test.go:35:15:35:30 | call to Text | semmle.label | call to Text |
53+
| test.go:36:15:36:44 | type conversion | semmle.label | type conversion |
54+
| test.go:36:22:36:38 | call to Token : Token | semmle.label | call to Token : Token |
55+
#select
56+
| 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 |
57+
| 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 |
58+
| 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 |
59+
| 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 |
60+
| 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 |
61+
| 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 |
62+
| 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 |
63+
| 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 |
64+
| 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 |
65+
| 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 |
66+
| 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/ReflectedXss.qlref

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Security/CWE-079/ReflectedXss.ql

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
module example.com/m
2+
3+
go 1.14
4+
5+
require (
6+
golang.org/x/net v0.0.0-20201010224723-4f7140c49acb
7+
)

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

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
package test
2+
3+
import (
4+
"golang.org/x/net/html"
5+
"net/http"
6+
)
7+
8+
func test(request *http.Request, writer http.ResponseWriter) {
9+
10+
cookie, _ := request.Cookie("SomeCookie")
11+
12+
writer.Write([]byte(html.EscapeString(cookie.Value))) // GOOD: escaped.
13+
14+
writer.Write([]byte(html.UnescapeString(cookie.Value))) // BAD: unescaped.
15+
16+
node, _ := html.Parse(request.Body)
17+
writer.Write([]byte(node.Data)) // BAD: writing unescaped HTML data
18+
19+
node2, _ := html.ParseWithOptions(request.Body)
20+
writer.Write([]byte(node2.Data)) // BAD: writing unescaped HTML data
21+
22+
nodes, _ := html.ParseFragment(request.Body, nil)
23+
writer.Write([]byte(nodes[0].Data)) // BAD: writing unescaped HTML data
24+
25+
nodes2, _ := html.ParseFragmentWithOptions(request.Body, nil)
26+
writer.Write([]byte(nodes2[0].Data)) // BAD: writing unescaped HTML data
27+
28+
html.Render(writer, node) // BAD: rendering untrusted HTML to `writer`
29+
30+
tokenizer := html.NewTokenizer(request.Body)
31+
writer.Write(tokenizer.Buffered()) // BAD: writing unescaped HTML data
32+
writer.Write(tokenizer.Raw()) // BAD: writing unescaped HTML data
33+
_, value, _ := tokenizer.TagAttr()
34+
writer.Write(value) // BAD: writing unescaped HTML data
35+
writer.Write(tokenizer.Text()) // BAD: writing unescaped HTML data
36+
writer.Write([]byte(tokenizer.Token().Data)) // BAD: writing unescaped HTML data
37+
38+
}

‎ql/test/library-tests/semmle/go/frameworks/XNetHtml/vendor/golang.org/x/net/html/stub.go

Lines changed: 130 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)