JS: add Deferred model in js/use-of-returnless-function

[erik-krogh]

Motivated by the last FP's in js/use-of-returnless-function

Turns out that Deferred is a tricky size, as it is not a single library, but rather a silent agreement between library developers to implement the same API.
(You can find more implementations by looking through these results: https://lgtm.com/query/4525530373913797062/)

Therefore this new model for Deferred looks for values named "Deferred" that are used like a promise library.

The Deferred library is used somewhat often: https://lgtm.com/query/8585906951513722374/.
Although most uses are from imports of Angular.

I evaluated performance using the js/use-of-returnless-function query: https://git.semmle.com/erik/dist-compare-reports/tree/florian.ti.semmle.com_1570605281526
Looks like there is no performance degradation, and the FP's are no longer reported.

[asger-semmle]

Could you add some tests in library-tests/TaintTracking to show that taint can propagate through these promises?

For context: If a promise resolves to a tainted value, we just consider the whole promise to be tainted. The abstract classes you're extending should give rise to some new taint steps.

[asger-semmle]

Improvements to library models aren't usually listed in this section. This is mainly for API changes that are relevant for QL writers. There is no need to use the Deferred module directly, and certainly no reason to import Promises.qll directly.

You could add a bullet in the general improvements section, saying that promises derived from a Deferred object are now recognized.

[ghost]

I think this requires a bit more work to reduce our reliance on heuristics. @asger-semmle WDYT?

[ghost]

This has to be a more semantic definition. We have heuristics in .ql files, and very, very, rarely in .qll files. This separation principle is in place to prevent a slow, catastrophic erosion of precision caused by interplay between multiple heuristics in the library files. I am glad to see the sanity check, but I do not think it is enough.

Some thoughts:

Can you perhaps use globalVarRef("Deferred") instead? We use globalVarRef in a few places if it is common that the object of interest is introduced globally instead of through an imported library.

I note that something like class Deferred is used in a few places in your query console hits. I am inclined to accept small class definitions (perhaps also pseudo class definitions with object literals and/or prototype hackery) with both resolve and reject fields as sources for deferred objects, but it depends on the results. These clases should also have at least one instance with your exists(instantiation.getAMemberCall("resolve")) heuristic, and perhaps also at least one instance with exists(instantiation.getAMemberCall("reject")). It may be necessary to use type tracking to identify such classes properly.

[erik-krogh]

Can you perhaps use globalVarRef("Deferred") instead?

Not really. It happens often enough that Deferred is not a global variable (including in the FP that motivated this PR).
I might be able to use globalVarRef("Deferred") together with any(ParameterNode p | p.getName() = "Deferred") to capture most uses.
I'll look into creating something more precise.

I note that something like class Deferred is used in a few places in your query console hits.

But I don't think its used often enough to rely solely on that pattern.

[erik-krogh]

So I identified the patterns used within the libraries and changed the model accordingly: 4ec825b

I don't think its much better than before, but it more precisely describes the patterns observed.

[erik-krogh]

I note that something like class Deferred is used in a few places in your query console hits. I am inclined to accept small class definitions (perhaps also pseudo class definitions with object literals and/or prototype hackery) with both resolve and reject fields as sources for deferred objects, but it depends on the results. These clases should also have at least one instance with your exists(instantiation.getAMemberCall("resolve")) heuristic, and perhaps also at least one instance with exists(instantiation.getAMemberCall("reject")). It may be necessary to use type tracking to identify such classes properly.

Something like this: 31009d9?

I still don't check if the "class"-definition itself has resolve/reject methods. Mostly because I can't do that check when Deferred is a parameter.

[asger-semmle]

Hm, actually the Deferred object is not itself a promise - the underlying promise is usually exposed through a property named promise.

Tainting the entire Deferred object is possibly acceptable in the short term though, but will need an evaluation. Looking at the examples in the query console run, there seems to be a case for using type tracking to track aliases of the Deferred object, but I'll leave it up to you if you want to pursue that or just focus on eliminating FPs from the other query.

[asger-semmle]

After reading @esben-semmle's comment I'm wondering if it's better to install a name-based FP filter in the js/use-of-returnless-function query and then later do a dedicated piece of work to support for the Deferred pattern with a focus on taint tracking.

[erik-krogh]

So type tracking has been added.
The current results on our "standard" benchmark of 236 projects is here: https://lgtm.com/query/8048324438418409201/

@esbena I ended up going more in your direction, which ended up simplifying the implementation.
I felt that requiring that reject was called was too restrictive, as I could find multiple Deferred implementations where a call to reject was nowhere to be found.
The heuristic therefore only requires a call to resolve on from something that was constructed from a callsite with calleeName() = "Deferred".

I did try something more complex, that looks for specific variants of how a Deferred class can appear. But that only removed TP's compared to the heuristic I've pushed here (e.g. it didn't catch dojo.Deferred and a Deferred implementation in jQuery mobile).

[esbena]

The implementation looks good now (except for some missing QL doc on DeferredInstance, and a mention about it being heuristic). But we still have a few higher level nits:

Hm, actually the Deferred object is not itself a promise - the underlying promise is usually exposed through a property named promise.

Tainting the entire Deferred object is possibly acceptable in the short term though, but will need an evaluation.

We have heuristics in .ql files, and very, very, rarely in .qll files. This separation principle is in place to prevent a slow, catastrophic erosion of precision caused by interplay between multiple heuristics in the library files.

Ultimately, I think we should split this PR into two:

1. add the entire new Deferred module in the ql file it would be used to eliminate some FPs for
2. move the Deferred module to a qll file once we have convinced ourselves that the heuristics and promise-interplay are good enough

[erik-krogh]

Hm, actually the Deferred object is not itself a promise - the underlying promise is usually exposed through a property named promise. Tainting the entire Deferred object is possibly acceptable in the short term though, but will need an evaluation.

Yes, but its not always exposed through a .promise property. I want to keep it as is for now.

add the entire new Deferred module in the ql file it would be used to eliminate some FPs for

Yeah, lets do that for now.
I moved the module and the tests to the relevant query.

[esbena]

The pull request title is misleading now, and the change note is no longer required. Other than that, I think this is ready to merge.

[esbena]

Even though we are intending to use this as a FP filter, we will effectively also be extending the dataflow reasoning that deals with promises. This may lead to surprising dataflow in this query caused by a bad heuristic one day. I am willing to accept that risk though.

esbena approved

[max-schaefer]

We don't generally use abbreviations like "e.g." in comments; see https://wiki.semmle.com/pages/viewpage.action?pageId=17105939 (internal link).