diff --git a/java/ql/lib/ext/org.apache.commons.lang3.model.yml b/java/ql/lib/ext/org.apache.commons.lang3.model.yml index 7c455d780b1..2f039bf6994 100644 --- a/java/ql/lib/ext/org.apache.commons.lang3.model.yml +++ b/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"] diff --git a/java/ql/lib/semmle/code/java/regex/RegexFlowConfigs.qll b/java/ql/lib/semmle/code/java/regex/RegexFlowConfigs.qll index 4deb2bc812b..4d09a54c92f 100644 --- a/java/ql/lib/semmle/code/java/regex/RegexFlowConfigs.qll +++ b/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 ) } diff --git a/java/ql/test/query-tests/security/CWE-730/PolyRedos/PolyRedosTest.java b/java/ql/test/query-tests/security/CWE-730/PolyRedos/PolyRedosTest.java index 34e527456c5..d8c467d3d6f 100644 --- a/java/ql/test/query-tests/security/CWE-730/PolyRedos/PolyRedosTest.java +++ b/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 + } }