From 972c4d61e556b4645c5cd9e822d4ef22dd69ab67 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Mon, 23 Nov 2020 17:28:58 +0000 Subject: [PATCH] JS: Add PrototypePollutingAssignment --- javascript/config/suites/javascript/security | 1 + .../PrototypePollutingAssignment.qhelp | 62 +++++++ .../CWE-471/PrototypePollutingAssignment.ql | 23 +++ .../examples/PrototypePollutingAssignment.js | 11 ++ .../PrototypePollutingAssignmentFixed.js | 12 ++ .../javascript/dataflow/Configuration.qll | 1 + .../dataflow/PrototypePollutingAssignment.qll | 175 ++++++++++++++++++ ...otypePollutingAssignmentCustomizations.qll | 60 ++++++ 8 files changed, 345 insertions(+) create mode 100644 javascript/ql/src/Security/CWE-471/PrototypePollutingAssignment.qhelp create mode 100644 javascript/ql/src/Security/CWE-471/PrototypePollutingAssignment.ql create mode 100644 javascript/ql/src/Security/CWE-471/examples/PrototypePollutingAssignment.js create mode 100644 javascript/ql/src/Security/CWE-471/examples/PrototypePollutingAssignmentFixed.js create mode 100644 javascript/ql/src/semmle/javascript/security/dataflow/PrototypePollutingAssignment.qll create mode 100644 javascript/ql/src/semmle/javascript/security/dataflow/PrototypePollutingAssignmentCustomizations.qll diff --git a/javascript/config/suites/javascript/security b/javascript/config/suites/javascript/security index e8575ed8db5..707890e8296 100644 --- a/javascript/config/suites/javascript/security +++ b/javascript/config/suites/javascript/security @@ -39,6 +39,7 @@ + semmlecode-javascript-queries/Security/CWE-352/MissingCsrfMiddleware.ql: /Security/CWE/CWE-352 + semmlecode-javascript-queries/Security/CWE-400/PrototypePollution.ql: /Security/CWE/CWE-400 + semmlecode-javascript-queries/Security/CWE-400/PrototypePollutionUtility.ql: /Security/CWE/CWE-400 ++ semmlecode-javascript-queries/Security/CWE-471/PrototypePollutingAssignment.ql: /Security/CWE/CWE-471 + semmlecode-javascript-queries/Security/CWE-400/RemotePropertyInjection.ql: /Security/CWE/CWE-400 + semmlecode-javascript-queries/Security/CWE-502/UnsafeDeserialization.ql: /Security/CWE/CWE-502 + semmlecode-javascript-queries/Security/CWE-506/HardcodedDataInterpretedAsCode.ql: /Security/CWE/CWE-506 diff --git a/javascript/ql/src/Security/CWE-471/PrototypePollutingAssignment.qhelp b/javascript/ql/src/Security/CWE-471/PrototypePollutingAssignment.qhelp new file mode 100644 index 00000000000..4ba2e9dc2d2 --- /dev/null +++ b/javascript/ql/src/Security/CWE-471/PrototypePollutingAssignment.qhelp @@ -0,0 +1,62 @@ + + + + +

+ Most JavaScript objects inherit the properties of the built-in Object.prototype object. + Prototype pollution is a type of vulnerability in which an attacker is able to modify Object.prototype. + Since most objects inherit from the compromised Object.prototype, the attacker can use this + to tamper with the application logic, and often escalate to remote code execution or cross-site scripting. +

+ +

+ One way to cause prototype pollution is by modifying an object obtained via a user-controlled property name. + Most objects have a special __proto__ property that refers to Object.prototype. + An attacker can abuse this special property to trick the application into performing unintended modifications + of Object.prototype. +

+ + + +

+ Use an associative data structure that is resilient to untrusted key values, such as a Map. + In some cases, a prototype-less object created with Object.create(null) + may be preferrable. +

+

+ Alternatively, restrict the computed property name so it can't clash with a built-in property, either by + prefixing it with a constant string, or by rejecting inputs that don't conform to the expected format. +

+ + + +

