Refactor regex-use sink kind to support polynomial ReDoS query

- Extended regexSinkKindInfo to handle plain "regex-use" sink kind
- Plain "regex-use" defaults to non-full-string matching with string arg at index 0
- Updated comment in org.apache.commons.lang3.model.yml
- Added test cases for RegExUtils methods in polynomial ReDoS test

Co-authored-by: owen-mc <62447351+owen-mc@users.noreply.github.com>

java/ql/lib/ext/org.apache.commons.lang3.model.yml

@@ -4,9 +4,9 @@ extensions:
       extensible: sinkModel
     data:
       - ["org.apache.commons.lang3", "SerializationUtils", False, "deserialize", "", "", "Argument[0]", "unsafe-deserialization", "manual"]
-      # Note these sinks do not use the sink kind `regex-use[0]` because the regex injection query needs to select them separately from
-      # other `regex-use[0]` sinks in order to avoid FPs. As a result, these sinks are currently not used in the polynomial ReDoS query.
-      # TODO: refactor the `regex-use%` sink kind so that the polynomial ReDoS query can also use these sinks.
+      # Note these sinks use the sink kind `regex-use` (without brackets) because the regex injection query needs to select them
+      # separately from other `regex-use[0]` sinks in order to avoid FPs. The polynomial ReDoS query handles the plain `regex-use`
+      # sink kind by treating it as `regex-use[0]` (non-full-string matching with the string argument at index 0).
       - ["org.apache.commons.lang3", "RegExUtils", False, "removeAll", "(String,String)", "", "Argument[1]", "regex-use", "manual"]
       - ["org.apache.commons.lang3", "RegExUtils", False, "removeFirst", "(String,String)", "", "Argument[1]", "regex-use", "manual"]
       - ["org.apache.commons.lang3", "RegExUtils", False, "removePattern", "(String,String)", "", "Argument[1]", "regex-use", "manual"]

java/ql/lib/semmle/code/java/regex/RegexFlowConfigs.qll

