hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-04-30 11:15:13 +02:00
Code Issues Packages Projects Releases Wiki Activity

JS: Generalize handling of route handler wrapper functions

Browse Source
This commit is contained in:
Asger Feldthaus
2021-04-20 12:12:20 +01:00
parent 1ab75eb6f4
commit 581f4ed757
5 changed files with 88 additions and 56 deletions
Download Patch File Download Diff File Expand all files Collapse all files

71
javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll
View File

@@ -1685,6 +1685,18 @@ module DataFlow {
predicate localTaintStep = TaintTracking::localTaintStep/2;
private predicate forwardsParameter(
DataFlow::FunctionNode function, int i, DataFlow::CallNode call
) {
exists(DataFlow::ParameterNode parameter | parameter = function.getParameter(i) |
not parameter.isRestParameter() and
parameter.flowsTo(call.getArgument(i))
or
parameter.isRestParameter() and
parameter.flowsTo(call.asExpr().(CallExpr).getArgument(i).(SpreadElement).getOperand().flow())
)
}
/**
* Holds if the function in `succ` forwards all its arguments to a call to `pred` and returns
* its result. This can thus be seen as a step `pred -> succ` used for tracking function values
@@ -1722,7 +1734,7 @@ module DataFlow {
exists(DataFlow::FunctionNode function, DataFlow::CallNode call |
call.flowsTo(function.getReturnNode()) and
forall(int i | exists([call.getArgument(i), function.getParameter(i)]) |
function.getParameter(i).flowsTo(call.getArgument(i))
forwardsParameter(function, i, call)
) and
pred = call.getCalleeNode() and
succ = function
@@ -1739,4 +1751,61 @@ module DataFlow {
functionForwardingStep(paramUse, wrapperFunction.getReturnNode().getALocalSource())
)
}
/**
* Holds if the function in `succ` forwards all its arguments to a call to `pred`.
* This can thus be seen as a step `pred -> succ` used for tracking function values
* through "wrapper functions", since the `succ` function partially replicates behavior of `pred`.
*
* This is similar to `functionForwardingStep` except the innermost forwarding call does not
* need flow to the return value; this can be useful for tracking callback-style functions
* where the result tends to be unused.
*
* Examples:
* ```js
* function f(x, callback) {
* g(x, callback); // step: g -> f
* }
*
* function doExec(x, callback) {
* console.log(x);
* exec(x, callback); // step: exec -> doExec
* }
*
* function doEither(x, y) {
* if (x > y) {
* return foo(x, y); // step: foo -> doEither
* } else {
* return bar(x, y); // step: bar -> doEither
* }
* }
*
* function wrapWithLogging(f) {
* return (x) => {
* console.log(x);
* return f(x); // step: f -> anonymous function
* }
* }
* wrapWithLogging(g); // step: g -> wrapWithLogging(g)
* ```
*/
predicate functionOneWayForwardingStep(DataFlow::Node pred, DataFlow::Node succ) {
exists(DataFlow::FunctionNode function, DataFlow::CallNode call |
call.getContainer() = function.getFunction() and
forall(int i | exists(function.getParameter(i)) | forwardsParameter(function, i, call)) and
pred = call.getCalleeNode() and
succ = function
)
or
// Given a generic wrapper function like,
//
// function wrap(f) { return (x, y) => f(x, y) };
//
// add steps through calls to that function: `g -> wrap(g)`
exists(DataFlow::FunctionNode wrapperFunction, SourceNode param, Node paramUse |
FlowSteps::argumentPassing(succ, pred, wrapperFunction.getFunction(), param) and
param.flowsTo(paramUse) and
functionOneWayForwardingStep(paramUse, wrapperFunction.getReturnNode().getALocalSource())
)
}
}

8
javascript/ql/src/semmle/javascript/frameworks/Express.qll
View File

