JS: expand the js/exception-xss to handle more types of exceptional flow #2512

erik-krogh · 2019-12-11T10:33:13Z

I expanded the scope of the js/exception-xss query.

The following new exceptional flows are considered:

Accessing a tainted field name on null/undefined.
E.g: try {null[tainted]} catch(err) {..}
Callback functions with an error argument.
E.g: foo(tainted, (err, v) => ..);
Exceptional flow from promise executor to catch handler.
E.g: new Promise((resolve, reject) => {throw tainted}).catch((err) => {..})

The results are still largely untested.
I'm decently confident in the performance of the current version, but I haven't tested it yet.

I put a cache modifier on the isAdditionalFlowStep predicate as I otherwise got a massive performance penalty on some benchmarks (oldj/SwitchHosts in particular).
For some reason the Configuration::BarrierGuardNode::internalBlocks_dispred#ffb predicate began taking a LOT of time (the query is 4X slower on some benchmarks without the cache modifier).

Additionally I've added (and used) the getEnclosingCall predicate.
This predicate allows me to support the following pattern where the tainted value is inside some object/array when it escapes to an unknown function.

try {
   unknown({foo: tainted});
} catch(err) {
   element.innerHtml = err;
}

max-schaefer · 2019-12-11T10:38:51Z

javascript/ql/src/semmle/javascript/security/dataflow/ExceptionXss.qll

@@ -65,16 +125,20 @@ module ExceptionXss {

    override predicate isSanitizer(DataFlow::Node node) { node instanceof Xss::Shared::Sanitizer }

+    cached


This doesn't immediately make sense to me: this predicate is only used in one query, so caching it is not useful. I suspect the performance benefits you were observing were due to cached preventing harmful magic. Could you try replacing this with pragma[nomagic]? If that has the same performance benefits, I would suggest using it instead since it more clearly communicates the intention.

Adding pragma[nomagic] on isAdditionalFlowStep does not fix the performance.

I also tried to add pragma[nomagic] to internalBlocks, and that got me from a ~4X slowdown to a ~2X slowdown.

I'll experiment some more.

OK, there are a few other pragmas that cached implies, notably pragma[noinline].

After a whole lot of benchmarking I found that adding pragma[noinline,nomagic] on Configuration::internalBlocks fixes the performance.
Adding just pragma[noinline] or pragma[nomagic] is not enough, both are needed to avoid a performance regression on oldj/SwitchHosts.

But with this change we will need a larger performance evaluation before we can merge.

esbena

I am very uncertain about the getEnclosingCall predicate, both in terms of performance and precision. Can we perhaps delay merging that feature? It seems independent of the rest from the features of this PR.

javascript/ql/src/semmle/javascript/security/dataflow/ExceptionXss.qll

… heuristic

erik-krogh · 2019-12-12T15:17:15Z

I got a performance result (that includes the now removed getEnclosingCall).
And performance looks somewhat OK.

esbena

Thanks, with getEnclosingCall gone, I have some hope that the pragma[xyz] can go away at the end of this PR. Lets iterate once more on the error parameter characteristic first though.

javascript/ql/src/semmle/javascript/security/dataflow/ExceptionXss.qll

erik-krogh · 2019-12-13T10:12:30Z

Thanks, with getEnclosingCall gone, I have some hope that the pragma[xyz] can go away.

The pragma can go away now, I just did the experiment.

esbena

Sorry for pivoting and arguing with myself. I would like to get a second pair of eyes on the reachability analysis.

javascript/ql/src/semmle/javascript/security/dataflow/ExceptionXss.qll

javascript/ql/test/query-tests/Security/CWE-079/exception-xss.js

esbena

Getting close. My only comments now are about visibility, style and documentation.

javascript/ql/src/semmle/javascript/security/dataflow/ExceptionXss.qll

esbena · 2019-12-16T07:58:36Z

javascript/ql/src/semmle/javascript/security/dataflow/ExceptionXss.qll

+      )
+    }
+
+    DataFlow::Node getErrorParam() { result = errorParameter }


Missing docstring.

esbena · 2019-12-16T07:58:40Z

javascript/ql/src/semmle/javascript/security/dataflow/ExceptionXss.qll

+    DataFlow::Node getErrorParam() { result = errorParameter }
+  }
+
+  // `someFunction(.. <pred> .., (<result>, value) => {...}).


Missing docstring.

esbena · 2019-12-16T08:03:14Z

javascript/ql/src/semmle/javascript/security/dataflow/ExceptionXss.qll