@@ -20,29 +20,32 @@ private class ExploitableStringLiteral extends StringLiteral {
  * `strArg` is the index of the argument to methods with this sink kind that
  * contain the string to be matched against, where -1 is the qualifier; or -2
  * if no such argument exists.
- *
- * Note that `regex-use` is deliberately not a possible value for `kind` here,
- * as it is used for regular expression injection sinks that need to be selected
- * separately from existing `regex-use[0]` sinks.
- * TODO: refactor the `regex-use%` sink kind so that the polynomial ReDoS query
- * can also use the `regex-use` sinks.
  */
 private predicate regexSinkKindInfo(string kind, boolean full, int strArg) {
   sinkModel(_, _, _, _, _, _, _, kind, _, _) and
-  exists(string fullStr, string strArgStr |
-    (
-      full = true and fullStr = "f"
-      or
-      full = false and fullStr = ""
-    ) and
-    (
-      strArgStr.toInt() = strArg
-      or
-      strArg = -2 and
-      strArgStr = ""
+  (
+    exists(string fullStr, string strArgStr |
+      (
+        full = true and fullStr = "f"
+        or
+        full = false and fullStr = ""
+      ) and
+      (
+        strArgStr.toInt() = strArg
+        or
+        strArg = -2 and
+        strArgStr = ""
+      )
+    |
+      kind = "regex-use[" + fullStr + strArgStr + "]"
     )
-  |
-    kind = "regex-use[" + fullStr + strArgStr + "]"
+    or
+    // Plain `regex-use` sinks default to non-full-string matching with the string argument at index 0.
+    // This is primarily used for methods like `RegExUtils.removeAll(text, regex)` where both the
+    // pattern and the input are provided in the same call.
+    kind = "regex-use" and
+    full = false and
+    strArg = 0
   )
 }
 

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

@@ -35,6 +35,11 @@ private class DefaultIntentRedirectionSink extends IntentRedirectionSink {
   DefaultIntentRedirectionSink() { sinkNode(this, "intent-redirection") }
 }
 
+/** External sanitizers for Intent redirection vulnerabilities. */
+private class ExternalIntentRedirectionSanitizer extends IntentRedirectionSanitizer {
+  ExternalIntentRedirectionSanitizer() { barrierNode(this, "intent-redirection") }
+}
+
 /**
  * A default sanitizer for `Intent` nodes dominated by calls to `ComponentName.getPackageName`
  * and `ComponentName.getClassName`. These are used to check whether the origin or destination

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

@@ -37,6 +37,10 @@ private class DefaultCommandInjectionSink extends CommandInjectionSink {
   DefaultCommandInjectionSink() { sinkNode(this, "command-injection") }
 }
 
+private class ExternalCommandInjectionSanitizer extends CommandInjectionSanitizer {
+  ExternalCommandInjectionSanitizer() { barrierNode(this, "command-injection") }
+}
+
 private class DefaultCommandInjectionSanitizer extends CommandInjectionSanitizer {
   DefaultCommandInjectionSanitizer() {
     this instanceof SimpleTypeSanitizer

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

@@ -49,6 +49,15 @@ private class DefaultFragmentInjectionSink extends FragmentInjectionSink {
   DefaultFragmentInjectionSink() { sinkNode(this, "fragment-injection") }
 }
 
+/**
+ * A barrier for Fragment injection vulnerabilities.
+ */
+abstract class FragmentInjectionSanitizer extends DataFlow::Node { }
+
+private class ExternalFragmentInjectionSanitizer extends FragmentInjectionSanitizer {
+  ExternalFragmentInjectionSanitizer() { barrierNode(this, "fragment-injection") }
+}
+
 private class DefaultFragmentInjectionAdditionalTaintStep extends FragmentInjectionAdditionalTaintStep
 {
   override predicate step(DataFlow::Node n1, DataFlow::Node n2) {

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

@@ -14,6 +14,8 @@ module FragmentInjectionTaintConfig implements DataFlow::ConfigSig {
 
   predicate isSink(DataFlow::Node sink) { sink instanceof FragmentInjectionSink }
 
+  predicate isBarrier(DataFlow::Node node) { node instanceof FragmentInjectionSanitizer }
+
   predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
     any(FragmentInjectionAdditionalTaintStep c).step(n1, n2)
   }

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

@@ -26,6 +26,13 @@ private class DefaultGroovyInjectionSink extends GroovyInjectionSink {
   DefaultGroovyInjectionSink() { sinkNode(this, "groovy-injection") }
 }
 
+/** A data flow sanitizer for Groovy expression injection vulnerabilities. */
+abstract class GroovyInjectionSanitizer extends DataFlow::ExprNode { }
+
+private class ExternalGroovyInjectionSanitizer extends GroovyInjectionSanitizer {
+  ExternalGroovyInjectionSanitizer() { barrierNode(this, "groovy-injection") }
+}
+
 /** A set of additional taint steps to consider when taint tracking Groovy related data flows. */
 private class DefaultGroovyInjectionAdditionalTaintStep extends GroovyInjectionAdditionalTaintStep {
   override predicate step(DataFlow::Node node1, DataFlow::Node node2) {

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

@@ -14,6 +14,8 @@ module GroovyInjectionConfig implements DataFlow::ConfigSig {
 
   predicate isSink(DataFlow::Node sink) { sink instanceof GroovyInjectionSink }
 
+  predicate isBarrier(DataFlow::Node node) { node instanceof GroovyInjectionSanitizer }
+
   predicate isAdditionalFlowStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
     any(GroovyInjectionAdditionalTaintStep c).step(fromNode, toNode)
   }

java/ql/test/query-tests/security/CWE-730/PolyRedos/PolyRedosTest.java

@@ -2,6 +2,7 @@ import java.util.regex.Pattern;
 import java.util.function.Predicate;
 import javax.servlet.http.HttpServletRequest;
 import com.google.common.base.Splitter;
+import org.apache.commons.lang3.RegExUtils;
 
 class PolyRedosTest {
     void test(HttpServletRequest request) {
@@ -81,4 +82,16 @@ class PolyRedosTest {
         p.matcher(request.getRequestURI()).matches();
         p.matcher(request.getCookies()[0].getName()).matches();
     }
+
+    void test7(HttpServletRequest request) {
+        String tainted = request.getParameter("inp"); // $ Source
+        String reg = "0\\.\\d+E?\\d+!";
+
+        RegExUtils.removeAll(tainted, reg); // $ Alert
+        RegExUtils.removeFirst(tainted, reg); // $ Alert
+        RegExUtils.removePattern(tainted, reg); // $ Alert
+        RegExUtils.replaceAll(tainted, reg, "replacement"); // $ Alert
+        RegExUtils.replaceFirst(tainted, reg, "replacement"); // $ Alert
+        RegExUtils.replacePattern(tainted, reg, "replacement"); // $ Alert
+    }
 }