@@ -224,7 +224,13 @@ module Express {
/**
* Gets the function body of this handler, if it is defined locally.
*/
RouteHandler getBody() { result.(DataFlow::SourceNode).flowsToExpr(this) }
RouteHandler getBody() {
exists(DataFlow::SourceNode source | source = flow().getALocalSource() |
result = source
or
DataFlow::functionOneWayForwardingStep(result.(DataFlow::SourceNode).getALocalUse(), source)
)
}
/**
* Holds if this is not followed by more handlers.

56
javascript/ql/src/semmle/javascript/frameworks/HTTP.qll
View File

@@ -235,64 +235,12 @@ module HTTP {
ResponseExpr getAResponseExpr() { result.getRouteHandler() = this }
}
/**
* Holds if `call` decorates the function `pred`.
* This means that `call` returns a function that forwards its arguments to `pred`.
* Only holds when the decorator looks like it is decorating a route-handler.
*
* Below is a code example relating `call`, `decoratee`, `outer`, `inner`.
* ```
* function outer(method) {
* return function inner(req, res) {
* return method.call(this, req, res);
* };
* }
* var route = outer(function decoratee(req, res) { // <- call
* res.end("foo");
* });
* ```
*/
private predicate isDecoratedCall(DataFlow::CallNode call, DataFlow::FunctionNode decoratee) {
// indirect route-handler `result` is given to function `outer`, which returns function `inner` which calls the function `pred`.
exists(int i, DataFlow::FunctionNode outer, HTTP::RouteHandlerCandidate inner |
inner = outer.getAReturn().getALocalSource() and
decoratee = call.getArgument(i).getALocalSource() and
outer.getFunction() = call.getACallee() and
hasForwardingHandlerParameter(i, outer, inner)
)
}
/**
* Holds if the `i`th parameter of `outer` has a call that `inner` forwards its parameters to.
*/
private predicate hasForwardingHandlerParameter(
int i, DataFlow::FunctionNode outer, HTTP::RouteHandlerCandidate inner
) {
isAForwardingRouteHandlerCall(outer.getParameter(i), inner)
}
/**
* Holds if `f` looks like a route-handler and a call to `callee` inside `f` forwards all of the parameters from `f` to that call.
*/
private predicate isAForwardingRouteHandlerCall(
DataFlow::SourceNode callee, HTTP::RouteHandlerCandidate f
) {
exists(DataFlow::CallNode call | call = callee.getACall() |
forall(int arg | arg = [0 .. f.getNumParameter() - 1] |
f.getParameter(arg).flowsTo(call.getArgument(arg))
) and
call.getContainer() = f.getFunction()
)
}
/**
* Holds if there exists a step from `pred` to `succ` for a RouteHandler - beyond the usual steps defined by TypeTracking.
*/
predicate routeHandlerStep(DataFlow::SourceNode pred, DataFlow::SourceNode succ) {
isDecoratedCall(succ, pred)
or
// A forwarding call
isAForwardingRouteHandlerCall(pred, succ)
DataFlow::functionOneWayForwardingStep(pred.getALocalUse(), succ)
or
// a container containing route-handlers.
exists(HTTP::RouteHandlerCandidateContainer container | pred = container.getRouteHandler(succ))
@@ -687,7 +635,7 @@ module HTTP {
DataFlow::SourceNode getAPossiblyDecoratedHandler(RouteHandlerCandidate candidate) {
result = candidate
or
isDecoratedCall(result, candidate)
DataFlow::functionOneWayForwardingStep(candidate, result)
}
private string mapValueProp() {

3
javascript/ql/test/query-tests/Security/CWE-770/MissingRateLimiting.expected
View File

@@ -7,3 +7,6 @@
| tst.js:37:20:37:36 | expensiveHandler3 | This route handler performs $@, but is not rate-limited. | tst.js:16:40:16:70 | child_p ... /true") | a system command |
| tst.js:38:20:38:36 | expensiveHandler4 | This route handler performs $@, but is not rate-limited. | tst.js:17:40:17:83 | connect ... ution') | a database access |
| tst.js:64:25:64:63 | functio ... req); } | This route handler performs $@, but is not rate-limited. | tst.js:64:46:64:60 | verifyUser(req) | authorization |
| tst.js:76:25:76:53 | catchAs ... ndler1) | This route handler performs $@, but is not rate-limited. | tst.js:14:40:14:46 | login() | authorization |
| tst.js:78:60:78:76 | expensiveHandler1 | This route handler performs $@, but is not rate-limited. | tst.js:14:40:14:46 | login() | authorization |
| tst.js:79:60:79:88 | catchAs ... ndler1) | This route handler performs $@, but is not rate-limited. | tst.js:14:40:14:46 | login() | authorization |

6
javascript/ql/test/query-tests/Security/CWE-770/tst.js
View File

@@ -71,3 +71,9 @@ const rateLimiterMiddleware = (req, res, next) => {
rateLimiter.consume(req.ip).then(next).catch(res.status(429).send('rate limited'));
};
express().get('/:path', rateLimiterMiddleware, expensiveHandler1);
const catchAsync = fn => (...args) => fn(...args).catch(args[2]);
express().get('/:path', catchAsync(expensiveHandler1)); // NOT OK
express().get('/:path', rateLimiterMiddleware, catchAsync(expensiveHandler1)); // OK
express().get('/:path', catchAsync(rateLimiterMiddleware), expensiveHandler1); // OK
express().get('/:path', catchAsync(rateLimiterMiddleware), catchAsync(expensiveHandler1)); // OK