From 1bc73ab59244b7e560456b73ab3f94d42862f96d Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esben@semmle.com>
Date: Tue, 11 Dec 2018 12:58:21 +0100
Subject: [PATCH] JS: address review comments

---
 .../CWE-020/IncompleteHostnameRegExp.qhelp    |  4 +--
 .../CWE-020/IncompleteHostnameRegExp.ql       | 35 +++++++++----------
 .../IncompleteUrlSubstringSanitization.ql     |  2 +-
 .../ql/src/semmle/javascript/Regexp.qll       | 16 ++++++++-
 4 files changed, 34 insertions(+), 23 deletions(-)

diff --git a/javascript/ql/src/Security/CWE-020/IncompleteHostnameRegExp.qhelp b/javascript/ql/src/Security/CWE-020/IncompleteHostnameRegExp.qhelp
index d6b4f279728..0b2b7369bc1 100644
--- a/javascript/ql/src/Security/CWE-020/IncompleteHostnameRegExp.qhelp
+++ b/javascript/ql/src/Security/CWE-020/IncompleteHostnameRegExp.qhelp
@@ -8,7 +8,7 @@
 
 			Sanitizing untrusted URLs is an important technique for
 			preventing attacks such as request forgeries and malicious
-			redirections. Usually, this is done by checking that the host of a URL
+			redirections. Often, this is done by checking that the host of a URL
 			is in a set of allowed hosts.
 
 		</p>
@@ -56,7 +56,7 @@
 			an attacker-controlled domain such as <code>wwwXexample.com</code>.
 
 			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>
 
diff --git a/javascript/ql/src/Security/CWE-020/IncompleteHostnameRegExp.ql b/javascript/ql/src/Security/CWE-020/IncompleteHostnameRegExp.ql
index 49344ee8fec..abe6d76d6f0 100644
--- a/javascript/ql/src/Security/CWE-020/IncompleteHostnameRegExp.ql
+++ b/javascript/ql/src/Security/CWE-020/IncompleteHostnameRegExp.ql
@@ -12,28 +12,25 @@
 
 import javascript
 
-module IncompleteHostnameRegExpTracking {
+/**
+ * A taint tracking configuration for incomplete hostname regular expressions sources.
+ */
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "IncompleteHostnameRegExpTracking" }
 
-  /**
-   * A taint tracking configuration for incomplete hostname regular expressions sources.
-   */
-  class Configuration extends TaintTracking::Configuration {
-    Configuration() { this = "IncompleteHostnameRegExpTracking" }
-
-    override
-    predicate isSource(DataFlow::Node source) {
-      isIncompleteHostNameRegExpPattern(source.asExpr().getStringValue(), _)
-    }
-
-    override
-    predicate isSink(DataFlow::Node sink) {
-      isInterpretedAsRegExp(sink)
-    }
+  override
+  predicate isSource(DataFlow::Node source) {
+    isIncompleteHostNameRegExpPattern(source.asExpr().getStringValue(), _)
+  }
 
+  override
+  predicate isSink(DataFlow::Node sink) {
+    isInterpretedAsRegExp(sink)
   }
 
 }
 
+
 /**
  * Holds if `pattern` is a regular expression pattern for URLs with a host matched by `hostPart`,
  * and `pattern` contains a subtle mistake that allows it to match unexpected hosts.
@@ -45,7 +42,7 @@ 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-]+(\\\\)?[.](com|org|edu|gov|uk|net))" +
+    "([():|?a-z0-9-]+(\\\\)?[.](" + RegExpPatterns::commonTLD() + "))" +
     ".*", 1)
 }
 
@@ -53,7 +50,7 @@ from Expr e, string pattern, string hostPart
 where
       (
         e.(RegExpLiteral).getValue() = pattern or
-        exists (IncompleteHostnameRegExpTracking::Configuration cfg |
+        exists (Configuration cfg |
           cfg.hasFlow(e.flow(), _) and
           e.mayHaveStringValue(pattern)
         )
@@ -61,7 +58,7 @@ where
       isIncompleteHostNameRegExpPattern(pattern, hostPart)
       and
       // ignore patterns with capture groups after the TLD
-      not pattern.regexpMatch("(?i).*[.](com|org|edu|gov|uk|net).*[(][?]:.*[)].*")
+      not pattern.regexpMatch("(?i).*[.](" + RegExpPatterns::commonTLD() + ").*[(][?]:.*[)].*")
 
 
 select e, "This regular expression has an unescaped '.' before '" + hostPart + "', so it might match more hosts than expected."
diff --git a/javascript/ql/src/Security/CWE-020/IncompleteUrlSubstringSanitization.ql b/javascript/ql/src/Security/CWE-020/IncompleteUrlSubstringSanitization.ql
index 27022e468df..8ea0a9106d1 100644
--- a/javascript/ql/src/Security/CWE-020/IncompleteUrlSubstringSanitization.ql
+++ b/javascript/ql/src/Security/CWE-020/IncompleteUrlSubstringSanitization.ql
@@ -21,7 +21,7 @@ where
       substring.mayHaveStringValue(target) and
       (
         // target contains a domain on a common TLD, and perhaps some other URL components
-        target.regexpMatch("(?i)([a-z]*:?//)?\\.?([a-z0-9-]+\\.)+(com|org|edu|gov|uk|net)(:[0-9]+)?/?") or
+        target.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]+)?/?")
       ) and
diff --git a/javascript/ql/src/semmle/javascript/Regexp.qll b/javascript/ql/src/semmle/javascript/Regexp.qll
index b8a5ab6eeea..ac34ce1ad7a 100644
--- a/javascript/ql/src/semmle/javascript/Regexp.qll
+++ b/javascript/ql/src/semmle/javascript/Regexp.qll
@@ -504,8 +504,22 @@ predicate isInterpretedAsRegExp(DataFlow::Node source) {
       methodName = "search" and
       source.asExpr() = mce.getArgument(0) and
       mce.getNumArgument() = 1 and
-      // `String.prototype.search` returns a number, so exclude chained accesses
+      // "search" is a common method name, and so we exclude chained accesses
+      // because `String.prototype.search` returns a number
       not exists(PropAccess p | p.getBase() = mce)
     )
   )
 }
+
+/**
+ * Provides regular expression patterns.
+ */
+module RegExpPatterns {
+  /**
+   * Gets a pattern that matches common top-level domain names.
+   */
+  string commonTLD() {
+    // according to ranking by http://google.com/search?q=site:.<<TLD>>
+    result = "com|org|edu|gov|uk|net|io"
+  }
+}
\ No newline at end of file