Merge pull request #1211 from esben-semmle/js/type-tracking-for-incomplete-hostname-regexp

JS: type tracking for js/incomplete-hostname-regexp

Author: Max Schaefer

change-notes/1.21/analysis-javascript.md

@@ -29,6 +29,7 @@
 | Client-side URL redirect       | More results and fewer false-positive results | This rule now recognizes additional uses of the document URL. This rule now treats URLs as safe in more cases where the hostname cannot be tampered with. |
 | Double escaping or unescaping | More results | This rule now considers the flow of regular expressions literals. |
 | Expression has no effect       | Fewer false-positive results | This rule now treats uses of `Object.defineProperty` more conservatively. |
+| Incomplete regular expression for hostnames | More results | This rule now tracks regular expressions for host names further. |
 | Incomplete string escaping or encoding | More results | This rule now considers the flow of regular expressions literals. |
 | Replacement of a substring with itself | More results | This rule now considers the flow of regular expressions literals. |
 | Server-side URL redirect       | Fewer false-positive results | This rule now treats URLs as safe in more cases where the hostname cannot be tampered with. |

javascript/ql/src/Security/CWE-020/IncompleteHostnameRegExp.qhelp

@@ -59,13 +59,14 @@
 		<p>
 
 			Address this vulnerability by escaping <code>.</code>
-			appropriately: <code>let regex = /(www|beta|)\.example\.com/</code>.
+			appropriately: <code>let regex = /((www|beta)\.)?example\.com/</code>.
 
 		</p>
 
 	</example>
 
 	<references>
+		<li>MDN: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions">Regular Expressions</a></li>
 		<li>OWASP: <a href="https://www.owasp.org/index.php/Server_Side_Request_Forgery">SSRF</a></li>
 		<li>OWASP: <a href="https://www.owasp.org/index.php/Unvalidated_Redirects_and_Forwards_Cheat_Sheet">XSS Unvalidated Redirects and Forwards Cheat Sheet</a>.</li>
 	</references>

javascript/ql/src/Security/CWE-020/IncompleteHostnameRegExp.ql

@@ -13,16 +13,35 @@
 import javascript
 
 /**
- * A taint tracking configuration for incomplete hostname regular expressions sources.
+ * Gets a node whose value may flow (inter-procedurally) to a position where it is interpreted
+ * as a regular expression.
  */
