From ff62c56df1499cf4779910375cc7cd83eeb165da Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Fri, 28 Jun 2019 09:21:41 +0100 Subject: [PATCH 1/2] JavaScript: Replace remaining uses of `TrackedExpr` with type tracking. --- .../semmle/javascript/frameworks/Express.qll | 18 ++++++++++++++++-- .../src/semmle/javascript/frameworks/HTTP.qll | 14 +++++++++++++- .../semmle/javascript/frameworks/Restify.qll | 3 +-- 3 files changed, 30 insertions(+), 5 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/frameworks/Express.qll b/javascript/ql/src/semmle/javascript/frameworks/Express.qll index 3792d02549e..1c667c26460 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/Express.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/Express.qll @@ -657,7 +657,7 @@ module Express { /** * An Express server application. */ - private class Application extends HTTP::ServerDefinition, DataFlow::TrackedExpr { + private class Application extends HTTP::ServerDefinition { Application() { this = appCreation().asExpr() } /** @@ -671,9 +671,23 @@ module Express { /** * An Express router. */ - class RouterDefinition extends InvokeExpr, DataFlow::TrackedExpr { + class RouterDefinition extends InvokeExpr { RouterDefinition() { this = routerCreation().asExpr() } + private DataFlow::SourceNode ref(DataFlow::TypeTracker t) { + t.start() and + result = DataFlow::exprNode(this) + or + exists(DataFlow::TypeTracker t2 | result = ref(t2).track(t2, t)) + } + + /** + * Holds if `sink` may refer to this router. + */ + predicate flowsTo(Expr sink) { + ref(DataFlow::TypeTracker::end()).flowsToExpr(sink) + } + /** * Gets a `RouteSetup` that was used for setting up a route on this router. */ diff --git a/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll b/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll index 17a41c8ba3d..5674c55ec3e 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll @@ -248,8 +248,20 @@ module HTTP { /** * A standard server definition. */ - abstract class StandardServerDefinition extends ServerDefinition, DataFlow::TrackedExpr { + abstract class StandardServerDefinition extends ServerDefinition { override RouteHandler getARouteHandler() { result.(StandardRouteHandler).getServer() = this } + + private DataFlow::SourceNode ref(DataFlow::TypeTracker t) { + t.start() and + result = DataFlow::exprNode(this) + or + exists(DataFlow::TypeTracker t2 | result = ref(t2).track(t2, t)) + } + + /** + * Holds if `sink` may refer to this server definition. + */ + predicate flowsTo(Expr sink) { ref(DataFlow::TypeTracker::end()).flowsToExpr(sink) } } /** diff --git a/javascript/ql/src/semmle/javascript/frameworks/Restify.qll b/javascript/ql/src/semmle/javascript/frameworks/Restify.qll index 70dbe2e5e0b..e42f19bf42a 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/Restify.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/Restify.qll @@ -9,8 +9,7 @@ module Restify { /** * An expression that creates a new Restify server. */ - class ServerDefinition extends HTTP::Servers::StandardServerDefinition, CallExpr, - DataFlow::TrackedExpr { + class ServerDefinition extends HTTP::Servers::StandardServerDefinition, CallExpr { ServerDefinition() { // `server = restify.createServer()` this = DataFlow::moduleMember("restify", "createServer").getACall().asExpr() From 3c3422e2219e4a36c31c745fa42a3c8fc54ace5a Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Fri, 28 Jun 2019 09:31:38 +0100 Subject: [PATCH 2/2] JavaScript: Refactor unpromoted-candidate queries to no longer rely on tracked nodes. --- .../meta/analysis-quality/CandidateTracking.qll | 16 ++++++++++++++++ .../UnpromotedRouteHandlerCandidate.ql | 3 ++- .../UnpromotedRouteSetupCandidate.ql | 3 ++- .../UnpromotedRouteHandlerCandidate.expected | 15 --------------- .../UnpromotedRouteSetupCandidate.expected | 17 +++++++++++++++++ 5 files changed, 37 insertions(+), 17 deletions(-) create mode 100644 javascript/ql/src/meta/analysis-quality/CandidateTracking.qll diff --git a/javascript/ql/src/meta/analysis-quality/CandidateTracking.qll b/javascript/ql/src/meta/analysis-quality/CandidateTracking.qll new file mode 100644 index 00000000000..509d5f2a482 --- /dev/null +++ b/javascript/ql/src/meta/analysis-quality/CandidateTracking.qll @@ -0,0 +1,16 @@ +/** + * Provides an auxiliary predicate shared among the unpromoted-candidate queries. + */ + +import javascript + +/** + * Gets a source node to which `cand` may flow inter-procedurally, with `t` tracking + * the state of flow. + */ +DataFlow::SourceNode track(HTTP::RouteHandlerCandidate cand, DataFlow::TypeTracker t) { + t.start() and + result = cand + or + exists(DataFlow::TypeTracker t2 | result = track(cand, t2).track(t2, t)) +} diff --git a/javascript/ql/src/meta/analysis-quality/UnpromotedRouteHandlerCandidate.ql b/javascript/ql/src/meta/analysis-quality/UnpromotedRouteHandlerCandidate.ql index 2a724b4161b..89b26d734c5 100644 --- a/javascript/ql/src/meta/analysis-quality/UnpromotedRouteHandlerCandidate.ql +++ b/javascript/ql/src/meta/analysis-quality/UnpromotedRouteHandlerCandidate.ql @@ -9,12 +9,13 @@ */ import javascript +import CandidateTracking from HTTP::RouteHandlerCandidate rh where not rh instanceof HTTP::RouteHandler and not exists(HTTP::RouteSetupCandidate setup | - rh.(DataFlow::TrackedNode).flowsTo(setup.getARouteHandlerArg()) + track(rh, DataFlow::TypeTracker::end()).flowsTo(setup.getARouteHandlerArg()) ) select rh, "A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`." diff --git a/javascript/ql/src/meta/analysis-quality/UnpromotedRouteSetupCandidate.ql b/javascript/ql/src/meta/analysis-quality/UnpromotedRouteSetupCandidate.ql index 2080a3259d7..361e2d8a619 100644 --- a/javascript/ql/src/meta/analysis-quality/UnpromotedRouteSetupCandidate.ql +++ b/javascript/ql/src/meta/analysis-quality/UnpromotedRouteSetupCandidate.ql @@ -9,12 +9,13 @@ */ import javascript +import CandidateTracking from HTTP::RouteSetupCandidate setup where not setup.asExpr() instanceof HTTP::RouteSetup and exists(HTTP::RouteHandlerCandidate rh | - rh.(DataFlow::TrackedNode).flowsTo(setup.getARouteHandlerArg()) + track(rh, DataFlow::TypeTracker::end()).flowsTo(setup.getARouteHandlerArg()) ) select setup, "A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`." diff --git a/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/UnpromotedRouteHandlerCandidate.expected b/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/UnpromotedRouteHandlerCandidate.expected index 62cf78ad113..445223631c0 100644 --- a/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/UnpromotedRouteHandlerCandidate.expected +++ b/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/UnpromotedRouteHandlerCandidate.expected @@ -3,9 +3,6 @@ | src/hapi.js:1:1:1:30 | functio ... t, h){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | | src/iterated-handlers.js:4:2:4:22 | functio ... res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | | src/middleware-attacher-getter.js:29:32:29:51 | function(req, res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | -| src/nodejs.js:5:22:5:41 | function(req, res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | -| src/nodejs.js:11:23:11:42 | function(req, res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | -| src/nodejs.js:12:25:12:44 | function(req, res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | | src/route-objects.js:7:19:7:38 | function(req, res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | | src/route-objects.js:8:12:10:5 | (req, res) {\\n\\n } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | | src/route-objects.js:20:16:22:9 | (req, r ... } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | @@ -13,20 +10,8 @@ | src/route-objects.js:40:12:42:5 | (req, res) {\\n\\n } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | | src/route-objects.js:50:12:52:5 | (req, res) {\\n\\n } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | | src/route-objects.js:56:12:58:5 | functio ... ;\\n } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | -| src/tst.js:6:32:6:52 | functio ... res) {} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | -| src/tst.js:8:32:8:61 | functio ... nse) {} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | -| src/tst.js:30:36:30:56 | functio ... res) {} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | -| src/tst.js:33:18:33:38 | functio ... res) {} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | -| src/tst.js:34:18:34:38 | functio ... res) {} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | -| src/tst.js:37:5:37:25 | functio ... res) {} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | -| src/tst.js:38:5:38:25 | functio ... res) {} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | -| src/tst.js:43:18:43:38 | functio ... res) {} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | | src/tst.js:46:1:46:23 | functio ... res) {} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | | src/tst.js:52:1:54:1 | functio ... req()\\n} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | | src/tst.js:61:1:63:1 | functio ... turn;\\n} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | | src/tst.js:70:5:72:5 | functio ... \\n\\n } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | -| src/tst.js:79:5:81:5 | functio ... \\n\\n } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | -| src/tst.js:84:5:86:5 | functio ... \\n\\n } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | | src/tst.js:109:16:111:9 | functio ... } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | -| src/tst.js:124:16:126:9 | functio ... } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | -| src/tst.js:132:16:134:9 | functio ... } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. | diff --git a/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/UnpromotedRouteSetupCandidate.expected b/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/UnpromotedRouteSetupCandidate.expected index e69de29bb2d..a349499ef27 100644 --- a/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/UnpromotedRouteSetupCandidate.expected +++ b/javascript/ql/test/library-tests/frameworks/HTTP-heuristics/UnpromotedRouteSetupCandidate.expected @@ -0,0 +1,17 @@ +| src/nodejs.js:5:1:5:42 | unknown ... res){}) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. | +| src/nodejs.js:11:1:11:43 | unknown ... res){}) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. | +| src/nodejs.js:12:1:12:45 | unknown ... res){}) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. | +| src/tst.js:6:1:6:53 | someOth ... es) {}) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. | +| src/tst.js:8:1:8:62 | someOth ... se) {}) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. | +| src/tst.js:30:1:30:57 | someOth ... es) {}) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. | +| src/tst.js:32:1:34:39 | someOth ... es) {}) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. | +| src/tst.js:36:1:39:2 | someOth ... ) {}\\n]) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. | +| src/tst.js:41:1:43:39 | someOth ... es) {}) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. | +| src/tst.js:87:5:87:57 | unknown ... cSetup) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. | +| src/tst.js:96:5:96:36 | unknown ... h', rh) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. | +| src/tst.js:98:5:98:38 | unknown ... , [rh]) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. | +| src/tst.js:104:5:104:45 | unknown ... wn, rh) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. | +| src/tst.js:137:5:137:57 | unknown ... cSetup) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. | +| src/tst.js:149:5:149:36 | unknown ... h', rh) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. | +| src/tst.js:151:5:151:38 | unknown ... , [rh]) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. | +| src/tst.js:157:5:157:45 | unknown ... wn, rh) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |