Ruby: IncompleteHostnameRegExp.ql

aibaars · aibaars · commit 9e8930c19278 · 2022-03-07T16:10:08.000+01:00
diff --git a/javascript/ql/src/Security/CWE-020/IncompleteHostnameRegExp.qhelp b/javascript/ql/src/Security/CWE-020/IncompleteHostnameRegExp.qhelp
@@ -30,7 +30,7 @@
 		<p>
 
 			Escape all meta-characters appropriately when constructing
-			regular expressions for security checks, pay special attention to the
+			regular expressions for security checks, and pay special attention to the
 			<code>.</code> meta-character.
 
 		</p>
diff --git a/python/ql/src/Security/CWE-020/IncompleteHostnameRegExp.qhelp b/python/ql/src/Security/CWE-020/IncompleteHostnameRegExp.qhelp
@@ -30,7 +30,7 @@
         <p>
 
             Escape all meta-characters appropriately when constructing
-            regular expressions for security checks, pay special attention to the
+            regular expressions for security checks, and pay special attention to the
             <code>.</code> meta-character.
 
         </p>
diff --git a/ruby/ql/lib/codeql/ruby/security/performance/RegExpTreeView.qll b/ruby/ql/lib/codeql/ruby/security/performance/RegExpTreeView.qll
@@ -637,6 +637,9 @@ class RegExpGroup extends RegExpTerm, TRegExpGroup {
    */
   int getNumber() { result = re.getGroupNumber(start, end) }
 
+  /** Holds if this is a capture group. */
+  predicate isCapture() { not exists(this.getNumber()) }
+
   /** Holds if this is a named capture group. */
   predicate isNamed() { exists(this.getName()) }
 
diff --git a/ruby/ql/src/change-notes/2022-02-10-incomplete-hostname-regexp.md b/ruby/ql/src/change-notes/2022-02-10-incomplete-hostname-regexp.md
@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* Added a new query, `rb/incomplete-hostname-regexp`. The query finds instances where a hostname is incompletely sanitized due to an unescaped character in a regular expression.
diff --git a/ruby/ql/src/queries/security/cwe-020/HostnameRegexpShared.qll b/ruby/ql/src/queries/security/cwe-020/HostnameRegexpShared.qll
@@ -3,7 +3,8 @@
  * that match URLs and hostname patterns.
  */
 
-import javascript
+private import codeql.ruby.security.performance.RegExpTreeView
+private import codeql.ruby.DataFlow
 
 /**
  * Holds if the given constant is unlikely to occur in the origin part of a URL.
@@ -19,7 +20,7 @@ predicate isConstantInvalidInsideOrigin(RegExpConstant term) {
 
 /** Holds if `term` is a dot constant of form `\.` or `[.]`. */
 predicate isDotConstant(RegExpTerm term) {
-  term.(RegExpCharEscape).getValue() = "."
+  term.(RegExpEscape).getValue() = "."
   or
   exists(RegExpCharacterClass cls |
     term = cls and
diff --git a/ruby/ql/src/queries/security/cwe-020/IncompleteHostnameRegExp.qhelp b/ruby/ql/src/queries/security/cwe-020/IncompleteHostnameRegExp.qhelp
@@ -30,7 +30,7 @@
 		<p>
 
 			Escape all meta-characters appropriately when constructing
-			regular expressions for security checks, pay special attention to the
+			regular expressions for security checks, and pay special attention to the
 			<code>.</code> meta-character.
 
 		</p>
@@ -46,7 +46,7 @@
 
 		</p>
 
-		<sample src="examples/IncompleteHostnameRegExp.js"/>
+		<sample src="examples/IncompleteHostnameRegExp.rb"/>
 
 		<p>
 
@@ -59,14 +59,13 @@
 		<p>
 
 			Address this vulnerability by escaping <code>.</code>
-			appropriately: <code>let regex = /((www|beta)\.)?example\.com/</code>.
+			appropriately: <code>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://cheatsheetseries.owasp.org/cheatsheets/Unvalidated_Redirects_and_Forwards_Cheat_Sheet.html">XSS Unvalidated Redirects and Forwards Cheat Sheet</a>.</li>
 	</references>
diff --git a/ruby/ql/src/queries/security/cwe-020/IncompleteHostnameRegExp.ql b/ruby/ql/src/queries/security/cwe-020/IncompleteHostnameRegExp.ql
@@ -5,15 +5,16 @@
  * @problem.severity warning
  * @security-severity 7.8
  * @precision high
- * @id js/incomplete-hostname-regexp
+ * @id rb/incomplete-hostname-regexp
  * @tags correctness
  *       security
  *       external/cwe/cwe-020
  */
 
-import javascript
-import semmle.javascript.CharacterEscapes
 import HostnameRegexpShared
+import codeql.ruby.security.performance.RegExpTreeView
+import codeql.ruby.DataFlow
+import codeql.ruby.AST as AST
 
 /**
  * Holds if `term` occurs inside a quantifier or alternative (and thus
@@ -102,6 +103,5 @@ where
     ) else (
       kind = "regular expression" and aux = re
     )
-  ) and
-  not CharacterEscapes::hasALikelyRegExpPatternMistake(re)
+  )
 select hostSequence, "This " + kind + " " + msg, aux, "here"
diff --git a/ruby/ql/src/queries/security/cwe-020/examples/IncompleteHostnameRegExp.rb b/ruby/ql/src/queries/security/cwe-020/examples/IncompleteHostnameRegExp.rb
@@ -1,9 +1,13 @@
-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/;
-    if (host.match(regex)) {
-        res.redirect(url);
-    }
-});
+class AppController < ApplicationController
+
+    def index
+        url = params[:url]
+        host = URI(url).host
+        # BAD: the host of `url` may be controlled by an attacker
+        regex = /^((www|beta).)?example.com/
+        if host.match(regex)
+            redirect_to url
+        end
+    end
+
+end
diff --git a/ruby/ql/test/query-tests/security/cwe-020/IncompleteHostnameRegExp/IncompleteHostnameRegExp.expected b/ruby/ql/test/query-tests/security/cwe-020/IncompleteHostnameRegExp/IncompleteHostnameRegExp.expected
diff --git a/ruby/ql/test/query-tests/security/cwe-020/IncompleteHostnameRegExp/IncompleteHostnameRegExp.qlref b/ruby/ql/test/query-tests/security/cwe-020/IncompleteHostnameRegExp/IncompleteHostnameRegExp.qlref
@@ -1 +1 @@
-Security/CWE-020/IncompleteHostnameRegExp.ql
+queries/security/cwe-020/IncompleteHostnameRegExp.ql
diff --git a/ruby/ql/test/query-tests/security/cwe-020/IncompleteHostnameRegExp/hosttest.rb b/ruby/ql/test/query-tests/security/cwe-020/IncompleteHostnameRegExp/hosttest.rb
@@ -1,23 +1,22 @@
-from flask import Flask, request, redirect
-import re
+UNSAFE_REGEX1 = /(www|beta).example.com\//
+UNSAFE_REGEX2 = Regexp.compile("(www|beta).example.com/")
+UNSAFE_REGEX3 = Regexp.new("(www|beta).example.com/")
+SAFE_REGEX = /(www|beta)\.example\.com\//
 
-app = Flask(__name__)
+def unsafe
+    target = params[:target]
+    if UNSAFE_REGEX1.match(target)
+        redirect_to target
+    end
+end
 
-UNSAFE_REGEX = re.compile("(www|beta).example.com/")
-SAFE_REGEX = re.compile(r"(www|beta)\.example\.com/")
-
-@app.route('/some/path/bad')
-def unsafe(request):
-    target = request.args.get('target', '')
-    if UNSAFE_REGEX.match(target):
-        return redirect(target)
-
-@app.route('/some/path/good')
-def safe(request):
-    target = request.args.get('target', '')
-    if SAFE_REGEX.match(target):
-        return redirect(target)
+def safe
+    target = params[:target]
+    if SAFE_REGEX.match(target)
+        redirect_to target
+    end
+end
 
 # FP reported in https://github.com/github/codeql/issues/3712
 # This does not define a regex (but could be used by other code to do so)
-escaped = re.escape("https://www.humblebundle.com/home/library")
+escaped = Regexp::escape("https://www.humblebundle.com/home/library")
diff --git a/ruby/ql/test/query-tests/security/cwe-020/IncompleteHostnameRegExp/tst-IncompleteHostnameRegExp.rb b/ruby/ql/test/query-tests/security/cwe-020/IncompleteHostnameRegExp/tst-IncompleteHostnameRegExp.rb
@@ -1,60 +1,67 @@
-(function() {
-	/^http:\/\/example.com/; // OK
-	/^http:\/\/test.example.com/; // NOT OK
-	/^http:\/\/test\\.example.com/; // OK
-	/^http:\/\/test.example.net/; // NOT OK
-	/^http:\/\/test.(example-a|example-b).com/; // NOT OK
-	/^http:\/\/(.+).example.com\//; // NOT OK
-	/^http:\/\/(\\.+)\\.example.com/; // OK
-	/^http:\/\/(?:.+)\\.test\\.example.com\//; // NOT OK
-	/^http:\/\/test.example.com\/(?:.*)/; // OK
-	new RegExp("^http://test.example.com"); // NOT OK
-	if (s.match("^http://test.example.com")) {} // NOT OK
-
-	function id(e) { return e; }
-	new RegExp(id(id(id("^http://test.example.com")))); // NOT OK
-
-	new RegExp(`test.example.com$`); // NOT OK
-
-	let hostname = '^test.example.com'; // NOT OK
-	new RegExp(`${hostname}$`);
-
-	let domain = { hostname: 'test.example.com$' }; // NOT OK
-	new RegExp(domain.hostname);
-
-	function convert1(domain) {
-		return new RegExp(domain.hostname);
-	}
-	convert1({ hostname: 'test.example.com$' }); // NOT OK
-
-	let domains = [ { hostname: 'test.example.com$' } ];  // NOT OK
-	function convert2(domain) {
-		return new RegExp(domain.hostname);
-	}
-	domains.map(d => convert2(d));
-
-	/^(.+\.(?:example-a|example-b)\.com)\//; // NOT OK
-	/^(https?:)?\/\/((service|www).)?example.com(?=$|\/)/; // NOT OK
-	/^(http|https):\/\/www.example.com\/p\/f\//; // NOT OK
-	/^(http:\/\/sub.example.com\/)/g; // NOT OK
-	/^https?:\/\/api.example.com/; // NOT OK
-	new RegExp('^http://localhost:8000|' + '^https?://.+\\.example\\.com/'); // NOT OK
-	new RegExp('^http[s]?:\/\/?sub1\\.sub2\\.example\\.com\/f\/(.+)'); // NOT OK
-	/^https:\/\/[a-z]*.example.com$/; // NOT OK
-	RegExp('^protos?://(localhost|.+.example.net|.+.example-a.com|.+.example-b.com|.+.example.internal)'); // NOT OK
-
-	/^(example.dev|example.com)/; // OK
-
-	new RegExp('^http://localhost:8000|' + '^https?://.+.example\\.com/'); // NOT OK
-
-	var primary = 'example.com$';
-	new RegExp('test.' + primary); // NOT OK, but not detected
-
-	new RegExp('test.' + 'example.com$'); // NOT OK
-
-	new RegExp('^http://test\.example.com'); // NOT OK, but flagged by js/useless-regexp-character-escape
-
-	/^http:\/\/(..|...)\.example\.com\/index\.html/; // OK, wildcards are intentional
-	/^http:\/\/.\.example\.com\/index\.html/; // OK, the wildcard is intentional
-	/^(foo.example\.com|whatever)$/; // kinda OK - one disjunction doesn't even look like a hostname
-});
+def foo
+	/^http:\/\/example.com/; # OK
+	/^http:\/\/test.example.com/; # NOT OK
+	/^http:\/\/test\.example.com/; # OK
+	/^http:\/\/test.example.net/; # NOT OK
+	/^http:\/\/test.(example-a|example-b).com/; # NOT OK
+	/^http:\/\/(.+).example.com\//; # NOT OK
+	/^http:\/\/(\.+)\.example.com/; # OK
+	/^http:\/\/(?:.+)\.test\.example.com\//; # NOT OK
+	/^http:\/\/test.example.com\/(?:.*)/; # OK
+	Regexp.new("^http://test.example.com"); # NOT OK
+	if (s.match("^http://test.example.com")); end # NOT OK
+
+
+	Regexp.new(id(id(id("^http://test.example.com")))); # NOT OK
+
+	Regexp.new(`test.example.com$`); # NOT OK
+
+	hostname = '^test.example.com'; # NOT OK
+	Regexp.new("#{hostname}$");
+
+	domain = { hostname: 'test.example.com$' }; # NOT OK
+	Regexp.new(domain[:hostname]);
+
+
+
+
+	convert1({ hostname: 'test.example.com$' }); # NOT OK
+
+	domains = [ { hostname: 'test.example.com$' } ];  # NOT OK
+
+
+
+	domains.map{ |d| convert2(d) };
+
+	/^(.+\.(?:example-a|example-b)\.com)\//; # NOT OK
+	/^(https?:)?\/\/((service|www).)?example.com(?=$|\/)/; # NOT OK
+	/^(http|https):\/\/www.example.com\/p\/f\//; # NOT OK
+	/^(http:\/\/sub.example.com\/)/i; # NOT OK
+	/^https?:\/\/api.example.com/; # NOT OK
+	Regexp.new('^http://localhost:8000|' + "^https?://.+\\.example\\.com/"); # NOT OK
+	Regexp.new("^http[s]?:\/\/?sub1\\.sub2\\.example\\.com\/f\/(.+)"); # NOT OK
+	/^https:\/\/[a-z]*.example.com$/; # NOT OK
+	Regexp.compile('^protos?://(localhost|.+.example.net|.+.example-a.com|.+.example-b.com|.+.example.internal)'); # NOT OK
+
+	/^(example.dev|example.com)/; # OK
+
+	Regexp.new('^http://localhost:8000|' + "^https?://.+.example\\.com/"); # NOT OK
+
+	primary = 'example.com$';
+	Regexp.new('test.' + primary); # NOT OK, but not detected
+
+	Regexp.new('test.' + 'example.com$'); # NOT OK
+
+	Regexp.new('^http://test\.example.com'); # NOT OK
+
+	/^http:\/\/(..|...)\.example\.com\/index\.html/; # OK, wildcards are intentional
+	/^http:\/\/.\.example\.com\/index\.html/; # OK, the wildcard is intentional
+	/^(foo.example\.com|whatever)$/; # kinda OK - one disjunction doesn't even look like a hostname
+end
+def id(e); return e; end
+def convert1(domain)
+	return Regexp.new(domain[:hostname]);
+end
+def convert2(domain)
+	return Regexp.new(domain[:hostname]);
+end

-Original file line number
+Diff line change
@@ @@ -0,0 +1,4 @@ @@
 +---
 +category: newQuery
 +---
 +* Added a new query, `rb/incomplete-hostname-regexp`. The query finds instances where a hostname is incompletely sanitized due to an unescaped character in a regular expression.
Original file line number	Diff line number	Diff line change
`@@ -1 +1 @@`
`1`		`-Security/CWE-020/IncompleteHostnameRegExp.ql`
	`1`	`+queries/security/cwe-020/IncompleteHostnameRegExp.ql`