JS: Call graph changes by asger-semmle · Pull Request #2106 · github/codeql

asger-semmle · 2019-10-09T15:56:08Z

Reorganizes the call graph computation motivated by the need for library models to reason about callbacks that escape into the library. For example, in the following we could now detect that event refers to an untrusted cross-window event:

function addListener(cb) {
  window.addEventListener("message", cb)
}
function foo(x, event) { /* ... */ }
addListener(foo.bind(this, x))

This consists of a number of changes seemed independent but I had to roll them up into a single change to avoid intermediate regressions or redundant work:

Call graph computation is now centralized in CallGraphs.qll
InvokeNode.getACallee() and Node.getAFunctionValue() now include the call edges discovered through type tracking -- previously only available internally through FlowSteps::calls.
AdditionalPartialInvokeNode has been renamed to PartialInvokeNode with a ::Range companion.
Added type tracking for the output of a PartialInvokeNode. (To keep the scope of this PR in check, we don't currently type-track function objects in general, just the result of partial invokes.)

Previously, the call graph exposed through getACallee(0) had the nice property that if any callees were found, we almost certainly had all of them. This is doesn't quite hold anymore. Since it now includes results from type tracking, there will be cases where a proper subset of the callees could be found (where previously nothing was found). This has a few ramifications:

Static types now contribute to the call graph even if some edges were also found through data flow. We would lose useful call edges otherwise.
The illegal-invocation and inconsistent-new queries had to be dialed back to be more conservative. In principle we could restore their original behaviour by using analyze().getAValue() directly, but the results we lose didn't seem valuable.

Also some changes that could in principle be landed separately, but helped me verify that things actually work:

Support for the goog.bind and angular.bind partial invokes.
PostMessageEventHandler uses the new API (as per the above example)

Evaluations:

Evaluation and re-run of the outlier
Call graph metrics show some ups and downs. I've spent a lot of time looking at call graph diffs, and concluded that the remaining changes are benign, although I obvs haven't check all of it. The lost call edges I've seen were either spurious or a call to an extern that is also defined by a polyfill (no longer found due to GlobalAcessPaths::isAssignedInUniqueFile).

xiemaisi

A few initial thoughts, I'll take another look tomorrow.

xiemaisi · 2019-10-14T16:32:54Z

javascript/ql/src/semmle/javascript/dataflow/Nodes.qll

  FunctionNode getCallback(int i) { result.flowsTo(getArgument(i)) }

+  /**
+   * Gets a function passed as the `i`th argument of this invocation,


There is no parameter i, and the meaning of param isn't documented. I also think this predicate is complex enough to merit a brief example in the doc comment.

xiemaisi · 2019-10-14T16:35:52Z

javascript/ql/src/semmle/javascript/dataflow/Nodes.qll

 }
+
+/**
+ * A data flow node that performs a partial function application.


Suggested change

* A data flow node that performs a partial function application.

* A data-flow node that performs a partial function application.

Are you sure? None of the other classes use that convention, but I can update all of them if you like.

Yeah, we're not being particularly consistent about this. I think the version with the hyphen is correct, but feel free to ignore.

xiemaisi · 2019-10-14T16:37:24Z

javascript/ql/src/semmle/javascript/dataflow/Nodes.qll

+
+module PartialInvokeNode {
+  /**
+   * A data flow node that performs a partial function application.


Suggested change

* A data flow node that performs a partial function application.

* A data-flow node that performs a partial function application.

Ditto in a few more places below.

There are 238 occurrences of data flow node. I'll do it in a separate PR to avoid conflicts.

xiemaisi · 2019-10-14T16:40:57Z

javascript/ql/src/semmle/javascript/dataflow/internal/CallGraphs.qll

+    result = getABoundFunctionReferenceAux(function, boundArgs, t)
+  }
+
+  pragma[noopt]


Oh, and it's in a recursive predicate, too! Are you absolutely positively sure this is needed?

As a concrete suggestion, a quick experiment suggests that you can weaken the pragma[noopt] to a pragma[inline] like this:

(...) or exists(DataFlow::StepSummary summary, DataFlow::TypeTracker t2 | result = getABoundFunctionReferenceAux(function, boundArgs, t2, summary) and t = t2.append(summary) ) } pragma[noinline] private DataFlow::SourceNode getABoundFunctionReferenceAux(DataFlow::FunctionNode function, int boundArgs, DataFlow::TypeTracker t, DataFlow::StepSummary summary) { exists(DataFlow::SourceNode prev | prev = getABoundFunctionReference(function, boundArgs, t) and DataFlow::StepSummary::step(prev, result, summary) ) }

Yes I remember it solving a performance problem, but I can run another evaluation with noinline if you like.

That would be great. I'm not opposed to pragmas on principle, but pragma[noopt] is perhaps an overly blunt instrument. In particular, I believe it prevents us from having different orders for different delta sizes, which is generally a good thing. (And yes, it absolutely does solve a performance problem; removing the pragma entirely makes PostMessageStar grind to a halt on brackets, for instance.)

Results on nightly look fine. I'm running another eval on default to be sure, but for now I'll merge it in the change.

The worker ran out of disk space during the rerun :-\

xiemaisi

A few more thoughts. On the whole I'm fine with this PR, though I am a little concerned about the pragma[noopt]. Making InconsistentNew and IllegalInvocation more conservative is fine by me; the former never gave interesting results, and the latter is slowly outliving its usefulness now that classes are natively supported everywhere.

xiemaisi · 2019-10-17T08:42:57Z

javascript/ql/src/semmle/javascript/Closure.qll

+  /**
+   * A call to `goog.bind`, as a partial function invocation.
+   */
+  private class BindCall extends DataFlow::PartialInvokeNode::Range, DataFlow::CallNode {


Perhaps add a test for this and angular.bind below?

xiemaisi · 2019-10-17T08:43:38Z

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

+      addEventListener = DataFlow::globalVarRef("addEventListener").getACall() and
      addEventListener.getArgument(0).mayHaveStringValue("message") and
-      addEventListener.getArgument(1).analyze().getAValue().(AbstractFunction).getFunction() = this
+      addEventListener.getArgument(1).getABoundFunctionValue(paramIndex).getFunction() = this


Could you add a test demonstrating the new results this allows us to find?

javascript/ql/src/semmle/javascript/dataflow/Nodes.qll

xiemaisi · 2019-10-17T10:08:23Z

javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll

    /** Gets a function value that may reach this node. */
    final FunctionNode getAFunctionValue() {
-      result = getAFunctionValue(_)
+      CallGraph::getAFunctionReference(result, 0).flowsTo(this)


Why do we restrict imprecision to zero now? Wasn't it unrestricted before?

This diff is relative to an earlier commit. On master, the imprecision bit wasn't easily available and I don't think it was a deliberate choice to include imprecise values here in the first place.

It seems to me that dealing with the imprecise callees is mainly for queries that rely on seeing all the potential callees, which has become a bit of a niche thing given that data flow queries usually don't do that.

But we should make sure it behaves the same way as InvokeNode.getACallee(), which defaults to any imprecision level. I'd prefer to make the default imprecision level 0 for both 0-argument predicates, so you have to "opt in" to see the imprecise callees. WDYT?

I'm not entirely sure. If we restrict ourselves to zero imprecision, what do we lose? Is it just calls to externs (modelling built-ins) and global functions defined elsewhere?

Pretty much yes.

Pushed a commit that makes imprecision zero the default for getAFunctionValue and for getACallee

asger-semmle added 19 commits October 9, 2019 12:16

JS: Simplify call graph metric

c5f29e0

JS: Restructure call graph computation

15f0e85

JS: Move AdditionalPartialInvokeNode to Nodes.qll

6534219

JS: Add getBoundFunction()

d6ba966

JS: Rename AdditionalPartialInvoke -> PartialInvoke::Range

3bf86ee

JS: Add goog.bind and angular.bind as partial invokes

96a13ff

JS: Move call graph computation into CallGraphs.qll

d6d89a0

JS: Use getABoundFunctionValue in PostMessageEventHandler

34497f6

JS: Performance tweaks

8404522

JS: Add library doc comment

4a0e54a

JS: Move getACallee into CallGraphs module

bdc409c

JS: InconsistentNew regression test

dbfd0ae

JS: Restrict class values flowing through globals

d3f587c

JS: IllegalInvocation regression test

ad8667d

JS: IllegalInvocation: be more convservative

07df479

JS: Angular: replace getAnInitialUse with parameterNode

ddf0d53

JS: Accept that types may degrade CG precision

b392559

JS: Update CallGraph test output

45b1088

JS: Dont use deprecated class

cf24fa2

asger-semmle added the JS label Oct 10, 2019

asger-semmle marked this pull request as ready for review October 10, 2019 13:40

asger-semmle requested a review from a team as a code owner October 10, 2019 13:40

asger-semmle changed the title ~~Call graph changes~~ JS: Call graph changes Oct 10, 2019

xiemaisi suggested changes Oct 14, 2019

View reviewed changes

xiemaisi suggested changes Oct 17, 2019

View reviewed changes

asger-semmle added 5 commits October 18, 2019 17:00

JS: Improve documentation

3dcb134

JS: Tests and fixes for PartialInvokeNode

96b6c83

JS: pragma[noopt] -> pragma[noinline]

0ad9067

JS: Add XSS test case for new PostMessageEventHandler cases

8aa34e6

JS: Default to imprecision zero by default

39e2d14

xiemaisi previously approved these changes Oct 25, 2019

View reviewed changes

max-schaefer previously approved these changes Oct 25, 2019

View reviewed changes

JS: update test output

04ee483

asger-semmle dismissed stale reviews from max-schaefer and xiemaisi via 04ee483 October 25, 2019 13:10

max-schaefer approved these changes Oct 25, 2019

View reviewed changes

max-schaefer merged commit b333c6a into github:master Oct 28, 2019

	* A data flow node that performs a partial function application.
	* A data-flow node that performs a partial function application.

Conversation

asger-semmle commented Oct 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xiemaisi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiemaisi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

asger-semmle commented Oct 9, 2019 •

edited

Loading