diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/ConditionalBypassQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/ConditionalBypassQuery.qll index 0d1319800a8..6482b09a754 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/ConditionalBypassQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/ConditionalBypassQuery.qll @@ -13,7 +13,28 @@ import ConditionalBypassCustomizations::ConditionalBypass /** * A taint tracking configuration for bypass of sensitive action guards. */ -class Configuration extends TaintTracking::Configuration { +module ConditionalBypassConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { source instanceof Source } + + predicate isSink(DataFlow::Node sink) { sink instanceof Sink } + + predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer } + + predicate isAdditionalFlowStep(DataFlow::Node src, DataFlow::Node dst) { + // comparing a tainted expression against a constant gives a tainted result + dst.asExpr().(Comparison).hasOperands(src.asExpr(), any(ConstantExpr c)) + } +} + +/** + * Taint tracking flow for bypass of sensitive action guards. + */ +module ConditionalBypassFlow = TaintTracking::Global; + +/** + * DEPRECATED. Use the `ConditionalBypassFlow` module instead. + */ +deprecated class Configuration extends TaintTracking::Configuration { Configuration() { this = "ConditionalBypass" } override predicate isSource(DataFlow::Node source) { source instanceof Source } @@ -26,8 +47,7 @@ class Configuration extends TaintTracking::Configuration { } override predicate isAdditionalTaintStep(DataFlow::Node src, DataFlow::Node dst) { - // comparing a tainted expression against a constant gives a tainted result - dst.asExpr().(Comparison).hasOperands(src.asExpr(), any(ConstantExpr c)) + ConditionalBypassConfig::isAdditionalFlowStep(src, dst) } } @@ -72,7 +92,67 @@ class SensitiveActionGuardComparisonOperand extends Sink { * If flow from `source` taints `sink`, then an attacker can * control if `action` should be executed or not. */ -predicate isTaintedGuardForSensitiveAction( +predicate isTaintedGuardNodeForSensitiveAction( + ConditionalBypassFlow::PathNode sink, ConditionalBypassFlow::PathNode source, + SensitiveAction action +) { + action = sink.getNode().(Sink).getAction() and + // exclude the intermediary sink + not sink.getNode() instanceof SensitiveActionGuardComparisonOperand and + ( + // ordinary taint tracking to a guard + ConditionalBypassFlow::flowPath(source, sink) + or + // taint tracking to both operands of a guard comparison + exists( + SensitiveActionGuardComparison cmp, ConditionalBypassFlow::PathNode lSource, + ConditionalBypassFlow::PathNode rSource, ConditionalBypassFlow::PathNode lSink, + ConditionalBypassFlow::PathNode rSink + | + sink.getNode() = cmp.getGuard() and + ConditionalBypassFlow::flowPath(lSource, lSink) and + lSink.getNode() = DataFlow::valueNode(cmp.getLeftOperand()) and + ConditionalBypassFlow::flowPath(rSource, rSink) and + rSink.getNode() = DataFlow::valueNode(cmp.getRightOperand()) + | + source = lSource or + source = rSource + ) + ) +} + +/** + * Holds if `e` effectively guards access to `action` by returning or throwing early. + * + * Example: `if (e) return; action(x)`. + */ +predicate isEarlyAbortGuardNode(ConditionalBypassFlow::PathNode e, SensitiveAction action) { + exists(IfStmt guard | + // `e` is in the condition of an if-statement ... + e.getNode().(Sink).asExpr().getParentExpr*() = guard.getCondition() and + // ... where the then-branch always throws or returns + exists(Stmt abort | + abort instanceof ThrowStmt or + abort instanceof ReturnStmt + | + abort.nestedIn(guard) and + abort.getBasicBlock().(ReachableBasicBlock).postDominates(guard.getThen().getBasicBlock()) + ) and + // ... and the else-branch does not exist + not exists(guard.getElse()) + | + // ... and `action` is outside the if-statement + not action.asExpr().getEnclosingStmt().nestedIn(guard) + ) +} + +/** + * Holds if `sink` guards `action`, and `source` taints `sink`. + * + * If flow from `source` taints `sink`, then an attacker can + * control if `action` should be executed or not. + */ +deprecated predicate isTaintedGuardForSensitiveAction( DataFlow::PathNode sink, DataFlow::PathNode source, SensitiveAction action ) { action = sink.getNode().(Sink).getAction() and @@ -104,7 +184,7 @@ predicate isTaintedGuardForSensitiveAction( * * Example: `if (e) return; action(x)`. */ -predicate isEarlyAbortGuard(DataFlow::PathNode e, SensitiveAction action) { +deprecated predicate isEarlyAbortGuard(DataFlow::PathNode e, SensitiveAction action) { exists(IfStmt guard | // `e` is in the condition of an if-statement ... e.getNode().(Sink).asExpr().getParentExpr*() = guard.getCondition() and diff --git a/javascript/ql/src/Security/CWE-807/ConditionalBypass.ql b/javascript/ql/src/Security/CWE-807/ConditionalBypass.ql index 492dc5b8b6e..a493662453e 100644 --- a/javascript/ql/src/Security/CWE-807/ConditionalBypass.ql +++ b/javascript/ql/src/Security/CWE-807/ConditionalBypass.ql @@ -13,11 +13,13 @@ import javascript import semmle.javascript.security.dataflow.ConditionalBypassQuery -import DataFlow::PathGraph +import ConditionalBypassFlow::PathGraph -from DataFlow::PathNode source, DataFlow::PathNode sink, SensitiveAction action +from + ConditionalBypassFlow::PathNode source, ConditionalBypassFlow::PathNode sink, + SensitiveAction action where - isTaintedGuardForSensitiveAction(sink, source, action) and - not isEarlyAbortGuard(sink, action) + isTaintedGuardNodeForSensitiveAction(sink, source, action) and + not isEarlyAbortGuardNode(sink, action) select sink.getNode(), source, sink, "This condition guards a sensitive $@, but a $@ controls it.", action, "action", source.getNode(), "user-provided value" diff --git a/javascript/ql/test/query-tests/Security/CWE-807/ConditionalBypass.expected b/javascript/ql/test/query-tests/Security/CWE-807/ConditionalBypass.expected index 6f4dcb31bd5..f78e2428b90 100644 --- a/javascript/ql/test/query-tests/Security/CWE-807/ConditionalBypass.expected +++ b/javascript/ql/test/query-tests/Security/CWE-807/ConditionalBypass.expected @@ -1,122 +1,56 @@ -nodes -| example_bypass.js:6:9:6:19 | req.cookies | -| example_bypass.js:6:9:6:19 | req.cookies | -| example_bypass.js:6:9:6:34 | req.coo ... nUserId | -| example_bypass.js:6:9:6:34 | req.coo ... nUserId | -| example_bypass.js:6:40:6:56 | req.params.userId | -| example_bypass.js:6:40:6:56 | req.params.userId | -| example_bypass.js:6:40:6:56 | req.params.userId | -| example_bypass.js:17:46:17:62 | req.params.userId | -| example_bypass.js:17:46:17:62 | req.params.userId | -| example_bypass.js:17:46:17:62 | req.params.userId | -| tst.js:9:8:9:26 | req.params.shutDown | -| tst.js:9:8:9:26 | req.params.shutDown | -| tst.js:9:8:9:26 | req.params.shutDown | -| tst.js:13:9:13:19 | req.cookies | -| tst.js:13:9:13:19 | req.cookies | -| tst.js:13:9:13:30 | req.coo ... inThing | -| tst.js:13:9:13:30 | req.coo ... inThing | -| tst.js:27:9:27:37 | v3 | -| tst.js:27:14:27:37 | id(req. ... okieId) | -| tst.js:27:17:27:27 | req.cookies | -| tst.js:27:17:27:27 | req.cookies | -| tst.js:27:17:27:36 | req.cookies.cookieId | -| tst.js:28:9:28:10 | v3 | -| tst.js:28:9:28:10 | v3 | -| tst.js:33:13:33:23 | req.cookies | -| tst.js:33:13:33:23 | req.cookies | -| tst.js:33:13:33:32 | req.cookies.cookieId | -| tst.js:33:13:33:32 | req.cookies.cookieId | -| tst.js:38:9:38:19 | req.cookies | -| tst.js:38:9:38:19 | req.cookies | -| tst.js:38:9:38:28 | req.cookies.cookieId | -| tst.js:38:9:38:28 | req.cookies.cookieId | -| tst.js:44:8:44:23 | req.params.login | -| tst.js:44:8:44:23 | req.params.login | -| tst.js:44:8:44:23 | req.params.login | -| tst.js:57:8:57:23 | req.params.login | -| tst.js:57:8:57:23 | req.params.login | -| tst.js:57:8:57:23 | req.params.login | -| tst.js:61:9:61:19 | req.cookies | -| tst.js:61:9:61:19 | req.cookies | -| tst.js:61:9:61:28 | req.cookies.cookieId | -| tst.js:61:9:61:28 | req.cookies.cookieId | -| tst.js:61:34:61:53 | req.params.requestId | -| tst.js:61:34:61:53 | req.params.requestId | -| tst.js:61:34:61:53 | req.params.requestId | -| tst.js:65:14:65:24 | req.cookies | -| tst.js:65:14:65:24 | req.cookies | -| tst.js:65:14:65:33 | req.cookies.cookieId | -| tst.js:65:14:65:33 | req.cookies.cookieId | -| tst.js:65:39:65:58 | req.params.requestId | -| tst.js:65:39:65:58 | req.params.requestId | -| tst.js:65:39:65:58 | req.params.requestId | -| tst.js:78:9:78:19 | req.cookies | -| tst.js:78:9:78:19 | req.cookies | -| tst.js:78:9:78:28 | req.cookies.cookieId | -| tst.js:78:9:78:28 | req.cookies.cookieId | -| tst.js:78:9:78:41 | req.coo ... secret" | -| tst.js:78:9:78:41 | req.coo ... secret" | -| tst.js:91:10:91:17 | req.body | -| tst.js:91:10:91:17 | req.body | -| tst.js:91:10:91:17 | req.body | -| tst.js:98:13:98:32 | req.query.vulnerable | -| tst.js:98:13:98:32 | req.query.vulnerable | -| tst.js:98:13:98:32 | req.query.vulnerable | -| tst.js:105:13:105:32 | req.query.vulnerable | -| tst.js:105:13:105:32 | req.query.vulnerable | -| tst.js:105:13:105:32 | req.query.vulnerable | -| tst.js:113:13:113:32 | req.query.vulnerable | -| tst.js:113:13:113:32 | req.query.vulnerable | -| tst.js:113:13:113:32 | req.query.vulnerable | edges | example_bypass.js:6:9:6:19 | req.cookies | example_bypass.js:6:9:6:34 | req.coo ... nUserId | -| example_bypass.js:6:9:6:19 | req.cookies | example_bypass.js:6:9:6:34 | req.coo ... nUserId | -| example_bypass.js:6:9:6:19 | req.cookies | example_bypass.js:6:9:6:34 | req.coo ... nUserId | -| example_bypass.js:6:9:6:19 | req.cookies | example_bypass.js:6:9:6:34 | req.coo ... nUserId | -| example_bypass.js:6:40:6:56 | req.params.userId | example_bypass.js:6:40:6:56 | req.params.userId | -| example_bypass.js:17:46:17:62 | req.params.userId | example_bypass.js:17:46:17:62 | req.params.userId | -| tst.js:9:8:9:26 | req.params.shutDown | tst.js:9:8:9:26 | req.params.shutDown | | tst.js:13:9:13:19 | req.cookies | tst.js:13:9:13:30 | req.coo ... inThing | -| tst.js:13:9:13:19 | req.cookies | tst.js:13:9:13:30 | req.coo ... inThing | -| tst.js:13:9:13:19 | req.cookies | tst.js:13:9:13:30 | req.coo ... inThing | -| tst.js:13:9:13:19 | req.cookies | tst.js:13:9:13:30 | req.coo ... inThing | -| tst.js:27:9:27:37 | v3 | tst.js:28:9:28:10 | v3 | +| tst.js:24:17:24:17 | v | tst.js:25:16:25:16 | v | | tst.js:27:9:27:37 | v3 | tst.js:28:9:28:10 | v3 | | tst.js:27:14:27:37 | id(req. ... okieId) | tst.js:27:9:27:37 | v3 | | tst.js:27:17:27:27 | req.cookies | tst.js:27:17:27:36 | req.cookies.cookieId | -| tst.js:27:17:27:27 | req.cookies | tst.js:27:17:27:36 | req.cookies.cookieId | +| tst.js:27:17:27:36 | req.cookies.cookieId | tst.js:24:17:24:17 | v | | tst.js:27:17:27:36 | req.cookies.cookieId | tst.js:27:14:27:37 | id(req. ... okieId) | | tst.js:33:13:33:23 | req.cookies | tst.js:33:13:33:32 | req.cookies.cookieId | -| tst.js:33:13:33:23 | req.cookies | tst.js:33:13:33:32 | req.cookies.cookieId | -| tst.js:33:13:33:23 | req.cookies | tst.js:33:13:33:32 | req.cookies.cookieId | -| tst.js:33:13:33:23 | req.cookies | tst.js:33:13:33:32 | req.cookies.cookieId | | tst.js:38:9:38:19 | req.cookies | tst.js:38:9:38:28 | req.cookies.cookieId | -| tst.js:38:9:38:19 | req.cookies | tst.js:38:9:38:28 | req.cookies.cookieId | -| tst.js:38:9:38:19 | req.cookies | tst.js:38:9:38:28 | req.cookies.cookieId | -| tst.js:38:9:38:19 | req.cookies | tst.js:38:9:38:28 | req.cookies.cookieId | -| tst.js:44:8:44:23 | req.params.login | tst.js:44:8:44:23 | req.params.login | -| tst.js:57:8:57:23 | req.params.login | tst.js:57:8:57:23 | req.params.login | | tst.js:61:9:61:19 | req.cookies | tst.js:61:9:61:28 | req.cookies.cookieId | -| tst.js:61:9:61:19 | req.cookies | tst.js:61:9:61:28 | req.cookies.cookieId | -| tst.js:61:9:61:19 | req.cookies | tst.js:61:9:61:28 | req.cookies.cookieId | -| tst.js:61:9:61:19 | req.cookies | tst.js:61:9:61:28 | req.cookies.cookieId | -| tst.js:61:34:61:53 | req.params.requestId | tst.js:61:34:61:53 | req.params.requestId | | tst.js:65:14:65:24 | req.cookies | tst.js:65:14:65:33 | req.cookies.cookieId | -| tst.js:65:14:65:24 | req.cookies | tst.js:65:14:65:33 | req.cookies.cookieId | -| tst.js:65:14:65:24 | req.cookies | tst.js:65:14:65:33 | req.cookies.cookieId | -| tst.js:65:14:65:24 | req.cookies | tst.js:65:14:65:33 | req.cookies.cookieId | -| tst.js:65:39:65:58 | req.params.requestId | tst.js:65:39:65:58 | req.params.requestId | -| tst.js:78:9:78:19 | req.cookies | tst.js:78:9:78:28 | req.cookies.cookieId | -| tst.js:78:9:78:19 | req.cookies | tst.js:78:9:78:28 | req.cookies.cookieId | | tst.js:78:9:78:19 | req.cookies | tst.js:78:9:78:28 | req.cookies.cookieId | | tst.js:78:9:78:19 | req.cookies | tst.js:78:9:78:28 | req.cookies.cookieId | | tst.js:78:9:78:28 | req.cookies.cookieId | tst.js:78:9:78:41 | req.coo ... secret" | -| tst.js:78:9:78:28 | req.cookies.cookieId | tst.js:78:9:78:41 | req.coo ... secret" | -| tst.js:91:10:91:17 | req.body | tst.js:91:10:91:17 | req.body | -| tst.js:98:13:98:32 | req.query.vulnerable | tst.js:98:13:98:32 | req.query.vulnerable | -| tst.js:105:13:105:32 | req.query.vulnerable | tst.js:105:13:105:32 | req.query.vulnerable | -| tst.js:113:13:113:32 | req.query.vulnerable | tst.js:113:13:113:32 | req.query.vulnerable | +nodes +| example_bypass.js:6:9:6:19 | req.cookies | semmle.label | req.cookies | +| example_bypass.js:6:9:6:34 | req.coo ... nUserId | semmle.label | req.coo ... nUserId | +| example_bypass.js:6:40:6:56 | req.params.userId | semmle.label | req.params.userId | +| example_bypass.js:17:46:17:62 | req.params.userId | semmle.label | req.params.userId | +| tst.js:9:8:9:26 | req.params.shutDown | semmle.label | req.params.shutDown | +| tst.js:13:9:13:19 | req.cookies | semmle.label | req.cookies | +| tst.js:13:9:13:30 | req.coo ... inThing | semmle.label | req.coo ... inThing | +| tst.js:24:17:24:17 | v | semmle.label | v | +| tst.js:25:16:25:16 | v | semmle.label | v | +| tst.js:27:9:27:37 | v3 | semmle.label | v3 | +| tst.js:27:14:27:37 | id(req. ... okieId) | semmle.label | id(req. ... okieId) | +| tst.js:27:17:27:27 | req.cookies | semmle.label | req.cookies | +| tst.js:27:17:27:36 | req.cookies.cookieId | semmle.label | req.cookies.cookieId | +| tst.js:28:9:28:10 | v3 | semmle.label | v3 | +| tst.js:33:13:33:23 | req.cookies | semmle.label | req.cookies | +| tst.js:33:13:33:32 | req.cookies.cookieId | semmle.label | req.cookies.cookieId | +| tst.js:38:9:38:19 | req.cookies | semmle.label | req.cookies | +| tst.js:38:9:38:28 | req.cookies.cookieId | semmle.label | req.cookies.cookieId | +| tst.js:44:8:44:23 | req.params.login | semmle.label | req.params.login | +| tst.js:57:8:57:23 | req.params.login | semmle.label | req.params.login | +| tst.js:61:9:61:19 | req.cookies | semmle.label | req.cookies | +| tst.js:61:9:61:28 | req.cookies.cookieId | semmle.label | req.cookies.cookieId | +| tst.js:61:34:61:53 | req.params.requestId | semmle.label | req.params.requestId | +| tst.js:65:14:65:24 | req.cookies | semmle.label | req.cookies | +| tst.js:65:14:65:33 | req.cookies.cookieId | semmle.label | req.cookies.cookieId | +| tst.js:65:39:65:58 | req.params.requestId | semmle.label | req.params.requestId | +| tst.js:78:9:78:19 | req.cookies | semmle.label | req.cookies | +| tst.js:78:9:78:28 | req.cookies.cookieId | semmle.label | req.cookies.cookieId | +| tst.js:78:9:78:28 | req.cookies.cookieId | semmle.label | req.cookies.cookieId | +| tst.js:78:9:78:41 | req.coo ... secret" | semmle.label | req.coo ... secret" | +| tst.js:91:10:91:17 | req.body | semmle.label | req.body | +| tst.js:98:13:98:32 | req.query.vulnerable | semmle.label | req.query.vulnerable | +| tst.js:105:13:105:32 | req.query.vulnerable | semmle.label | req.query.vulnerable | +| tst.js:113:13:113:32 | req.query.vulnerable | semmle.label | req.query.vulnerable | +subpaths +| tst.js:27:17:27:36 | req.cookies.cookieId | tst.js:24:17:24:17 | v | tst.js:25:16:25:16 | v | tst.js:27:14:27:37 | id(req. ... okieId) | #select | tst.js:9:8:9:26 | req.params.shutDown | tst.js:9:8:9:26 | req.params.shutDown | tst.js:9:8:9:26 | req.params.shutDown | This condition guards a sensitive $@, but a $@ controls it. | tst.js:10:9:10:22 | process.exit() | action | tst.js:9:8:9:26 | req.params.shutDown | user-provided value | | tst.js:13:9:13:30 | req.coo ... inThing | tst.js:13:9:13:19 | req.cookies | tst.js:13:9:13:30 | req.coo ... inThing | This condition guards a sensitive $@, but a $@ controls it. | tst.js:14:9:14:17 | o.login() | action | tst.js:13:9:13:19 | req.cookies | user-provided value |