diff --git a/javascript/change-notes/2021-03-01-ajv.md b/javascript/change-notes/2021-03-01-ajv.md new file mode 100644 index 000000000000..c06bd4f5807f --- /dev/null +++ b/javascript/change-notes/2021-03-01-ajv.md @@ -0,0 +1,8 @@ +lgtm,codescanning +* The security queries now recognize the effect of JSON schema validation, and highlights + cases where this validation is susceptible to denial-of-service attacks. + Affects the package [ajv](https://npmjs.com/package/ajv). +* A new query, `js/resource-exhaustion-from-deep-object-traversal`, has been added to the query suite, + highlighting denial-of-service attacks exploiting operations that traverse deeply user-controlled objects. +* The `js/xss-through-exception` query now recognizes JSON schema validation errors as a source, as they + may contain part of the input data. diff --git a/javascript/ql/src/Security/CWE-079/ExceptionXss.qhelp b/javascript/ql/src/Security/CWE-079/ExceptionXss.qhelp index f815794f6b80..7ab1038b067e 100644 --- a/javascript/ql/src/Security/CWE-079/ExceptionXss.qhelp +++ b/javascript/ql/src/Security/CWE-079/ExceptionXss.qhelp @@ -5,8 +5,8 @@

-Directly writing exceptions to a webpage without sanitization allows for a cross-site scripting -vulnerability if the value of the exception can be influenced by a user. +Directly writing error messages to a webpage without sanitization allows for a cross-site scripting +vulnerability if parts of the error message can be influenced by a user.

@@ -27,6 +27,19 @@ leaving the website vulnerable to cross-site scripting. + +

+This second example shows an input being validated using the JSON schema validator ajv, +and in case of an error, the error message is sent directly back in the response. +

+ +

+This is unsafe, because the error message can contain parts of the input. +For example, the input {'<img src=x onerror=alert(1)>': 'foo'} will generate the error +data/<img src=x onerror=alert(1)> should be number, causing reflected XSS. +

+ +
  • OWASP: diff --git a/javascript/ql/src/Security/CWE-079/ExceptionXss.ql b/javascript/ql/src/Security/CWE-079/ExceptionXss.ql index 0926894fb7ec..7c9cedc17056 100644 --- a/javascript/ql/src/Security/CWE-079/ExceptionXss.ql +++ b/javascript/ql/src/Security/CWE-079/ExceptionXss.ql @@ -19,4 +19,4 @@ from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink where cfg.hasFlowPath(source, sink) select sink.getNode(), source, sink, "$@ is reinterpreted as HTML without escaping meta-characters.", source.getNode(), - "Exception text" + source.getNode().(Source).getDescription() diff --git a/javascript/ql/src/Security/CWE-079/examples/ExceptionXssAjv.js b/javascript/ql/src/Security/CWE-079/examples/ExceptionXssAjv.js new file mode 100644 index 000000000000..8a3ddedb249b --- /dev/null +++ b/javascript/ql/src/Security/CWE-079/examples/ExceptionXssAjv.js @@ -0,0 +1,13 @@ +import express from 'express'; +import Ajv from 'ajv'; + +let app = express(); +let ajv = new Ajv(); + +ajv.addSchema({type: 'object', additionalProperties: {type: 'number'}}, 'pollData'); + +app.post('/polldata', (req, res) => { + if (!ajv.validate('pollData', req.body)) { + res.send(ajv.errorsText()); + } +}); diff --git a/javascript/ql/src/Security/CWE-400/DeepObjectResourceExhaustion.qhelp b/javascript/ql/src/Security/CWE-400/DeepObjectResourceExhaustion.qhelp new file mode 100644 index 000000000000..e778d9237d5d --- /dev/null +++ b/javascript/ql/src/Security/CWE-400/DeepObjectResourceExhaustion.qhelp @@ -0,0 +1,55 @@ + + + + +

    + Processing user-controlled data with a method that allocates excessive amounts + of memory can lead to denial of service. +

    + +

    + If the JSON schema validation library ajv is configured with + allErrors: true there is no limit to how many error objects + will be allocated. An attacker can exploit this by sending an object that + deliberately contains a huge number of errors, and in some cases, with + longer and longer error messages. This can cause the service to become + unresponsive due to the slow error-checking process. +

    +     + + +

    + Do not use allErrors: true in production. +

    +     + + +

    + In the example below, the user-submitted object req.body is + validated using ajv and allErrors: true: +

    + + + +

    + Although this ensures that req.body conforms to the schema, + the validation itself could be vulnerable to a denial-of-service attack. + An attacker could send an object containing so many errors that the server + runs out of memory. +

    + +

    + A solution is to not pass in allErrors: true, which means + ajv will only report the first error, not all of them: +

    + + +     + + +
  • Ajv documentation: security considerations +
    • +     + diff --git a/javascript/ql/src/Security/CWE-400/DeepObjectResourceExhaustion.ql b/javascript/ql/src/Security/CWE-400/DeepObjectResourceExhaustion.ql new file mode 100644 index 000000000000..69ce56fd17f4 --- /dev/null +++ b/javascript/ql/src/Security/CWE-400/DeepObjectResourceExhaustion.ql @@ -0,0 +1,23 @@ +/** + * @name Resources exhaustion from deep object traversal + * @description Processing user-controlled object hierarchies inefficiently can lead to denial of service. + * @kind path-problem + * @problem.severity warning + * @precision high + * @id js/resource-exhaustion-from-deep-object-traversal + * @tags security + * external/cwe/cwe-400 + */ + +import javascript +import DataFlow::PathGraph +import semmle.javascript.security.dataflow.DeepObjectResourceExhaustion::DeepObjectResourceExhaustion + +from + Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, DataFlow::Node link, + string reason +where + cfg.hasFlowPath(source, sink) and + sink.getNode().(Sink).hasReason(link, reason) +select sink, source, sink, "Denial of service caused by processing user input from $@ with $@.", + source.getNode(), "here", link, reason diff --git a/javascript/ql/src/Security/CWE-400/examples/DeepObjectResourceExhaustion.js b/javascript/ql/src/Security/CWE-400/examples/DeepObjectResourceExhaustion.js new file mode 100644 index 000000000000..dfeac1695583 --- /dev/null +++ b/javascript/ql/src/Security/CWE-400/examples/DeepObjectResourceExhaustion.js @@ -0,0 +1,14 @@ +import express from 'express'; +import Ajv from 'ajv'; + +let ajv = new Ajv({ allErrors: true }); +ajv.addSchema(require('./input-schema'), 'input'); + +var app = express(); +app.get('/user/:id', function(req, res) { + if (!ajv.validate('input', req.body)) { + res.end(ajv.errorsText()); + return; + } + // ... +}); diff --git a/javascript/ql/src/Security/CWE-400/examples/DeepObjectResourceExhaustion_fixed.js b/javascript/ql/src/Security/CWE-400/examples/DeepObjectResourceExhaustion_fixed.js new file mode 100644 index 000000000000..0c16bf565b23 --- /dev/null +++ b/javascript/ql/src/Security/CWE-400/examples/DeepObjectResourceExhaustion_fixed.js @@ -0,0 +1,14 @@ +import express from 'express'; +import Ajv from 'ajv'; + +let ajv = new Ajv({ allErrors: process.env['REST_DEBUG'] }); +ajv.addSchema(require('./input-schema'), 'input'); + +var app = express(); +app.get('/user/:id', function(req, res) { + if (!ajv.validate('input', req.body)) { + res.end(ajv.errorsText()); + return; + } + // ... +}); diff --git a/javascript/ql/src/javascript.qll b/javascript/ql/src/javascript.qll index 8b0996c740c0..62d1c3f0eb09 100644 --- a/javascript/ql/src/javascript.qll +++ b/javascript/ql/src/javascript.qll @@ -36,6 +36,7 @@ import semmle.javascript.InclusionTests import semmle.javascript.JSDoc import semmle.javascript.JSON import semmle.javascript.JsonParsers +import semmle.javascript.JsonSchema import semmle.javascript.JsonStringifiers import semmle.javascript.JSX import semmle.javascript.Lines diff --git a/javascript/ql/src/semmle/javascript/JsonSchema.qll b/javascript/ql/src/semmle/javascript/JsonSchema.qll new file mode 100644 index 000000000000..5c3306bf64be --- /dev/null +++ b/javascript/ql/src/semmle/javascript/JsonSchema.qll @@ -0,0 +1,129 @@ +/** + * Provides classes and predicates for working with JSON schema libraries. + */ + +import javascript + +/** + * Provides classes and predicates for working with JSON schema libraries. + */ +module JsonSchema { + /** A call that validates an input against a JSON schema. */ + abstract class ValidationCall extends DataFlow::CallNode { + /** Gets the data flow node whose value is being validated. */ + abstract DataFlow::Node getInput(); + + /** Gets the return value that indicates successful validation. */ + boolean getPolarity() { result = true } + } + + /** A data flow node that is used a JSON schema. */ + abstract class SchemaRoot extends DataFlow::Node { } + + /** An object literal with a `$schema` property indicating it is the root of a JSON schema. */ + private class SchemaNodeByTag extends SchemaRoot, DataFlow::ObjectLiteralNode { + SchemaNodeByTag() { + getAPropertyWrite("$schema").getRhs().getStringValue().matches("%//json-schema.org%") + } + } + + /** Gets a data flow node that is part of a JSON schema. */ + private DataFlow::SourceNode getAPartOfJsonSchema(DataFlow::TypeBackTracker t) { + t.start() and + result = any(SchemaRoot n).getALocalSource() + or + result = getAPartOfJsonSchema(t.continue()).getAPropertySource() + or + exists(DataFlow::TypeBackTracker t2 | result = getAPartOfJsonSchema(t2).backtrack(t2, t)) + } + + /** Gets a data flow node that is part of a JSON schema. */ + DataFlow::SourceNode getAPartOfJsonSchema() { + result = getAPartOfJsonSchema(DataFlow::TypeBackTracker::end()) + } + + /** Provides a model of the `ajv` library. */ + module Ajv { + /** A method on `Ajv` that returns `this`. */ + private string chainedMethod() { + result = + ["addSchema", "addMetaSchema", "removeSchema", "addFormat", "addKeyword", "removeKeyword"] + } + + /** An instance of `ajv`. */ + class Instance extends API::InvokeNode { + Instance() { this = API::moduleImport("ajv").getAnInstantiation() } + + /** Gets the data flow node holding the options passed to this `Ajv` instance. */ + DataFlow::Node getOptionsArg() { result = getArgument(0) } + + /** Gets an API node that refers to this object. */ + API::Node ref() { + result = getReturn() + or + result = ref().getMember(chainedMethod()).getReturn() + } + + /** + * Gets an API node for a function produced by `new Ajv().compile()` or similar. + * + * Note that this does not include the instance method `new Ajv().validate` as its + * signature is different. + */ + API::Node getAValidationFunction() { + result = ref().getMember(["compile", "getSchema"]).getReturn() + or + result = ref().getMember("compileAsync").getPromised() + } + + /** + * Gets an API node that refers to an error produced by this Ajv instance. + */ + API::Node getAValidationError() { + exists(API::Node base | base = [ref(), getAValidationFunction()] | + result = base.getMember("errors") + or + result = base.getMember("errorsText").getReturn() + ) + } + } + + /** A call to the `validate` method of `ajv`. */ + class AjvValidationCall extends ValidationCall { + Instance instance; + int argIndex; + + AjvValidationCall() { + this = instance.ref().getMember("validate").getACall() and argIndex = 1 + or + this = instance.getAValidationFunction().getACall() and argIndex = 0 + } + + override DataFlow::Node getInput() { result = getArgument(argIndex) } + + /** Gets the argument holding additional options to the call. */ + DataFlow::Node getOwnOptionsArg() { result = getArgument(argIndex + 1) } + + /** Gets a data flow passed as the extra options to this validation call or to the underlying `Ajv` instance. */ + DataFlow::Node getAnOptionsArg() { + result = getOwnOptionsArg() + or + result = instance.getOptionsArg() + } + + /** Gets the ajv instance doing the validation. */ + Instance getAjvInstance() { result = instance } + } + + private class AjvSchemaNode extends SchemaRoot { + AjvSchemaNode() { + this = + any(Instance i) + .ref() + .getMember(["addSchema", "validate", "compile", "compileAsync"]) + .getParameter(0) + .getARhs() + } + } + } +} diff --git a/javascript/ql/src/semmle/javascript/Regexp.qll b/javascript/ql/src/semmle/javascript/Regexp.qll index 485848f40c46..a4612c3b49fd 100644 --- a/javascript/ql/src/semmle/javascript/Regexp.qll +++ b/javascript/ql/src/semmle/javascript/Regexp.qll @@ -865,6 +865,17 @@ predicate isInterpretedAsRegExp(DataFlow::Node source) { // because `String.prototype.search` returns a number not exists(PropAccess p | p.getBase() = mce.getEnclosingExpr()) ) + or + exists(DataFlow::SourceNode schema | schema = JsonSchema::getAPartOfJsonSchema() | + source = schema.getAPropertyWrite("pattern").getRhs() + or + source = + schema + .getAPropertySource("patternProperties") + .getAPropertyWrite() + .getPropertyNameExpr() + .flow() + ) ) } diff --git a/javascript/ql/src/semmle/javascript/security/TaintedObject.qll b/javascript/ql/src/semmle/javascript/security/TaintedObject.qll index 7f66594254f6..444dec2ae712 100644 --- a/javascript/ql/src/semmle/javascript/security/TaintedObject.qll +++ b/javascript/ql/src/semmle/javascript/security/TaintedObject.qll @@ -16,22 +16,16 @@ import javascript private import semmle.javascript.dataflow.InferredTypes +/** Provides classes and predicates for reasoning about deeply tainted objects. */ module TaintedObject { private import DataFlow + import TaintedObjectCustomizations::TaintedObject - private class TaintedObjectLabel extends FlowLabel { - TaintedObjectLabel() { this = "tainted-object" } + // Materialize flow labels + private class ConcreteTaintedObjectLabel extends TaintedObjectLabel { + ConcreteTaintedObjectLabel() { this = this } } - /** - * 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. */ @@ -79,11 +73,6 @@ module TaintedObject { */ 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() } @@ -120,4 +109,19 @@ module TaintedObject { label = label() } } + + /** + * A sanitizer guard that validates an input against a JSON schema. + */ + private class JsonSchemaValidationGuard extends SanitizerGuard { + JsonSchema::ValidationCall call; + + JsonSchemaValidationGuard() { this = call } + + override predicate sanitizes(boolean outcome, Expr e, FlowLabel label) { + outcome = call.getPolarity() and + e = call.getInput().asExpr() and + label = label() + } + } } diff --git a/javascript/ql/src/semmle/javascript/security/TaintedObjectCustomizations.qll b/javascript/ql/src/semmle/javascript/security/TaintedObjectCustomizations.qll new file mode 100644 index 000000000000..8e0fd38f15a4 --- /dev/null +++ b/javascript/ql/src/semmle/javascript/security/TaintedObjectCustomizations.qll @@ -0,0 +1,28 @@ +/** + * Provides access to the "tainted object" flow label defined in `TaintedObject.qll`, without + * materializing that flow label. + */ + +import javascript + +/** Provides classes and predicates for reasoning about deeply tainted objects. */ +module TaintedObject { + /** A flow label representing a deeply tainted object. */ + abstract class TaintedObjectLabel extends DataFlow::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. + */ + DataFlow::FlowLabel label() { result instanceof TaintedObjectLabel } + + /** + * A source of a user-controlled deep object. + */ + abstract class Source extends DataFlow::Node { } +} diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/DeepObjectResourceExhaustion.qll b/javascript/ql/src/semmle/javascript/security/dataflow/DeepObjectResourceExhaustion.qll new file mode 100644 index 000000000000..31bccc111629 --- /dev/null +++ b/javascript/ql/src/semmle/javascript/security/dataflow/DeepObjectResourceExhaustion.qll @@ -0,0 +1,41 @@ +/** + * Provides a taint tracking configuration for reasoning about DoS attacks + * due to inefficient handling of user-controlled objects. + */ + +import javascript +import semmle.javascript.security.TaintedObject + +/** + * Provides a taint tracking configuration for reasoning about DoS attacks + * due to inefficient handling of user-controlled objects. + */ +module DeepObjectResourceExhaustion { + import DeepObjectResourceExhaustionCustomizations::DeepObjectResourceExhaustion + + /** + * A taint tracking configuration for reasoning about DoS attacks due to inefficient handling + * of user-controlled objects. + */ + class Configuration extends TaintTracking::Configuration { + Configuration() { this = "DeepObjectResourceExhaustion" } + + override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) { + source.(Source).getAFlowLabel() = label + } + + override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) { + sink instanceof Sink and label = TaintedObject::label() + } + + 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) + } + } +} diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/DeepObjectResourceExhaustionCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/DeepObjectResourceExhaustionCustomizations.qll new file mode 100644 index 000000000000..52b3874c8cc4 --- /dev/null +++ b/javascript/ql/src/semmle/javascript/security/dataflow/DeepObjectResourceExhaustionCustomizations.qll @@ -0,0 +1,94 @@ +/** + * Provides sources, sinks and sanitizers for reasoning about + * DoS attacks due to inefficient handling of user-controlled objects. + */ + +import javascript +private import semmle.javascript.security.TaintedObjectCustomizations + +/** + * Provides sources, sinks and sanitizers for reasoning about + * DoS attacks due to inefficient handling of user-controlled objects. + */ +module DeepObjectResourceExhaustion { + /** + * A data flow source for inefficient handling of user-controlled objects. + */ + abstract class Source extends DataFlow::Node { + /** Gets a flow label to associate with this source. */ + DataFlow::FlowLabel getAFlowLabel() { result = TaintedObject::label() } + } + + private class TaintedObjectSourceAsSource extends Source { + TaintedObjectSourceAsSource() { this instanceof TaintedObject::Source } + + override DataFlow::FlowLabel getAFlowLabel() { result = TaintedObject::label() } + } + + private class RemoteFlowSourceAsSource extends Source { + RemoteFlowSourceAsSource() { this instanceof RemoteFlowSource } + + override DataFlow::FlowLabel getAFlowLabel() { result.isTaint() } + } + + /** + * A data flow sink for inefficient handling of user-controlled objects. + */ + abstract class Sink extends DataFlow::Node { + /** + * Holds if `link` and `text` should be included in the message to explain + * why the handling of the object is slow. + */ + abstract predicate hasReason(DataFlow::Node link, string text); + } + + /** + * A sanitizer for inefficient handling of user-controlled objects. + */ + abstract class Sanitizer extends DataFlow::Node { } + + /** Gets a node that may refer to an object with `allErrors` set to `true`. */ + private DataFlow::SourceNode allErrorsObject( + DataFlow::TypeTracker t, DataFlow::PropWrite allErrors + ) { + t.start() and + exists(JsonSchema::Ajv::AjvValidationCall call) and // only compute if `ajv` is used + allErrors.getPropertyName() = "allErrors" and + allErrors.getRhs().mayHaveBooleanValue(true) and + result = allErrors.getBase().getALocalSource() + or + exists(ExtendCall call | + allErrorsObject(t.continue(), allErrors).flowsTo(call.getAnOperand()) and + (result = call or result = call.getDestinationOperand().getALocalSource()) + ) + or + exists(DataFlow::ObjectLiteralNode obj | + allErrorsObject(t.continue(), allErrors).flowsTo(obj.getASpreadProperty()) and + result = obj + ) + or + exists(DataFlow::TypeTracker t2 | result = allErrorsObject(t2, allErrors).track(t2, t)) + } + + /** Gets a node that may refer to an object with `allErrors` set to `true`. */ + private DataFlow::SourceNode allErrorsObject(DataFlow::PropWrite allErrors) { + result = allErrorsObject(DataFlow::TypeTracker::end(), allErrors) + } + + /** Argument to an `ajv` validation call configured with `allErrors: true`. */ + private class AjvValidationSink extends Sink { + DataFlow::PropWrite allErrors; + + AjvValidationSink() { + exists(JsonSchema::Ajv::AjvValidationCall call | + this = call.getInput() and + allErrorsObject(allErrors).flowsTo(call.getAnOptionsArg()) + ) + } + + override predicate hasReason(DataFlow::Node link, string text) { + link = allErrors and + text = "allErrors: true" + } + } +} diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ExceptionXss.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ExceptionXss.qll index a23e9fecf4cd..3340df26e72d 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/ExceptionXss.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ExceptionXss.qll @@ -10,6 +10,7 @@ module ExceptionXss { import DomBasedXssCustomizations::DomBasedXss as DomBasedXssCustom import ReflectedXssCustomizations::ReflectedXss as ReflectedXssCustom import Xss as Xss + import Xss::ExceptionXss private import semmle.javascript.dataflow.InferredTypes /** @@ -71,14 +72,9 @@ module ExceptionXss { ) } - /** - * A FlowLabel representing tainted data that has not been thrown in an exception. - * In the js/xss-through-exception query data-flow can only reach a sink after - * the data has been thrown as an exception, and data that has not been thrown - * as an exception therefore has this flow label, and only this flow label, associated with it. - */ - class NotYetThrown extends DataFlow::FlowLabel { - NotYetThrown() { this = "NotYetThrown" } + // Materialize flow labels + private class ConcreteNotYetThrown extends Xss::ExceptionXss::NotYetThrown { + ConcreteNotYetThrown() { this = this } } /** @@ -139,7 +135,7 @@ module ExceptionXss { Configuration() { this = "ExceptionXss" } override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) { - source instanceof Xss::Shared::Source and label instanceof NotYetThrown + source.(Xss::ExceptionXss::Source).getAFlowLabel() = label } override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) { diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll b/javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll index ea911f953002..85b31b909954 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll @@ -602,3 +602,59 @@ module XssThroughDom { /** A data flow source for XSS through DOM vulnerabilities. */ abstract class Source extends Shared::Source { } } + +/** Provides classes for customizing the `ExceptionXss` query. */ +module ExceptionXss { + /** A data flow source for XSS caused by interpreting exception or error text as HTML. */ + abstract class Source extends DataFlow::Node { + /** + * Gets a flow label to associate with this source. + * + * For sources that should pass through a `throw/catch` before reaching the sink, use the + * `NotYetThrown` labe. Otherwise use `taint` (the default). + */ + DataFlow::FlowLabel getAFlowLabel() { result.isTaint() } + + /** + * Gets a human-readable description of what type of error this refers to. + * + * The result should be capitalized and usable in the context of a noun. + */ + string getDescription() { result = "Error text" } + } + + /** + * A FlowLabel representing tainted data that has not been thrown in an exception. + * In the js/xss-through-exception query data-flow can only reach a sink after + * the data has been thrown as an exception, and data that has not been thrown + * as an exception therefore has this flow label, and only this flow label, associated with it. + */ + abstract class NotYetThrown extends DataFlow::FlowLabel { + NotYetThrown() { this = "NotYetThrown" } + } + + private class XssSourceAsSource extends Source { + XssSourceAsSource() { this instanceof Shared::Source } + + override DataFlow::FlowLabel getAFlowLabel() { result instanceof NotYetThrown } + + override string getDescription() { result = "Exception text" } + } + + /** + * An error produced by validating using `ajv`. + * + * Such an error can contain property names from the input if the + * underlying schema uses `additionalProperties` or `propertyPatterns`. + * + * For example, an input of form `{"": 45}` might produce the error + * `data/ should be string`. + */ + private class JsonSchemaValidationError extends Source { + JsonSchemaValidationError() { + this = any(JsonSchema::Ajv::Instance i).getAValidationError().getAnImmediateUse() + } + + override string getDescription() { result = "JSON schema validation error" } + } +} diff --git a/javascript/ql/test/query-tests/Performance/ReDoS/PolynomialBackTracking.expected b/javascript/ql/test/query-tests/Performance/ReDoS/PolynomialBackTracking.expected index 8452d81ee07f..cb7c526af5d2 100644 --- a/javascript/ql/test/query-tests/Performance/ReDoS/PolynomialBackTracking.expected +++ b/javascript/ql/test/query-tests/Performance/ReDoS/PolynomialBackTracking.expected @@ -24,6 +24,9 @@ | highlight.js:38:54:38:59 | [^()]* | Strings starting with 'A((' and with many repetitions of ''' can start matching anywhere after the start of the preceeding [^()]* | | highlight.js:38:64:38:69 | [^()]* | Strings starting with 'A(' and with many repetitions of ''' can start matching anywhere after the start of the preceeding [^()]* | | highlight.js:39:22:39:24 | \\w* | Strings starting with 'A' and with many repetitions of 'A' can start matching anywhere after the start of the preceeding [a-zA-Z_]\\w*\\([^()]*(\\([^()]*(\\([^()]*\\)[^()]*)*\\)[^()]*)*\\)\\s*\\{ | +| jsonschema.js:5:15:5:21 | (a?a?)* | Strings with many repetitions of 'a' can start matching anywhere after the start of the preceeding (a?a?)*b | +| jsonschema.js:15:23:15:29 | (a?a?)* | Strings with many repetitions of 'a' can start matching anywhere after the start of the preceeding (a?a?)*b | +| jsonschema.js:20:18:20:24 | (a?a?)* | Strings with many repetitions of 'a' can start matching anywhere after the start of the preceeding (a?a?)*b | | lib/closure.js:4:6:4:7 | u* | Strings with many repetitions of 'u' can start matching anywhere after the start of the preceeding u*o | | lib/lib.js:1:15:1:16 | a* | Strings with many repetitions of 'a' can start matching anywhere after the start of the preceeding a*b | | lib/lib.js:8:3:8:4 | f* | Strings with many repetitions of 'f' can start matching anywhere after the start of the preceeding f*g | diff --git a/javascript/ql/test/query-tests/Performance/ReDoS/ReDoS.expected b/javascript/ql/test/query-tests/Performance/ReDoS/ReDoS.expected index a3aa59d63b10..4229d8f0c5fa 100644 --- a/javascript/ql/test/query-tests/Performance/ReDoS/ReDoS.expected +++ b/javascript/ql/test/query-tests/Performance/ReDoS/ReDoS.expected @@ -12,6 +12,9 @@ | highlight.js:30:13:30:25 | (?:\\\\.\|[^`])+ | This part of the regular expression may cause exponential backtracking on strings starting with '`' and containing many repetitions of '\\\\_'. | | highlight.js:34:25:34:27 | \\w* | This part of the regular expression may cause exponential backtracking on strings starting with '?A' and containing many repetitions of 'A'. | | highlight.js:38:35:38:40 | [^()]* | This part of the regular expression may cause exponential backtracking on strings starting with 'A((' and containing many repetitions of '')('. | +| jsonschema.js:5:15:5:21 | (a?a?)* | This part of the regular expression may cause exponential backtracking on strings containing many repetitions of 'a'. | +| jsonschema.js:15:23:15:29 | (a?a?)* | This part of the regular expression may cause exponential backtracking on strings containing many repetitions of 'a'. | +| jsonschema.js:20:18:20:24 | (a?a?)* | This part of the regular expression may cause exponential backtracking on strings containing many repetitions of 'a'. | | polynomial-redos.js:17:5:17:6 | .* | This part of the regular expression may cause exponential backtracking on strings containing many repetitions of ','. | | polynomial-redos.js:41:52:41:63 | [\\x21-\\x7E]* | This part of the regular expression may cause exponential backtracking on strings containing many repetitions of '?'. | | polynomial-redos.js:46:33:46:45 | [a-zA-Z_0-9]* | This part of the regular expression may cause exponential backtracking on strings starting with 'A' and containing many repetitions of 'A'. | diff --git a/javascript/ql/test/query-tests/Performance/ReDoS/jsonschema.js b/javascript/ql/test/query-tests/Performance/ReDoS/jsonschema.js new file mode 100644 index 000000000000..542145de4848 --- /dev/null +++ b/javascript/ql/test/query-tests/Performance/ReDoS/jsonschema.js @@ -0,0 +1,26 @@ +import Ajv from 'ajv'; + +let thing = { + type: 'string', + pattern: '(a?a?)*b' // NOT OK +} +new Ajv().addSchema(thing, 'thing'); + +export default { + $schema: "http://json-schema.org/draft-07/schema#", + type: "object", + properties: { + foo: { + type: "string", + pattern: "(a?a?)*b" // NOT OK + }, + bar: { + type: "object", + patternProperties: { + "(a?a?)*b": { // NOT OK + type: "number" + } + } + } + } +}; diff --git a/javascript/ql/test/query-tests/Security/CWE-079/ExceptionXss/ExceptionXss.expected b/javascript/ql/test/query-tests/Security/CWE-079/ExceptionXss/ExceptionXss.expected index aba89bbe379a..f73127267863 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/ExceptionXss/ExceptionXss.expected +++ b/javascript/ql/test/query-tests/Security/CWE-079/ExceptionXss/ExceptionXss.expected @@ -1,4 +1,7 @@ nodes +| ajv.js:11:18:11:33 | ajv.errorsText() | +| ajv.js:11:18:11:33 | ajv.errorsText() | +| ajv.js:11:18:11:33 | ajv.errorsText() | | exception-xss.js:2:6:2:28 | foo | | exception-xss.js:2:12:2:28 | document.location | | exception-xss.js:2:12:2:28 | document.location | @@ -87,6 +90,7 @@ nodes | exception-xss.js:182:19:182:23 | error | | exception-xss.js:182:19:182:23 | error | edges +| ajv.js:11:18:11:33 | ajv.errorsText() | ajv.js:11:18:11:33 | ajv.errorsText() | | exception-xss.js:2:6:2:28 | foo | exception-xss.js:9:11:9:13 | foo | | exception-xss.js:2:6:2:28 | foo | exception-xss.js:15:9:15:11 | foo | | exception-xss.js:2:6:2:28 | foo | exception-xss.js:21:11:21:13 | foo | @@ -169,6 +173,7 @@ edges | exception-xss.js:180:26:180:30 | error | exception-xss.js:182:19:182:23 | error | | exception-xss.js:180:26:180:30 | error | exception-xss.js:182:19:182:23 | error | #select +| ajv.js:11:18:11:33 | ajv.errorsText() | ajv.js:11:18:11:33 | ajv.errorsText() | ajv.js:11:18:11:33 | ajv.errorsText() | $@ is reinterpreted as HTML without escaping meta-characters. | ajv.js:11:18:11:33 | ajv.errorsText() | JSON schema validation error | | exception-xss.js:11:18:11:18 | e | exception-xss.js:2:12:2:28 | document.location | exception-xss.js:11:18:11:18 | e | $@ is reinterpreted as HTML without escaping meta-characters. | exception-xss.js:2:12:2:28 | document.location | Exception text | | exception-xss.js:17:18:17:18 | e | exception-xss.js:2:12:2:28 | document.location | exception-xss.js:17:18:17:18 | e | $@ is reinterpreted as HTML without escaping meta-characters. | exception-xss.js:2:12:2:28 | document.location | Exception text | | exception-xss.js:23:18:23:18 | e | exception-xss.js:2:12:2:28 | document.location | exception-xss.js:23:18:23:18 | e | $@ is reinterpreted as HTML without escaping meta-characters. | exception-xss.js:2:12:2:28 | document.location | Exception text | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/ExceptionXss/ajv.js b/javascript/ql/test/query-tests/Security/CWE-079/ExceptionXss/ajv.js new file mode 100644 index 000000000000..2ebbc46a4b3a --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-079/ExceptionXss/ajv.js @@ -0,0 +1,13 @@ +import express from 'express'; +import Ajv from 'ajv'; + +let app = express(); +let ajv = new Ajv(); + +ajv.addSchema({type: 'object', additionalProperties: {type: 'number'}}, 'pollData'); + +app.post('/polldata', (req, res) => { + if (!ajv.validate('pollData', req.body)) { + res.send(ajv.errorsText()); // NOT OK + } +}); diff --git a/javascript/ql/test/query-tests/Security/CWE-089/untyped/DatabaseAccesses.expected b/javascript/ql/test/query-tests/Security/CWE-089/untyped/DatabaseAccesses.expected index d17ff8d1cc4d..5bf367a4c18f 100644 --- a/javascript/ql/test/query-tests/Security/CWE-089/untyped/DatabaseAccesses.expected +++ b/javascript/ql/test/query-tests/Security/CWE-089/untyped/DatabaseAccesses.expected @@ -1,3 +1,7 @@ +| json-schema-validator.js:27:13:27:27 | doc.find(query) | +| json-schema-validator.js:30:13:30:27 | doc.find(query) | +| json-schema-validator.js:33:13:33:27 | doc.find(query) | +| json-schema-validator.js:35:9:35:23 | doc.find(query) | | marsdb-flow-to.js:14:3:14:22 | db.myDoc.find(query) | | marsdb.js:16:3:16:17 | doc.find(query) | | minimongo.js:18:3:18:17 | doc.find(query) | diff --git a/javascript/ql/test/query-tests/Security/CWE-089/untyped/SqlInjection.expected b/javascript/ql/test/query-tests/Security/CWE-089/untyped/SqlInjection.expected index e33f30d01ba6..427e2fb853c9 100644 --- a/javascript/ql/test/query-tests/Security/CWE-089/untyped/SqlInjection.expected +++ b/javascript/ql/test/query-tests/Security/CWE-089/untyped/SqlInjection.expected @@ -1,4 +1,12 @@ nodes +| json-schema-validator.js:25:15:25:48 | query | +| json-schema-validator.js:25:23:25:48 | JSON.pa ... y.data) | +| json-schema-validator.js:25:34:25:47 | req.query.data | +| json-schema-validator.js:25:34:25:47 | req.query.data | +| json-schema-validator.js:33:22:33:26 | query | +| json-schema-validator.js:33:22:33:26 | query | +| json-schema-validator.js:35:18:35:22 | query | +| json-schema-validator.js:35:18:35:22 | query | | marsdb-flow-to.js:10:9:10:18 | query | | marsdb-flow-to.js:10:17:10:18 | {} | | marsdb-flow-to.js:11:17:11:24 | req.body | @@ -250,6 +258,13 @@ nodes | tst.js:10:46:10:58 | req.params.id | | tst.js:10:46:10:58 | req.params.id | edges +| json-schema-validator.js:25:15:25:48 | query | json-schema-validator.js:33:22:33:26 | query | +| json-schema-validator.js:25:15:25:48 | query | json-schema-validator.js:33:22:33:26 | query | +| json-schema-validator.js:25:15:25:48 | query | json-schema-validator.js:35:18:35:22 | query | +| json-schema-validator.js:25:15:25:48 | query | json-schema-validator.js:35:18:35:22 | query | +| json-schema-validator.js:25:23:25:48 | JSON.pa ... y.data) | json-schema-validator.js:25:15:25:48 | query | +| json-schema-validator.js:25:34:25:47 | req.query.data | json-schema-validator.js:25:23:25:48 | JSON.pa ... y.data) | +| json-schema-validator.js:25:34:25:47 | req.query.data | json-schema-validator.js:25:23:25:48 | JSON.pa ... y.data) | | marsdb-flow-to.js:10:9:10:18 | query | marsdb-flow-to.js:14:17:14:21 | query | | marsdb-flow-to.js:10:9:10:18 | query | marsdb-flow-to.js:14:17:14:21 | query | | marsdb-flow-to.js:10:17:10:18 | {} | marsdb-flow-to.js:10:9:10:18 | query | @@ -586,6 +601,8 @@ edges | tst.js:10:46:10:58 | req.params.id | tst.js:10:10:10:64 | 'SELECT ... d + '"' | | tst.js:10:46:10:58 | req.params.id | tst.js:10:10:10:64 | 'SELECT ... d + '"' | #select +| json-schema-validator.js:33:22:33:26 | query | json-schema-validator.js:25:34:25:47 | req.query.data | json-schema-validator.js:33:22:33:26 | query | This query depends on $@. | json-schema-validator.js:25:34:25:47 | req.query.data | a user-provided value | +| json-schema-validator.js:35:18:35:22 | query | json-schema-validator.js:25:34:25:47 | req.query.data | json-schema-validator.js:35:18:35:22 | query | This query depends on $@. | json-schema-validator.js:25:34:25:47 | req.query.data | a user-provided value | | marsdb-flow-to.js:14:17:14:21 | query | marsdb-flow-to.js:11:17:11:24 | req.body | marsdb-flow-to.js:14:17:14:21 | query | This query depends on $@. | marsdb-flow-to.js:11:17:11:24 | req.body | a user-provided value | | marsdb.js:16:12:16:16 | query | marsdb.js:13:17:13:24 | req.body | marsdb.js:16:12:16:16 | query | This query depends on $@. | marsdb.js:13:17:13:24 | req.body | a user-provided value | | minimongo.js:18:12:18:16 | query | minimongo.js:15:17:15:24 | req.body | minimongo.js:18:12:18:16 | query | This query depends on $@. | minimongo.js:15:17:15:24 | req.body | a user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-089/untyped/json-schema-validator.js b/javascript/ql/test/query-tests/Security/CWE-089/untyped/json-schema-validator.js new file mode 100644 index 000000000000..93f09a51adbd --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-089/untyped/json-schema-validator.js @@ -0,0 +1,37 @@ +import Ajv from 'ajv'; +import express from 'express'; +import { MongoClient } from 'mongodb'; + +const app = express(); + +const schema = { + type: 'object', + properties: { + date: { type: 'string' }, + title: { type: 'string' }, + }, +}; +const ajv = new Ajv(); +const checkSchema = ajv.compile(schema); + +function validate(x) { + return x != null; +} + +app.post('/documents/find', (req, res) => { + MongoClient.connect('mongodb://localhost:27017/test', (err, db) => { + let doc = db.collection('doc'); + + const query = JSON.parse(req.query.data); + if (checkSchema(query)) { + doc.find(query); // OK + } + if (ajv.validate(schema, query)) { + doc.find(query); // OK + } + if (validate(query)) { + doc.find(query); // NOT OK - validate() doesn't sanitize + } + doc.find(query); // NOT OK + }); +}); diff --git a/javascript/ql/test/query-tests/Security/CWE-400/DeepObjectResourceExhaustion/DeepObjectResourceExhaustion.expected b/javascript/ql/test/query-tests/Security/CWE-400/DeepObjectResourceExhaustion/DeepObjectResourceExhaustion.expected new file mode 100644 index 000000000000..c93bc1c71114 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-400/DeepObjectResourceExhaustion/DeepObjectResourceExhaustion.expected @@ -0,0 +1,8 @@ +nodes +| tst.js:9:29:9:36 | req.body | +| tst.js:9:29:9:36 | req.body | +| tst.js:9:29:9:36 | req.body | +edges +| tst.js:9:29:9:36 | req.body | tst.js:9:29:9:36 | req.body | +#select +| tst.js:9:29:9:36 | req.body | tst.js:9:29:9:36 | req.body | tst.js:9:29:9:36 | req.body | Denial of service caused by processing user input from $@ with $@. | tst.js:9:29:9:36 | req.body | here | tst.js:4:21:4:35 | allErrors: true | allErrors: true | diff --git a/javascript/ql/test/query-tests/Security/CWE-400/DeepObjectResourceExhaustion/DeepObjectResourceExhaustion.qlref b/javascript/ql/test/query-tests/Security/CWE-400/DeepObjectResourceExhaustion/DeepObjectResourceExhaustion.qlref new file mode 100644 index 000000000000..659d91ba4b89 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-400/DeepObjectResourceExhaustion/DeepObjectResourceExhaustion.qlref @@ -0,0 +1 @@ +Security/CWE-400/DeepObjectResourceExhaustion.ql \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Security/CWE-400/DeepObjectResourceExhaustion/tst.js b/javascript/ql/test/query-tests/Security/CWE-400/DeepObjectResourceExhaustion/tst.js new file mode 100644 index 000000000000..9ecdad2dbbc9 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-400/DeepObjectResourceExhaustion/tst.js @@ -0,0 +1,12 @@ +import express from 'express'; +import Ajv from 'ajv'; + +let ajv = new Ajv({ allErrors: true }); +ajv.addSchema(require('./input-schema'), 'input'); + +var app = express(); +app.get('/user/:id', function(req, res) { + if (!ajv.validate('input', req.body)) { // NOT OK + return; + } +}); diff --git a/javascript/ql/test/query-tests/Security/CWE-400/RemotePropertyInjection.expected b/javascript/ql/test/query-tests/Security/CWE-400/RemovePropertyInjection/RemotePropertyInjection.expected similarity index 100% rename from javascript/ql/test/query-tests/Security/CWE-400/RemotePropertyInjection.expected rename to javascript/ql/test/query-tests/Security/CWE-400/RemovePropertyInjection/RemotePropertyInjection.expected diff --git a/javascript/ql/test/query-tests/Security/CWE-400/RemotePropertyInjection.qlref b/javascript/ql/test/query-tests/Security/CWE-400/RemovePropertyInjection/RemotePropertyInjection.qlref similarity index 100% rename from javascript/ql/test/query-tests/Security/CWE-400/RemotePropertyInjection.qlref rename to javascript/ql/test/query-tests/Security/CWE-400/RemovePropertyInjection/RemotePropertyInjection.qlref diff --git a/javascript/ql/test/query-tests/Security/CWE-400/tst.js b/javascript/ql/test/query-tests/Security/CWE-400/RemovePropertyInjection/tst.js similarity index 100% rename from javascript/ql/test/query-tests/Security/CWE-400/tst.js rename to javascript/ql/test/query-tests/Security/CWE-400/RemovePropertyInjection/tst.js diff --git a/javascript/ql/test/query-tests/Security/CWE-400/tstNonExpr.js b/javascript/ql/test/query-tests/Security/CWE-400/RemovePropertyInjection/tstNonExpr.js similarity index 100% rename from javascript/ql/test/query-tests/Security/CWE-400/tstNonExpr.js rename to javascript/ql/test/query-tests/Security/CWE-400/RemovePropertyInjection/tstNonExpr.js