diff --git a/javascript/ql/lib/semmle/javascript/ApiGraphs.qll b/javascript/ql/lib/semmle/javascript/ApiGraphs.qll index 0f8dd3178d7b..f35e74f829b1 100644 --- a/javascript/ql/lib/semmle/javascript/ApiGraphs.qll +++ b/javascript/ql/lib/semmle/javascript/ApiGraphs.qll @@ -865,6 +865,14 @@ module API { result = awaited(call, DataFlow::TypeTracker::end()) } + cached + predicate memberEdge(TApiNode pred, string member, TApiNode succ) { + edge(pred, Label::member(member), succ) + } + + cached + predicate anyMemberEdge(TApiNode pred, TApiNode succ) { memberEdge(pred, _, succ) } + /** * Holds if there is an edge from `pred` to `succ` in the API graph that is labeled with `lbl`. */ diff --git a/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll b/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll index 1d5f49d6e2f4..bfff49245b91 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll @@ -1586,13 +1586,70 @@ module DataFlow { */ predicate localFieldStep(DataFlow::Node pred, DataFlow::Node succ) { exists(ClassNode cls, string prop | - pred = cls.getADirectSuperClass*().getAReceiverNode().getAPropertyWrite(prop).getRhs() or - pred = cls.getInstanceMethod(prop) - | - succ = cls.getAReceiverNode().getAPropertyRead(prop) + localFieldStoreStep(cls, pred, _, prop) and + localFieldLoadStep(cls, _, succ, prop) ) } + private predicate localFieldStoreStep( + ClassNode cls, DataFlow::Node pred, DataFlow::Node succ, string prop + ) { + ( + pred = cls.getADirectSuperClass*().getAReceiverNode().getAPropertyWrite(prop).getRhs() + or + // add support for writes on nested properties + pred = cls.getAReceiverNode().getAPropertyRead(prop) and + pred = any(DataFlow::PropRef ref).getBase() + or + pred = cls.getInstanceMethod(prop) + ) and + succ = cls.getConstructor().getReceiver() + } + + private predicate localFieldLoadStep( + ClassNode cls, DataFlow::Node pred, DataFlow::Node succ, string prop + ) { + pred = cls.getConstructor().getReceiver() and + succ = cls.getAReceiverNode().getAPropertyRead(prop) + } + + // split into many smaller steps, because that helps performance A LOT! + private class LocalFieldStep extends DataFlow::SharedFlowStep { + /* + * override predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { + * exists(ClassNode cls | + * pred = cls.getConstructor().getReceiver() and + * succ = cls.getAReceiverNode().getAPropertyRead(prop) + * ) + * } + */ + + override predicate step(DataFlow::Node pred, DataFlow::Node succ) { + exists(ClassNode cls | + pred = cls.getConstructor().getReceiver() and + succ = cls.getAReceiverNode() + ) + or + exists(ClassNode cls | + pred = cls.getADirectSuperClass().getConstructor().getReceiver() and + succ = cls.getConstructor().getReceiver() + ) + } + + override predicate storeStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) { + exists(ClassNode cls | + ( + pred = cls.getAReceiverNode().getAPropertyWrite(prop).getRhs() + or + // add support for writes on nested properties + pred = cls.getAReceiverNode().getAPropertyRead(prop) and + pred = any(DataFlow::PropRef ref).getBase() + ) and + succ = cls.getConstructor().getReceiver() + ) + } + } + predicate argumentPassingStep = FlowSteps::argumentPassing/4; /** diff --git a/javascript/ql/lib/semmle/javascript/frameworks/RxJS.qll b/javascript/ql/lib/semmle/javascript/frameworks/RxJS.qll index 4131b9bd61e1..8468e74ba707 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/RxJS.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/RxJS.qll @@ -102,3 +102,44 @@ private class RxJsPipeMapStep extends TaintTracking::SharedTaintStep { ) } } + +/** + * Gets a instance of the `Subject` class from RxJS. + */ +private API::Node getARxJSSubject() { + result = + API::moduleImport("rxjs") + .getMember(["Subject", "BehaviorSubject", "ReplaySubject", "AsyncSubject"]) + .getInstance() + or + result = + API::Node::ofType("rxjs", ["Subject", "BehaviorSubject", "ReplaySubject", "AsyncSubject"]) +} + +/** + * A step from a call to the `next` method of a `Subject` to a read of the value. + */ +private class RxJSNextStep extends DataFlow::SharedFlowStep { + override predicate storeStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) { + exists(API::CallNode call | + call = getARxJSSubject().getMember("next").getACall() and + pred = call.getArgument(0) and + succ = call.getReceiver().getALocalSource() and + prop = "value" + ) + } + + override predicate step(DataFlow::Node pred, DataFlow::Node succ) { + exists(API::Node subject | + subject = getARxJSSubject() and + pred = subject.getMember("next").getACall().getArgument(0) and + succ = + subject + .getMember("subscribe") + .getParameter(0) + .getMember("next") + .getParameter(0) + .getAnImmediateUse() + ) + } +} diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll index 56af9ad7ab30..21dd94079b7b 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll @@ -35,8 +35,4 @@ class Configration extends TaintTracking::Configuration { super.hasFlowPath(source, sink) and DataFlow::hasPathWithoutUnmatchedReturn(source, sink) } - - override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { - DataFlow::localFieldStep(pred, succ) - } } diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeJQueryPluginQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeJQueryPluginQuery.qll index a84e7474ea39..980c533e8c7b 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeJQueryPluginQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeJQueryPluginQuery.qll @@ -26,8 +26,6 @@ class Configuration extends TaintTracking::Configuration { } override predicate isAdditionalTaintStep(DataFlow::Node src, DataFlow::Node sink) { - // jQuery plugins tend to be implemented as classes that store data in fields initialized by the constructor. - DataFlow::localFieldStep(src, sink) or aliasPropertyPresenceStep(src, sink) } diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionQuery.qll index 219731dcbde7..648e9d537f18 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionQuery.qll @@ -32,8 +32,4 @@ class Configuration extends TaintTracking::Configuration { super.hasFlowPath(source, sink) and DataFlow::hasPathWithoutUnmatchedReturn(source, sink) } - - override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { - DataFlow::localFieldStep(pred, succ) - } } diff --git a/javascript/ql/src/Security/CWE-094/CodeInjection.ql b/javascript/ql/src/Security/CWE-094/CodeInjection.ql index fbf39beaca4e..93295317c9e2 100644 --- a/javascript/ql/src/Security/CWE-094/CodeInjection.ql +++ b/javascript/ql/src/Security/CWE-094/CodeInjection.ql @@ -17,6 +17,7 @@ import javascript import semmle.javascript.security.dataflow.CodeInjectionQuery import DataFlow::PathGraph +import semmle.javascript.heuristics.AdditionalSources from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink where cfg.hasFlowPath(source, sink) diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction.expected b/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction.expected index 3ff369a3b8dd..38168d2d4bcf 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction.expected +++ b/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction.expected @@ -166,6 +166,10 @@ nodes | lib/lib.js:277:23:277:26 | opts | | lib/lib.js:277:23:277:30 | opts.bla | | lib/lib.js:277:23:277:30 | opts.bla | +| lib/lib.js:279:19:279:22 | opts | +| lib/lib.js:279:19:279:26 | opts.bla | +| lib/lib.js:281:23:281:35 | this.opts.bla | +| lib/lib.js:281:23:281:35 | this.opts.bla | | lib/lib.js:307:39:307:42 | name | | lib/lib.js:307:39:307:42 | name | | lib/lib.js:308:23:308:26 | name | @@ -468,8 +472,13 @@ edges | lib/lib.js:268:22:268:24 | obj | lib/lib.js:268:22:268:32 | obj.version | | lib/lib.js:276:8:276:11 | opts | lib/lib.js:277:23:277:26 | opts | | lib/lib.js:276:8:276:11 | opts | lib/lib.js:277:23:277:26 | opts | +| lib/lib.js:276:8:276:11 | opts | lib/lib.js:279:19:279:22 | opts | +| lib/lib.js:276:8:276:11 | opts | lib/lib.js:279:19:279:22 | opts | | lib/lib.js:277:23:277:26 | opts | lib/lib.js:277:23:277:30 | opts.bla | | lib/lib.js:277:23:277:26 | opts | lib/lib.js:277:23:277:30 | opts.bla | +| lib/lib.js:279:19:279:22 | opts | lib/lib.js:279:19:279:26 | opts.bla | +| lib/lib.js:279:19:279:26 | opts.bla | lib/lib.js:281:23:281:35 | this.opts.bla | +| lib/lib.js:279:19:279:26 | opts.bla | lib/lib.js:281:23:281:35 | this.opts.bla | | lib/lib.js:307:39:307:42 | name | lib/lib.js:308:23:308:26 | name | | lib/lib.js:307:39:307:42 | name | lib/lib.js:308:23:308:26 | name | | lib/lib.js:307:39:307:42 | name | lib/lib.js:308:23:308:26 | name | @@ -636,6 +645,7 @@ edges | lib/lib.js:261:11:261:33 | "rm -rf ... + name | lib/lib.js:257:35:257:38 | name | lib/lib.js:261:30:261:33 | name | $@ based on $@ is later used in $@. | lib/lib.js:261:11:261:33 | "rm -rf ... + name | String concatenation | lib/lib.js:257:35:257:38 | name | library input | lib/lib.js:261:3:261:34 | cp.exec ... + name) | shell command | | lib/lib.js:268:10:268:32 | "rm -rf ... version | lib/lib.js:267:46:267:48 | obj | lib/lib.js:268:22:268:32 | obj.version | $@ based on $@ is later used in $@. | lib/lib.js:268:10:268:32 | "rm -rf ... version | String concatenation | lib/lib.js:267:46:267:48 | obj | library input | lib/lib.js:268:2:268:33 | cp.exec ... ersion) | shell command | | lib/lib.js:277:11:277:30 | "rm -rf " + opts.bla | lib/lib.js:276:8:276:11 | opts | lib/lib.js:277:23:277:30 | opts.bla | $@ based on $@ is later used in $@. | lib/lib.js:277:11:277:30 | "rm -rf " + opts.bla | String concatenation | lib/lib.js:276:8:276:11 | opts | library input | lib/lib.js:277:3:277:31 | cp.exec ... ts.bla) | shell command | +| lib/lib.js:281:11:281:35 | "rm -rf ... pts.bla | lib/lib.js:276:8:276:11 | opts | lib/lib.js:281:23:281:35 | this.opts.bla | $@ based on $@ is later used in $@. | lib/lib.js:281:11:281:35 | "rm -rf ... pts.bla | String concatenation | lib/lib.js:276:8:276:11 | opts | library input | lib/lib.js:281:3:281:36 | cp.exec ... ts.bla) | shell command | | lib/lib.js:308:11:308:26 | "rm -rf " + name | lib/lib.js:307:39:307:42 | name | lib/lib.js:308:23:308:26 | name | $@ based on $@ is later used in $@. | lib/lib.js:308:11:308:26 | "rm -rf " + name | String concatenation | lib/lib.js:307:39:307:42 | name | library input | lib/lib.js:308:3:308:27 | cp.exec ... + name) | shell command | | lib/lib.js:315:10:315:25 | "rm -rf " + name | lib/lib.js:314:40:314:43 | name | lib/lib.js:315:22:315:25 | name | $@ based on $@ is later used in $@. | lib/lib.js:315:10:315:25 | "rm -rf " + name | String concatenation | lib/lib.js:314:40:314:43 | name | library input | lib/lib.js:315:2:315:26 | cp.exec ... + name) | shell command | | lib/lib.js:320:11:320:26 | "rm -rf " + name | lib/lib.js:314:40:314:43 | name | lib/lib.js:320:23:320:26 | name | $@ based on $@ is later used in $@. | lib/lib.js:320:11:320:26 | "rm -rf " + name | String concatenation | lib/lib.js:314:40:314:43 | name | library input | lib/lib.js:320:3:320:27 | cp.exec ... + name) | shell command | diff --git a/javascript/ql/test/query-tests/Security/CWE-078/lib/lib.js b/javascript/ql/test/query-tests/Security/CWE-078/lib/lib.js index f87f10f83a85..9df87ac96fa4 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/lib/lib.js +++ b/javascript/ql/test/query-tests/Security/CWE-078/lib/lib.js @@ -278,7 +278,7 @@ module.exports.Foo = class Foo { this.opts = {}; this.opts.bla = opts.bla - cp.exec("rm -rf " + this.opts.bla); // NOT OK - but FN [INCONSISTENCY] + cp.exec("rm -rf " + this.opts.bla); // NOT OK } } diff --git a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/CodeInjection.expected b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/CodeInjection.expected index a0b4a89d54fa..890047be35d7 100644 --- a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/CodeInjection.expected +++ b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/CodeInjection.expected @@ -110,6 +110,15 @@ nodes | react.js:10:56:10:77 | documen ... on.hash | | react.js:10:56:10:77 | documen ... on.hash | | react.js:10:56:10:77 | documen ... on.hash | +| rxjs.js:9:9:9:35 | taint | +| rxjs.js:9:17:9:35 | req.param("wobble") | +| rxjs.js:9:17:9:35 | req.param("wobble") | +| rxjs.js:12:16:12:20 | taint | +| rxjs.js:14:12:14:12 | v | +| rxjs.js:15:12:15:12 | v | +| rxjs.js:15:12:15:12 | v | +| rxjs.js:19:10:19:22 | subject.value | +| rxjs.js:19:10:19:22 | subject.value | | template-sinks.js:17:9:17:31 | tainted | | template-sinks.js:17:19:17:31 | req.query.foo | | template-sinks.js:17:19:17:31 | req.query.foo | @@ -232,6 +241,14 @@ edges | react-native.js:7:17:7:33 | req.param("code") | react-native.js:7:7:7:33 | tainted | | react-native.js:7:17:7:33 | req.param("code") | react-native.js:7:7:7:33 | tainted | | react.js:10:56:10:77 | documen ... on.hash | react.js:10:56:10:77 | documen ... on.hash | +| rxjs.js:9:9:9:35 | taint | rxjs.js:12:16:12:20 | taint | +| rxjs.js:9:17:9:35 | req.param("wobble") | rxjs.js:9:9:9:35 | taint | +| rxjs.js:9:17:9:35 | req.param("wobble") | rxjs.js:9:9:9:35 | taint | +| rxjs.js:12:16:12:20 | taint | rxjs.js:14:12:14:12 | v | +| rxjs.js:12:16:12:20 | taint | rxjs.js:19:10:19:22 | subject.value | +| rxjs.js:12:16:12:20 | taint | rxjs.js:19:10:19:22 | subject.value | +| rxjs.js:14:12:14:12 | v | rxjs.js:15:12:15:12 | v | +| rxjs.js:14:12:14:12 | v | rxjs.js:15:12:15:12 | v | | template-sinks.js:17:9:17:31 | tainted | template-sinks.js:19:17:19:23 | tainted | | template-sinks.js:17:9:17:31 | tainted | template-sinks.js:19:17:19:23 | tainted | | template-sinks.js:17:9:17:31 | tainted | template-sinks.js:20:16:20:22 | tainted | @@ -313,6 +330,8 @@ edges | react-native.js:8:32:8:38 | tainted | react-native.js:7:17:7:33 | req.param("code") | react-native.js:8:32:8:38 | tainted | $@ flows to here and is interpreted as code. | react-native.js:7:17:7:33 | req.param("code") | User-provided value | | react-native.js:10:23:10:29 | tainted | react-native.js:7:17:7:33 | req.param("code") | react-native.js:10:23:10:29 | tainted | $@ flows to here and is interpreted as code. | react-native.js:7:17:7:33 | req.param("code") | User-provided value | | react.js:10:56:10:77 | documen ... on.hash | react.js:10:56:10:77 | documen ... on.hash | react.js:10:56:10:77 | documen ... on.hash | $@ flows to here and is interpreted as code. | react.js:10:56:10:77 | documen ... on.hash | User-provided value | +| rxjs.js:15:12:15:12 | v | rxjs.js:9:17:9:35 | req.param("wobble") | rxjs.js:15:12:15:12 | v | $@ flows to here and is interpreted as code. | rxjs.js:9:17:9:35 | req.param("wobble") | User-provided value | +| rxjs.js:19:10:19:22 | subject.value | rxjs.js:9:17:9:35 | req.param("wobble") | rxjs.js:19:10:19:22 | subject.value | $@ flows to here and is interpreted as code. | rxjs.js:9:17:9:35 | req.param("wobble") | User-provided value | | template-sinks.js:19:17:19:23 | tainted | template-sinks.js:17:19:17:31 | req.query.foo | template-sinks.js:19:17:19:23 | tainted | $@ flows to here and is interpreted as a template, which may contain code. | template-sinks.js:17:19:17:31 | req.query.foo | User-provided value | | template-sinks.js:20:16:20:22 | tainted | template-sinks.js:17:19:17:31 | req.query.foo | template-sinks.js:20:16:20:22 | tainted | $@ flows to here and is interpreted as a template, which may contain code. | template-sinks.js:17:19:17:31 | req.query.foo | User-provided value | | template-sinks.js:21:18:21:24 | tainted | template-sinks.js:17:19:17:31 | req.query.foo | template-sinks.js:21:18:21:24 | tainted | $@ flows to here and is interpreted as a template, which may contain code. | template-sinks.js:17:19:17:31 | req.query.foo | User-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/HeuristicSourceCodeInjection.expected b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/HeuristicSourceCodeInjection.expected index d1a6a3deef3c..82a89178cd9b 100644 --- a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/HeuristicSourceCodeInjection.expected +++ b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/HeuristicSourceCodeInjection.expected @@ -114,6 +114,15 @@ nodes | react.js:10:56:10:77 | documen ... on.hash | | react.js:10:56:10:77 | documen ... on.hash | | react.js:10:56:10:77 | documen ... on.hash | +| rxjs.js:9:9:9:35 | taint | +| rxjs.js:9:17:9:35 | req.param("wobble") | +| rxjs.js:9:17:9:35 | req.param("wobble") | +| rxjs.js:12:16:12:20 | taint | +| rxjs.js:14:12:14:12 | v | +| rxjs.js:15:12:15:12 | v | +| rxjs.js:15:12:15:12 | v | +| rxjs.js:19:10:19:22 | subject.value | +| rxjs.js:19:10:19:22 | subject.value | | template-sinks.js:17:9:17:31 | tainted | | template-sinks.js:17:19:17:31 | req.query.foo | | template-sinks.js:17:19:17:31 | req.query.foo | @@ -240,6 +249,14 @@ edges | react-native.js:7:17:7:33 | req.param("code") | react-native.js:7:7:7:33 | tainted | | react-native.js:7:17:7:33 | req.param("code") | react-native.js:7:7:7:33 | tainted | | react.js:10:56:10:77 | documen ... on.hash | react.js:10:56:10:77 | documen ... on.hash | +| rxjs.js:9:9:9:35 | taint | rxjs.js:12:16:12:20 | taint | +| rxjs.js:9:17:9:35 | req.param("wobble") | rxjs.js:9:9:9:35 | taint | +| rxjs.js:9:17:9:35 | req.param("wobble") | rxjs.js:9:9:9:35 | taint | +| rxjs.js:12:16:12:20 | taint | rxjs.js:14:12:14:12 | v | +| rxjs.js:12:16:12:20 | taint | rxjs.js:19:10:19:22 | subject.value | +| rxjs.js:12:16:12:20 | taint | rxjs.js:19:10:19:22 | subject.value | +| rxjs.js:14:12:14:12 | v | rxjs.js:15:12:15:12 | v | +| rxjs.js:14:12:14:12 | v | rxjs.js:15:12:15:12 | v | | template-sinks.js:17:9:17:31 | tainted | template-sinks.js:19:17:19:23 | tainted | | template-sinks.js:17:9:17:31 | tainted | template-sinks.js:19:17:19:23 | tainted | | template-sinks.js:17:9:17:31 | tainted | template-sinks.js:20:16:20:22 | tainted | diff --git a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/rxjs.js b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/rxjs.js new file mode 100644 index 000000000000..37f4b7631402 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/rxjs.js @@ -0,0 +1,21 @@ +var express = require('express'); + +var app = express(); + +import { BehaviorSubject } from 'rxjs'; + + +app.get('/some/path', function(req, res) { + const taint = req.param("wobble"); + + const subject = new BehaviorSubject(); + subject.next(taint); + subject.subscribe({ + next: (v) => { + eval(v); // NOT OK + } + }); + setTimeout(() => { + eval(subject.value); // NOT OK + }, 100); +});