Merge pull request #910 from xiemaisi/js/regexp-taint

Approved by esben-semmle

Author: semmle-qlci

change-notes/1.20/analysis-javascript.md

@@ -9,7 +9,7 @@
   - asynchronous code, for example [a-sync-waterfall](https://www.npmjs.com/package/a-sync-waterfall)
 * File classification has been improved to recognize additional generated files, for example files from [HTML Tidy](html-tidy.org).
 
-* The taint tracking library now recognizes flow through persistent storage, class fields, and callbacks in certain cases. This may give more results for the security queries.
+* The taint tracking library now recognizes flow through persistent storage, class fields, and callbacks in certain cases. Handling of regular expressions has also been improved. This may give more results for the security queries.
 
 * Type inference for function calls has been improved. This may give additional results for queries that rely on type inference.
 
@@ -33,9 +33,11 @@
 
 | **Query**                                  | **Expected impact**          | **Change**                                                                   |
 |--------------------------------------------|------------------------------|------------------------------------------------------------------------------|
-| Client-side cross-site scripting           | More true-positive results, fewer false-positive results.                 | This rule now recognizes WinJS functions that are vulnerable to HTML injection, and no longer flags certain safe uses of jQuery. |
+| Client-side cross-site scripting           | More true-positive results, fewer false-positive results. | This rule now recognizes WinJS functions that are vulnerable to HTML injection. It no longer flags certain safe uses of jQuery, and recognizes custom sanitizers. |
 | Hard-coded credentials                     | Fewer false-positive results | This rule no longer flag the empty string as a hardcoded username. |
 | Insecure randomness | More results | This rule now flags insecure uses of `crypto.pseudoRandomBytes`. |
+| Reflected cross-site scripting             | Fewer false-positive results. | This rule now recognizes custom sanitizers. |
+| Stored cross-site scripting                | Fewer false-positive results. | This rule now recognizes custom sanitizers. |
 | Uncontrolled data used in network request  | More results                 | This rule now recognizes host values that are vulnerable to injection. |
 | Unused parameter                           | Fewer false-positive results | This rule no longer flags parameters with leading underscore. |
 | Unused variable, import, function or class | Fewer false-positive results | This rule now flags fewer variables that are implictly used by JSX elements, and no longer flags variables with leading underscore. |

javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll

@@ -473,6 +473,27 @@ module TaintTracking {
     }
   }
 
+  /**
+   * A taint-propagating data flow edge from the first (and only) argument in a call to
+   * `RegExp.prototype.exec` to its result.
+   */
+  private class RegExpExecTaintStep extends AdditionalTaintStep {
+    DataFlow::MethodCallNode self;
+
+    RegExpExecTaintStep() {
+      this = self and
+      self.getReceiver().analyze().getAType() = TTRegExp() and
+      self.getMethodName() = "exec" and
+      self.getNumArgument() = 1
+    }
+
+
+    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+      pred = self.getArgument(0) and
+      succ = this
+    }
+  }
+
   /**
    * A taint propagating data flow edge arising from JSON unparsing.
    */

javascript/ql/src/semmle/javascript/security/dataflow/DomBasedXss.qll

@@ -4,32 +4,9 @@
  */
 
 import javascript
