From d916118ea42663cb46e0ccb1afd8644bfb58136e Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Tue, 2 Mar 2021 13:16:10 +0000 Subject: [PATCH] JS: Move ExceptionXss source into Xss.qll --- .../ql/src/Security/CWE-079/ExceptionXss.ql | 12 +--- .../security/dataflow/ExceptionXss.qll | 46 ++------------- .../javascript/security/dataflow/Xss.qll | 56 +++++++++++++++++++ 3 files changed, 62 insertions(+), 52 deletions(-) diff --git a/javascript/ql/src/Security/CWE-079/ExceptionXss.ql b/javascript/ql/src/Security/CWE-079/ExceptionXss.ql index 406c9145bc7..7c9cedc1705 100644 --- a/javascript/ql/src/Security/CWE-079/ExceptionXss.ql +++ b/javascript/ql/src/Security/CWE-079/ExceptionXss.ql @@ -15,18 +15,8 @@ import javascript import semmle.javascript.security.dataflow.ExceptionXss::ExceptionXss import DataFlow::PathGraph -/** - * Gets a description of the source. - */ -string getSourceDescription(DataFlow::Node source) { - result = source.(ErrorSource).getDescription() - or - not source instanceof ErrorSource and - result = "Exception text" -} - 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(), - getSourceDescription(source.getNode()) + source.getNode().(Source).getDescription() diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ExceptionXss.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ExceptionXss.qll index 020231947e4..3340df26e72 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 } } /** @@ -130,35 +126,6 @@ module ExceptionXss { result = getCallbackErrorParam(pred) } - /** - * A source of error values that is likely to contain unencoded user input. - */ - abstract class ErrorSource extends DataFlow::Node { - /** - * Gets a human-readable description of what type of error this refers to. - * - * The result should be captialized and usable in the context of a noun. - */ - abstract string getDescription(); - } - - /** - * 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 ErrorSource { - JsonSchemaValidationError() { - this = any(JsonSchema::Ajv::Instance i).getAValidationError().getAnImmediateUse() - } - - override string getDescription() { result = "JSON schema validation error" } - } - /** * A taint-tracking configuration for reasoning about XSS with possible exceptional flow. * Flow labels are used to ensure that we only report taint-flow that has been thrown in @@ -168,10 +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 - or - source instanceof ErrorSource and - label.isTaint() + 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 ea911f95300..85b31b90995 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" } + } +}