-class Configuration extends TaintTracking::Configuration {
-  Configuration() { this = "IncompleteHostnameRegExpTracking" }
+DataFlow::Node regExpSource(DataFlow::Node re, DataFlow::TypeBackTracker t) {
+  t.start() and
+  re = result and
+  isInterpretedAsRegExp(result)
+  or
+  exists(DataFlow::TypeBackTracker t2, DataFlow::Node succ | succ = regExpSource(re, t2) |
+    t2 = t.smallstep(result, succ)
+    or
+    any(TaintTracking::AdditionalTaintStep dts).step(result, succ) and
+    t = t2
+  )
+}
 
-  override predicate isSource(DataFlow::Node source) {
-    isIncompleteHostNameRegExpPattern(source.getStringValue(), _)
-  }
+DataFlow::Node regExpSource(DataFlow::Node re) {
+  result = regExpSource(re, DataFlow::TypeBackTracker::end())
+}
 
-  override predicate isSink(DataFlow::Node sink) { isInterpretedAsRegExp(sink) }
+/** Holds if `re` is a regular expression with value `pattern`. */
+predicate regexp(DataFlow::Node re, string pattern, string kind, DataFlow::Node aux) {
+  re.asExpr().(RegExpLiteral).getValue() = pattern and
+  kind = "regular expression" and
+  aux = re
+  or
+  re = regExpSource(aux) and
+  pattern = re.getStringValue() and
+  kind = "string, which is used as a regular expression $@,"
 }
 
 /**
@@ -36,22 +55,14 @@ predicate isIncompleteHostNameRegExpPattern(string pattern, string hostPart) {
             // an unescaped single `.`
             "(?<!\\\\)[.]" +
             // immediately followed by a sequence of subdomains, perhaps with some regex characters mixed in, followed by a known TLD
-            "([():|?a-z0-9-]+(\\\\)?[.](" + RegExpPatterns::commonTLD() + "))" + ".*", 1)
+            "([():|?a-z0-9-]+(\\\\)?[.]" + RegExpPatterns::commonTLD() + ")" + ".*", 1)
 }
 
-from Expr e, string pattern, string hostPart
-where
-  (
-    e.(RegExpLiteral).getValue() = pattern
-    or
-    exists(Configuration cfg |
-      cfg.hasFlow(e.flow(), _) and
-      e.mayHaveStringValue(pattern)
-    )
-  ) and
+from DataFlow::Node re, string pattern, string hostPart, string kind, DataFlow::Node aux
+where regexp(re, pattern, kind, aux) and
   isIncompleteHostNameRegExpPattern(pattern, hostPart) and
   // ignore patterns with capture groups after the TLD
   not pattern.regexpMatch("(?i).*[.](" + RegExpPatterns::commonTLD() + ").*[(][?]:.*[)].*")
-select e,
-  "This regular expression has an unescaped '.' before '" + hostPart +
-    "', so it might match more hosts than expected."
+select re,
+  "This " + kind + " has an unescaped '.' before '" + hostPart +
+    "', so it might match more hosts than expected.", aux, "here"

javascript/ql/src/Security/CWE-020/IncompleteUrlSubstringSanitization.ql

@@ -38,8 +38,8 @@ where
   (
     // target contains a domain on a common TLD, and perhaps some other URL components
     target
-        .regexpMatch("(?i)([a-z]*:?//)?\\.?([a-z0-9-]+\\.)+(" + RegExpPatterns::commonTLD() +
-            ")(:[0-9]+)?/?")
+        .regexpMatch("(?i)([a-z]*:?//)?\\.?([a-z0-9-]+\\.)+" + RegExpPatterns::commonTLD() +
+            "(:[0-9]+)?/?")
     or
     // target is a HTTP URL to a domain on any TLD
     target.regexpMatch("(?i)https?://([a-z0-9-]+\\.)+([a-z]+)(:[0-9]+)?/?")

javascript/ql/src/Security/CWE-020/examples/IncompleteHostnameRegExp.js

@@ -2,7 +2,7 @@ app.get('/some/path', function(req, res) {
     let url = req.param('url'),
         host = urlLib.parse(url).host;
     // BAD: the host of `url` may be controlled by an attacker
-    let regex = /(www|beta|).example.com/;
+    let regex = /((www|beta).)?example.com/;
     if (host.match(regex)) {
         res.redirect(url);
     }

javascript/ql/src/semmle/javascript/Regexp.qll

@@ -397,10 +397,10 @@ predicate isInterpretedAsRegExp(DataFlow::Node source) {
  */
 module RegExpPatterns {
   /**
-   * Gets a pattern that matches common top-level domain names.
+   * Gets a pattern that matches common top-level domain names in lower case.
    */
   string commonTLD() {
     // according to ranking by http://google.com/search?q=site:.<<TLD>>
-    result = "com|org|edu|gov|uk|net|io"
+    result = "(?:com|org|edu|gov|uk|net|io)(?![a-z0-9])"
   }
 }

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

@@ -179,10 +179,7 @@ class SourceNode extends DataFlow::Node {
    */
   pragma[inline]
   DataFlow::SourceNode backtrack(TypeBackTracker t2, TypeBackTracker t) {
-    exists(StepSummary summary |
-      StepSummary::step(result, this, summary) and
-      t = t2.prepend(summary)
-    )
+    t2 = t.step(result, this)
   }
 }
 

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

@@ -41,13 +41,9 @@ class StepSummary extends TStepSummary {
     or
     this instanceof ReturnStep and result = "return"
     or
-    exists(string prop | this = StoreStep(prop) |
-      result = "store " + prop
-    )
+    exists(string prop | this = StoreStep(prop) | result = "store " + prop)
     or
-    exists(string prop | this = LoadStep(prop) |
-      result = "load" + prop
-    )
+    exists(string prop | this = LoadStep(prop) | result = "load " + prop)
   }
 }
 
