Merge pull request #2330 from erik-krogh/exceptionXss

JS: Added query for detecting XSS that happens through an exception

javascript/ql/src/Security/CWE-079/ExceptionXss.qhelp

@@ -0,0 +1,54 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+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.
+</p>
+</overview>
+
+<recommendation>
+<p>
+To guard against cross-site scripting, consider using contextual output encoding/escaping before
+writing user input to the page, or one of the other solutions that are mentioned in the
+references.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following example shows an exception being written directly to the document, 
+and this exception can potentially be influenced by the page URL, 
+leaving the website vulnerable to cross-site scripting.
+</p>
+<sample src="examples/ExceptionXss.js" />
+</example>
+
+<references>
+<li>
+OWASP:
+<a href="https://cheatsheetseries.owasp.org/cheatsheets/DOM_based_XSS_Prevention_Cheat_Sheet.html">DOM based
+XSS Prevention Cheat Sheet</a>.
+</li>
+<li>
+OWASP:
+<a href="https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html">XSS
+(Cross Site Scripting) Prevention Cheat Sheet</a>.
+</li>
+<li>
+OWASP
+<a href="https://www.owasp.org/index.php/DOM_Based_XSS">DOM Based XSS</a>.
+</li>
+<li>
+OWASP
+<a href="https://www.owasp.org/index.php/Types_of_Cross-Site_Scripting">Types of Cross-Site
+Scripting</a>.
+</li>
+<li>
+Wikipedia: <a href="http://en.wikipedia.org/wiki/Cross-site_scripting">Cross-site scripting</a>.
+</li>
+</references>
+</qhelp>

javascript/ql/src/Security/CWE-079/ExceptionXss.ql

@@ -0,0 +1,24 @@
+/**
+ * @name Cross-site scripting through exception
+ * @description Inserting data from an exception containing user
+ *              input into the DOM may enable cross-site scripting.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision medium
+ * @id js/xss-through-exception
+ * @tags security
+ *       external/cwe/cwe-079
+ *       external/cwe/cwe-116
+ */
+
+import javascript
+import semmle.javascript.security.dataflow.ExceptionXss::ExceptionXss
+import DataFlow::PathGraph
+
+from
+  Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where
+  cfg.hasFlowPath(source, sink)
+select sink.getNode(), source, sink,
+  sink.getNode().(Xss::Shared::Sink).getVulnerabilityKind() + " vulnerability due to $@.", source.getNode(),
+  "user-provided value"

javascript/ql/src/Security/CWE-079/examples/ExceptionXss.js

@@ -0,0 +1,10 @@
+function setLanguageOptions() {
+    var href = document.location.href,
+        deflt = href.substring(href.indexOf("default=")+8);
+    
+    try {
+        var parsed = unknownParseFunction(deflt); 
+    } catch(e) {
+        document.write("Had an error: " + e + ".");
+    }
+}

javascript/ql/src/semmle/javascript/Expr.qll

@@ -245,6 +245,21 @@ class Expr extends @expr, ExprOrStmt, ExprOrType, AST::ValueNode {
       ctx.(ConditionalExpr).inNullSensitiveContext()
     )
   }
