diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/LoopBoundInjectionCustomizations.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/LoopBoundInjectionCustomizations.qll index 75f48032f3f..c140eed0785 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/LoopBoundInjectionCustomizations.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/LoopBoundInjectionCustomizations.qll @@ -166,6 +166,30 @@ module LoopBoundInjection { */ abstract class Source extends DataFlow::Node { } + /** + * A barrier guard for looping on tainted objects with unbounded length. + */ + abstract class BarrierGuard extends DataFlow::Node { + /** + * Holds if this node acts as a barrier for data flow, blocking further flow from `e` if `this` evaluates to `outcome`. + */ + predicate blocksExpr(boolean outcome, Expr e) { none() } + + /** + * Holds if this node acts as a barrier for `label`, blocking further flow from `e` if `this` evaluates to `outcome`. + */ + predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) { none() } + } + + /** A subclass of `BarrierGuard` that is used for backward compatibility with the old data flow library. */ + abstract class BarrierGuardLegacy extends BarrierGuard, TaintTracking::SanitizerGuardNode { + override predicate sanitizes(boolean outcome, Expr e) { this.blocksExpr(outcome, e) } + + override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) { + this.blocksExpr(outcome, e, label) + } + } + /** * A source of remote user input objects. */ @@ -174,12 +198,12 @@ module LoopBoundInjection { /** * A sanitizer that blocks taint flow if the array is checked to be an array using an `isArray` function. */ - class IsArraySanitizerGuard extends TaintTracking::LabeledSanitizerGuardNode, DataFlow::ValueNode { + class IsArraySanitizerGuard extends BarrierGuardLegacy, DataFlow::ValueNode { override CallExpr astNode; IsArraySanitizerGuard() { astNode.getCalleeName() = "isArray" } - override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) { + override predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) { true = outcome and e = astNode.getAnArgument() and label = TaintedObject::label() @@ -189,9 +213,7 @@ module LoopBoundInjection { /** * A sanitizer that blocks taint flow if the array is checked to be an array using an `X instanceof Array` check. */ - class InstanceofArraySanitizerGuard extends TaintTracking::LabeledSanitizerGuardNode, - DataFlow::ValueNode - { + class InstanceofArraySanitizerGuard extends BarrierGuardLegacy, DataFlow::ValueNode { override BinaryExpr astNode; InstanceofArraySanitizerGuard() { @@ -199,7 +221,7 @@ module LoopBoundInjection { DataFlow::globalVarRef("Array").flowsToExpr(astNode.getRightOperand()) } - override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) { + override predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) { true = outcome and e = astNode.getLeftOperand() and label = TaintedObject::label() @@ -211,9 +233,7 @@ module LoopBoundInjection { * * Also implicitly makes sure that only the first DoS-prone loop is selected by the query (as the .length test has outcome=false when exiting the loop). */ - class LengthCheckSanitizerGuard extends TaintTracking::LabeledSanitizerGuardNode, - DataFlow::ValueNode - { + class LengthCheckSanitizerGuard extends BarrierGuardLegacy, DataFlow::ValueNode { override RelationalComparison astNode; DataFlow::PropRead propRead; @@ -222,7 +242,7 @@ module LoopBoundInjection { propRead.getPropertyName() = "length" } - override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) { + override predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) { false = outcome and e = propRead.getBase().asExpr() and label = TaintedObject::label() diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/LoopBoundInjectionQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/LoopBoundInjectionQuery.qll index 165f96f7f29..a8316705a38 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/LoopBoundInjectionQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/LoopBoundInjectionQuery.qll @@ -14,7 +14,42 @@ import LoopBoundInjectionCustomizations::LoopBoundInjection /** * A taint tracking configuration for reasoning about looping on tainted objects with unbounded length. */ -class Configuration extends TaintTracking::Configuration { +module LoopBoundInjectionConfig implements DataFlow::StateConfigSig { + class FlowState = DataFlow::FlowLabel; + + predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) { + source instanceof Source and label = TaintedObject::label() + } + + predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) { + sink instanceof Sink and label = TaintedObject::label() + } + + predicate isBarrier(DataFlow::Node node) { + node = DataFlow::MakeBarrierGuard::getABarrierNode() + } + + predicate isBarrier(DataFlow::Node node, DataFlow::FlowLabel label) { + node = DataFlow::MakeLabeledBarrierGuard::getABarrierNode(label) or + node = TaintedObject::SanitizerGuard::getABarrierNode(label) + } + + predicate isAdditionalFlowStep( + DataFlow::Node src, DataFlow::FlowLabel inlbl, DataFlow::Node trg, DataFlow::FlowLabel outlbl + ) { + TaintedObject::step(src, trg, inlbl, outlbl) + } +} + +/** + * Taint tracking configuration for reasoning about looping on tainted objects with unbounded length. + */ +module LoopBoundInjectionFlow = TaintTracking::GlobalWithState; + +/** + * DEPRECATED. Use the `LoopBoundInjectionFlow` module instead. + */ +deprecated class Configuration extends TaintTracking::Configuration { Configuration() { this = "LoopBoundInjection" } override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) { diff --git a/javascript/ql/src/Security/CWE-834/LoopBoundInjection.ql b/javascript/ql/src/Security/CWE-834/LoopBoundInjection.ql index 1970378ea9a..8a8c74e9847 100644 --- a/javascript/ql/src/Security/CWE-834/LoopBoundInjection.ql +++ b/javascript/ql/src/Security/CWE-834/LoopBoundInjection.ql @@ -14,10 +14,10 @@ import javascript import semmle.javascript.security.dataflow.LoopBoundInjectionQuery -import DataFlow::PathGraph +import LoopBoundInjectionFlow::PathGraph -from Configuration dataflow, DataFlow::PathNode source, DataFlow::PathNode sink -where dataflow.hasFlowPath(source, sink) +from LoopBoundInjectionFlow::PathNode source, LoopBoundInjectionFlow::PathNode sink +where LoopBoundInjectionFlow::flowPath(source, sink) select sink, source, sink, "Iteration over a user-controlled object with a potentially unbounded .length property from a $@.", source, "user-provided value" diff --git a/javascript/ql/test/query-tests/Security/CWE-834/LoopBoundInjection.expected b/javascript/ql/test/query-tests/Security/CWE-834/LoopBoundInjection.expected index 7000c777eee..464b21ca14e 100644 --- a/javascript/ql/test/query-tests/Security/CWE-834/LoopBoundInjection.expected +++ b/javascript/ql/test/query-tests/Security/CWE-834/LoopBoundInjection.expected @@ -1,86 +1,51 @@ -nodes -| LoopBoundInjectionBad.js:8:13:8:20 | req.body | -| LoopBoundInjectionBad.js:8:13:8:20 | req.body | -| LoopBoundInjectionBad.js:10:15:10:22 | req.body | -| LoopBoundInjectionBad.js:10:15:10:22 | req.body | -| LoopBoundInjectionBad.js:12:25:12:32 | req.body | -| LoopBoundInjectionBad.js:12:25:12:32 | req.body | -| LoopBoundInjectionBad.js:14:19:14:26 | req.body | -| LoopBoundInjectionBad.js:14:19:14:26 | req.body | -| LoopBoundInjectionBad.js:17:18:17:20 | val | -| LoopBoundInjectionBad.js:20:25:20:27 | val | -| LoopBoundInjectionBad.js:20:25:20:27 | val | -| LoopBoundInjectionBad.js:25:20:25:22 | val | -| LoopBoundInjectionBad.js:29:16:29:18 | val | -| LoopBoundInjectionBad.js:29:16:29:18 | val | -| LoopBoundInjectionBad.js:35:30:35:32 | val | -| LoopBoundInjectionBad.js:38:15:38:17 | val | -| LoopBoundInjectionBad.js:38:15:38:17 | val | -| LoopBoundInjectionBad.js:46:24:46:26 | val | -| LoopBoundInjectionBad.js:51:25:51:27 | val | -| LoopBoundInjectionBad.js:51:25:51:27 | val | -| LoopBoundInjectionExitBad.js:8:9:8:16 | req.body | -| LoopBoundInjectionExitBad.js:8:9:8:16 | req.body | -| LoopBoundInjectionExitBad.js:10:9:10:16 | req.body | -| LoopBoundInjectionExitBad.js:10:9:10:16 | req.body | -| LoopBoundInjectionExitBad.js:12:10:12:17 | req.body | -| LoopBoundInjectionExitBad.js:12:10:12:17 | req.body | -| LoopBoundInjectionExitBad.js:14:14:14:21 | req.body | -| LoopBoundInjectionExitBad.js:14:14:14:21 | req.body | -| LoopBoundInjectionExitBad.js:17:17:17:19 | val | -| LoopBoundInjectionExitBad.js:20:22:20:24 | val | -| LoopBoundInjectionExitBad.js:20:22:20:24 | val | -| LoopBoundInjectionExitBad.js:31:17:31:19 | val | -| LoopBoundInjectionExitBad.js:34:22:34:24 | val | -| LoopBoundInjectionExitBad.js:34:22:34:24 | val | -| LoopBoundInjectionExitBad.js:46:18:46:20 | val | -| LoopBoundInjectionExitBad.js:49:22:49:24 | val | -| LoopBoundInjectionExitBad.js:49:22:49:24 | val | -| LoopBoundInjectionExitBad.js:59:22:59:24 | val | -| LoopBoundInjectionExitBad.js:60:8:60:10 | val | -| LoopBoundInjectionExitBad.js:60:8:60:10 | val | -| LoopBoundInjectionLodash.js:9:13:9:20 | req.body | -| LoopBoundInjectionLodash.js:9:13:9:20 | req.body | -| LoopBoundInjectionLodash.js:12:18:12:20 | val | -| LoopBoundInjectionLodash.js:13:13:13:15 | val | -| LoopBoundInjectionLodash.js:13:13:13:15 | val | edges | LoopBoundInjectionBad.js:8:13:8:20 | req.body | LoopBoundInjectionBad.js:17:18:17:20 | val | -| LoopBoundInjectionBad.js:8:13:8:20 | req.body | LoopBoundInjectionBad.js:17:18:17:20 | val | -| LoopBoundInjectionBad.js:10:15:10:22 | req.body | LoopBoundInjectionBad.js:25:20:25:22 | val | | LoopBoundInjectionBad.js:10:15:10:22 | req.body | LoopBoundInjectionBad.js:25:20:25:22 | val | | LoopBoundInjectionBad.js:12:25:12:32 | req.body | LoopBoundInjectionBad.js:35:30:35:32 | val | -| LoopBoundInjectionBad.js:12:25:12:32 | req.body | LoopBoundInjectionBad.js:35:30:35:32 | val | -| LoopBoundInjectionBad.js:14:19:14:26 | req.body | LoopBoundInjectionBad.js:46:24:46:26 | val | | LoopBoundInjectionBad.js:14:19:14:26 | req.body | LoopBoundInjectionBad.js:46:24:46:26 | val | | LoopBoundInjectionBad.js:17:18:17:20 | val | LoopBoundInjectionBad.js:20:25:20:27 | val | -| LoopBoundInjectionBad.js:17:18:17:20 | val | LoopBoundInjectionBad.js:20:25:20:27 | val | -| LoopBoundInjectionBad.js:25:20:25:22 | val | LoopBoundInjectionBad.js:29:16:29:18 | val | | LoopBoundInjectionBad.js:25:20:25:22 | val | LoopBoundInjectionBad.js:29:16:29:18 | val | | LoopBoundInjectionBad.js:35:30:35:32 | val | LoopBoundInjectionBad.js:38:15:38:17 | val | -| LoopBoundInjectionBad.js:35:30:35:32 | val | LoopBoundInjectionBad.js:38:15:38:17 | val | -| LoopBoundInjectionBad.js:46:24:46:26 | val | LoopBoundInjectionBad.js:51:25:51:27 | val | | LoopBoundInjectionBad.js:46:24:46:26 | val | LoopBoundInjectionBad.js:51:25:51:27 | val | | LoopBoundInjectionExitBad.js:8:9:8:16 | req.body | LoopBoundInjectionExitBad.js:17:17:17:19 | val | -| LoopBoundInjectionExitBad.js:8:9:8:16 | req.body | LoopBoundInjectionExitBad.js:17:17:17:19 | val | -| LoopBoundInjectionExitBad.js:10:9:10:16 | req.body | LoopBoundInjectionExitBad.js:31:17:31:19 | val | | LoopBoundInjectionExitBad.js:10:9:10:16 | req.body | LoopBoundInjectionExitBad.js:31:17:31:19 | val | | LoopBoundInjectionExitBad.js:12:10:12:17 | req.body | LoopBoundInjectionExitBad.js:46:18:46:20 | val | -| LoopBoundInjectionExitBad.js:12:10:12:17 | req.body | LoopBoundInjectionExitBad.js:46:18:46:20 | val | -| LoopBoundInjectionExitBad.js:14:14:14:21 | req.body | LoopBoundInjectionExitBad.js:59:22:59:24 | val | | LoopBoundInjectionExitBad.js:14:14:14:21 | req.body | LoopBoundInjectionExitBad.js:59:22:59:24 | val | | LoopBoundInjectionExitBad.js:17:17:17:19 | val | LoopBoundInjectionExitBad.js:20:22:20:24 | val | -| LoopBoundInjectionExitBad.js:17:17:17:19 | val | LoopBoundInjectionExitBad.js:20:22:20:24 | val | -| LoopBoundInjectionExitBad.js:31:17:31:19 | val | LoopBoundInjectionExitBad.js:34:22:34:24 | val | | LoopBoundInjectionExitBad.js:31:17:31:19 | val | LoopBoundInjectionExitBad.js:34:22:34:24 | val | | LoopBoundInjectionExitBad.js:46:18:46:20 | val | LoopBoundInjectionExitBad.js:49:22:49:24 | val | -| LoopBoundInjectionExitBad.js:46:18:46:20 | val | LoopBoundInjectionExitBad.js:49:22:49:24 | val | -| LoopBoundInjectionExitBad.js:59:22:59:24 | val | LoopBoundInjectionExitBad.js:60:8:60:10 | val | | LoopBoundInjectionExitBad.js:59:22:59:24 | val | LoopBoundInjectionExitBad.js:60:8:60:10 | val | | LoopBoundInjectionLodash.js:9:13:9:20 | req.body | LoopBoundInjectionLodash.js:12:18:12:20 | val | -| LoopBoundInjectionLodash.js:9:13:9:20 | req.body | LoopBoundInjectionLodash.js:12:18:12:20 | val | -| LoopBoundInjectionLodash.js:12:18:12:20 | val | LoopBoundInjectionLodash.js:13:13:13:15 | val | | LoopBoundInjectionLodash.js:12:18:12:20 | val | LoopBoundInjectionLodash.js:13:13:13:15 | val | +nodes +| LoopBoundInjectionBad.js:8:13:8:20 | req.body | semmle.label | req.body | +| LoopBoundInjectionBad.js:10:15:10:22 | req.body | semmle.label | req.body | +| LoopBoundInjectionBad.js:12:25:12:32 | req.body | semmle.label | req.body | +| LoopBoundInjectionBad.js:14:19:14:26 | req.body | semmle.label | req.body | +| LoopBoundInjectionBad.js:17:18:17:20 | val | semmle.label | val | +| LoopBoundInjectionBad.js:20:25:20:27 | val | semmle.label | val | +| LoopBoundInjectionBad.js:25:20:25:22 | val | semmle.label | val | +| LoopBoundInjectionBad.js:29:16:29:18 | val | semmle.label | val | +| LoopBoundInjectionBad.js:35:30:35:32 | val | semmle.label | val | +| LoopBoundInjectionBad.js:38:15:38:17 | val | semmle.label | val | +| LoopBoundInjectionBad.js:46:24:46:26 | val | semmle.label | val | +| LoopBoundInjectionBad.js:51:25:51:27 | val | semmle.label | val | +| LoopBoundInjectionExitBad.js:8:9:8:16 | req.body | semmle.label | req.body | +| LoopBoundInjectionExitBad.js:10:9:10:16 | req.body | semmle.label | req.body | +| LoopBoundInjectionExitBad.js:12:10:12:17 | req.body | semmle.label | req.body | +| LoopBoundInjectionExitBad.js:14:14:14:21 | req.body | semmle.label | req.body | +| LoopBoundInjectionExitBad.js:17:17:17:19 | val | semmle.label | val | +| LoopBoundInjectionExitBad.js:20:22:20:24 | val | semmle.label | val | +| LoopBoundInjectionExitBad.js:31:17:31:19 | val | semmle.label | val | +| LoopBoundInjectionExitBad.js:34:22:34:24 | val | semmle.label | val | +| LoopBoundInjectionExitBad.js:46:18:46:20 | val | semmle.label | val | +| LoopBoundInjectionExitBad.js:49:22:49:24 | val | semmle.label | val | +| LoopBoundInjectionExitBad.js:59:22:59:24 | val | semmle.label | val | +| LoopBoundInjectionExitBad.js:60:8:60:10 | val | semmle.label | val | +| LoopBoundInjectionLodash.js:9:13:9:20 | req.body | semmle.label | req.body | +| LoopBoundInjectionLodash.js:12:18:12:20 | val | semmle.label | val | +| LoopBoundInjectionLodash.js:13:13:13:15 | val | semmle.label | val | +subpaths #select | LoopBoundInjectionBad.js:20:25:20:27 | val | LoopBoundInjectionBad.js:8:13:8:20 | req.body | LoopBoundInjectionBad.js:20:25:20:27 | val | Iteration over a user-controlled object with a potentially unbounded .length property from a $@. | LoopBoundInjectionBad.js:8:13:8:20 | req.body | user-provided value | | LoopBoundInjectionBad.js:29:16:29:18 | val | LoopBoundInjectionBad.js:10:15:10:22 | req.body | LoopBoundInjectionBad.js:29:16:29:18 | val | Iteration over a user-controlled object with a potentially unbounded .length property from a $@. | LoopBoundInjectionBad.js:10:15:10:22 | req.body | user-provided value |