-import semmle.javascript.security.dataflow.RemoteFlowSources
-import semmle.javascript.frameworks.jQuery
 
 module DomBasedXss {
-  /**
-   * A data flow source for XSS vulnerabilities.
-   */
-  abstract class Source extends DataFlow::Node { }
-
-  /**
-   * A data flow sink for XSS vulnerabilities.
-   */
-  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.
-     */
-    string getVulnerabilityKind() { result = "Cross-site scripting" }
-  }
-
-  /**
-   * A sanitizer for XSS vulnerabilities.
-   */
-  abstract class Sanitizer extends DataFlow::Node { }
+  import Xss::DomBasedXss
 
   /**
    * A taint-tracking configuration for reasoning about XSS.
@@ -68,146 +45,4 @@ module DomBasedXss {
   class LocationSource extends Source, DataFlow::ValueNode {
     LocationSource() { isDocumentURL(astNode) }
   }
-
-  /**
-   * An expression whose value is interpreted as HTML
-   * and may be inserted into the DOM through a library.
-   */
-  class LibrarySink extends Sink, DataFlow::ValueNode {
-    LibrarySink() {
-      // call to a jQuery method that interprets its argument as HTML
-      exists(JQueryMethodCall call | call.interpretsArgumentAsHtml(astNode) |
-        // either the argument is always interpreted as HTML
-        not call.interpretsArgumentAsSelector(astNode)
-        or
-        // or it doesn't start with something other than `<`, and so at least
-        // _may_ be interpreted as HTML
-        not exists(DataFlow::Node prefix, string strval |
-          isPrefixOfJQueryHtmlString(astNode, prefix) and
-          strval = prefix.asExpr().getStringValue() and
-          not strval.regexpMatch("\\s*<.*")
-        ) and
-        not isDocumentURL(astNode)
-      )
-      or
-      // call to an Angular method that interprets its argument as HTML
-      any(AngularJS::AngularJSCall call).interpretsArgumentAsHtml(this.asExpr())
-      or
-      // call to a WinJS function that interprets its argument as HTML
-      exists(DataFlow::MethodCallNode mcn, string m |
-        m = "setInnerHTMLUnsafe" or m = "setOuterHTMLUnsafe"
-      |
-        mcn.getMethodName() = m and
-        this = mcn.getArgument(1)
-      )
-    }
-  }
-
-  /**
-   * Holds if `prefix` is a prefix of `htmlString`, which may be intepreted as
-   * HTML by a jQuery method.
-   */
-  private predicate isPrefixOfJQueryHtmlString(Expr htmlString, DataFlow::Node prefix) {
-    any(JQueryMethodCall call).interpretsArgumentAsHtml(htmlString) and
-    prefix = htmlString.flow()
-    or
-    exists(DataFlow::Node pred | isPrefixOfJQueryHtmlString(htmlString, pred) |
-      prefix = StringConcatenation::getFirstOperand(pred)
-      or
-      prefix = pred.getAPredecessor()
-    )
-  }
-
-  /**
-   * An expression whose value is interpreted as HTML or CSS
-   * and may be inserted into the DOM.
-   */
-  class DomSink extends Sink {
-    DomSink() {
-      // Call to a DOM function that inserts its argument into the DOM
-      any(DomMethodCallExpr call).interpretsArgumentsAsHTML(this.asExpr())
-      or
-      // Assignment to a dangerous DOM property
-      exists(DomPropWriteNode pw |
-        pw.interpretsValueAsHTML() and
-        this = DataFlow::valueNode(pw.getRhs())
-      )
-      or
-      // `html` or `source.html` properties of React Native `WebView`
-      exists(ReactNative::WebViewElement webView, DataFlow::SourceNode source |
-        source = webView or
-        source = webView.getAPropertyWrite("source").getRhs().getALocalSource()
-      |
-        this = source.getAPropertyWrite("html").getRhs()
-      )
-    }
-  }
-
-  /**
-   * An expression whose value is interpreted as HTML by a DOMParser.
-   */
-  class DomParserSink extends Sink {
-    DomParserSink() {
-      exists(DataFlow::GlobalVarRefNode domParser |
-        domParser.getName() = "DOMParser" and
-        this = domParser.getAnInstantiation().getAMethodCall("parseFromString").getArgument(0)
-      )
-    }
-  }
-
-  /**
-   * A React `dangerouslySetInnerHTML` attribute, viewed as an XSS sink.
-   *
-   * Any write to the `__html` property of an object assigned to this attribute
-   * is considered an XSS sink.
-   */
-  class DangerouslySetInnerHtmlSink extends Sink, DataFlow::ValueNode {
-    DangerouslySetInnerHtmlSink() {
-      exists(DataFlow::Node danger, DataFlow::SourceNode valueSrc |
-        exists(JSXAttribute attr |
-          attr.getName() = "dangerouslySetInnerHTML" and
-          attr.getValue() = danger.asExpr()
-        )
-        or
-        exists(ReactElementDefinition def, DataFlow::ObjectLiteralNode props |
-          props.flowsTo(def.getProps()) and
-          props.hasPropertyWrite("dangerouslySetInnerHTML", danger)
-        )
-      |
-        valueSrc.flowsTo(danger) and
-        valueSrc.hasPropertyWrite("__html", this)
-      )
-    }
-  }
-
-  /**
-   * The HTML body of an email, viewed as an XSS sink.
-   */
-  class EmailHtmlBodySink extends Sink {
-    EmailHtmlBodySink() { this = any(EmailSender sender).getHtmlBody() }
-
-    override string getVulnerabilityKind() { result = "HTML injection" }
-  }
-
-
-  /**
-   * A write to the `template` option of a Vue instance, viewed as an XSS sink.
-   */
-  class VueTemplateSink extends DomBasedXss::Sink {
-    VueTemplateSink() { this = any(Vue::Instance i).getTemplate() }
-  }
-
-  /**
-   * The tag name argument to the `createElement` parameter of the
-   * `render` method of a Vue instance, viewed as an XSS sink.
-   */
-  class VueCreateElementSink extends DomBasedXss::Sink {
-    VueCreateElementSink() {
-      exists(Vue::Instance i, DataFlow::FunctionNode f |
-        f.flowsTo(i.getRender()) and
-        this = f.getParameter(0).getACall().getArgument(0)
-      )
-    }
-  }
-
 }

