diff --git a/javascript/ql/src/experimental/Performance/TaintedScheduleMethod.qhelp b/javascript/ql/src/experimental/Performance/TaintedScheduleMethod.qhelp new file mode 100644 index 000000000000..fd4a900f6109 --- /dev/null +++ b/javascript/ql/src/experimental/Performance/TaintedScheduleMethod.qhelp @@ -0,0 +1,47 @@ + + + + +

+A schedule method that accepts a user-controlled delay could lead to a memory leak. +When the setTimeout or setInterval method is invoked, a reference is created which will not be released until the timeout happens. +In particular, if a user provides an extremely long delay, the reference will stay in memory until it is cleared. +

+ +
+ + +

+Either prevent users for setting delays on schedule methods, or cap the delay to a maximum value. +

+ +
+ + +

+The following program snippet executes the notifyUser(); method when it received a POST-request for /notify; +This endpoint accepts a request body with a delay. If no delay provided it uses the default 200ms delay. +As a user I can provide a random value. E.g. submitting 86400000 as a delay causes this setTimeout to stay in memory for 20 days. + +In the following example, function sum takes two arguments xs and +start, and returns the sum of all elements in the array xs, plus +the value of start. The argument start is optional and defaults +to 0. +

+ + + +

+In this example, you can prevent this from happening by capping the delay that can be set to a sensible number. +

+ + + +
+ +
  • MSN Web Docs: setTimeout.
  • +
  • MSN Web Docs: setInterval.
  • +
    +
    diff --git a/javascript/ql/src/experimental/Performance/TaintedScheduleMethod.ql b/javascript/ql/src/experimental/Performance/TaintedScheduleMethod.ql new file mode 100644 index 000000000000..9c8f4f0f72b2 --- /dev/null +++ b/javascript/ql/src/experimental/Performance/TaintedScheduleMethod.ql @@ -0,0 +1,27 @@ +/** + * @name Use of externally-controlled delay + * @description User-controlled delay on scheduling methods can lead to memory leaks + * @kind path-problem + * @problem.severity warning + * @id js/tainted-schedule-method + * @tags efficiency + * maintainability + */ + +import javascript +import DataFlow +import DataFlow::PathGraph + +class TaintedScheduleMethod extends TaintTracking::Configuration { + TaintedScheduleMethod() { this = "TaintedScheduleMethod" } + override predicate isSource(Node node) { node instanceof RemoteFlowSource } + override predicate isSink(DataFlow::Node sink) { + DataFlow::globalVarRef("setInterval").getACall().getArgument(1) = sink + or + DataFlow::globalVarRef("setTimeout").getACall().getArgument(1) = sink + } +} + +from TaintedScheduleMethod cfg, PathNode source, PathNode sink +where cfg.hasFlowPath(source, sink) +select sink.getNode(), source, sink, "setTimeout with user-controlled delay from $@.", source.getNode(), "here" diff --git a/javascript/ql/src/experimental/Performance/examples/TaintedScheduleMethod.js b/javascript/ql/src/experimental/Performance/examples/TaintedScheduleMethod.js new file mode 100644 index 000000000000..aa5ffccb0d1d --- /dev/null +++ b/javascript/ql/src/experimental/Performance/examples/TaintedScheduleMethod.js @@ -0,0 +1,8 @@ +const app = require("express")(); + +app.post("/notify", function handler(req, res) { + const delay = req.body['delay'] || 200; + setTimeout(() => { + notifyUser(); + }, delay) +}); diff --git a/javascript/ql/src/experimental/Performance/examples/TaintedScheduleMethodGood.js b/javascript/ql/src/experimental/Performance/examples/TaintedScheduleMethodGood.js new file mode 100644 index 000000000000..16b2bd8720f7 --- /dev/null +++ b/javascript/ql/src/experimental/Performance/examples/TaintedScheduleMethodGood.js @@ -0,0 +1,11 @@ +const app = require("express")(); + +app.post("/notify", function handler(req, res) { + const delay = req.body['delay'] || 200; + const maxDelay = 3600000; // 1 hour + const cappedDelay = delay > maxDelay ? maxDelay : delay; + + setTimeout(() => { + notifyUser(); + }, cappedDelay) +});