JS: Add TypeTracker library and Firebase model #1035

asger-semmle · 2019-03-04T14:21:53Z

This contains the TypeTracker library I've talking about and a model of Firebase using it. I'd appreciate some feedback on the TypeTracker library in the first commit for now. I've included the firebase model in the PR so the use-case can be seen but I'm in no rush to land it.

Evaluation on default.slugs shows a slight slow-down, but I'd like some feedback on the type-tracking library before spending more time on it.

Evaluation on firebase.slugs shows a couple of results, but not too exciting.

TypeTracker library
This adds the TypeTracker library for tracking values of a certain type, without tracking where it came from. The differences with TrackedNode is:

Isn't restricted to a single recursive layer.
The source of one type can depend on another.
Tracks at the level of SourceNodes so the final relation is smaller.
Requires more boilerplate.
Doesn't do call summarization.
Doesn't necessarily track where a value came from, though this can be done manually by adding more parameters to each predicate.

Also, type tracker tracks values through instance fields more aggressively (note: currently conflicting with #989). This could of course easily be added to TrackedNode, but as discussed, it's not clear that this step is safe for use in general. It should be safe for detecting API calls, though.

Usage:
The idea is that each type is a QL predicate with a TypeTracker parameter. TypeTracker is just a fancy facade for a boolean - it has the value true if a call step was needed to track the type there, but this is hidden from the user. Use code looks like this:

  /** Gets a reference to the firebase API object. */
  private DataFlow::SourceNode firebase(DataFlow::TypeTracker t) {
    t.start() and
    result = DataFlow::moduleImport("firebase/app")
    or
    exists (DataFlow::TypeTracker t2 |
      result = firebase(t2).track(t2, t)
    )
  }

The exists clause is boilerplate, which can be copy/pasted out of the qldoc for TypeTracker.

Monomorphic API use assumption
The lack of summarization can in practice be sidestepped by not inheriting the TypeTracker value from the dependent type. For example, take the function:

function getDatabase(fb) {
  return fb.database();
}

If fb is a firebase app in any calling context, it should be safe to assume that this method always returns a Firebase database. To do that, the predicate for recognizing .database() calls might look like this:

result = firebase(_).getAMethodCall("database") and t.start()

Alternatively, if this assumption isn't considered safe, it could be written like this:

result = firebase(t).getAMethodCall("database")

That is, by inheriting t from the firebase instance, the database can't be tracked out of the function again.

xiemaisi

I've only looked at the first commit so far, and I'm loving it! I'll give it a try on my socket.io model.

Only a few minor comments on the first commit otherwise, I'll come back to this later.

xiemaisi · 2019-03-05T08:59:39Z

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

+ *
+ * Identical to `TPathSummary` except without flow labels.
+ */
+private newtype TStepSummary = MkStepSummary(boolean hasReturn, boolean hasCall) {


If you change the types of the parameters to Boolean you can omit the body, I think.

xiemaisi · 2019-03-05T09:00:52Z

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

+  }
+}
+
+private newtype TTypeTracker = MkTypeTracker(boolean hasCall) {


xiemaisi · 2019-03-05T09:02:18Z

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

+ * source/sink relation, that is, it may determine that a node has a given type,
+ * but it won't determine where that type came from.
+ *
+ * It is recommended that all uses of this type is written on the following form,


s/is written/are written/

Also, I'm not sure "on the following form" is idiomatic. Perhaps "in the following way" would be better.

xiemaisi

A few minor comments; now that the type tracking library has landed perhaps it would be good to rebase this pr.

xiemaisi · 2019-03-12T08:42:15Z

javascript/ql/src/semmle/javascript/frameworks/Firebase.qll

+    /**
+     * Gets a node that is passed as the callback to a `Reference.transaction` call.
+     */
+    DataFlow::SourceNode transactionCallback(DataFlow::TypeTracker t) {


Should this be private?

(same for all other predicates with a TypeTracker parameter)

Probably a good idea

xiemaisi · 2019-03-12T08:42:19Z

javascript/ql/src/semmle/javascript/frameworks/Firebase.qll

+        (getMethodName() = "on" or getMethodName() = "once")
+      }
+
+      DataFlow::Node getCallbackNode() {


xiemaisi · 2019-03-12T08:43:15Z

javascript/ql/src/semmle/javascript/frameworks/Firebase.qll

+      result = database(_)
+    }
+
+    /** Gets a dataflow node holding a `RefBuilder` object. */


Suggested change

/** Gets a dataflow node holding a `RefBuilder` object. */

/** Gets a data flow node holding a `RefBuilder` object. */

xiemaisi · 2019-03-12T08:45:23Z

javascript/ql/src/semmle/javascript/frameworks/Firebase.qll

+import javascript
+
+module Firebase {
+  /** Gets a reference to the firebase API object. */


Here and elsewhere: it would be nice to settle on a consistent capitalisation of [fF]irebase.

xiemaisi · 2019-03-20T08:25:14Z

This is still marked as WIP. Are you planning on doing another performance evaluation? (If so, I have a few commits that might help.)

asger-semmle · 2019-03-21T11:09:27Z

I'll rebase and do another evaluation just to check

asger-semmle · 2019-03-25T09:33:27Z

Still 15% slow down on rhino. Probably related to the regression @xiemaisi has seen, so I'll wait and rebase once his tweaks have landed.

asger-semmle · 2019-03-29T10:10:33Z

After rebasing on #1163, performance looks good, and new results are the same as last time.

asger-semmle added JS WIP This is a work-in-progress, do not merge yet! labels Mar 4, 2019

asger-semmle requested a review from a team as a code owner March 4, 2019 14:21

xiemaisi reviewed Mar 5, 2019

View reviewed changes

asger-semmle mentioned this pull request Mar 6, 2019

JS: Add TypeTracking library #1049

Merged

xiemaisi suggested changes Mar 12, 2019

View reviewed changes

asger-semmle force-pushed the firebase branch from e327738 to bfb6bf7 Compare March 19, 2019 14:44

xiemaisi previously approved these changes Mar 20, 2019

View reviewed changes

asger-semmle added WIP This is a work-in-progress, do not merge yet! and removed WIP This is a work-in-progress, do not merge yet! labels Mar 20, 2019

asger-semmle added 14 commits March 27, 2019 13:21

JS: Add Firebase model

06b0851

JS: handle Query methods

e0c06cb

JS: handle 'firebase-admin' package

f554f85

JS: handle Reference.transaction()

49a746b

JS: handle CloudFunctions

0401b26

JS: handle .after and .before

ad592d7

JS: add test

42c0efd

JS: use TypeBackTracker where appropriate

99cc09d

JS: Capitalize Firebase in comments

c0b58f6

JS: dataflow -> data flow

28a776a

JS: missing qldoc

9bbdf84

JS: trailing whitespace

7bfad8c

JS: Make type-tracking predicates private

208bcd4

JS: Make use of TypeTracker::end()

0eb9231

asger-semmle dismissed xiemaisi’s stale review via 0eb9231 March 27, 2019 13:26

asger-semmle force-pushed the firebase branch from bfb6bf7 to 0eb9231 Compare March 27, 2019 13:26

asger-semmle mentioned this pull request Mar 28, 2019

JavaScript: Fix uses of TypeTracker with custom flow steps. #1176

Merged

asger-semmle removed the WIP This is a work-in-progress, do not merge yet! label Mar 29, 2019

xiemaisi approved these changes Mar 29, 2019

View reviewed changes

semmle-qlci merged commit ed0ef36 into github:master Mar 29, 2019

	/** Gets a dataflow node holding a `RefBuilder` object. */
	/** Gets a data flow node holding a `RefBuilder` object. */

JS: Add TypeTracker library and Firebase model #1035

JS: Add TypeTracker library and Firebase model #1035

Uh oh!

Conversation

asger-semmle commented Mar 4, 2019

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!

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!

xiemaisi commented Mar 20, 2019

Uh oh!

asger-semmle commented Mar 21, 2019

Uh oh!

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

Uh oh!

Uh oh!

asger-semmle commented Mar 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

asger-semmle commented Mar 25, 2019 •

edited

Loading