javascript/ql/src/semmle/javascript/security/dataflow/ReflectedXss.qll

@@ -4,24 +4,9 @@
  */
 
 import javascript
-import semmle.javascript.security.dataflow.RemoteFlowSources
-import semmle.javascript.frameworks.jQuery
 
 module ReflectedXss {
-  /**
-   * A data flow source for XSS vulnerabilities.
-   */
-  abstract class Source extends DataFlow::Node { }
-
-  /**
-   * A data flow sink for XSS vulnerabilities.
-   */
-  abstract class Sink extends DataFlow::Node { }
-
-  /**
-   * A sanitizer for XSS vulnerabilities.
-   */
-  abstract class Sanitizer extends DataFlow::Node { }
+  import Xss::ReflectedXss
 
   /**
    * A taint-tracking configuration for reasoning about XSS.
@@ -47,23 +32,4 @@ module ReflectedXss {
       this.(HTTP::RequestHeaderAccess).getAHeaderName() = "referer"
     }
   }
-
-  /**
-   * An expression that is sent as part of an HTTP response, considered as an XSS sink.
-   *
-   * We exclude cases where the route handler sets either an unknown content type or
-   * a content type that does not (case-insensitively) contain the string "html". This
-   * is to prevent us from flagging plain-text or JSON responses as vulnerable.
-   */
-  private class HttpResponseSink extends Sink {
-    HttpResponseSink() {
-      exists(HTTP::ResponseSendArgument sendarg | sendarg = asExpr() |
-        forall(HTTP::HeaderDefinition hd |
-          hd = sendarg.getRouteHandler().getAResponseHeader("content-type")
-        |
-          exists(string tp | hd.defines("content-type", tp) | tp.toLowerCase().matches("%html%"))
-        )
-      )
-    }
-  }
 }

javascript/ql/src/semmle/javascript/security/dataflow/StoredXss.qll

@@ -4,25 +4,9 @@
  */
 
 import javascript
-import semmle.javascript.security.dataflow.RemoteFlowSources
-import semmle.javascript.security.dataflow.ReflectedXss as ReflectedXss
-import semmle.javascript.security.dataflow.DomBasedXss as DomBasedXss
 
 module StoredXss {
-  /**
-   * A data flow source for XSS vulnerabilities.
-   */
-  abstract class Source extends DataFlow::Node { }
-
-  /**
-   * A data flow sink for XSS vulnerabilities.
-   */
-  abstract class Sink extends DataFlow::Node { }
-
-  /**
-   * A sanitizer for XSS vulnerabilities.
-   */
-  abstract class Sanitizer extends DataFlow::Node { }
+  import Xss::StoredXss
 
   /**
    * A taint-tracking configuration for reasoning about XSS.
@@ -44,12 +28,4 @@ module StoredXss {
   class FileNameSourceAsSource extends Source {
     FileNameSourceAsSource() { this instanceof FileNameSource }
   }
-
-  /** An ordinary XSS sink, considered as a flow sink for stored XSS. */
-  class XssSinkAsSink extends Sink {
-    XssSinkAsSink() {
-      this instanceof ReflectedXss::ReflectedXss::Sink or
-      this instanceof DomBasedXss::DomBasedXss::Sink
-    }
-  }
 }