Add models for the read side of golang.org/x/net/html by smowton · Pull Request #373 · github/codeql-go

smowton · 2020-10-12T11:24:58Z

This covers cases where an HTML document is retrieved and then parts of its structure are output without proper escaping.

smowton · 2020-10-14T10:30:17Z

Expanded this a little to cover the Render method and to generally allow modelled functions to be seen as writing to an http.Response. Benchmarks on this so far show no new results, but will test that generalisation of Http::ResponseBody now.

smowton · 2020-10-14T12:27:18Z

The generalisation finds us 23 new true positives on LGTM, mostly due to use of other fmt functions, ioutil.Copy or ioutil.TeeCopy to write to the ResponseWriter, which we didn't spot before.

The base net/html stuff still doesn't find any results, but I'm inclined to keep it as generally useful and reasonably small.

intrigus-lgtm · 2020-10-14T12:33:17Z

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

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* Added partial support for the `golang.org/x/net/html` package, modelling tainted data flow from a retrieved HTML document to its attributes and other data.


Suggested change

* Added partial support for the `golang.org/x/net/html` package, modelling tainted data flow from a retrieved HTML document to its attributes and other data.

* 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.

As far as I know QL uses American English.

Same below.

sauyon

Sorry for the slow review, I've vaguely been waiting for you to address comments but I suppose it makes more sense for you to wait for all the reviews before making changes...

sauyon · 2020-10-21T06:39:30Z

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

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* Added partial support for the `golang.org/x/net/html` package, modelling tainted data flow from a retrieved HTML document to its attributes and other data.


Same below.

sauyon · 2020-10-21T06:47:10Z

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

+  /** Gets the package name `golang.org/x/net/html`. */
+  string packagePath() { result = "golang.org/x/net/html" }
+
+  private class EscapeString extends HtmlEscapeFunction, TaintTracking::FunctionModel {


I think it would be good to have EscapeFunction extend FunctionModel and then have its output for be exposed on it so that its specific output so that escape functions that don't return the escaped result can be modeled, but that should probably be separate from this PR.

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

sauyon · 2020-10-21T06:53:02Z

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

+    override predicate hasTaintFlow(DataFlow::FunctionInput input, DataFlow::FunctionOutput output) {
+      getName() = ["Buffered", "Raw", "Text", "Token"] and input.isReceiver() and output.isResult(0)
+      or
+      getName() = "TagAttr" and input.isReceiver() and output.isResult(1)


Also TagName?

I skipped TagName along with attribute keys guessing that the charset restrictions on tag and attribute names would render them sanitary for most purposes

Fair enough. I don't see these functions being much use in taint flow anyway. Maybe add a comment, though?

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

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

smowton · 2020-10-26T11:11:56Z

Applied Sauyon's comments; will merge once passing CI

smowton · 2020-10-26T14:37:52Z

So this proved difficult -- the FunctionOutput gives me a postUpdateNode, which doesn't match up with the ResponseBody.getANode() logic. I've worked around this by adding a little utility method enabling jumping between a FunctionInput and its corresponding output and asking for the input node (i.e., the use) (note this is not the same as FunctionOutput.getEntryNode). @sauyon please re-review and let me know if you can see a better way to distangle this!

sauyon · 2020-10-29T14:26:01Z

Hm, I feel like getANode should be fixed to include PostUpdateNodes that come from a variable.

smowton · 2020-10-29T16:10:12Z

I thought walking having the model take us to the post-update node, then reversing that in getANode (even though we're identifying an input here -- the response writer that was written) was messier than asking for the input to begin with. Do you feel strongly against the current solution? Shall we get a third opinion?

sauyon · 2020-10-29T19:19:00Z

Well, I think that .getANode not getting PostUpdateNodes is a bug we should fix. I don't find that backtracking from the PostUpdateNode is messy, since the .getANode logic is really only there because we're modeling ResponseWriters as variables, and it's an easy way to associate a use of the ResponseWriter to the concept.

smowton · 2020-11-05T15:31:20Z

@sauyon ok, I've gone for the reversing-from-post-update-node approach.

sauyon

LGTM. I'll put up a PR fixing .getANode.

sauyon · 2020-11-06T05:42:17Z

I hadn't realised that getANode was part of the ResponseWriter API; I've opened smowton#1 to discuss what I had in mind.

This covers cases where an HTML document is retrieved and then parts of its structure are output without proper escaping.

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

smowton · 2020-11-06T11:19:36Z

Pushed Sauyon's changes here, will merge on green

smowton requested a review from a team October 12, 2020 11:24

smowton force-pushed the smowton/feature/golang-x-net-html branch from 607f4b3 to f8b4734 Compare October 12, 2020 11:29

smowton force-pushed the smowton/feature/golang-x-net-html branch from 8d1f35a to 014a734 Compare October 14, 2020 10:38

intrigus-lgtm reviewed Oct 14, 2020

View reviewed changes

owen-mc approved these changes Oct 16, 2020

View reviewed changes

smowton force-pushed the smowton/feature/golang-x-net-html branch from 014a734 to d4ab06a Compare October 19, 2020 12:11

sauyon reviewed Oct 21, 2020

View reviewed changes

smowton force-pushed the smowton/feature/golang-x-net-html branch from d4ab06a to b1bdc8a Compare October 26, 2020 11:10

smowton force-pushed the smowton/feature/golang-x-net-html branch from b1bdc8a to d28c3bb Compare October 26, 2020 14:35

smowton force-pushed the smowton/feature/golang-x-net-html branch from d28c3bb to 2acee74 Compare November 5, 2020 15:31

sauyon approved these changes Nov 6, 2020

View reviewed changes

smowton and others added 5 commits November 6, 2020 11:04

Add models for the read side of golang.org/x/net/html

03bbef7

This covers cases where an HTML document is retrieved and then parts of its structure are output without proper escaping.

Add models for the read side of golang.org/x/net/html

02f353e

This covers cases where an HTML document is retrieved and then parts of its structure are output without proper escaping.

Add support for html.Render method.

3817ae8

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

Make HTTP::ResponseWriter handle PostUpdateNodes in getANode

8a306af

Simplify net/http ResponseBody logic

a78c35b

smowton force-pushed the smowton/feature/golang-x-net-html branch from 2acee74 to a78c35b Compare November 6, 2020 11:19

smowton merged commit 0938437 into github:main Nov 6, 2020

		@@ -0,0 +1,2 @@
		lgtm,codescanning
		* Added partial support for the `golang.org/x/net/html` package, modelling tainted data flow from a retrieved HTML document to its attributes and other data.

Conversation

smowton commented Oct 12, 2020

Uh oh!

smowton commented Oct 14, 2020

Uh oh!

smowton commented Oct 14, 2020

Uh oh!

intrigus-lgtm Oct 14, 2020

Choose a reason for hiding this comment

Uh oh!

sauyon Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

sauyon left a comment

Choose a reason for hiding this comment

Uh oh!

sauyon Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

sauyon Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sauyon Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

smowton Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

sauyon Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

smowton commented Oct 26, 2020

Uh oh!

smowton commented Oct 26, 2020

Uh oh!

sauyon commented Oct 29, 2020

Uh oh!

smowton commented Oct 29, 2020

Uh oh!

sauyon commented Oct 29, 2020

Uh oh!

smowton commented Nov 5, 2020

Uh oh!

sauyon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sauyon commented Nov 6, 2020

Uh oh!

smowton commented Nov 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sauyon left a comment •

edited

Loading