diff --git a/javascript/change-notes/2021-04-26-unsafe-html-construction.md b/javascript/change-notes/2021-04-26-unsafe-html-construction.md new file mode 100644 index 000000000000..8b0f513d7605 --- /dev/null +++ b/javascript/change-notes/2021-04-26-unsafe-html-construction.md @@ -0,0 +1,3 @@ +lgtm,codescanning +* A new query, `js/html-constructed-from-input`, has been added to the query suite, + highlighting libraries that may leave clients vulnerable to cross-site-scripting attacks. diff --git a/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.qhelp b/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.qhelp new file mode 100644 index 000000000000..0bc3bd53a0d0 --- /dev/null +++ b/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.qhelp @@ -0,0 +1,77 @@ + + + +

+ When a library function dynamically constructs HTML in a potentially unsafe + way, then it's important to document to clients of the library that the function + should only be used with trusted inputs. + + If the function is not documented as being potentially unsafe, then a client + may inadvertently use inputs containing unsafe HTML fragments, and thereby leave + the client vulnerable to cross-site scripting attacks. +

+ + + +

+ Document all library functions that can lead to cross-site scripting + attacks, and guard against unsafe inputs where dynamic HTML + construction is not intended. +

+ + + +

+ The following example has a library function that renders a boldface name + by writing to the innerHTML property of an element. +

+ + + +

+ This library function, however, does not escape unsafe HTML, and a client + that calls the function with user-supplied input may be vulnerable to + cross-site scripting attacks. +

+ +

+ The library could either document that this function should not be used + with unsafe inputs, or use safe APIs such as innerText. +

+ + + +

+ Alternatively, a HTML sanitizer can be used to remove unsafe content. +