+ In the example below, the untrusted value req.params.id is used as the property name + req.session.todos[id]. If a malicious user passes in the ID value __proto__, + the variable todo will then refer to Object.prototype. + Finally, the modification of todo then allows the attacker to inject arbitrary properties + onto Object.prototype. +

+ + + +

+ One way to fix this is to use Map objects to associate key/value pairs + instead of regular objects, as shown below: +

+ + + + + + +
  • MDN: + Object.prototype.__proto__ +
    • +
  • MDN: + Map +
    • +     +     diff --git a/javascript/ql/src/Security/CWE-471/PrototypePollutingAssignment.ql b/javascript/ql/src/Security/CWE-471/PrototypePollutingAssignment.ql new file mode 100644 index 00000000000..1992b8e5014 --- /dev/null +++ b/javascript/ql/src/Security/CWE-471/PrototypePollutingAssignment.ql @@ -0,0 +1,23 @@ +/** + * @name Prototype-polluting assignment + * @description Modifying an object obtained via a user-controlled property name may + * lead to accidental modification of the built-in Object.prototype, + * and possibly escalate to remote code execution or cross-site scripting. + * @kind path-problem + * @problem.severity warning + * @precision high + * @id js/prototype-polluting-assignment + * @tags security + * external/cwe/cwe-250 + * external/cwe/cwe-471 + */ + +import javascript +import semmle.javascript.security.dataflow.PrototypePollutingAssignment::PrototypePollutingAssignment +import DataFlow::PathGraph + +from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink +where cfg.hasFlowPath(source, sink) +select sink, source, sink, + "This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@.", + source.getNode(), "here" diff --git a/javascript/ql/src/Security/CWE-471/examples/PrototypePollutingAssignment.js b/javascript/ql/src/Security/CWE-471/examples/PrototypePollutingAssignment.js new file mode 100644 index 00000000000..d78fc490acd --- /dev/null +++ b/javascript/ql/src/Security/CWE-471/examples/PrototypePollutingAssignment.js @@ -0,0 +1,11 @@ +let express = require('express'); + +express.put('/todos/:id', (req, res) => { + let id = req.params.id; + let items = req.session.todos[id]; + if (!items) { + items = req.session.todos[id] = {}; + } + items[req.query.name] = req.query.text; + res.end(200); +}); diff --git a/javascript/ql/src/Security/CWE-471/examples/PrototypePollutingAssignmentFixed.js b/javascript/ql/src/Security/CWE-471/examples/PrototypePollutingAssignmentFixed.js new file mode 100644 index 00000000000..f010537acf1 --- /dev/null +++ b/javascript/ql/src/Security/CWE-471/examples/PrototypePollutingAssignmentFixed.js @@ -0,0 +1,12 @@ +let express = require('express'); + +express.put('/todos/:id', (req, res) => { + let id = req.params.id; + let items = req.session.todos.get(id); + if (!items) { + items = new Map(); + req.sessions.todos.set(id, items); + } + items.set(req.query.name, req.query.text); + res.end(200); +}); diff --git a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll index b0aad4460e1..62f04db9055 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll @@ -1281,6 +1281,7 @@ private predicate summarizedHigherOrderCall( DataFlow::Node innerArg, DataFlow::SourceNode cbParm, PathSummary oldSummary | reachableFromInput(f, outer, arg, innerArg, cfg, oldSummary) and + not arg = DataFlow::capturedVariableNode(_) and // Only track actual parameter flow argumentPassing(outer, cb, f, cbParm) and innerArg = inner.getArgument(j) | diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/PrototypePollutingAssignment.qll b/javascript/ql/src/semmle/javascript/security/dataflow/PrototypePollutingAssignment.qll new file mode 100644 index 00000000000..eb9359b29ea --- /dev/null +++ b/javascript/ql/src/semmle/javascript/security/dataflow/PrototypePollutingAssignment.qll @@ -0,0 +1,175 @@ +/** + * Provides a taint tracking configuration for reasoning about + * prototype-polluting assignments. + * + * Note, for performance reasons: only import this file if + * `PrototypePollutingAssignment::Configuration` is needed, otherwise + * `PrototypePollutingAssignmentCustomizations` should be imported instead. + */ + +private import javascript +private import semmle.javascript.DynamicPropertyAccess + +module PrototypePollutingAssignment { + private import PrototypePollutingAssignmentCustomizations::PrototypePollutingAssignment + + // Materialize flow labels + private class ConcreteObjectPrototype extends ObjectPrototype { } + + /** A taint-tracking configuration for reasoning about prototype-polluting assignments. */ + class Configuration extends TaintTracking::Configuration { + Configuration() { this = "PrototypePollutingAssignment" } + + override predicate isSource(DataFlow::Node node) { node instanceof Source } + + override predicate isSink(DataFlow::Node node, DataFlow::FlowLabel lbl) { + node.(Sink).getAFlowLabel() = lbl + } + + override predicate isSanitizer(DataFlow::Node node) { + node instanceof Sanitizer + or + // Concatenating with a string will in practice prevent the string `__proto__` from arising. + node instanceof StringOps::ConcatenationRoot + } + + override predicate isAdditionalFlowStep( + DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel inlbl, + DataFlow::FlowLabel outlbl + ) { + // Step from x -> obj[x] while switching to the ObjectPrototype label + // (If `x` can have the value `__proto__` then the result can be Object.prototype) + exists(DataFlow::PropRead read | + pred = read.getPropertyNameExpr().flow() and + succ = read and + inlbl.isTaint() and + outlbl instanceof ObjectPrototype and + // Exclude cases where the property name came from a property enumeration. + // If the property name is an own property of the base object, the read won't + // return Object.prototype. + not read = any(EnumeratedPropName n).getASourceProp() and + // Exclude cases where the read has no prototype, or a prototype other than Object.prototype. + not read = prototypeLessObject().getAPropertyRead() and + // Exclude cases where this property has just been assigned to + not read.(DynamicPropRead).hasDominatingAssignment() + ) + or + // Same as above, but for property projection. + exists(PropertyProjection proj | + proj.isSingletonProjection() and + pred = proj.getASelector() and + succ = proj and + inlbl.isTaint() and + outlbl instanceof ObjectPrototype + ) + } + + override predicate isLabeledBarrier(DataFlow::Node node, DataFlow::FlowLabel lbl) { + // Don't propagate the receiver into method calls, as the method lookup will fail on Object.prototype. + node = any(DataFlow::MethodCallNode m).getReceiver() and + lbl instanceof ObjectPrototype + } + + override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) { + guard instanceof PropertyPresenceCheck or + guard instanceof InExprCheck or + guard instanceof InstanceofCheck or + guard instanceof IsArrayCheck or + guard instanceof EqualityCheck + } + } + + /** Gets a data flow node referring to an object created with `Object.create`. */ + DataFlow::SourceNode prototypeLessObject() { + result = prototypeLessObject(DataFlow::TypeTracker::end()) + } + + private DataFlow::SourceNode prototypeLessObject(DataFlow::TypeTracker t) { + t.start() and + // We assume the argument to Object.create is not Object.prototype, since most + // users wouldn't bother to call Object.create in that case. + result = DataFlow::globalVarRef("Object").getAMemberCall("create") + or + // Allow use of AdditionalFlowSteps and AdditionalTaintSteps to track a bit further + exists(DataFlow::Node mid | + prototypeLessObject(t.continue()).flowsTo(mid) and + any(DataFlow::AdditionalFlowStep s).step(mid, result) + ) + or + exists(DataFlow::TypeTracker t2 | result = prototypeLessObject(t2).track(t2, t)) + } + + /** Holds if `Object.prototype` has a member named `prop`. */ + private predicate isPropertyPresentOnObjectPrototype(string prop) { + exists(ExternalInstanceMemberDecl decl | + decl.getBaseName() = "Object" and + decl.getName() = prop + ) + } + + /** A check of form `e.prop` where `prop` is not present on `Object.prototype`. */ + private class PropertyPresenceCheck extends TaintTracking::LabeledSanitizerGuardNode, + DataFlow::ValueNode { + override PropAccess astNode; + + PropertyPresenceCheck() { not isPropertyPresentOnObjectPrototype(astNode.getPropertyName()) } + + override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) { + e = astNode.getBase() and + outcome = true and + label instanceof ObjectPrototype + } + } + + /** A check of form `"prop" in e` where `prop` is not present on `Object.prototype`. */ + private class InExprCheck extends TaintTracking::LabeledSanitizerGuardNode, DataFlow::ValueNode { + override InExpr astNode; + + InExprCheck() { + not isPropertyPresentOnObjectPrototype(astNode.getLeftOperand().getStringValue()) + } + + override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) { + e = astNode.getRightOperand() and + outcome = true and + label instanceof ObjectPrototype + } + } + + /** A check of form `e instanceof X`, which is always false for `Object.prototype`. */ + private class InstanceofCheck extends TaintTracking::LabeledSanitizerGuardNode, + DataFlow::ValueNode { + override InstanceofExpr astNode; + + override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) { + e = astNode.getLeftOperand() and + outcome = true and + label instanceof ObjectPrototype + } + } + + /** A call to `Array.isArray`, which is false for `Object.prototype`. */ + private class IsArrayCheck extends TaintTracking::LabeledSanitizerGuardNode, DataFlow::CallNode { + IsArrayCheck() { this = DataFlow::globalVarRef("Array").getAMemberCall("isArray") } + + override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) { + e = getArgument(0).asExpr() and + outcome = true and + label instanceof ObjectPrototype + } + } + + /** + * Sanitizer guard of form `x !== "__proto__"`. + */ + private class EqualityCheck extends TaintTracking::SanitizerGuardNode, DataFlow::ValueNode { + override EqualityTest astNode; + + EqualityCheck() { astNode.getAnOperand().getStringValue() = "__proto__" } + + override predicate sanitizes(boolean outcome, Expr e) { + e = astNode.getAnOperand() and + outcome = astNode.getPolarity().booleanNot() + } + } +} diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/PrototypePollutingAssignmentCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/PrototypePollutingAssignmentCustomizations.qll new file mode 100644 index 00000000000..f4fa9441bdf --- /dev/null +++ b/javascript/ql/src/semmle/javascript/security/dataflow/PrototypePollutingAssignmentCustomizations.qll @@ -0,0 +1,60 @@ +/** + * Provides sources, sinks, and sanitizers for reasoning about assignments + * that my cause prototype pollution. + */ + +private import javascript + +/** + * Provides sources, sinks, and sanitizers for reasoning about assignments + * that my cause prototype pollution. + */ +module PrototypePollutingAssignment { + /** + * A data flow source for untrusted data from which the special `__proto__` property name may be arise. + */ + abstract class Source extends DataFlow::Node { } + + /** + * A data flow sink for prototype-polluting assignments or untrusted property names. + */ + abstract class Sink extends DataFlow::Node { + /** + * The flow label relevant for this sink. + * + * Use the `taint` label for untrusted property names, and the `ObjectPrototype` label for + * object mutations. + */ + abstract DataFlow::FlowLabel getAFlowLabel(); + } + + /** + * A sanitizer for untrusted property names. + */ + abstract class Sanitizer extends DataFlow::Node { } + + /** Flow label representing the `Object.prototype` value. */ + abstract class ObjectPrototype extends DataFlow::FlowLabel { + ObjectPrototype() { this = "Object.prototype" } + } + + /** The base of an assignment or extend call, as a sink for `Object.prototype` references. */ + private class DefaultSink extends Sink { + DefaultSink() { + this = any(DataFlow::PropWrite write).getBase() + or + this = any(ExtendCall c).getDestinationOperand() + } + + override DataFlow::FlowLabel getAFlowLabel() { result instanceof ObjectPrototype } + } + + /** A remote flow source or location.{hash,search} as a taint source. */ + private class DefaultSource extends Source { + DefaultSource() { + this instanceof RemoteFlowSource + or + this = DOM::locationRef().getAPropertyRead(["hash", "search"]) + } + } +}