Add CWE-79: HTML template escaping passthrough #493

gagliardetto · 2021-03-03T21:41:26Z

This PR adds a new experimental query regarding CWE-79: Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting'), and more specifically the cases when user-values are converted to types that allow completely avoiding the escaping that normally values would undergo in the HTML template execution.

Usage of such types on user-provided values leads to XSS.

ql/src/experimental/CWE-79/HTMLTemplateEscapingPassthrough.ql

smowton · 2021-03-04T09:19:17Z

Have started an all-projects evaluation

ql/src/experimental/CWE-79/HTMLTemplateEscapingPassthrough.qhelp

ql/src/experimental/CWE-79/HTMLTemplateEscapingPassthrough.ql

sauyon · 2021-03-04T09:24:28Z

ql/src/experimental/CWE-79/HTMLTemplateEscapingPassthrough.ql

+ * this allows the injection of arbitrary content (html, css, js) into the generated
+ * output of the templates.
+ */
+class ConversionFlowToPassthroughTypeConf extends TaintTracking::Configuration {


Hm. I'd be curious whether something like

exists(PassthroughTypeNode node | edges*(source, node) and edges*(node, sink) | node.getNode().getType() <is passthrough type>

would work here.

How would that ever be done?

I mean, how does that work? I haven't seen anything similar before.

Sorry, I should have asked what you meant. edges is a predicate that's automatically generated when you import PathGraph and contains the final data-flow graph that's used for displaying results. It's therefore not actually complete, and does the flow-summary that you'll see.

gagliardetto · 2021-03-07T23:03:03Z

Have started an all-projects evaluation

Any news on that front?

smowton · 2021-03-08T10:13:46Z

Have started an all-projects evaluation

Any news on that front?

47 hits. Observations from triaging the first 20:

Some FPs due to not checking the templated element flows to the sink (i.e., you currently check we have input -> conversion and input -> execution, but not conversion -> execution. This sort of "waypoint" query, where we want A -> B -> C (here A = remoteflowsource, B = template.JS, C = template.Execute) we need to use three configs: one checking A -> B, one B -> C (either of these can be optimised by restricting to those Bs that match the other; note in this case the configs are dependent and TaintTracking2 must be used), and a third checking A -> C where the B steps are marked as additional flow steps. The long-term fix here is flow tags, which will allow us to solve this in one config.
Some FPs because the escaped value has been converted to an integer (e.g. template.HTML("<b>" + userControlledInt + "</b>"))

gagliardetto · 2021-03-08T14:40:47Z

Is it this: https://codeql.github.com/docs/codeql-language-guides/using-flow-labels-for-precise-data-flow-analysis/

gagliardetto · 2021-03-08T15:02:30Z

Is there an example of flow tags in codeql-go?

gagliardetto · 2021-03-08T16:57:35Z

I added an extra flow condition, and it seems to work (added a test). Not sure about where and how/why use TaintTracking2.

gagliardetto · 2021-03-08T17:51:12Z

It's probably very inefficient 😬

smowton · 2021-03-08T19:33:27Z

Flow tags are a feature available in the Javascript dataflow lib but regrettably not in Go yet.

gagliardetto · 2021-03-08T22:43:42Z

Is my code correct? AFAICT it works.

smowton · 2021-03-09T09:23:36Z

Looks good -- it's possible to make a bit more efficient by restricting some of the flows to only investigate sources/sinks that are involved in a flow according to one of the others (starting with whichever flow we expect to be most rare, then working out from there). That would require using TaintTracking2 etc so dependent flows don't use the same instance of TaintTracking; however it's not necessary particularly when the waypoint (B, type conversion to html/template.JS or similar) is quite unusual. I'll try this out on LGTM now.

gagliardetto · 2021-03-09T15:42:13Z

Using the same instance of TaintTracking for all the flows is kinda messing up the steps that are displayed in the results (I think that's the cause).

How can that be avoided? I'm trying to use TaintTracking2 but don't know how to combine them together.

smowton · 2021-03-09T15:51:53Z

Use a different version of TaintTracking for each Configuration. The DataFlow::Node types they use can be directly compared.

gagliardetto · 2021-03-09T16:02:40Z

There's only TaintTracking and TaintTracking2.

gagliardetto · 2021-03-09T16:28:36Z

Nope, still same.

gagliardetto · 2021-03-09T17:23:39Z

Using two ~instances, TaintTracking and TaintTracking2, removes things from the .expected file (probably registering only steps from the TaintTracking).

Not sure whether that's good.

smowton · 2021-03-09T18:19:34Z

The expected file has three subsections: nodes, edges, and results (the top-level #select). If it removes only nodes and edges that's at least harmless; if it removes results too that's more of a problem. Can you post a diff between two different revisions, with the code changes and corresponding expected file changes?

gagliardetto · 2021-03-09T20:34:36Z

I get the same results with both versions of the query (the first version that uses only one TaintTracking, and the second version using the two different TaintTracking).

gagliardetto · 2021-03-11T14:19:27Z

Never mind. That seems to be the flow steps in a regular flow, too.

gagliardetto · 2021-03-11T14:23:09Z

Going back to using one instance of TaintTracking because it saves all nodes and edges (the results don't change).

smowton

Mostly looking good -- I see a few results we could easily exclude though; since this is quite closely related to an XSS query, we should probably include the same sanitizers. They are defined here:

codeql-go/ql/src/semmle/go/security/Xss.qll

Line 24 in fecf265

abstract class Sanitizer extends DataFlow::Node { }

And here's an example of a user (https://github.com/github/codeql-go/blob/main/ql/src/semmle/go/security/ReflectedXssCustomizations.qll#L28), though we can just directly include these using the isSanitizer method of any appropriate configuration.

ql/src/experimental/CWE-79/HTMLTemplateEscapingPassthrough.ql

smowton

A few minor cleanups, then lgtm

ql/src/experimental/CWE-79/HTMLTemplateEscapingPassthrough.qhelp

ql/src/experimental/CWE-79/HTMLTemplateEscapingPassthrough.ql

smowton · 2021-04-08T13:22:23Z

Just noticed these had been done -- please do ping when you've applied review suggestions, or it's quite easy not to notice

Co-authored-by: Sauyon Lee <sauyon@github.com>

…`conversion` flows to `template-exec`.

…Call, and a test for it

Co-authored-by: Chris Smowton <smowton@github.com>

smowton · 2021-04-08T13:25:39Z

Rebased and set to automerge if CI passes.

gagliardetto · 2021-04-08T15:33:41Z

CI passed, but the automerge didn't happen.

gagliardetto · 2021-04-08T15:34:50Z

please do ping when you've applied review suggestions, or it's quite easy not to notice

I will 🤗

gagliardetto · 2021-04-08T15:36:11Z

CI passed, but the automerge didn't happen.

Maybe it's missing the approval.

gagliardetto requested a review from a team as a code owner March 3, 2021 21:41

smowton reviewed Mar 4, 2021

View reviewed changes

ql/src/experimental/CWE-79/HTMLTemplateEscapingPassthrough.ql Outdated Show resolved Hide resolved

ql/src/experimental/CWE-79/HTMLTemplateEscapingPassthrough.ql Outdated Show resolved Hide resolved

sauyon reviewed Mar 4, 2021

View reviewed changes

gagliardetto force-pushed the html-template-escaping-passthrough branch from ef5cbe8 to 3e1916e Compare March 8, 2021 22:40

gagliardetto force-pushed the html-template-escaping-passthrough branch from 5bfdc0d to a3ad589 Compare March 9, 2021 14:38

gagliardetto mentioned this pull request Mar 9, 2021

[codeql-go]: Add CWE-79: HTML template escaping passthrough github/securitylab#306

Closed

1 task

This comment has been minimized.

Sign in to view

gagliardetto requested review from smowton and sauyon March 11, 2021 14:23

smowton reviewed Mar 11, 2021

View reviewed changes

smowton reviewed Apr 1, 2021

View reviewed changes

gagliardetto and others added 20 commits April 8, 2021 14:24

Add CWE-79: HTML template escaping passthrough

4989434

Apply suggestions from code review

63d5120

Co-authored-by: Sauyon Lee <sauyon@github.com>

Remove DummySource

ad91e4a

Fixes from code review

687e556

Improve naming.

963631d

Refactor flow logic to ensure untrusted flows to conversion, and …

7e9f23a

…`conversion` flows to `template-exec`.

Remove DummySource

7b4a748

Fix test

0bb5ef6

Fix test

cc31cd2

Cleanup

d5355eb

Improve comments and naming.

b42d21f

Use TaintTracking an TaintTracking2

5351a8e

Fix test

280ffdf

Use only one instance of TaintTracking.

e2b7c03

Add missing docs

8f124f8

Add isSanitizer predicate to FlowConfFromUntrustedToTemplateExecution…

541c411

…Call, and a test for it

Add NumericType sanitizer

1a9b09e

Apply suggestions from code review

dc95902

Co-authored-by: Chris Smowton <smowton@github.com>

Use DataFlow::Node as parameters

7c35902

Use PassthroughTypeName instead of string

68c0073

smowton force-pushed the html-template-escaping-passthrough branch from ecb7ac9 to 68c0073 Compare April 8, 2021 13:25

smowton enabled auto-merge April 8, 2021 13:25

smowton approved these changes Apr 8, 2021

View reviewed changes

smowton merged commit 7bf5abf into github:main Apr 8, 2021

Add CWE-79: HTML template escaping passthrough #493

Add CWE-79: HTML template escaping passthrough #493

Uh oh!

Conversation

gagliardetto commented Mar 3, 2021

Uh oh!

Uh oh!

Uh oh!

smowton commented Mar 4, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sauyon Mar 4, 2021

Choose a reason for hiding this comment

Uh oh!

gagliardetto Mar 5, 2021

Choose a reason for hiding this comment

Uh oh!

gagliardetto Mar 7, 2021

Choose a reason for hiding this comment

Uh oh!

sauyon Mar 7, 2021

Choose a reason for hiding this comment

Uh oh!

gagliardetto commented Mar 7, 2021

Uh oh!

smowton commented Mar 8, 2021

Uh oh!

gagliardetto commented Mar 8, 2021

Uh oh!

gagliardetto commented Mar 8, 2021

Uh oh!

gagliardetto commented Mar 8, 2021

Uh oh!

gagliardetto commented Mar 8, 2021

Uh oh!

smowton commented Mar 8, 2021

Uh oh!

gagliardetto commented Mar 8, 2021

Uh oh!

smowton commented Mar 9, 2021

Uh oh!

gagliardetto commented Mar 9, 2021

Uh oh!

smowton commented Mar 9, 2021

Uh oh!

gagliardetto commented Mar 9, 2021

Uh oh!

gagliardetto commented Mar 9, 2021

Uh oh!

gagliardetto commented Mar 9, 2021

Uh oh!

smowton commented Mar 9, 2021

Uh oh!

This comment has been minimized.

gagliardetto commented Mar 9, 2021

Uh oh!

This comment has been minimized.

gagliardetto commented Mar 11, 2021

Uh oh!

gagliardetto commented Mar 11, 2021

Uh oh!

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smowton commented Apr 8, 2021

Uh oh!