+
+  /**
+   * Gets the data-flow node where exceptions thrown by this expression will
+   * propagate if this expression causes an exception to be thrown.
+   */
+  DataFlow::Node getExceptionTarget() {
+    if exists(this.getEnclosingStmt().getEnclosingTryCatchStmt())
+    then
+      result = DataFlow::parameterNode(this
+              .getEnclosingStmt()
+              .getEnclosingTryCatchStmt()
+              .getACatchClause()
+              .getAParameter())
+    else result = any(DataFlow::FunctionNode f | f.getFunction() = this.getContainer()).getExceptionalReturn()
+  }
 }
 
 /**

javascript/ql/src/semmle/javascript/Stmt.qll

@@ -55,6 +55,18 @@ class Stmt extends @stmt, ExprOrStmt, Documentable {
   }
 
   override predicate isAmbient() { hasDeclareKeyword(this) or getParent().isAmbient() }
+  
+  /**
+  * Gets the `try` statement with a catch block containing this statement without
+  * crossing function boundaries or other `try ` statements with catch blocks.
+  */
+  TryStmt getEnclosingTryCatchStmt() {
+    getParentStmt+() = result.getBody() and
+    exists(result.getACatchClause()) and
+    not exists(TryStmt mid | exists(mid.getACatchClause()) | 
+      getParentStmt+() = mid.getBody() and mid.getParentStmt+() = result.getBody()
+    )
+  }
 }
 
 /**

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

@@ -1125,6 +1125,9 @@ class MidPathNode extends PathNode, MkMidNode {
     // Skip to the top of big left-leaning string concatenation trees.
     nd = any(AddExpr add).flow() and
     nd = any(AddExpr add).getAnOperand().flow()
+    or
+    // Skip the exceptional return on functions, as this highlights the entire function.
+    nd = any(DataFlow::FunctionNode f).getExceptionalReturn()
   }
 }
 

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

@@ -558,6 +558,23 @@ module TaintTracking {
       succ = this
     }
   }
+  
+
+  /**
+   * A taint propagating data flow edge arising from calling `String.prototype.match()`.
+   */
+  private class StringMatchTaintStep extends AdditionalTaintStep, DataFlow::MethodCallNode {
+    StringMatchTaintStep() {
+      this.getMethodName() = "match" and
+      this.getNumArgument() = 1 and
+      this.getArgument(0) .analyze().getAType() = TTRegExp()
+    }
+
+    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+      pred = this.getReceiver() and
+      succ = this
+    }
+  }
 
   /**
    * A taint propagating data flow edge arising from JSON unparsing.

javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll

@@ -66,19 +66,7 @@ predicate localExceptionStep(DataFlow::Node pred, DataFlow::Node succ) {
     or
     DataFlow::exceptionalInvocationReturnNode(pred, expr)
   |
-    // Propagate out of enclosing function.
-    not exists(getEnclosingTryStmt(expr.getEnclosingStmt())) and
-    exists(Function f |
-      f = expr.getEnclosingFunction() and
-      DataFlow::exceptionalFunctionReturnNode(succ, f)
-    )
-    or
-    // Propagate to enclosing try/catch.
-    // To avoid false flow, we only propagate to an unguarded catch clause.
-    exists(TryStmt try |
-      try = getEnclosingTryStmt(expr.getEnclosingStmt()) and
-      DataFlow::parameterNode(succ, try.getCatchClause().getAParameter())
-    )
+    succ = expr.getExceptionTarget()
   )
 }
 
@@ -156,19 +144,6 @@ private module CachedSteps {
   cached
   predicate callStep(DataFlow::Node pred, DataFlow::Node succ) { argumentPassing(_, pred, _, succ) }
 
-  /**
-   * Gets the `try` statement containing `stmt` without crossing function boundaries
-   * or other `try ` statements.
-   */
-  cached
-  TryStmt getEnclosingTryStmt(Stmt stmt) {
-    result.getBody() = stmt
-    or
-    not stmt instanceof Function and
-    not stmt = any(TryStmt try).getBody() and
-    result = getEnclosingTryStmt(stmt.getParentStmt())
-  }
-
   /**
    * Holds if there is a flow step from `pred` to `succ` through:
    * - returning a value from a function call, or

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

@@ -6,7 +6,7 @@
 import javascript
 
 module DomBasedXss {
-  import Xss::DomBasedXss
+  import DomBasedXssCustomizations::DomBasedXss
 
   /**
    * A taint-tracking configuration for reasoning about XSS.
@@ -33,16 +33,4 @@ module DomBasedXss {
       node instanceof Sanitizer
     }
   }
-
-  /** A source of remote user input, considered as a flow source for DOM-based XSS. */
-  class RemoteFlowSourceAsSource extends Source {
-    RemoteFlowSourceAsSource() { this instanceof RemoteFlowSource }
-  }
-
-  /**
-   * An access of the URL of this page, or of the referrer to this page.
-   */
-  class LocationSource extends Source {
-    LocationSource() { this = DOM::locationSource() }
-  }
 }

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

@@ -0,0 +1,23 @@
+/**
+ * Provides default sources for reasoning about DOM-based
+ * cross-site scripting vulnerabilities. 
+ */
+
+
+import javascript
+
+module DomBasedXss {
+  import Xss::DomBasedXss
+
+  /** A source of remote user input, considered as a flow source for DOM-based XSS. */
+  class RemoteFlowSourceAsSource extends Source {
+    RemoteFlowSourceAsSource() { this instanceof RemoteFlowSource }
+  }
+
+  /**
+   * An access of the URL of this page, or of the referrer to this page.
+   */
+  class LocationSource extends Source {
+    LocationSource() { this = DOM::locationSource() }
+  }
+}

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