@@ -56,40 +52,44 @@ module StepSummary {
    * INTERNAL: Use `SourceNode.track()` or `SourceNode.backtrack()` instead.
    */
   predicate step(DataFlow::SourceNode pred, DataFlow::SourceNode succ, StepSummary summary) {
-    exists(DataFlow::Node predNode | pred.flowsTo(predNode) |
-      // Flow through properties of objects
-      propertyFlowStep(predNode, succ) and
-      summary = LevelStep()
-      or
-      // Flow through global variables
-      globalFlowStep(predNode, succ) and
-      summary = LevelStep()
-      or
-      // Flow into function
-      callStep(predNode, succ) and
-      summary = CallStep()
-      or
-      // Flow out of function
-      returnStep(predNode, succ) and
-      summary = ReturnStep()
-      or
-      // Flow through an instance field between members of the same class
-      DataFlow::localFieldStep(predNode, succ) and
-      summary = LevelStep()
+    exists(DataFlow::Node mid | pred.flowsTo(mid) | smallstep(mid, succ, summary))
+  }
+
+  /**
+   * INTERNAL: Use `TypeBackTracker.smallstep()` instead.
+   */
+  predicate smallstep(DataFlow::Node pred, DataFlow::Node succ, StepSummary summary) {
+    // Flow through properties of objects
+    propertyFlowStep(pred, succ) and
+    summary = LevelStep()
+    or
+    // Flow through global variables
+    globalFlowStep(pred, succ) and
+    summary = LevelStep()
+    or
+    // Flow into function
+    callStep(pred, succ) and
+    summary = CallStep()
+    or
+    // Flow out of function
+    returnStep(pred, succ) and
+    summary = ReturnStep()
+    or
+    // Flow through an instance field between members of the same class
+    DataFlow::localFieldStep(pred, succ) and
+    summary = LevelStep()
+    or
+    exists(string prop |
+      basicStoreStep(pred, succ, prop) and
+      summary = StoreStep(prop)
       or
-      exists(string prop |
-        basicStoreStep(predNode, succ, prop) and
-        summary = StoreStep(prop)
-        or
-        loadStep(predNode, succ, prop) and
-        summary = LoadStep(prop)
-      )
+      loadStep(pred, succ, prop) and
+      summary = LoadStep(prop)
     )
   }
 }
 
-private newtype TTypeTracker =
-  MkTypeTracker(Boolean hasCall, OptionalPropertyName prop)
+private newtype TTypeTracker = MkTypeTracker(Boolean hasCall, OptionalPropertyName prop)
 
 /**
  * EXPERIMENTAL.
@@ -136,9 +136,7 @@ class TypeTracker extends TTypeTracker {
     or
     step = LoadStep(prop) and result = MkTypeTracker(hasCall, "")
     or
-    exists(string p |
-      step = StoreStep(p) and prop = "" and result = MkTypeTracker(hasCall, p)
-    )
+    exists(string p | step = StoreStep(p) and prop = "" and result = MkTypeTracker(hasCall, p))
   }
 
   /** Gets a textual representation of this summary. */
@@ -180,8 +178,7 @@ module TypeTracker {
   TypeTracker end() { result.end() }
 }
 
-private newtype TTypeBackTracker =
-  MkTypeBackTracker(Boolean hasReturn, OptionalPropertyName prop)
+private newtype TTypeBackTracker = MkTypeBackTracker(Boolean hasReturn, OptionalPropertyName prop)
 
 /**
  * EXPERIMENTAL.
@@ -192,7 +189,7 @@ private newtype TTypeBackTracker =
  * therefore expected to called with a certain type of value.
  *
  * Note that type back-tracking does not provide a source/sink relation, that is,
- * it may determine that a node will be used in an API call somwwhere, but it won't
+ * it may determine that a node will be used in an API call somewhere, but it won't
  * determine exactly where that use was, or the path that led to the use.
  *
  * It is recommended that all uses of this type is written on the following form,
@@ -203,7 +200,7 @@ private newtype TTypeBackTracker =
  *   result = (< some API call >).getArgument(< n >).getALocalSource()
  *   or
  *   exists (DataFlow::TypeBackTracker t2 |
- *     result = myCallback(t2).backtrack(t2, t)
+ *     t2 = t.step(result, myCallback(t2))
  *   )
  * }
  *
@@ -225,9 +222,7 @@ class TypeBackTracker extends TTypeBackTracker {
     or
     step = ReturnStep() and result = MkTypeBackTracker(true, prop)
     or
-    exists(string p |
-      step = LoadStep(p) and prop = "" and result = MkTypeBackTracker(hasReturn, p)
-    )
+    exists(string p | step = LoadStep(p) and prop = "" and result = MkTypeBackTracker(hasReturn, p))
     or
     step = StoreStep(prop) and result = MkTypeBackTracker(hasReturn, "")
   }
@@ -265,6 +260,37 @@ class TypeBackTracker extends TTypeBackTracker {
    * This predicate is only defined if the type has not been tracked into a property.
    */
   TypeBackTracker continue() { prop = "" and result = this }
+
+  /**
+   * Gets the summary that corresponds to having taken a backwards
+   * heap and/or inter-procedural step from `succ` to `pred`.
+   */
+  pragma[inline]
+  TypeBackTracker step(DataFlow::SourceNode pred, DataFlow::SourceNode succ) {
+    exists(StepSummary summary |
+      StepSummary::step(pred, succ, summary) and
+      this = result.prepend(summary)
+    )
+  }
+
+  /**
+   * Gets the summary that corresponds to having taken a backwards
+   * local, heap and/or inter-procedural step from `succ` to `pred`.
+   *
+   * Unlike `TypeBackTracker::step`, this predicate exposes all edges
+   * in the flowgraph, and not just the edges between
+   * `SourceNode`s. It may therefore be less performant.
+   */
+  pragma[inline]
+  TypeBackTracker smallstep(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(StepSummary summary |
+      StepSummary::smallstep(pred, succ, summary) and
+      this = result.prepend(summary)
+    )
+    or
+    pred = succ.getAPredecessor() and
+    this = result
+  }
 }
 
 module TypeBackTracker {