+ + + + + +
  • + OWASP: + DOM based + XSS Prevention Cheat Sheet. +
    • +
  • + OWASP: + XSS + (Cross Site Scripting) Prevention Cheat Sheet. +
    • +
  • + OWASP + DOM Based XSS. +
    • +
  • + OWASP + Types of Cross-Site + Scripting. +
    • +
  • + Wikipedia: Cross-site scripting. +
    • +     +     diff --git a/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.ql b/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.ql new file mode 100644 index 000000000000..d6199af45984 --- /dev/null +++ b/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.ql @@ -0,0 +1,22 @@ +/** + * @name Unsafe HTML constructed from library input + * @description Using externally controlled strings to construct HTML might allow a malicious + * user to perform a cross-site scripting attack. + * @kind path-problem + * @problem.severity error + * @precision high + * @id js/html-constructed-from-input + * @tags security + * external/cwe/cwe-079 + * external/cwe/cwe-116 + */ + +import javascript +import DataFlow::PathGraph +import semmle.javascript.security.dataflow.UnsafeHtmlConstruction::UnsafeHtmlConstruction + +from DataFlow::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, Sink sinkNode +where cfg.hasFlowPath(source, sink) and sink.getNode() = sinkNode +select sinkNode, source, sink, "$@ based on $@ might later cause $@.", sinkNode, + sinkNode.describe(), source.getNode(), "library input", sinkNode.getSink(), + sinkNode.getVulnerabilityKind().toLowerCase() diff --git a/javascript/ql/src/Security/CWE-079/examples/unsafe-html-construction.js b/javascript/ql/src/Security/CWE-079/examples/unsafe-html-construction.js new file mode 100644 index 000000000000..ab4bfee0a25b --- /dev/null +++ b/javascript/ql/src/Security/CWE-079/examples/unsafe-html-construction.js @@ -0,0 +1,3 @@ +module.exports = function showBoldName(name) { + document.getElementById('name').innerHTML = "" + name + ""; +} diff --git a/javascript/ql/src/Security/CWE-079/examples/unsafe-html-construction_safe.js b/javascript/ql/src/Security/CWE-079/examples/unsafe-html-construction_safe.js new file mode 100644 index 000000000000..b2b3af9bcfbb --- /dev/null +++ b/javascript/ql/src/Security/CWE-079/examples/unsafe-html-construction_safe.js @@ -0,0 +1,5 @@ +module.exports = function showBoldName(name) { + const bold = document.createElement('b'); + bold.innerText = name; + document.getElementById('name').appendChild(bold); +} diff --git a/javascript/ql/src/Security/CWE-079/examples/unsafe-html-construction_sanitizer.js b/javascript/ql/src/Security/CWE-079/examples/unsafe-html-construction_sanitizer.js new file mode 100644 index 000000000000..9c63e278720c --- /dev/null +++ b/javascript/ql/src/Security/CWE-079/examples/unsafe-html-construction_sanitizer.js @@ -0,0 +1,5 @@ + +const striptags = require('striptags'); +module.exports = function showBoldName(name) { + document.getElementById('name').innerHTML = "" + striptags(name) + ""; +} diff --git a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll index b4842026e24c..45e7515df916 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll @@ -2068,3 +2068,14 @@ class VarAccessBarrier extends DataFlow::Node { ) } } + +/** + * Holds if there is a path without unmatched return steps from `source` to `sink`. + */ +predicate hasPathWithoutUnmatchedReturn(SourcePathNode source, SinkPathNode sink) { + exists(MidPathNode mid | + source.getASuccessor*() = mid and + sink = mid.getASuccessor() and + mid.getPathSummary().hasReturn() = false + ) +} diff --git a/javascript/ql/src/semmle/javascript/frameworks/Markdown.qll b/javascript/ql/src/semmle/javascript/frameworks/Markdown.qll index 743341325a7a..b2235577b04b 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/Markdown.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/Markdown.qll @@ -2,159 +2,188 @@ * Provides classes for modelling common markdown parsers and generators. */ +import semmle.javascript.Unit import javascript /** - * A taint step for the `marked` library, that converts markdown to HTML. + * A module providing taint-steps for common markdown parsers and generators. */ -private class MarkedStep extends TaintTracking::SharedTaintStep { - override predicate step(DataFlow::Node pred, DataFlow::Node succ) { - exists(DataFlow::CallNode call | - call = DataFlow::globalVarRef("marked").getACall() - or - call = DataFlow::moduleImport("marked").getACall() - | - succ = call and - pred = call.getArgument(0) - ) - } -} +module Markdown { + /** + * A taint-step that parses a markdown document, but preserves taint. + */ + class MarkdownStep extends Unit { + /** + * Holds if there is a taint-step from `pred` to `succ` for a taint-preserving markdown parser. + */ + abstract predicate step(DataFlow::Node pred, DataFlow::Node succ); -/** - * A taint step for the `markdown-table` library. - */ -private class MarkdownTableStep extends TaintTracking::SharedTaintStep { - override predicate step(DataFlow::Node pred, DataFlow::Node succ) { - exists(DataFlow::CallNode call | call = DataFlow::moduleImport("markdown-table").getACall() | - succ = call and - pred = call.getArgument(0) - ) + /** + * Holds if the taint-step preserves HTML. + */ + predicate preservesHTML() { any() } } -} -/** - * A taint step for the `showdown` library. - */ -private class ShowDownStep extends TaintTracking::SharedTaintStep { - override predicate step(DataFlow::Node pred, DataFlow::Node succ) { - exists(DataFlow::CallNode call | - call = - [DataFlow::globalVarRef("showdown"), DataFlow::moduleImport("showdown")] - .getAConstructorInvocation("Converter") - .getAMemberCall(["makeHtml", "makeMd"]) - | - succ = call and - pred = call.getArgument(0) - ) + private class MarkdownStepAsTaintStep extends TaintTracking::SharedTaintStep { + override predicate step(DataFlow::Node pred, DataFlow::Node succ) { + any(MarkdownStep step).step(pred, succ) + } } -} -/** - * Classes and predicates for modelling taint steps in `unified` and `remark`. - */ -private module Unified { /** - * The creation of a parser from `unified`. - * The `remark` module is a shorthand that initializes `unified` with a markdown parser. + * A taint step for the `marked` library, that converts markdown to HTML. */ - DataFlow::CallNode unified() { result = DataFlow::moduleImport(["unified", "remark"]).getACall() } + private class MarkedStep extends MarkdownStep { + override predicate step(DataFlow::Node pred, DataFlow::Node succ) { + exists(DataFlow::CallNode call | + call = DataFlow::globalVarRef("marked").getACall() + or + call = DataFlow::moduleImport("marked").getACall() + | + succ = call and + pred = call.getArgument(0) + ) + } + } /** - * A chain of method calls that process an input with `unified`. + * A taint step for the `markdown-table` library. */ - class UnifiedChain extends DataFlow::CallNode { - DataFlow::CallNode root; + private class MarkdownTableStep extends MarkdownStep { + override predicate step(DataFlow::Node pred, DataFlow::Node succ) { + exists(DataFlow::CallNode call | call = DataFlow::moduleImport("markdown-table").getACall() | + succ = call and + pred = call.getArgument(0) + ) + } + } - UnifiedChain() { - root = unified() and - this = root.getAChainedMethodCall(["process", "processSync"]) + /** + * A taint step for the `showdown` library. + */ + private class ShowDownStep extends MarkdownStep { + override predicate step(DataFlow::Node pred, DataFlow::Node succ) { + exists(DataFlow::CallNode call | + call = + [DataFlow::globalVarRef("showdown"), DataFlow::moduleImport("showdown")] + .getAConstructorInvocation("Converter") + .getAMemberCall(["makeHtml", "makeMd"]) + | + succ = call and + pred = call.getArgument(0) + ) } + } + /** + * Classes and predicates for modelling taint steps in `unified` and `remark`. + */ + private module Unified { /** - * Gets a plugin that is used in this chain. + * The creation of a parser from `unified`. + * The `remark` module is a shorthand that initializes `unified` with a markdown parser. */ - DataFlow::Node getAUsedPlugin() { result = root.getAChainedMethodCall("use").getArgument(0) } + DataFlow::CallNode unified() { + result = DataFlow::moduleImport(["unified", "remark"]).getACall() + } /** - * Gets the input that is processed. + * A chain of method calls that process an input with `unified`. */ - DataFlow::Node getInput() { result = getArgument(0) } + class UnifiedChain extends DataFlow::CallNode { + DataFlow::CallNode root; + + UnifiedChain() { + root = unified() and + this = root.getAChainedMethodCall(["process", "processSync"]) + } + + /** + * Gets a plugin that is used in this chain. + */ + DataFlow::Node getAUsedPlugin() { result = root.getAChainedMethodCall("use").getArgument(0) } + + /** + * Gets the input that is processed. + */ + DataFlow::Node getInput() { result = getArgument(0) } + + /** + * Gets the processed output. + */ + DataFlow::Node getOutput() { + this.getCalleeName() = "process" and result = getABoundCallbackParameter(1, 1) + or + this.getCalleeName() = "processSync" and result = this + } + } /** - * Gets the processed output. + * A taint step for the `unified` library. */ - DataFlow::Node getOutput() { - this.getCalleeName() = "process" and result = getABoundCallbackParameter(1, 1) - or - this.getCalleeName() = "processSync" and result = this + class UnifiedStep extends MarkdownStep { + override predicate step(DataFlow::Node pred, DataFlow::Node succ) { + exists(UnifiedChain chain | + // sanitizer. Mostly looking for `rehype-sanitize`, but also other plugins with `sanitize` in their name. + not chain.getAUsedPlugin().getALocalSource() = + DataFlow::moduleImport(any(string s | s.matches("%sanitize%"))) + | + pred = chain.getInput() and + succ = chain.getOutput() + ) + } } } /** - * A taint step for the `unified` library. + * A taint step for the `snarkdown` library. */ - class UnifiedStep extends TaintTracking::SharedTaintStep { + private class SnarkdownStep extends MarkdownStep { override predicate step(DataFlow::Node pred, DataFlow::Node succ) { - exists(UnifiedChain chain | - // sanitizer. Mostly looking for `rehype-sanitize`, but also other plugins with `sanitize` in their name. - not chain.getAUsedPlugin().getALocalSource() = - DataFlow::moduleImport(any(string s | s.matches("%sanitize%"))) - | - pred = chain.getInput() and - succ = chain.getOutput() + exists(DataFlow::CallNode call | call = DataFlow::moduleImport("snarkdown").getACall() | + call = succ and + pred = call.getArgument(0) ) } } -} - -/** - * A taint step for the `snarkdown` library. - */ -private class SnarkdownStep extends TaintTracking::SharedTaintStep { - override predicate step(DataFlow::Node pred, DataFlow::Node succ) { - exists(DataFlow::CallNode call | call = DataFlow::moduleImport("snarkdown").getACall() | - call = succ and - pred = call.getArgument(0) - ) - } -} -/** - * Classes and predicates for modelling taint steps the `markdown-it` library. - */ -private module MarkdownIt { /** - * The creation of a parser from `markdown-it`. + * Classes and predicates for modelling taint steps the `markdown-it` library. */ - private API::Node markdownIt() { - exists(API::InvokeNode call | - call = API::moduleImport("markdown-it").getAnInvocation() + private module MarkdownIt { + /** + * The creation of a parser from `markdown-it`. + */ + private API::Node markdownIt() { + exists(API::InvokeNode call | + call = API::moduleImport("markdown-it").getAnInvocation() + or + call = API::moduleImport("markdown-it").getMember("Markdown").getAnInvocation() + | + call.getParameter(0).getMember("html").getARhs().mayHaveBooleanValue(true) and + result = call.getReturn() + ) or - call = API::moduleImport("markdown-it").getMember("Markdown").getAnInvocation() - | - call.getParameter(0).getMember("html").getARhs().mayHaveBooleanValue(true) and - result = call.getReturn() - ) - or - exists(API::CallNode call | - call = markdownIt().getMember(["use", "set", "configure", "enable", "disable"]).getACall() and - result = call.getReturn() and - not call.getParameter(0).getAValueReachingRhs() = - DataFlow::moduleImport("markdown-it-sanitizer") - ) - } - - /** - * A taint step for the `markdown-it` library. - */ - private class MarkdownItStep extends TaintTracking::SharedTaintStep { - override predicate step(DataFlow::Node pred, DataFlow::Node succ) { exists(API::CallNode call | - call = markdownIt().getMember(["render", "renderInline"]).getACall() - | - succ = call and - pred = call.getArgument(0) + call = markdownIt().getMember(["use", "set", "configure", "enable", "disable"]).getACall() and + result = call.getReturn() and + not call.getParameter(0).getAValueReachingRhs() = + DataFlow::moduleImport("markdown-it-sanitizer") ) } + + /** + * A taint step for the `markdown-it` library. + */ + private class MarkdownItStep extends MarkdownStep { + override predicate step(DataFlow::Node pred, DataFlow::Node succ) { + exists(API::CallNode call | + call = markdownIt().getMember(["render", "renderInline"]).getACall() + | + succ = call and + pred = call.getArgument(0) + ) + } + } } } diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll new file mode 100644 index 000000000000..4fe675b16d9c --- /dev/null +++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll @@ -0,0 +1,48 @@ +/** + * Provides a taint-tracking configuration for reasoning about + * unsafe HTML constructed from library input vulnerabilities. + */ + +import javascript + +/** + * Classes and predicates for the unsafe HTML constructed from library input query. + */ +module UnsafeHtmlConstruction { + private import semmle.javascript.security.dataflow.DomBasedXssCustomizations::DomBasedXss as DomBasedXss + private import semmle.javascript.security.dataflow.UnsafeJQueryPluginCustomizations::UnsafeJQueryPlugin as UnsafeJQueryPlugin + import UnsafeHtmlConstructionCustomizations::UnsafeHtmlConstruction + + /** + * A taint-tracking configuration for reasoning about unsafe HTML constructed from library input vulnerabilities. + */ + class Configration extends TaintTracking::Configuration { + Configration() { this = "UnsafeHtmlConstruction" } + + override predicate isSource(DataFlow::Node source) { source instanceof Source } + + override predicate isSink(DataFlow::Node sink) { sink instanceof Sink } + + override predicate isSanitizer(DataFlow::Node node) { + super.isSanitizer(node) + or + node instanceof DomBasedXss::Sanitizer + or + node instanceof UnsafeJQueryPlugin::Sanitizer + } + + override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) { + DomBasedXss::isOptionallySanitizedEdge(pred, succ) + } + + // override to require that there is a path without unmatched return steps + override predicate hasFlowPath(DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink) { + super.hasFlowPath(source, sink) and + DataFlow::hasPathWithoutUnmatchedReturn(source, sink) + } + + override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { + DataFlow::localFieldStep(pred, succ) + } + } +} diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll new file mode 100644 index 000000000000..c660657a322e --- /dev/null +++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll @@ -0,0 +1,177 @@ +/** + * Provides default sources, sinks and sanitizers for reasoning about + * unsafe HTML constructed from library input, as well as extension points + * for adding your own. + */ + +import javascript + +/** + * Module containing sources, sinks, and sanitizers for unsafe HTML constructed from library input. + */ +module UnsafeHtmlConstruction { + private import semmle.javascript.security.dataflow.DomBasedXssCustomizations::DomBasedXss as DomBasedXss + private import semmle.javascript.security.dataflow.UnsafeJQueryPluginCustomizations::UnsafeJQueryPlugin as UnsafeJQueryPlugin + private import semmle.javascript.PackageExports as Exports + + /** + * A source for unsafe HTML constructed from library input. + */ + abstract class Source extends DataFlow::Node { } + + /** + * A parameter of an exported function, seen as a source for usnafe HTML constructed from input. + */ + class ExternalInputSource extends Source, DataFlow::ParameterNode { + ExternalInputSource() { + this = Exports::getALibraryInputParameter() and + // An AMD-style module sometimes loads the jQuery library in a way which looks like library input. + not this = JQuery::dollarSource() + } + } + + /** + * A jQuery plugin options object, seen as a source for unsafe HTML constructed from input. + */ + class JQueryPluginOptionsAsSource extends Source { + JQueryPluginOptionsAsSource() { this instanceof UnsafeJQueryPlugin::JQueryPluginOptions } + } + + /** + * A sink for unsafe HTML constructed from library input. + * This sink transforms its input into a value that can cause XSS if it ends up in a XSS sink. + */ + abstract class Sink extends DataFlow::Node { + /** + * Gets the kind of vulnerability to report in the alert message. + * + * Defaults to `Cross-site scripting`, but may be overriden for sinks + * that do not allow script injection, but injection of other undesirable HTML elements. + */ + abstract string getVulnerabilityKind(); + + /** + * Gets the XSS sink that this transformed input ends up in. + */ + abstract DataFlow::Node getSink(); + + /** + * Gets a string describing the transformation that this sink represents. + */ + abstract string describe(); + } + + /** + * A sink for `js/html-constructed-from-input` that constructs some HTML where + * that HTML is later used in `xssSink`. + */ + abstract class XssSink extends Sink { + DomBasedXss::Sink xssSink; + + final override string getVulnerabilityKind() { result = xssSink.getVulnerabilityKind() } + + final override DomBasedXss::Sink getSink() { result = xssSink } + } + + /** + * Gets a dataflow node that flows to `sink` tracked by `t`. + */ + private DataFlow::Node isUsedInXssSink(DataFlow::TypeBackTracker t, DomBasedXss::Sink sink) { + t.start() and + result = sink + or + exists(DataFlow::TypeBackTracker t2 | t = t2.smallstep(result, isUsedInXssSink(t2, sink))) + or + exists(DataFlow::TypeBackTracker t2 | + t.continue() = t2 and + domBasedTaintStep(result, isUsedInXssSink(t2, sink)) + ) + } + + /** + * Gets a dataflow node that flows to `sink`. + */ + DataFlow::Node isUsedInXssSink(DomBasedXss::Sink sink) { + result = isUsedInXssSink(DataFlow::TypeBackTracker::end(), sink) + } + + /** + * Holds if there is a taint step from `pred` to `succ` for DOM strings/nodes. + * These steps are mostly relevant for DOM nodes that are created by an XML parser. + */ + predicate domBasedTaintStep(DataFlow::Node pred, DataFlow::SourceNode succ) { + // node.appendChild(newChild) and similar + exists(DataFlow::MethodCallNode call | + call.getMethodName() = ["insertBefore", "replaceChild", "appendChild"] + | + pred = call.getArgument(0) and + succ = [call.getReceiver().getALocalSource(), call] + ) + or + // element.{prepend,append}(node) and similar + exists(DataFlow::MethodCallNode call | + call.getMethodName() = ["prepend", "append", "replaceWith", "replaceChildren"] + | + pred = call.getAnArgument() and + succ = call.getReceiver().getALocalSource() + ) + or + // node.insertAdjacentElement("location", newChild) + exists(DataFlow::MethodCallNode call | call.getMethodName() = "insertAdjacentElement" | + pred = call.getArgument(1) and + succ = call.getReceiver().getALocalSource() + ) + or + // clone = node.cloneNode() + exists(DataFlow::MethodCallNode cloneNode | cloneNode.getMethodName() = "cloneNode" | + pred = cloneNode.getReceiver() and + succ = cloneNode + ) + or + // var succ = pred.documentElement; + // documentElement is the root element of the document, and childNodes is the list of all children + exists(DataFlow::PropRead read | read.getPropertyName() = ["documentElement", "childNodes"] | + pred = read.getBase() and + succ = read + ) + } + + /** + * A string-concatenation of HTML, where the result is used as an XSS sink. + */ + class HTMLConcatenationSink extends XssSink, StringOps::HtmlConcatenationLeaf { + HTMLConcatenationSink() { isUsedInXssSink(xssSink) = this.getRoot() } + + override string describe() { result = "HTML construction" } + } + + /** + * A string parsed as XML, which is later used in an XSS sink. + */ + class XMLParsedSink extends XssSink { + XML::ParserInvocation parser; + + XMLParsedSink() { + this.asExpr() = parser.getSourceArgument() and + isUsedInXssSink(xssSink) = parser.getAResult() + } + + override string describe() { result = "XML parsing" } + } + + /** + * A string rendered as markdown, where the rendering preserves HTML. + */ + class MarkdownSink extends XssSink { + MarkdownSink() { + exists(DataFlow::Node pred, DataFlow::Node succ, Markdown::MarkdownStep step | + step.step(pred, succ) and + step.preservesHTML() and + this = pred and + succ = isUsedInXssSink(xssSink) + ) + } + + override string describe() { result = "Markdown rendering" } + } +} diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeShellCommandConstruction.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeShellCommandConstruction.qll index 90caf59ad33d..d47b3696de61 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeShellCommandConstruction.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeShellCommandConstruction.qll @@ -35,21 +35,11 @@ module UnsafeShellCommandConstruction { // override to require that there is a path without unmatched return steps override predicate hasFlowPath(DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink) { super.hasFlowPath(source, sink) and - exists(DataFlow::MidPathNode mid | - source.getASuccessor*() = mid and - sink = mid.getASuccessor() and - mid.getPathSummary().hasReturn() = false - ) + DataFlow::hasPathWithoutUnmatchedReturn(source, sink) } override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { - // flow-step from a property written in the constructor to a use in an instance method. - // "simulates" client usage of a class, and regains some flow-steps lost by `hasFlowPath` above. - exists(DataFlow::ClassNode clz, string name | - pred = - DataFlow::thisNode(clz.getConstructor().getFunction()).getAPropertyWrite(name).getRhs() and - succ = DataFlow::thisNode(clz.getInstanceMethod(_).getFunction()).getAPropertyRead(name) - ) + DataFlow::localFieldStep(pred, succ) } } } diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/ConsistencyUnsafeHtmlConstruction.expected b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/ConsistencyUnsafeHtmlConstruction.expected new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/ConsistencyUnsafeHtmlConstruction.ql b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/ConsistencyUnsafeHtmlConstruction.ql new file mode 100644 index 000000000000..823644730b2c --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/ConsistencyUnsafeHtmlConstruction.ql @@ -0,0 +1,3 @@ +import javascript +import testUtilities.ConsistencyChecking +import semmle.javascript.security.dataflow.UnsafeHtmlConstruction as UnsafeHtmlConstruction diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected new file mode 100644 index 000000000000..7994fbad9342 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected @@ -0,0 +1,117 @@ +nodes +| main.js:1:55:1:55 | s | +| main.js:1:55:1:55 | s | +| main.js:2:29:2:29 | s | +| main.js:2:29:2:29 | s | +| main.js:6:49:6:49 | s | +| main.js:6:49:6:49 | s | +| main.js:7:49:7:49 | s | +| main.js:7:49:7:49 | s | +| main.js:11:60:11:60 | s | +| main.js:11:60:11:60 | s | +| main.js:12:49:12:49 | s | +| main.js:12:49:12:49 | s | +| main.js:21:47:21:47 | s | +| main.js:21:47:21:47 | s | +| main.js:22:34:22:34 | s | +| main.js:22:34:22:34 | s | +| main.js:41:17:41:17 | s | +| main.js:42:21:42:21 | s | +| main.js:47:65:47:73 | this.step | +| main.js:47:65:47:73 | this.step | +| main.js:52:41:52:41 | s | +| main.js:52:41:52:41 | s | +| main.js:53:20:53:20 | s | +| main.js:56:28:56:34 | options | +| main.js:56:28:56:34 | options | +| main.js:57:11:59:5 | defaults | +| main.js:57:22:59:5 | {\\n ... "\\n } | +| main.js:60:11:60:48 | settings | +| main.js:60:22:60:48 | $.exten ... ptions) | +| main.js:60:31:60:38 | defaults | +| main.js:60:41:60:47 | options | +| main.js:62:19:62:26 | settings | +| main.js:62:19:62:31 | settings.name | +| main.js:62:19:62:31 | settings.name | +| main.js:66:35:66:41 | attrVal | +| main.js:66:35:66:41 | attrVal | +| main.js:67:63:67:69 | attrVal | +| main.js:67:63:67:69 | attrVal | +| typed.ts:1:39:1:39 | s | +| typed.ts:1:39:1:39 | s | +| typed.ts:2:29:2:29 | s | +| typed.ts:2:29:2:29 | s | +| typed.ts:6:43:6:43 | s | +| typed.ts:6:43:6:43 | s | +| typed.ts:8:40:8:40 | s | +| typed.ts:8:40:8:40 | s | +| typed.ts:11:20:11:20 | s | +| typed.ts:11:20:11:20 | s | +| typed.ts:12:12:12:12 | s | +| typed.ts:16:11:16:21 | s | +| typed.ts:16:15:16:21 | id("x") | +| typed.ts:17:29:17:29 | s | +| typed.ts:17:29:17:29 | s | +edges +| main.js:1:55:1:55 | s | main.js:2:29:2:29 | s | +| main.js:1:55:1:55 | s | main.js:2:29:2:29 | s | +| main.js:1:55:1:55 | s | main.js:2:29:2:29 | s | +| main.js:1:55:1:55 | s | main.js:2:29:2:29 | s | +| main.js:6:49:6:49 | s | main.js:7:49:7:49 | s | +| main.js:6:49:6:49 | s | main.js:7:49:7:49 | s | +| main.js:6:49:6:49 | s | main.js:7:49:7:49 | s | +| main.js:6:49:6:49 | s | main.js:7:49:7:49 | s | +| main.js:11:60:11:60 | s | main.js:12:49:12:49 | s | +| main.js:11:60:11:60 | s | main.js:12:49:12:49 | s | +| main.js:11:60:11:60 | s | main.js:12:49:12:49 | s | +| main.js:11:60:11:60 | s | main.js:12:49:12:49 | s | +| main.js:21:47:21:47 | s | main.js:22:34:22:34 | s | +| main.js:21:47:21:47 | s | main.js:22:34:22:34 | s | +| main.js:21:47:21:47 | s | main.js:22:34:22:34 | s | +| main.js:21:47:21:47 | s | main.js:22:34:22:34 | s | +| main.js:41:17:41:17 | s | main.js:42:21:42:21 | s | +| main.js:42:21:42:21 | s | main.js:47:65:47:73 | this.step | +| main.js:42:21:42:21 | s | main.js:47:65:47:73 | this.step | +| main.js:52:41:52:41 | s | main.js:53:20:53:20 | s | +| main.js:52:41:52:41 | s | main.js:53:20:53:20 | s | +| main.js:53:20:53:20 | s | main.js:41:17:41:17 | s | +| main.js:56:28:56:34 | options | main.js:60:41:60:47 | options | +| main.js:56:28:56:34 | options | main.js:60:41:60:47 | options | +| main.js:57:11:59:5 | defaults | main.js:60:31:60:38 | defaults | +| main.js:57:22:59:5 | {\\n ... "\\n } | main.js:57:11:59:5 | defaults | +| main.js:60:11:60:48 | settings | main.js:62:19:62:26 | settings | +| main.js:60:22:60:48 | $.exten ... ptions) | main.js:60:11:60:48 | settings | +| main.js:60:31:60:38 | defaults | main.js:60:22:60:48 | $.exten ... ptions) | +| main.js:60:41:60:47 | options | main.js:57:22:59:5 | {\\n ... "\\n } | +| main.js:60:41:60:47 | options | main.js:60:22:60:48 | $.exten ... ptions) | +| main.js:62:19:62:26 | settings | main.js:62:19:62:31 | settings.name | +| main.js:62:19:62:26 | settings | main.js:62:19:62:31 | settings.name | +| main.js:66:35:66:41 | attrVal | main.js:67:63:67:69 | attrVal | +| main.js:66:35:66:41 | attrVal | main.js:67:63:67:69 | attrVal | +| main.js:66:35:66:41 | attrVal | main.js:67:63:67:69 | attrVal | +| main.js:66:35:66:41 | attrVal | main.js:67:63:67:69 | attrVal | +| typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s | +| typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s | +| typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s | +| typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s | +| typed.ts:6:43:6:43 | s | typed.ts:8:40:8:40 | s | +| typed.ts:6:43:6:43 | s | typed.ts:8:40:8:40 | s | +| typed.ts:6:43:6:43 | s | typed.ts:8:40:8:40 | s | +| typed.ts:6:43:6:43 | s | typed.ts:8:40:8:40 | s | +| typed.ts:11:20:11:20 | s | typed.ts:12:12:12:12 | s | +| typed.ts:11:20:11:20 | s | typed.ts:12:12:12:12 | s | +| typed.ts:12:12:12:12 | s | typed.ts:16:15:16:21 | id("x") | +| typed.ts:16:11:16:21 | s | typed.ts:17:29:17:29 | s | +| typed.ts:16:11:16:21 | s | typed.ts:17:29:17:29 | s | +| typed.ts:16:15:16:21 | id("x") | typed.ts:16:11:16:21 | s | +#select +| main.js:2:29:2:29 | s | main.js:1:55:1:55 | s | main.js:2:29:2:29 | s | $@ based on $@ might later cause $@. | main.js:2:29:2:29 | s | HTML construction | main.js:1:55:1:55 | s | library input | main.js:3:49:3:52 | html | cross-site scripting | +| main.js:7:49:7:49 | s | main.js:6:49:6:49 | s | main.js:7:49:7:49 | s | $@ based on $@ might later cause $@. | main.js:7:49:7:49 | s | XML parsing | main.js:6:49:6:49 | s | library input | main.js:8:48:8:66 | doc.documentElement | cross-site scripting | +| main.js:12:49:12:49 | s | main.js:11:60:11:60 | s | main.js:12:49:12:49 | s | $@ based on $@ might later cause $@. | main.js:12:49:12:49 | s | XML parsing | main.js:11:60:11:60 | s | library input | main.js:16:21:16:35 | xml.cloneNode() | cross-site scripting | +| main.js:12:49:12:49 | s | main.js:11:60:11:60 | s | main.js:12:49:12:49 | s | $@ based on $@ might later cause $@. | main.js:12:49:12:49 | s | XML parsing | main.js:11:60:11:60 | s | library input | main.js:17:48:17:50 | tmp | cross-site scripting | +| main.js:22:34:22:34 | s | main.js:21:47:21:47 | s | main.js:22:34:22:34 | s | $@ based on $@ might later cause $@. | main.js:22:34:22:34 | s | Markdown rendering | main.js:21:47:21:47 | s | library input | main.js:23:53:23:56 | html | cross-site scripting | +| main.js:47:65:47:73 | this.step | main.js:52:41:52:41 | s | main.js:47:65:47:73 | this.step | $@ based on $@ might later cause $@. | main.js:47:65:47:73 | this.step | HTML construction | main.js:52:41:52:41 | s | library input | main.js:47:54:47:85 | " ... /span>" | cross-site scripting | +| main.js:62:19:62:31 | settings.name | main.js:56:28:56:34 | options | main.js:62:19:62:31 | settings.name | $@ based on $@ might later cause $@. | main.js:62:19:62:31 | settings.name | HTML construction | main.js:56:28:56:34 | options | library input | main.js:62:11:62:40 | "" + ... "" | cross-site scripting | +| main.js:67:63:67:69 | attrVal | main.js:66:35:66:41 | attrVal | main.js:67:63:67:69 | attrVal | $@ based on $@ might later cause $@. | main.js:67:63:67:69 | attrVal | HTML construction | main.js:66:35:66:41 | attrVal | library input | main.js:67:47:67:78 | "" | cross-site scripting | +| typed.ts:2:29:2:29 | s | typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s | $@ based on $@ might later cause $@. | typed.ts:2:29:2:29 | s | HTML construction | typed.ts:1:39:1:39 | s | library input | typed.ts:3:31:3:34 | html | cross-site scripting | +| typed.ts:8:40:8:40 | s | typed.ts:6:43:6:43 | s | typed.ts:8:40:8:40 | s | $@ based on $@ might later cause $@. | typed.ts:8:40:8:40 | s | HTML construction | typed.ts:6:43:6:43 | s | library input | typed.ts:8:29:8:52 | " ... /span>" | cross-site scripting | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.qlref b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.qlref new file mode 100644 index 000000000000..0fbe0ed0ba1a --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.qlref @@ -0,0 +1 @@ +Security/CWE-079/UnsafeHtmlConstruction.ql \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/jquery-plugin.js b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/jquery-plugin.js new file mode 100644 index 000000000000..07b25b558f9b --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/jquery-plugin.js @@ -0,0 +1,9 @@ +(function (factory) { + if (typeof define === 'function' && define.amd) { + define(['jquery', 'jquery-ui'], factory); + } else { + factory(jQuery); + } +}(function ($) { + $("" + $.trim("foo") + ""); // OK +})); diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/main.js b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/main.js new file mode 100644 index 000000000000..1097e126feb2 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/main.js @@ -0,0 +1,77 @@ +module.exports.xssThroughHTMLConstruction = function (s) { + const html = "" + s + "";// NOT OK + document.querySelector("#html").innerHTML = html; +} + +module.exports.xssThroughXMLParsing = function (s) { + const doc = new DOMParser().parseFromString(s, "text/xml"); // NOT OK + document.querySelector("#xml").appendChild(doc.documentElement); +} + +module.exports.xssThroughMoreComplexXMLParsing = function (s) { + const doc = new DOMParser().parseFromString(s, "text/xml"); // NOT OK + const xml = doc.documentElement; + + const tmp = document.createElement('span'); + tmp.appendChild(xml.cloneNode()); + document.querySelector("#xml").appendChild(tmp); +} + +const markdown = require('markdown-it')({html: true}); +module.exports.xssThroughMarkdown = function (s) { + const html = markdown.render(s); // NOT OK + document.querySelector("#markdown").innerHTML = html; +} + +const striptags = require('striptags'); +module.exports.sanitizedHTML = function (s) { + const html = striptags("" + s + ""); // OK + document.querySelector("#sanitized").innerHTML = html; +} + +module.exports.ts = require("./typed"); + +module.exports.jquery = require("./jquery-plugin"); + +module.exports.plainDOMXMLParsing = function (s) { + const doc = new DOMParser().parseFromString(s, "text/xml"); // OK - is never added to the DOM. +} + +class Foo { + constructor(s) { + this.step = s; + } + + doXss() { + // not called here, but still bad. + document.querySelector("#class").innerHTML = "" + this.step + ""; // NOT OK + } + +} + +module.exports.createsClass = function (s) { + return new Foo(s); +} + +$.fn.xssPlugin = function (options) { + const defaults = { + name: "name" + }; + const settings = $.extend(defaults, options); + return this.each(function () { + $("" + settings.name + "").appendTo(this); // NOT OK + }); +} + +module.exports.guards = function (attrVal) { + document.querySelector("#id").innerHTML = "\"""; // NOT OK + document.querySelector("#id").innerHTML = "\"""; // OK + if (attrVal.indexOf("\"") === -1 && attrVal.indexOf("'") === -1) { + document.querySelector("#id").innerHTML = "\"""; // OK + } +} + +module.exports.intentionalTemplate = function (obj) { + const html = "" + obj.spanTemplate + ""; // OK + document.querySelector("#template").innerHTML = html; +} diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/package.json b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/package.json new file mode 100644 index 000000000000..f4fa73f79d9a --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/package.json @@ -0,0 +1,4 @@ +{ + "name": "my-unsafe-library", + "main": "./main.js" +} \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/tsconfig.json b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/tsconfig.json new file mode 100644 index 000000000000..9e26dfeeb6e6 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/tsconfig.json @@ -0,0 +1 @@ +{} \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/typed.ts b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/typed.ts new file mode 100644 index 000000000000..0f04e92cdc04 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/typed.ts @@ -0,0 +1,20 @@ +export function basicHtmlConstruction(s: string) { + const html = "" + s + ""; // NOT OK + document.body.innerHTML = html; +} + +export function insertIntoCreatedDocument(s: string) { + const newDoc = document.implementation.createHTMLDocument(""); + newDoc.body.innerHTML = "" + s + ""; // OK - inserted into document disconnected from the main DOM. [INCONSISTENCY] +} + +export function id(s: string) { + return s; +} + +export function notVulnerable() { + const s = id("x"); + const html = "" + s + ""; // OK + document.body.innerHTML = html; +} + \ No newline at end of file