@@ -0,0 +1,80 @@
+/**
+ * Provides a taint-tracking configuration for reasoning about cross-site
+ * scripting vulnerabilities where the taint-flow passes through a thrown
+ * exception.
+ */
+
+import javascript
+
+module ExceptionXss {
+  import DomBasedXssCustomizations::DomBasedXss as DomBasedXssCustom
+  import ReflectedXssCustomizations::ReflectedXss as ReflectedXssCustom
+  import Xss as Xss
+  
+  /**
+   * Holds if `node` is unlikely to cause an exception containing sensitive information to be thrown.
+   */
+  private predicate isUnlikelyToThrowSensitiveInformation(DataFlow::Node node) {
+    node = any(DataFlow::CallNode call | call.getCalleeName() = "getElementById").getAnArgument()
+    or
+    node = any(DataFlow::CallNode call | call.getCalleeName() = "indexOf").getAnArgument()
+    or
+    node = any(DataFlow::CallNode call | call.getCalleeName() = "stringify").getAnArgument()
+    or
+    node = DataFlow::globalVarRef("console").getAMemberCall(_).getAnArgument()
+  }
+
+  /**
+   * Holds if `node` can possibly cause an exception containing sensitive information to be thrown.
+   */
+  predicate canThrowSensitiveInformation(DataFlow::Node node) {
+    not isUnlikelyToThrowSensitiveInformation(node) and 
+    (
+      // in the case of reflective calls the below ensures that both InvokeNodes have no known callee.
+      forex(DataFlow::InvokeNode call | node = call.getAnArgument() | not exists(call.getACallee()))
+      or
+      node.asExpr().getEnclosingStmt() instanceof ThrowStmt
+    )
+  }
+
+  /**
+   * 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" }
+  }
+
+  /**
+   * 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
+   * an exception.
+   */
+  class Configuration extends TaintTracking::Configuration {
+    Configuration() { this = "ExceptionXss" }
+
+    override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {
+      source instanceof Xss::Shared::Source and label instanceof NotYetThrown
+    }
+
+    override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
+      sink instanceof Xss::Shared::Sink and not label instanceof NotYetThrown
+    }
+
+    override predicate isSanitizer(DataFlow::Node node) { node instanceof Xss::Shared::Sanitizer }
+
+    override predicate isAdditionalFlowStep(
+      DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel inlbl,
+      DataFlow::FlowLabel outlbl
+    ) {
+      inlbl instanceof NotYetThrown and (outlbl.isTaint() or outlbl instanceof NotYetThrown) and
+      succ = pred.asExpr().getExceptionTarget() and
+      canThrowSensitiveInformation(pred)
+      or
+      // All the usual taint-flow steps apply on data-flow before it has been thrown in an exception.
+      this.isAdditionalFlowStep(pred, succ) and inlbl instanceof NotYetThrown and outlbl instanceof NotYetThrown
+    }
+  }
+}

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

@@ -6,7 +6,7 @@
 import javascript
 
 module ReflectedXss {
-  import Xss::ReflectedXss
+  import ReflectedXssCustomizations::ReflectedXss
 
   /**
    * A taint-tracking configuration for reasoning about XSS.
@@ -23,13 +23,4 @@ module ReflectedXss {
       node instanceof Sanitizer
     }
   }
-
-  /** A third-party controllable request input, considered as a flow source for reflected XSS. */
-  class ThirdPartyRequestInputAccessAsSource extends Source {
-    ThirdPartyRequestInputAccessAsSource() {
-      this.(HTTP::RequestInputAccess).isThirdPartyControllable()
-      or
-      this.(HTTP::RequestHeaderAccess).getAHeaderName() = "referer"
-    }
-  }
 }

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

@@ -0,0 +1,19 @@
+/**
+ * Provides default sources for reasoning about reflected
+ * cross-site scripting vulnerabilities.
+ */
+
+import javascript
+
+module ReflectedXss {
+  import Xss::ReflectedXss
+
+  /** A third-party controllable request input, considered as a flow source for reflected XSS. */
+  class ThirdPartyRequestInputAccessAsSource extends Source {
+    ThirdPartyRequestInputAccessAsSource() {
+      this.(HTTP::RequestInputAccess).isThirdPartyControllable()
+      or
+      this.(HTTP::RequestHeaderAccess).getAHeaderName() = "referer"
+    }
+  }
+}