JS: Refactor the XSS / Client-side-url queries #8304

erik-krogh · 2022-03-01T20:54:24Z

This PR replaces #6632

See this comment by Asger for what this PR tries to fix.

This is a deep fix where flow-labels are used to hopefully correctly model how url sinks can cause XSS.

There are now 3 flow labels in the XSS query:

TaintedUrlSuffix: a URL where the attacker only controls a suffix.
Taint: a tainted value where the attacker controls part of the value.
PrefixLabel: a tainted value where the attacker controls the prefix

All tests pass on every commit (the expected files are updated on each commit).
This should help in doing a commit-by-commit review.

An evaluation looks good (I think).
I like the new alerts, and I don't miss the alerts that got removed.
(I didn't triage the xss-through-dom results).

(Sidenote: If you think a user-controlled src=".." attribute on an <img/> tag is safe, then checkout SVG parsing in browsers).

There is a small performance penalty, but I think we can live with that.

asgerf

Great! I'm very happy to see progress on this issue.

This result seems to be a FP due to not recognizing the call url.startsWith('https://3p.ampproject.net/') as a sanitizer guard.

The URL sinks could be further divided into cases where

tainted scheme leads to XSS (<a href=...>). Use the Prefix label.
tainted hostname leads to XSS (<script src=...>). Currently no flow label for this.

We could add another flow label for the hostname URL sinks, and use sanitizers from UrlConcatenation for that label. Would be interesting to see how much it affects performance.

asgerf · 2022-03-02T17:01:17Z

javascript/ql/lib/semmle/javascript/security/dataflow/ClientSideUrlRedirectCustomizations.qll

-  abstract class Sink extends DataFlow::Node { }
+  abstract class Sink extends DataFlow::Node {
+    /** Holds if the sink can execute JavaScript code in the current context. */
+    predicate isXSSSink() {


Suggested change

predicate isXSSSink() {

predicate isXssSink() {

Acronyms should be PascalCase/camelCase

(ql-for-ql: four upper-case characters in a row are suspicious...)

(ql-for-ql: four upper-case characters in a row are suspicious...)

That rabbit hole was deep.

asgerf · 2022-03-02T17:07:23Z

javascript/ql/lib/semmle/javascript/security/TaintedUrlSuffix.qll

+  ClientSideRemoteFlowSource source() {
+    result.getKind().isFragment()
+    or
+    result.getKind().isQuery()


It's my impression that location.search and location.hash are the only sources that actually include the ? or # character. Sources from library models, like Angular, tend not to have this leading character in it.

We could make the ClientSideRemoteFlowSource model richer, or just use locationRef().getAPropertyRead(["search", "hash"]).

I still think we need to include all sources of kind url.
In the QLDoc for ClientSideRemoteFlowKind::isUrl is states: the untrusted part of the URL is prefixed by trusted data.

asgerf · 2022-03-02T17:19:13Z

javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssCustomizations.qll

+  /**
+   * A sanitizer that blocks the `PrefixString` label when the start of the string is being tested as being of a particular prefix.
+   */
+  class PrefixStringSanitizer extends SanitizerGuard instanceof StringOps::StartsWith {


Is it possible to put this sanitizer guard in the Query.qll file instead? There's this annoying problem where the SanitizerGuard#blocks override will affect other sanitizer guards based on StringOps::StartsWith, even if PrefixStringSanitizer wasn't explicitly mentioned in isSanitizerGuard.

Also, could this extend LabeledSanitizerGuardNode?

asgerf · 2022-03-02T17:22:11Z

javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssQuery.qll

+    )
+    or
+    // we assume that `.join()` calls have a prefix, and thus block the prefix label.
+    node = any(DataFlow::MethodCallNode call | call.getMethodName() = "join") and


join calls are also modelled as string concatenations. Is the above case not enough?

Ideally [taint, "constant"].join() would remain prefix-tainted.

join calls are also modelled as string concatenations. Is the above case not enough?

Yes join calls are modeled as string concats, but only when join is called with the empty string.
So e.g. [taint, "constant"].join("/") is not modeled as a string concatenation.

So no, the above case is not enough.

erik-krogh · 2022-03-04T17:23:59Z

This result seems to be a FP due to not recognizing the call url.startsWith('https://3p.ampproject.net/') as a sanitizer guard.

It's actually from url being a global variable, and that causes us to not recognize the use within the if as sanitized.
The sanitizer works if you place the entire thing inside a function.

erik-krogh · 2022-03-04T19:26:44Z

The URL sinks could be further divided into cases where

If possible I would prefer waiting with that.
Your suggested change requires changing HostnameSanitizerGuard so that it's somehow configurable which labels should be sanitized (possibly make the class abstract and instantiate the class in all the queries that use it).

asgerf · 2022-03-15T12:18:16Z

It's actually from url being a global variable, and that causes us to not recognize the use within the if as sanitized.
The sanitizer works if you place the entire thing inside a function.

Ah, in that case I'm OK with going ahead with this.

Some tests are currently red, but otherwise LGTM.

…tain a URL query/fragment

…and use it to generalize AttributeUrlSink

…tion

…d `LabeledSanitizerGuardNode`

…based sources

erik-krogh · 2022-03-16T21:34:12Z

The latest CI failure was due to a semantic merge conflict with #8323

I rebased on main and updated updated a name-reference to a non-deprecated name.

github-actions bot added the JS label Mar 1, 2022

erik-krogh mentioned this pull request Mar 1, 2022

JS: add jQuery writes to href/src as sinks in js/client-side-unvaledated-url-redirection #6632

Closed

erik-krogh force-pushed the xssUrl branch from e931433 to 7c86216 Compare March 2, 2022 10:12

erik-krogh marked this pull request as ready for review March 2, 2022 15:51

erik-krogh requested a review from a team as a code owner March 2, 2022 15:52

erik-krogh added the no-change-note-required This PR does not need a change note label Mar 2, 2022

asgerf reviewed Mar 2, 2022

View reviewed changes

erik-krogh force-pushed the xssUrl branch from d07090a to 4abed37 Compare March 4, 2022 19:19

erik-krogh added 13 commits March 16, 2022 22:32

remove unused import

2d9d383

remove unnecessary module qualifier

559f03e

add tests

fc79242

add a isXSSSink predicate to the client-side-url-redirection sinks

67e6a4c

add utility predicate to get client-side remote-flow-sources that con…

2576e1f

…tain a URL query/fragment

split interpretsArgumentsAsURL out of interpretsArgumentsAsHTML, …

b471fec

…and use it to generalize AttributeUrlSink

add client-side-url sinks that may execute JavaScript as XSS sinks

87842bb

refactor the js/xss query to use three flowlabels and one configura…

f083e87

…tion

rename isXSSSink to isXssSink

562dce5

move PrefixStringSanitizer to the Query.qll file, and have it exten…

b3de5d9

…d `LabeledSanitizerGuardNode`

simplify TaintedUrlSuffix::source() to only consider window.location …

d8a5947

…based sources

update expected output

6cdc387

update reference to deprecated class name

aa8b7c8

erik-krogh force-pushed the xssUrl branch from 0a214db to aa8b7c8 Compare March 16, 2022 21:33

asgerf approved these changes Mar 17, 2022

View reviewed changes

erik-krogh merged commit 86398a8 into github:main Mar 17, 2022

erik-krogh mentioned this pull request Mar 21, 2022

JS: filter away reads of .src that end in a URL sink for js/xss-through-dom #8509

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JS: Refactor the XSS / Client-side-url queries #8304

JS: Refactor the XSS / Client-side-url queries #8304

erik-krogh commented Mar 1, 2022 •

edited

Loading

asgerf left a comment

asgerf Mar 2, 2022

esbena Mar 3, 2022

erik-krogh Mar 4, 2022

asgerf Mar 2, 2022

erik-krogh Mar 4, 2022

asgerf Mar 2, 2022

asgerf Mar 2, 2022

erik-krogh Mar 4, 2022

erik-krogh commented Mar 4, 2022

erik-krogh commented Mar 4, 2022

asgerf commented Mar 15, 2022

erik-krogh commented Mar 16, 2022

JS: Refactor the XSS / Client-side-url queries #8304

JS: Refactor the XSS / Client-side-url queries #8304

Conversation

erik-krogh commented Mar 1, 2022 • edited Loading

asgerf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erik-krogh commented Mar 4, 2022

erik-krogh commented Mar 4, 2022

asgerf commented Mar 15, 2022

erik-krogh commented Mar 16, 2022

erik-krogh commented Mar 1, 2022 •

edited

Loading