diff --git a/javascript/ql/src/Security/CWE-089/SqlInjection.actual b/javascript/ql/src/Security/CWE-089/SqlInjection.actual new file mode 100644 index 00000000000..e69de29bb2d diff --git a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll index 8fab8b9064f..e2c5f325b0d 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll @@ -93,7 +93,7 @@ abstract class Configuration extends string { } /** - * Holds if `source` is a source of flow labelled with `lbl` that is relevant + * Holds if `source` is a source of flow labeled with `lbl` that is relevant * for this configuration. */ predicate isSource(DataFlow::Node source, FlowLabel lbl) { @@ -108,7 +108,7 @@ abstract class Configuration extends string { } /** - * Holds if `sink` is a sink of flow labelled with `lbl` that is relevant + * Holds if `sink` is a sink of flow labeled with `lbl` that is relevant * for this configuration. */ predicate isSink(DataFlow::Node sink, FlowLabel lbl) { @@ -146,6 +146,7 @@ abstract class Configuration extends string { */ predicate isBarrier(DataFlow::Node node) { exists (BarrierGuardNode guard | + not guard instanceof LabeledBarrierGuardNode and isBarrierGuard(guard) and guard.blocks(node) ) @@ -161,6 +162,17 @@ abstract class Configuration extends string { */ predicate isBarrier(DataFlow::Node src, DataFlow::Node trg, FlowLabel lbl) { none() } + /** + * Holds if flow with label `lbl` cannot flow into `node`. + */ + predicate isLabeledBarrier(DataFlow::Node node, FlowLabel lbl) { + exists (LabeledBarrierGuardNode guard | + lbl = guard.getALabel() and + isBarrierGuard(guard) and + guard.blocks(node) + ) + } + /** * Holds if data flow node `guard` can act as a barrier when appearing * in a condition. @@ -297,7 +309,16 @@ abstract class BarrierGuardNode extends DataFlow::Node { * Holds if this node blocks expression `e` provided it evaluates to `outcome`. */ abstract predicate blocks(boolean outcome, Expr e); +} +/** + * A guard node that only blocks specific labels. + */ +abstract class LabeledBarrierGuardNode extends BarrierGuardNode { + /** + * Get a flow label blocked by this guard node. + */ + abstract FlowLabel getALabel(); } /** @@ -570,7 +591,8 @@ private predicate flowThroughCall(DataFlow::Node input, DataFlow::Node invk, ret.asExpr() = f.getAReturnedExpr() and calls(invk, f) and // Do not consider partial calls reachableFromInput(f, invk, input, ret, cfg, summary) and - not cfg.isBarrier(ret, invk) + not cfg.isBarrier(ret, invk) and + not cfg.isLabeledBarrier(invk, summary.getEndLabel()) ) } @@ -641,7 +663,8 @@ private predicate flowStep(DataFlow::Node pred, DataFlow::Configuration cfg, flowThroughProperty(pred, succ, cfg, summary) ) and not cfg.isBarrier(succ) and - not cfg.isBarrier(pred, succ) + not cfg.isBarrier(pred, succ) and + not cfg.isLabeledBarrier(succ, summary.getEndLabel()) } /** @@ -666,6 +689,7 @@ private predicate reachableFromSource(DataFlow::Node nd, DataFlow::Configuration exists (FlowLabel lbl | isSource(nd, cfg, lbl) and not cfg.isBarrier(nd) and + not cfg.isLabeledBarrier(nd, lbl) and summary = MkPathSummary(false, false, lbl, lbl) ) or @@ -684,7 +708,8 @@ private predicate onPath(DataFlow::Node nd, DataFlow::Configuration cfg, PathSummary summary) { reachableFromSource(nd, cfg, summary) and isSink(nd, cfg, summary.getEndLabel()) and - not cfg.isBarrier(nd) + not cfg.isBarrier(nd) and + not cfg.isLabeledBarrier(nd, summary.getEndLabel()) or exists (DataFlow::Node mid, PathSummary stepSummary | reachableFromSource(nd, cfg, summary) and diff --git a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll index 8a33ebb5c80..53cf8767f6d 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll @@ -130,7 +130,7 @@ module TaintTracking { * configurations it is used in. * * Note: For performance reasons, all subclasses of this class should be part - * of the standard library. Override `Configuration::isTaintSanitizerGuard` + * of the standard library. Override `Configuration::isSanitizer` * for analysis-specific taint steps. */ abstract class AdditionalSanitizerGuardNode extends SanitizerGuardNode { @@ -159,6 +159,12 @@ module TaintTracking { } + /** + * A sanitizer guard node that only blocks specific flow labels. + */ + abstract class LabeledSanitizerGuardNode extends SanitizerGuardNode, DataFlow::LabeledBarrierGuardNode { + } + /** * DEPRECATED: Override `Configuration::isAdditionalTaintStep` or use * `AdditionalTaintStep` instead. diff --git a/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll b/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll index 69646cec816..95fb763f8cd 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll @@ -74,10 +74,12 @@ predicate localFlowStep(DataFlow::Node pred, DataFlow::Node succ, any(DataFlow::AdditionalFlowStep afs).step(pred, succ) and predlbl = succlbl or exists (boolean vp | configuration.isAdditionalFlowStep(pred, succ, vp) | - if vp = false and (predlbl = FlowLabel::data() or predlbl = FlowLabel::taint()) then - succlbl = FlowLabel::taint() - else - predlbl = succlbl + vp = true and + predlbl = succlbl + or + vp = false and + (predlbl = FlowLabel::data() or predlbl = FlowLabel::taint()) and + succlbl = FlowLabel::taint() ) or configuration.isAdditionalFlowStep(pred, succ, predlbl, succlbl) diff --git a/javascript/ql/src/semmle/javascript/frameworks/Express.qll b/javascript/ql/src/semmle/javascript/frameworks/Express.qll index 9c97daf932d..eb9467049bc 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/Express.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/Express.qll @@ -488,6 +488,31 @@ module Express { override string getKind() { result = kind } + + override predicate isUserControlledObject() { + kind = "body" and + exists (ExpressLibraries::BodyParser bodyParser, RouteHandlerExpr expr | + expr.getBody() = rh and + bodyParser.producesUserControlledObjects() and + bodyParser.flowsToExpr(expr.getAMatchingAncestor()) + ) + or + // If we can't find the middlewares for the route handler, + // but all known body parsers are deep, assume req.body is a deep object. + kind = "body" and + forall(ExpressLibraries::BodyParser bodyParser | bodyParser.producesUserControlledObjects()) + or + kind = "parameter" and + exists (DataFlow::Node request | request = DataFlow::valueNode(rh.getARequestExpr()) | + this.(DataFlow::MethodCallNode).calls(request, "param") + or + exists (DataFlow::PropRead base | + // `req.query.name` + base.accesses(request, "query") and + this = base.getAPropertyReference(_) + ) + ) + } } /** diff --git a/javascript/ql/src/semmle/javascript/frameworks/ExpressModules.qll b/javascript/ql/src/semmle/javascript/frameworks/ExpressModules.qll index e7538c54612..6f184880b16 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/ExpressModules.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/ExpressModules.qll @@ -230,4 +230,53 @@ module ExpressLibraries { } + /** + * An instance of the Express `body-parser` middleware. + */ + class BodyParser extends DataFlow::InvokeNode { + string kind; + + BodyParser() { + this = DataFlow::moduleImport("body-parser").getACall() and kind = "json" + or + exists (string moduleName | + (moduleName = "body-parser" or moduleName = "express") and + (kind = "json" or kind = "urlencoded") and + this = DataFlow::moduleMember(moduleName, kind).getACall() + ) + } + + /** + * Holds if this is a JSON body parser. + */ + predicate isJson() { + kind = "json" + } + + /** + * Holds if this is a URL-encoded body parser. + */ + predicate isUrlEncoded() { + kind = "urlencoded" + } + + /** + * Holds if this is an extended URL-encoded body parser. + * + * The extended URL-encoding allows for nested objects, like JSON. + */ + predicate isExtendedUrlEncoded() { + kind = "urlencoded" and + not getOptionArgument(0, "extended").mayHaveBooleanValue(false) + } + + /** + * Holds if this parses the input as JSON or extended URL-encoding, resulting + * in user-controlled objects (as opposed to user-controlled strings). + */ + predicate producesUserControlledObjects() { + isJson() or isExtendedUrlEncoded() + } + } + } diff --git a/javascript/ql/src/semmle/javascript/security/TaintedObject.qll b/javascript/ql/src/semmle/javascript/security/TaintedObject.qll new file mode 100644 index 00000000000..efb5545b2c3 --- /dev/null +++ b/javascript/ql/src/semmle/javascript/security/TaintedObject.qll @@ -0,0 +1,119 @@ +/** + * Provides methods for reasoning about the flow of deeply tainted objects, such as JSON objects + * parsed from user-controlled data. + * + * Deeply tainted objects are arrays or objects with user-controlled property names, containing + * tainted values or deeply tainted objects in their properties. + * + * To track deeply tainted objects, a flow-tracking configuration should generally include the following: + * + * 1. One or more sinks associated with the label `TaintedObject::label()`. + * 2. The sources from `TaintedObject::isSource`. + * 3. The flow steps from `TaintedObject::step`. + * 4. The sanitizing guards `TaintedObject::SanitizerGuard`. + */ +import javascript + +module TaintedObject { + private import DataFlow + + private class TaintedObjectLabel extends FlowLabel { + TaintedObjectLabel() { this = "tainted-object" } + } + + /** + * Gets the flow label representing a deeply tainted object. + * + * A "tainted object" is an array or object whose property values are all assumed to be tainted as well. + * + * Note that the presence of the this label generally implies the presence of the `taint` label as well. + */ + FlowLabel label() { result instanceof TaintedObjectLabel } + + /** + * Holds for the flows steps that are relevant for tracking user-controlled JSON objects. + */ + predicate step(Node src, Node trg, FlowLabel inlbl, FlowLabel outlbl) { + // JSON parsers map tainted inputs to tainted JSON + (inlbl = FlowLabel::data() or inlbl = FlowLabel::taint()) and + outlbl = label() and + exists (JsonParserCall parse | + src = parse.getInput() and + trg = parse.getOutput()) + or + // Property reads preserve deep object taint. + inlbl = label() and + outlbl = label() and + trg.(PropRead).getBase() = src + or + // Property projection preserves deep object taint + inlbl = label() and + outlbl = label() and + trg.(PropertyProjection).getObject() = src + or + // Extending objects preserves deep object taint + inlbl = label() and + outlbl = label() and + exists (ExtendCall call | + src = call.getAnOperand() and + trg = call + or + src = call.getASourceOperand() and + trg = call.getDestinationOperand().getALocalSource()) + } + + /** + * Holds if `node` is a source of JSON taint and label is the JSON taint label. + */ + predicate isSource(Node source, FlowLabel label) { + source instanceof Source and label = label() + } + + /** + * A source of a user-controlled deep object. + */ + abstract class Source extends DataFlow::Node {} + + /** Request input accesses as a JSON source. */ + private class RequestInputAsSource extends Source { + RequestInputAsSource() { + this.(HTTP::RequestInputAccess).isUserControlledObject() + } + } + + /** + * Sanitizer guard that blocks deep object taint. + */ + abstract class SanitizerGuard extends TaintTracking::LabeledSanitizerGuardNode { + override FlowLabel getALabel() { + result = label() + } + } + + /** + * A test of form `typeof x === "something"`, preventing `x` from being an object in some cases. + */ + private class TypeTestGuard extends SanitizerGuard, ValueNode { + override EqualityTest astNode; + TypeofExpr typeof; + boolean polarity; + + TypeTestGuard() { + astNode.getAnOperand() = typeof and + ( + // typeof x === "object" sanitizes `x` when it evaluates to false + astNode.getAnOperand().getStringValue() = "object" and + polarity = astNode.getPolarity().booleanNot() + or + // typeof x === "string" sanitizes `x` when it evaluates to true + astNode.getAnOperand().getStringValue() != "object" and + polarity = astNode.getPolarity() + ) + } + + override predicate sanitizes(boolean outcome, Expr e) { + polarity = outcome and + e = typeof.getOperand() + } + } +} diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirect.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirect.qll index 35bb5fa950d..dc331b7d2a8 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirect.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirect.qll @@ -65,6 +65,11 @@ module ClientSideUrlRedirect { queryAccess(pred, succ) and f instanceof DocumentUrl and g = DataFlow::FlowLabel::taint() + or + // preserve document.url label in step from `location` to `location.href` + f instanceof DocumentUrl and + g instanceof DocumentUrl and + succ.(DataFlow::PropRead).accesses(pred, "href") } } diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/NosqlInjection.qll b/javascript/ql/src/semmle/javascript/security/dataflow/NosqlInjection.qll index 9c031728d7e..c338e6ced50 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/NosqlInjection.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/NosqlInjection.qll @@ -4,6 +4,7 @@ */ import javascript +import semmle.javascript.security.TaintedObject module NosqlInjection { /** @@ -14,7 +15,16 @@ module NosqlInjection { /** * A data flow sink for SQL-injection vulnerabilities. */ - abstract class Sink extends DataFlow::Node { } + abstract class Sink extends DataFlow::Node { + /** + * Gets a flow label relevant for this sink. + * + * Defaults to deeply tainted objects only. + */ + DataFlow::FlowLabel getAFlowLabel() { + result = TaintedObject::label() + } + } /** * A sanitizer for SQL-injection vulnerabilities. @@ -31,8 +41,12 @@ module NosqlInjection { source instanceof Source } - override predicate isSink(DataFlow::Node sink) { - sink instanceof Sink + override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) { + TaintedObject::isSource(source, label) + } + + override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) { + sink.(Sink).getAFlowLabel() = label } override predicate isSanitizer(DataFlow::Node node) { @@ -40,12 +54,20 @@ module NosqlInjection { node instanceof Sanitizer } - override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { + override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) { + guard instanceof TaintedObject::SanitizerGuard + } + + override predicate isAdditionalFlowStep(DataFlow::Node src, DataFlow::Node trg, DataFlow::FlowLabel inlbl, DataFlow::FlowLabel outlbl) { + TaintedObject::step(src, trg, inlbl, outlbl) + or // additional flow step to track taint through NoSQL query objects + inlbl = TaintedObject::label() and + outlbl = TaintedObject::label() and exists (NoSQL::Query query, DataFlow::SourceNode queryObj | queryObj.flowsToExpr(query) and - queryObj.flowsTo(succ) and - pred = queryObj.getAPropertyWrite().getRhs() + queryObj.flowsTo(trg) and + src = queryObj.getAPropertyWrite().getRhs() ) } } diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/RemoteFlowSources.qll b/javascript/ql/src/semmle/javascript/security/dataflow/RemoteFlowSources.qll index a1138a0eb5e..964f9e988ef 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/RemoteFlowSources.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/RemoteFlowSources.qll @@ -10,6 +10,11 @@ import semmle.javascript.security.dataflow.DOM abstract class RemoteFlowSource extends DataFlow::Node { /** Gets a string that describes the type of this remote flow source. */ abstract string getSourceType(); + + /** + * Holds if this can be a user-controlled object, such as a JSON object parsed from user-controlled data. + */ + predicate isUserControlledObject() { none() } } /** diff --git a/javascript/ql/test/query-tests/Security/CWE-089/SqlInjection.expected b/javascript/ql/test/query-tests/Security/CWE-089/SqlInjection.expected index 7a99482adb8..e6c84156dd0 100644 --- a/javascript/ql/test/query-tests/Security/CWE-089/SqlInjection.expected +++ b/javascript/ql/test/query-tests/Security/CWE-089/SqlInjection.expected @@ -1,5 +1,7 @@ | mongodb.js:18:16:18:20 | query | This query depends on $@. | mongodb.js:13:19:13:26 | req.body | a user-provided value | -| mongodb.js:39:16:39:20 | query | This query depends on $@. | mongodb.js:34:19:34:33 | req.query.title | a user-provided value | +| mongodb.js:32:18:32:45 | { title ... itle) } | This query depends on $@. | mongodb.js:26:19:26:26 | req.body | a user-provided value | +| mongodb.js:54:16:54:20 | query | This query depends on $@. | mongodb.js:49:19:49:33 | req.query.title | a user-provided value | +| mongodb_bodySafe.js:29:16:29:20 | query | This query depends on $@. | mongodb_bodySafe.js:24:19:24:33 | req.query.title | a user-provided value | | mongoose.js:27:20:27:24 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value | | mongoose.js:30:25:30:29 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value | | mongoose.js:33:24:33:28 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-089/mongodb.js b/javascript/ql/test/query-tests/Security/CWE-089/mongodb.js index 900cad1cbba..00f3422ca40 100644 --- a/javascript/ql/test/query-tests/Security/CWE-089/mongodb.js +++ b/javascript/ql/test/query-tests/Security/CWE-089/mongodb.js @@ -16,6 +16,21 @@ app.post('/documents/find', (req, res) => { // NOT OK: query is tainted by user-provided object value doc.find(query); + + // OK: user-data is coerced to a string + doc.find({ title: '' + query.body.title }); + + // OK: throws unless user-data is a string + doc.find({ title: query.body.title.substr(1) }); + + let title = req.body.title; + if (typeof title === "string") { + // OK: input checked to be a string + doc.find({ title: title }); + + // NOT OK: input is parsed as JSON after string check + doc.find({ title: JSON.parse(title) }); + } }); }); diff --git a/javascript/ql/test/query-tests/Security/CWE-089/mongodb_bodySafe.js b/javascript/ql/test/query-tests/Security/CWE-089/mongodb_bodySafe.js new file mode 100644 index 00000000000..61a87bfaa5a --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-089/mongodb_bodySafe.js @@ -0,0 +1,31 @@ +const express = require('express'), + mongodb = require('mongodb'), + bodyParser = require('body-parser'); + +const MongoClient = mongodb.MongoClient; + +const app = express(); + +app.use(bodyParser.urlencoded({ extended: false })); + +app.post('/documents/find', (req, res) => { + const query = {}; + query.title = req.body.title; + MongoClient.connect('mongodb://localhost:27017/test', (err, db) => { + let doc = db.collection('doc'); + + // OK: req.body is safe + doc.find(query); + }); +}); + +app.post('/documents/find', (req, res) => { + const query = {}; + query.title = req.query.title; + MongoClient.connect('mongodb://localhost:27017/test', (err, db) => { + let doc = db.collection('doc'); + + // NOT OK: regardless of body parser, query value is still tainted + doc.find(query); + }); +});