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

Merge pull request #5748 from asgerf/js/rate-limiting-fixes

Browse Source 
Approved by erik-krogh
This commit is contained in:
CodeQL CI
2021-04-22 05:56:50 -07:00
committed by GitHub
parent 2b8afe55e8 fe8deeaf6b
commit bdb41423e2
9 changed files with 201 additions and 120 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
javascript/change-notes/2021-04-21-rate-limiting-fixes.md Normal file
View File

@@ -0,0 +1,3 @@
lgtm,codescanning
* Tracking of HTTP route handlers has improved, which may lead to additional
security results, and fewer false-positive results from the `js/missing-rate-limiting` query.

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

@@ -1682,61 +1682,7 @@ module DataFlow {
import Configuration
import TrackedNodes
import TypeTracking
import internal.FunctionWrapperSteps
predicate localTaintStep = TaintTracking::localTaintStep/2;
/**
* 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
* through "wrapper functions", since the `succ` function partially replicates behavior of `pred`.
*
* Examples:
* ```js
* function f(x) {
* return g(x); // step: g -> f
* }
*
* function doExec(x) {
* console.log(x);
* return exec(x); // 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 functionForwardingStep(DataFlow::Node pred, DataFlow::Node succ) {
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))
) 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
functionForwardingStep(paramUse, wrapperFunction.getReturnNode().getALocalSource())
)
}
}

147
javascript/ql/src/semmle/javascript/dataflow/internal/FunctionWrapperSteps.qll Normal file
View File

@@ -0,0 +1,147 @@
/** Provides predicates for tracking functions through wrapper functions. */
private import javascript
private import FlowSteps as FlowSteps
private import semmle.javascript.internal.CachedStages
cached
private module Cached {
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.getASpreadArgument())
)
}
cached
private module Stage {
// Forces the module to be computed as part of the type-tracking stage.
cached
predicate forceStage() { Stages::TypeTracking::ref() }
}
/**
* 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
* through "wrapper functions", since the `succ` function partially replicates behavior of `pred`.
*
* Examples:
* ```js
* function f(x) {
* return g(x); // step: g -> f
* }
*
* function doExec(x) {
* console.log(x);
* return exec(x); // 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)
* ```
*/
cached
predicate functionForwardingStep(DataFlow::Node pred, DataFlow::Node succ) {
exists(DataFlow::FunctionNode function, DataFlow::CallNode call |
call.flowsTo(function.getReturnNode()) and
forall(int i | exists([call.getArgument(i), 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, DataFlow::SourceNode param, DataFlow::Node paramUse
|
FlowSteps::argumentPassing(succ, pred, wrapperFunction.getFunction(), param) and
param.flowsTo(paramUse) and
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)
* ```
*/
cached
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, DataFlow::SourceNode param, DataFlow::Node paramUse
|
FlowSteps::argumentPassing(succ, pred, wrapperFunction.getFunction(), param) and
param.flowsTo(paramUse) and
functionOneWayForwardingStep(paramUse, wrapperFunction.getReturnNode().getALocalSource())
)
}
}
import Cached

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() {

2
javascript/ql/src/semmle/javascript/internal/CachedStages.qll
View File

@@ -196,6 +196,8 @@ module Stages {
exists(any(DataFlow::TypeTracker t).append(_))
or
exists(any(DataFlow::TypeBackTracker t).prepend(_))
or
DataFlow::functionForwardingStep(_, _)
}
}

42
javascript/ql/src/semmle/javascript/security/dataflow/MissingRateLimiting.qll
View File

@@ -113,29 +113,53 @@ class DatabaseAccessAsExpensiveAction extends ExpensiveAction {
* A route handler expression that is rate-limited by a rate-limiting middleware.
*/
class RouteHandlerExpressionWithRateLimiter extends RateLimitedRouteHandlerExpr {
RouteHandlerExpressionWithRateLimiter() { getAMatchingAncestor() instanceof RateLimiter }
RouteHandlerExpressionWithRateLimiter() {
any(RateLimitingMiddleware m).ref().flowsToExpr(getAMatchingAncestor())
}
}
/**
* DEPRECATED. Use `RateLimitingMiddleware` instead.
*
* A middleware that acts as a rate limiter.
*/
abstract class RateLimiter extends Express::RouteHandlerExpr { }
deprecated class RateLimiter extends Express::RouteHandlerExpr {
RateLimiter() { any(RateLimitingMiddleware m).ref().flowsToExpr(this) }
}
/**
* Creation of a middleware function that acts as a rate limiter.
*/
abstract class RateLimitingMiddleware extends DataFlow::SourceNode {
/** Gets a data flow node referring to this middleware. */
private DataFlow::SourceNode ref(DataFlow::TypeTracker t) {
t.start() and
result = this
or
DataFlow::functionOneWayForwardingStep(ref(t.continue()).getALocalUse(), result)
or
exists(DataFlow::TypeTracker t2 | result = ref(t2).track(t2, t))
}
/** Gets a data flow node referring to this middleware. */
DataFlow::SourceNode ref() { result = ref(DataFlow::TypeTracker::end()) }
}
/**
* A rate limiter constructed using the `express-rate-limit` package.
*/
class ExpressRateLimit extends RateLimiter {
class ExpressRateLimit extends RateLimitingMiddleware {
ExpressRateLimit() {
this = API::moduleImport("express-rate-limit").getReturn().getAUse().asExpr()
this = API::moduleImport("express-rate-limit").getReturn().getAnImmediateUse()
}
}
/**
* A rate limiter constructed using the `express-brute` package.
*/
class BruteForceRateLimit extends RateLimiter {
class BruteForceRateLimit extends RateLimitingMiddleware {
BruteForceRateLimit() {
this = API::moduleImport("express-brute").getInstance().getMember("prevent").getAUse().asExpr()
this = API::moduleImport("express-brute").getInstance().getMember("prevent").getAnImmediateUse()
}
}
@@ -183,8 +207,6 @@ class RateLimiterFlexibleRateLimiter extends DataFlow::FunctionNode {
/**
* A route-handler expression that is rate-limited by the `rate-limiter-flexible` package.
*/
class RouteHandlerLimitedByRateLimiterFlexible extends RateLimiter {
RouteHandlerLimitedByRateLimiterFlexible() {
any(RateLimiterFlexibleRateLimiter rl).flowsToExpr(this)
}
class RouteHandlerLimitedByRateLimiterFlexible extends RateLimitingMiddleware {
RouteHandlerLimitedByRateLimiterFlexible() { this instanceof RateLimiterFlexibleRateLimiter }
}

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

@@ -7,3 +7,4 @@
| 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 |

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