add Pattern.quote sanitizer

Author: Jami Cogswell

java/ql/lib/semmle/code/java/security/RegexInjectionQuery.qll

@@ -11,25 +11,26 @@ class RegexSink extends DataFlow::ExprNode {
       (
         m.getDeclaringType() instanceof TypeString and
         (
-          ma.getArgument(0) = this.asExpr() and
-          // TODO: confirm if more/less than the below need to be handled
+          ma.getArgument(0) = this.asExpr() and // ! combine this line with the below at least? e.g. TypeString and TypePattern both use it
+          // ! test below more?
+          // ! (are there already classes for these methods in a regex library?)
           m.hasName(["matches", "split", "replaceFirst", "replaceAll"])
         )
         or
-        // TODO: review Java Pattern API
+        // ! make class for the below? (is there already a class for this and its methods in a regex library?)
         m.getDeclaringType().hasQualifiedName("java.util.regex", "Pattern") and
         (
           ma.getArgument(0) = this.asExpr() and
-          // TODO: confirm if more/less than the below need to be handled
+          // ! look into further: Pattern.matcher, .pattern() and .toString() as taint steps, .split and .splitAsStream
           m.hasName(["compile", "matches"])
         )
         or
-        // TODO: read docs about regex APIs in Java
+        // ! make class for the below? (is there already a class for this and its methods in a regex library?)
         m.getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "RegExUtils") and
         (
           ma.getArgument(1) = this.asExpr() and
           m.getParameterType(1) instanceof TypeString and
-          // TODO: confirm if more/less than the below need to be handled
+          // ! test below more?
           m.hasName([
               "removeAll", "removeFirst", "removePattern", "replaceAll", "replaceFirst",
               "replacePattern"
@@ -40,24 +41,38 @@ class RegexSink extends DataFlow::ExprNode {
   }
 }
 
-// TODO: is this abstract class needed? Are there pre-existing sanitizer classes that can be used instead?
+// ! keep and rename to RegexInjectionSanitizer IF makes sense to have two sanitizers extending it?;
+// ! else, ask Tony/others about if stylistically better to keep it (see default example in LogInjection.qll, etc.)
+// ! maybe make abstract classes for source and sink as well (if you do this, mention it in PR description as an attempt to be similar to the other languages' implementations)
 abstract class Sanitizer extends DataFlow::ExprNode { }
 
 /**
  * A call to a function whose name suggests that it escapes regular
  * expression meta-characters.
  */
+// ! rename as DefaultRegexInjectionSanitizer?
 class RegExpSanitizationCall extends Sanitizer {
   RegExpSanitizationCall() {
     exists(string calleeName, string sanitize, string regexp |
       calleeName = this.asExpr().(Call).getCallee().getName() and
+      // ! add test case for sanitize? I think current tests only check escape
       sanitize = "(?:escape|saniti[sz]e)" and // TODO: confirm this is sufficient
       regexp = "regexp?" // TODO: confirm this is sufficient
     |
       calleeName
           .regexpMatch("(?i)(" + sanitize + ".*" + regexp + ".*)" + "|(" + regexp + ".*" + sanitize +
               ".*)") // TODO: confirm this is sufficient
     )
+    or
+    // adds Pattern.quote() as a sanitizer
+    // see https://rules.sonarsource.com/java/RSPEC-2631 and https://sensei.securecodewarrior.com/recipes/scw:java:regex-injection
+    exists(MethodAccess ma, Method m | m = ma.getMethod() |
+      m.getDeclaringType().hasQualifiedName("java.util.regex", "Pattern") and
+      (
+        ma.getArgument(0) = this.asExpr() and
+        m.hasName("quote")
+      )
+    )
   }
 }
 

java/ql/test/query-tests/security/CWE-730/RegexInjectionTest.java

@@ -44,7 +44,7 @@ public boolean pattern1(javax.servlet.http.HttpServletRequest request) {
     Pattern pt = Pattern.compile(pattern); // $ hasRegexInjection
     Matcher matcher = pt.matcher(input);
 
-    return matcher.find();  // BAD // ! double-check that line 44 is the correct location for this alert (seems like it based on the old .expected file)
+    return matcher.find();
   }
 
   public boolean pattern2(javax.servlet.http.HttpServletRequest request) {
@@ -76,14 +76,15 @@ public boolean pattern5(javax.servlet.http.HttpServletRequest request) {
     String pattern = request.getParameter("pattern");
     String input = request.getParameter("input");
 
-    // GOOD: User input is sanitized before constructing the regex
+    // Safe: User input is sanitized before constructing the regex
     return input.matches("^" + escapeSpecialRegexChars(pattern) + "=.*$");
   }
 
   public boolean pattern6(javax.servlet.http.HttpServletRequest request) {
     String pattern = request.getParameter("pattern");
     String input = request.getParameter("input");
 
+    // TODO: add a "Safe" test for situation when this return value is stored in a variable, then the variable is used in the regex
     escapeSpecialRegexChars(pattern);
 
     // BAD: the pattern is not really sanitized
@@ -92,6 +93,7 @@ public boolean pattern6(javax.servlet.http.HttpServletRequest request) {
 
   Pattern SPECIAL_REGEX_CHARS = Pattern.compile("[{}()\\[\\]><-=!.+*?^$\\\\|]");
 
+  // TODO: check if any other inbuilt escape/sanitizers in Java APIs
   String escapeSpecialRegexChars(String str) {
     return SPECIAL_REGEX_CHARS.matcher(str).replaceAll("\\\\$0");
   }
@@ -136,7 +138,7 @@ public boolean apache6(javax.servlet.http.HttpServletRequest request) {
     String input = request.getParameter("input");
 
     Pattern pt = (Pattern)(Object) pattern;
-    return RegExUtils.replaceFirst(input, pt, "").length() > 0;  // GOOD, Pattern compile is the sink instead
+    return RegExUtils.replaceFirst(input, pt, "").length() > 0;  // Safe: Pattern compile is the sink instead
   }
 
   public boolean apache7(javax.servlet.http.HttpServletRequest request) {
@@ -145,4 +147,14 @@ public boolean apache7(javax.servlet.http.HttpServletRequest request) {
 
     return RegExUtils.replacePattern(input, pattern, "").length() > 0;  // $ hasRegexInjection
   }
+
+  // from https://rules.sonarsource.com/java/RSPEC-2631 to test for Pattern.quote as safe
+  public boolean validate(javax.servlet.http.HttpServletRequest request) {
+    String regex = request.getParameter("regex");
+    String input = request.getParameter("input");
+
+    return input.matches(Pattern.quote(regex));  // Safe: with Pattern.quote, metacharacters or escape sequences will be given no special meaning
+  }
 }
+
+// ! see the following for potential additional test case ideas: https://www.baeldung.com/regular-expressions-java