+    )
+  }
+
+  /**


This can be private.

This predicates makes up a verbose alias for getExceptionTarget.
How about renaming this predicate to getExceptionTarget, and explaining in the docstring how it adds additional cases to the builtin syntactic Expr::getExceptionTarget?
Alternatively we should just inline this predicate.

I like the idea of renaming, and then explaining how it adds cases.

esbena · 2019-12-16T08:05:26Z

javascript/ql/test/query-tests/Security/CWE-079/exception-xss.js

+	}
+
+	new Promise(resolve => inner(foo, resolve)).catch((e) => {
+      $('myId').html(e); // NOT OK


Minor.
This line is definitely not indented correctly.
The are other similar indentation inconsistencies in this file.

Can you autoformat the entire file, and update the expected output?

esbena · 2019-12-16T09:32:52Z

javascript/ql/src/semmle/javascript/StandardLibrary.qll

+/**
+ * A data flow edge from the exceptional return of the promise executor to the promise catch handler.
+ */
+class PromiseExceptionalStep extends DataFlow::AdditionalFlowStep {


Is this step really feature complete enough yet? Perhaps we should just keep it private, and make a note of the features that we are aware that it is missing:

support for flow of exceptions to the catch block of an await

support for flow of exceptions to the first catch-handler in a chain of promise-steps: new Promise(res, rej).then(..).then(..).catch(e => FLOWS_HERE).then(...).catch(e => BUT_NOT_HERE)

Also, since this is a default taint step for all TaintTracking::Configuration, I want to make sure that we have done an evaluation with --suite security.

I put in a comment about what is missing.
I'll maybe return to it at some and add more flow edges.

I'll get an evaluation going.

Co-Authored-By: Esben Sparre Andreasen <esbena@github.com>

max-schaefer

A few minor suggestions, but overall this looks fairly uncontroversial. 👍

javascript/ql/src/semmle/javascript/StandardLibrary.qll

max-schaefer · 2019-12-17T09:27:27Z

javascript/ql/src/semmle/javascript/StandardLibrary.qll

+/**
+ * A data flow edge from the exceptional return of the promise executor to the promise catch handler.
+ * This only adds an edge from the exceptional return of the promise Executor and to a `.catch()` handler.
+ * Missing are (at least): 


This sounds like something that should be put into an issue, not a code comment.

max-schaefer · 2019-12-17T09:34:18Z

javascript/ql/src/semmle/javascript/security/dataflow/ExceptionXss.qll

+      or
+      exists(DataFlow::PropRef prop |
+        node = DataFlow::valueNode(prop.getPropertyNameExpr()) and
+        isNullOrUndefined(prop.getBase().analyze().getAType())


Hm, I'm not sure about this condition. Our type inference is very conservative, and in particular is likely to infer undefined as a possible type of just about everything (apart from constants and sometimes SSA variables).

I think you may want to strengthen this to forex(InferredType t | t = prop.getBase().analyze().getAType() | isNullOrUndefined(t)) to only cover those cases where we are quite sure the base is null-ish.

(Note that we don't want to use forall, since expressions in dead code sometimes don't have any types inferred for them.)

javascript/ql/src/semmle/javascript/security/dataflow/ExceptionXss.qll

max-schaefer · 2019-12-17T09:41:41Z

javascript/ql/src/semmle/javascript/security/dataflow/ExceptionXss.qll

+   * Gets the data-flow node where exceptions thrown by this expression will
+   * propagate if this expression causes an exception to be thrown. 


This sentence is a bit repetitive. How about shortening to "Gets the data-flow node to which any exceptions thrown by this expression will propagate"?

Co-Authored-By: Max Schaefer <54907921+max-schaefer@users.noreply.github.com>

max-schaefer

LGTM, but we should probably do another (small) performance evaluation.

erik-krogh · 2019-12-18T14:21:29Z

LGTM, but we should probably do another (small) performance evaluation.

I already had a larger performance evaluation running.
It looks somewhat OK to me.

max-schaefer · 2019-12-18T14:23:29Z

OK, that looks fairly healthy. Could you retry nuclide and JazminServer? Looks like that should be quick, and it should give us an idea of whether the reported slowdowns are likely to be flukes.

Also, what were the query failures on bwip-js about? And what about the build failures on babel?

erik-krogh · 2019-12-19T09:37:04Z

OK, that looks fairly healthy. Could you retry nuclide and JazminServer? Looks like that should be quick, and it should give us an idea of whether the reported slowdowns are likely to be flukes.

The slowdowns seem to be flukes.
https://git.semmle.com/erik/dist-compare-reports/tree/profiling-erik-krogh.northeurope.cloudapp.azure.com_1576690361008

Also, what were the query failures on bwip-js about?

CWE-807/DifferentKindsComparisonBypass.ql ran into a timeout.
But the exact same timeout happened on both master and moarExceptions.

And what about the build failures on babel?

That has nothing to do with this PR.
The build failure comes from a new file in babel that we cannot parse.
(I'm creating an issue).

The following exception is thrown:

Exception while extracting /home/erik/dev/dist-compare/working/repositories/babel/packages/babel-parser/test/fixtures/core/categorized/invalid-assignment-pattern-2/input.js.
com.semmle.util.exception.CatastrophicError: Assignment patterns should not appear in the AST.
   at com.semmle.js.ast.DefaultVisitor.visit(DefaultVisitor.java:101)

max-schaefer · 2019-12-19T09:43:41Z

CWE-807/DifferentKindsComparisonBypass.ql ran into a timeout.
But the exact same timeout happened on both master and moarExceptions.

That is very concerning. Can you file an issue please?

erik-krogh · 2019-12-19T10:12:37Z

@esbena can I get an approve?

expand the js/exception-xss to handle more types of exceptional flow

62512dd

erik-krogh added the JS label Dec 11, 2019

erik-krogh requested a review from a team as a code owner December 11, 2019 10:33

max-schaefer reviewed Dec 11, 2019

View reviewed changes

add pragma to internalBlocks predicate to fix performance

f537e28

esbena requested changes Dec 12, 2019

View reviewed changes

erik-krogh added 3 commits December 12, 2019 15:14

revert the introduction of getEnclosingCall

08d0cb7

change callback to rely on an behavior heuristic rather than a naming…

1735860

… heuristic

Merge remote-tracking branch 'upstream/master' into moarExceptions

f35dc5d

esbena requested changes Dec 12, 2019

View reviewed changes

javascript/ql/src/semmle/javascript/security/dataflow/ExceptionXss.qll Outdated Show resolved Hide resolved

javascript/ql/src/semmle/javascript/security/dataflow/ExceptionXss.qll Outdated Show resolved Hide resolved

erik-krogh added 2 commits December 13, 2019 11:44

changes based on review feedback

e164f46

autoformat

3b2cc46

esbena reviewed Dec 13, 2019

View reviewed changes

erik-krogh added 3 commits December 13, 2019 19:00

inline ifStmt field

1efe2ba

update tests after removing control-flow checks from error-callbacks

904976c

Merge remote-tracking branch 'upstream/master' into moarExceptions

8c0b6f2

esbena reviewed Dec 16, 2019

View reviewed changes

erik-krogh and others added 3 commits December 16, 2019 13:32

add quotes on code in documentation

3ca3fa7

Co-Authored-By: Esben Sparre Andreasen <esbena@github.com>

autoformat

7c93145

changes based on review feedback

8f17db6

max-schaefer requested changes Dec 17, 2019

View reviewed changes

erik-krogh and others added 2 commits December 17, 2019 13:05

minor documentation fixes

f9ddd58

Co-Authored-By: Max Schaefer <54907921+max-schaefer@users.noreply.github.com>

changes based on review feedback

9dd7d1c

max-schaefer reviewed Dec 17, 2019

View reviewed changes

update expected output of tests

bf56797

max-schaefer approved these changes Dec 19, 2019

View reviewed changes

esbena approved these changes Dec 20, 2019

View reviewed changes

semmle-qlci merged commit f921cf7 into github:master Dec 20, 2019

erik-krogh added a commit to erik-krogh/ql that referenced this pull request Jan 16, 2020

add pragma to fix performance (same issue as in github#2512)

9998059

erik-krogh mentioned this pull request Jan 16, 2020

JS: add additional data-flow edges to data-flow related to promises #2618

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JS: expand the js/exception-xss to handle more types of exceptional flow #2512

JS: expand the js/exception-xss to handle more types of exceptional flow #2512

erik-krogh commented Dec 11, 2019

max-schaefer Dec 11, 2019

erik-krogh Dec 11, 2019

max-schaefer Dec 11, 2019

erik-krogh Dec 11, 2019

esbena left a comment

erik-krogh commented Dec 12, 2019

esbena left a comment

erik-krogh commented Dec 13, 2019

esbena left a comment

esbena left a comment

esbena Dec 16, 2019

esbena Dec 16, 2019

esbena Dec 16, 2019

erik-krogh Dec 16, 2019

esbena Dec 16, 2019

esbena Dec 16, 2019

erik-krogh Dec 16, 2019

max-schaefer left a comment

max-schaefer Dec 17, 2019

max-schaefer Dec 17, 2019

max-schaefer Dec 17, 2019

max-schaefer left a comment

erik-krogh commented Dec 18, 2019

max-schaefer commented Dec 18, 2019

erik-krogh commented Dec 19, 2019

max-schaefer commented Dec 19, 2019

erik-krogh commented Dec 19, 2019

		@@ -65,16 +125,20 @@ module ExceptionXss {

		override predicate isSanitizer(DataFlow::Node node) { node instanceof Xss::Shared::Sanitizer }

		cached

+                  )
+                }
+                /**

		* Gets the data-flow node where exceptions thrown by this expression will
		* propagate if this expression causes an exception to be thrown.

JS: expand the js/exception-xss to handle more types of exceptional flow #2512

JS: expand the js/exception-xss to handle more types of exceptional flow #2512

Conversation

erik-krogh commented Dec 11, 2019

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

esbena left a comment

Choose a reason for hiding this comment

erik-krogh commented Dec 12, 2019

esbena left a comment

Choose a reason for hiding this comment

erik-krogh commented Dec 13, 2019

esbena left a comment

Choose a reason for hiding this comment

esbena 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

max-schaefer 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

max-schaefer left a comment

Choose a reason for hiding this comment

erik-krogh commented Dec 18, 2019

max-schaefer commented Dec 18, 2019

erik-krogh commented Dec 19, 2019

max-schaefer commented Dec 19, 2019

erik-krogh commented Dec 19, 2019