diff --git a/change-notes/1.26/analysis-javascript.md b/change-notes/1.26/analysis-javascript.md index af26bc476b0..c3ea90995f9 100644 --- a/change-notes/1.26/analysis-javascript.md +++ b/change-notes/1.26/analysis-javascript.md @@ -58,3 +58,5 @@ ## Changes to libraries * The predicate `TypeAnnotation.hasQualifiedName` now works in more cases when the imported library was not present during extraction. +* The class `DomBasedXss::Configuration` has been deprecated, as it has been split into `DomBasedXss::HtmlInjectionConfiguration` and `DomBasedXss::JQueryHtmlOrSelectorInjectionConfiguration`. Unless specifically working with jQuery sinks, uses of that +class should be replaced with `HtmlInjectionConfiguration`. diff --git a/javascript/ql/src/Security/CWE-079/Xss.ql b/javascript/ql/src/Security/CWE-079/Xss.ql index 4764e3abbbf..3925febb008 100644 --- a/javascript/ql/src/Security/CWE-079/Xss.ql +++ b/javascript/ql/src/Security/CWE-079/Xss.ql @@ -19,7 +19,7 @@ from DataFlow::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode where ( cfg instanceof HtmlInjectionConfiguration or - cfg instanceof JQuerySelectorInjectionConfiguration + cfg instanceof JQueryHtmlOrSelectorInjectionConfiguration ) and cfg.hasFlowPath(source, sink) select sink.getNode(), source, sink, diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/DomBasedXss.qll b/javascript/ql/src/semmle/javascript/security/dataflow/DomBasedXss.qll index 8a4a9429b4b..621e7acbc83 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/DomBasedXss.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/DomBasedXss.qll @@ -9,7 +9,7 @@ module DomBasedXss { import DomBasedXssCustomizations::DomBasedXss /** - * DEPRECATED. Use `HtmlInjectionConfiguration` or `JQuerySelectorInjectionConfiguration`. + * DEPRECATED. Use `HtmlInjectionConfiguration` or `JQueryHtmlOrSelectorInjectionConfiguration`. */ deprecated class Configuration = HtmlInjectionConfiguration; @@ -23,7 +23,7 @@ module DomBasedXss { override predicate isSink(DataFlow::Node sink) { sink instanceof Sink and - not sink instanceof JQuerySelectorSink // Handled by JQuerySelectorInjectionConfiguration below + not sink instanceof JQueryHtmlOrSelectorSink // Handled by JQueryHtmlOrSelectorInjectionConfiguration below } override predicate isSanitizer(DataFlow::Node node) { @@ -47,8 +47,8 @@ module DomBasedXss { * * Values are only considered tainted if they can start with the `<` character. */ - class JQuerySelectorInjectionConfiguration extends TaintTracking::Configuration { - JQuerySelectorInjectionConfiguration() { this = "JQuerySelectorInjection" } + class JQueryHtmlOrSelectorInjectionConfiguration extends TaintTracking::Configuration { + JQueryHtmlOrSelectorInjectionConfiguration() { this = "JQueryHtmlOrSelectorInjection" } override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) { // Reuse any source not derived from location @@ -64,7 +64,7 @@ module DomBasedXss { } override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) { - sink instanceof JQuerySelectorSink and label.isTaint() + sink instanceof JQueryHtmlOrSelectorSink and label.isTaint() } override predicate isAdditionalFlowStep( diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeJQueryPluginCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeJQueryPluginCustomizations.qll index 2e2fe77d21b..2ddc0fca917 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeJQueryPluginCustomizations.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeJQueryPluginCustomizations.qll @@ -34,18 +34,11 @@ module UnsafeJQueryPlugin { /** * An argument that may act as a HTML fragment rather than a CSS selector, as a sink for remote unsafe jQuery plugins. */ - class AmbiguousHtmlOrSelectorArgument extends DataFlow::Node { + class AmbiguousHtmlOrSelectorArgument extends DataFlow::Node, + DomBasedXss::JQueryHtmlOrSelectorArgument { AmbiguousHtmlOrSelectorArgument() { - exists(JQuery::MethodCall call | - call.interpretsArgumentAsSelector(this) and call.interpretsArgumentAsHtml(this) - ) and - // the $-function in particular will not construct HTML for non-string values - analyze().getAType() = TTString() and // any fixed prefix makes the call unambiguous - not exists(DataFlow::Node prefix | - DomBasedXss::isPrefixOfJQueryHtmlString(this, prefix) and - prefix.mayHaveStringValue(_) - ) + not exists(getAPrefix()) } } diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll b/javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll index ee04c06e329..1538343f973 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll @@ -3,6 +3,7 @@ */ import javascript +private import semmle.javascript.dataflow.InferredTypes /** Provides classes and predicates shared between the XSS queries. */ module Shared { @@ -201,20 +202,33 @@ module DomBasedXss { } /** - * An argument to the jQuery `$` function, which is interpreted as either a selector + * An argument to the jQuery `$` function or similar, which is interpreted as either a selector * or as an HTML string depending on its first character. */ - class JQuerySelectorSink extends Sink { - JQuerySelectorSink() { + class JQueryHtmlOrSelectorArgument extends DataFlow::Node { + JQueryHtmlOrSelectorArgument() { exists(JQuery::MethodCall call | call.interpretsArgumentAsHtml(this) and call.interpretsArgumentAsSelector(this) and - // If a prefix of the string is known, it must start with '<' or be an empty string - forall(string strval | strval = getAPrefixOfJQuerySelectorString(this).getStringValue() | - strval.regexpMatch("(?s)\\s*<.*|") - ) + analyze().getAType() = TTString() ) } + + /** Gets a string that flows to the prefix of this argument. */ + string getAPrefix() { result = getAPrefixOfJQuerySelectorString(this).getStringValue() } + } + + /** + * An argument to the jQuery `$` function or similar, which may be interpreted as HTML. + * + * This is the same as `JQueryHtmlOrSelectorArgument`, excluding cases where the value + * is prefixed by something other than `<`. + */ + class JQueryHtmlOrSelectorSink extends Sink, JQueryHtmlOrSelectorArgument { + JQueryHtmlOrSelectorSink() { + // If a prefix of the string is known, it must start with '<' or be an empty string + forall(string strval | strval = getAPrefix() | strval.regexpMatch("(?s)\\s*<.*|")) + } } /**