From 29e320627fcf1e34126f0070e021b4edd9e5a72a Mon Sep 17 00:00:00 2001 From: edvraa <80588099+edvraa@users.noreply.github.com> Date: Fri, 16 Apr 2021 23:19:29 +0300 Subject: [PATCH 1/7] Regex injection --- .../Security/CWE/CWE-730/RegexInjection.java | 38 ++++++++ .../Security/CWE/CWE-730/RegexInjection.qhelp | 48 +++++++++++ .../Security/CWE/CWE-730/RegexInjection.ql | 72 ++++++++++++++++ .../security/CWE-730/RegexInjection.expected | 39 +++++++++ .../security/CWE-730/RegexInjection.java | 86 +++++++++++++++++++ .../security/CWE-730/RegexInjection.qlref | 1 + .../query-tests/security/CWE-730/options | 1 + 7 files changed, 285 insertions(+) create mode 100644 java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.java create mode 100644 java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.qhelp create mode 100644 java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql create mode 100644 java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.expected create mode 100644 java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.java create mode 100644 java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.qlref create mode 100644 java/ql/test/experimental/query-tests/security/CWE-730/options diff --git a/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.java b/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.java new file mode 100644 index 000000000000..387648a443e6 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.java @@ -0,0 +1,38 @@ +package com.example.demo; + +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.RequestParam; +import org.springframework.web.bind.annotation.RestController; + +@RestController +public class DemoApplication { + + @GetMapping("/string1") + public String string1(@RequestParam(value = "input", defaultValue = "test") String input, + @RequestParam(value = "pattern", defaultValue = ".*") String pattern) { + // BAD: Unsanitized user input is used to construct a regular expression + if (input.matches("^" + pattern + "=.*$")) + return "match!"; + + return "doesn't match!"; + } + + @GetMapping("/string2") + public String string2(@RequestParam(value = "input", defaultValue = "test") String input, + @RequestParam(value = "pattern", defaultValue = ".*") String pattern) { + // GOOD: User input is sanitized before constructing the regex + if (input.matches("^" + escapeSpecialRegexChars(pattern) + "=.*$")) + return "match!"; + + return "doesn't match!"; + } + + Pattern SPECIAL_REGEX_CHARS = Pattern.compile("[{}()\\[\\]><-=!.+*?^$\\\\|]"); + + String escapeSpecialRegexChars(String str) { + return SPECIAL_REGEX_CHARS.matcher(str).replaceAll("\\\\$0"); + } +} \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.qhelp b/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.qhelp new file mode 100644 index 000000000000..6cd80b52f755 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.qhelp @@ -0,0 +1,48 @@ + + + + +

+Constructing a regular expression with unsanitized user input is dangerous as a malicious user may +be able to modify the meaning of the expression. In particular, such a user may be able to provide +a regular expression fragment that takes exponential time in the worst case, and use that to +perform a Denial of Service attack. +

+ + + +

+Before embedding user input into a regular expression, use a sanitization function +to escape meta-characters that have special meaning. +

+ + + +

+The following example shows a HTTP request parameter that is used to construct a regular expression: +

+ +

+In the first case the user-provided regex is not escaped. +If a malicious user provides a regex that has exponential worst case performance, +then this could lead to a Denial of Service. +

+

+In the second case, the user input is escaped using escapeSpecialRegexChars before being included +in the regular expression. This ensures that the user cannot insert characters which have a special +meaning in regular expressions. +

+ + + +
  • +OWASP: +Regular expression Denial of Service - ReDoS. +
    • +
  • +Wikipedia: ReDoS. +
    • +     +     diff --git a/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql b/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql new file mode 100644 index 000000000000..58d0d81d6157 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql @@ -0,0 +1,72 @@ +/** + * @name Regular expression injection + * @description User input should not be used in regular expressions without first being sanitized, + * otherwise a malicious user may be able to provide a regex that could require + * exponential time on certain inputs. + * @kind path-problem + * @problem.severity error + * @precision high + * @id java/regex-injection + * @tags security + * external/cwe/cwe-730 + * external/cwe/cwe-400 + */ + +import java +import semmle.code.java.dataflow.FlowSources +import semmle.code.java.dataflow.TaintTracking +import DataFlow::PathGraph + +class RegexSink extends DataFlow::ExprNode { + RegexSink() { + exists(MethodAccess ma, Method m | m = ma.getMethod() | + ( + ma.getArgument(0) = this.asExpr() and + ( + m.getDeclaringType().hasQualifiedName("java.lang", "String") and + ( + m.hasName("matches") or + m.hasName("split") or + m.hasName("replaceFirst") or + m.hasName("replaceAll") + ) + or + m.getDeclaringType().hasQualifiedName("java.util.regex", "Pattern") and + ( + m.hasName("compile") or + m.hasName("matches") + ) + ) + ) + ) + } +} + +abstract class Sanitizer extends DataFlow::ExprNode { } + +class RegExpSanitizationCall extends Sanitizer { + RegExpSanitizationCall() { + exists(string calleeName, string sanitize, string regexp | + calleeName = this.asExpr().(Call).getCallee().getName() and + sanitize = "(?:escape|saniti[sz]e)" and + regexp = "regexp?" + | + calleeName.regexpMatch("(?i)(" + sanitize + ".*" + regexp + ".*)" + "|(" + regexp + ".*" + sanitize + ".*)") + ) + } +} + +class RegexInjectionConfiguration extends TaintTracking::Configuration { + RegexInjectionConfiguration() { this = "RegexInjectionConfiguration" } + + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + + override predicate isSink(DataFlow::Node sink) { sink instanceof RegexSink } + + override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer } +} + +from DataFlow::PathNode source, DataFlow::PathNode sink, RegexInjectionConfiguration c +where c.hasFlowPath(source, sink) +select sink.getNode(), source, sink, "$@ is user controlled.", source.getNode(), + "This regular expression pattern" diff --git a/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.expected b/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.expected new file mode 100644 index 000000000000..521a45d3d47a --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.expected @@ -0,0 +1,39 @@ +edges +| RegexInjection.java:11:22:11:52 | getParameter(...) : String | RegexInjection.java:14:26:14:47 | ... + ... | +| RegexInjection.java:18:22:18:52 | getParameter(...) : String | RegexInjection.java:21:24:21:30 | pattern | +| RegexInjection.java:25:22:25:52 | getParameter(...) : String | RegexInjection.java:28:31:28:37 | pattern | +| RegexInjection.java:32:22:32:52 | getParameter(...) : String | RegexInjection.java:35:29:35:35 | pattern | +| RegexInjection.java:39:22:39:52 | getParameter(...) : String | RegexInjection.java:42:34:42:40 | pattern | +| RegexInjection.java:49:22:49:52 | getParameter(...) : String | RegexInjection.java:52:28:52:34 | pattern | +| RegexInjection.java:56:22:56:52 | getParameter(...) : String | RegexInjection.java:59:28:59:34 | pattern | +| RegexInjection.java:63:22:63:52 | getParameter(...) : String | RegexInjection.java:66:36:66:42 | pattern : String | +| RegexInjection.java:66:32:66:43 | foo(...) : String | RegexInjection.java:66:26:66:52 | ... + ... | +| RegexInjection.java:66:36:66:42 | pattern : String | RegexInjection.java:66:32:66:43 | foo(...) : String | +nodes +| RegexInjection.java:11:22:11:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:14:26:14:47 | ... + ... | semmle.label | ... + ... | +| RegexInjection.java:18:22:18:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:21:24:21:30 | pattern | semmle.label | pattern | +| RegexInjection.java:25:22:25:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:28:31:28:37 | pattern | semmle.label | pattern | +| RegexInjection.java:32:22:32:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:35:29:35:35 | pattern | semmle.label | pattern | +| RegexInjection.java:39:22:39:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:42:34:42:40 | pattern | semmle.label | pattern | +| RegexInjection.java:49:22:49:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:52:28:52:34 | pattern | semmle.label | pattern | +| RegexInjection.java:56:22:56:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:59:28:59:34 | pattern | semmle.label | pattern | +| RegexInjection.java:63:22:63:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:66:26:66:52 | ... + ... | semmle.label | ... + ... | +| RegexInjection.java:66:32:66:43 | foo(...) : String | semmle.label | foo(...) : String | +| RegexInjection.java:66:36:66:42 | pattern : String | semmle.label | pattern : String | +#select +| RegexInjection.java:14:26:14:47 | ... + ... | RegexInjection.java:11:22:11:52 | getParameter(...) : String | RegexInjection.java:14:26:14:47 | ... + ... | $@ is user controlled. | RegexInjection.java:11:22:11:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:21:24:21:30 | pattern | RegexInjection.java:18:22:18:52 | getParameter(...) : String | RegexInjection.java:21:24:21:30 | pattern | $@ is user controlled. | RegexInjection.java:18:22:18:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:28:31:28:37 | pattern | RegexInjection.java:25:22:25:52 | getParameter(...) : String | RegexInjection.java:28:31:28:37 | pattern | $@ is user controlled. | RegexInjection.java:25:22:25:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:35:29:35:35 | pattern | RegexInjection.java:32:22:32:52 | getParameter(...) : String | RegexInjection.java:35:29:35:35 | pattern | $@ is user controlled. | RegexInjection.java:32:22:32:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:42:34:42:40 | pattern | RegexInjection.java:39:22:39:52 | getParameter(...) : String | RegexInjection.java:42:34:42:40 | pattern | $@ is user controlled. | RegexInjection.java:39:22:39:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:52:28:52:34 | pattern | RegexInjection.java:49:22:49:52 | getParameter(...) : String | RegexInjection.java:52:28:52:34 | pattern | $@ is user controlled. | RegexInjection.java:49:22:49:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:59:28:59:34 | pattern | RegexInjection.java:56:22:56:52 | getParameter(...) : String | RegexInjection.java:59:28:59:34 | pattern | $@ is user controlled. | RegexInjection.java:56:22:56:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:66:26:66:52 | ... + ... | RegexInjection.java:63:22:63:52 | getParameter(...) : String | RegexInjection.java:66:26:66:52 | ... + ... | $@ is user controlled. | RegexInjection.java:63:22:63:52 | getParameter(...) | This regular expression pattern | diff --git a/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.java b/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.java new file mode 100644 index 000000000000..c203360e105b --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.java @@ -0,0 +1,86 @@ +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +import javax.servlet.http.HttpServlet; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import javax.servlet.ServletException; + +public class RegexInjection extends HttpServlet { + public boolean string1(javax.servlet.http.HttpServletRequest request) { + String pattern = request.getParameter("pattern"); + String input = request.getParameter("input"); + + return input.matches("^" + pattern + "=.*$"); // BAD + } + + public boolean string2(javax.servlet.http.HttpServletRequest request) { + String pattern = request.getParameter("pattern"); + String input = request.getParameter("input"); + + return input.split(pattern).length > 0; // BAD + } + + public boolean string3(javax.servlet.http.HttpServletRequest request) { + String pattern = request.getParameter("pattern"); + String input = request.getParameter("input"); + + return input.replaceFirst(pattern, "").length() > 0; // BAD + } + + public boolean string4(javax.servlet.http.HttpServletRequest request) { + String pattern = request.getParameter("pattern"); + String input = request.getParameter("input"); + + return input.replaceAll(pattern, "").length() > 0; // BAD + } + + public boolean pattern1(javax.servlet.http.HttpServletRequest request) { + String pattern = request.getParameter("pattern"); + String input = request.getParameter("input"); + + Pattern pt = Pattern.compile(pattern); + Matcher matcher = pt.matcher(input); + + return matcher.find(); // BAD + } + + public boolean pattern2(javax.servlet.http.HttpServletRequest request) { + String pattern = request.getParameter("pattern"); + String input = request.getParameter("input"); + + return Pattern.compile(pattern).matcher(input).matches(); // BAD + } + + public boolean pattern3(javax.servlet.http.HttpServletRequest request) { + String pattern = request.getParameter("pattern"); + String input = request.getParameter("input"); + + return Pattern.matches(pattern, input); // BAD + } + + public boolean pattern4(javax.servlet.http.HttpServletRequest request) { + String pattern = request.getParameter("pattern"); + String input = request.getParameter("input"); + + return input.matches("^" + foo(pattern) + "=.*$"); // BAD + } + + String foo(String str) { + return str; + } + + 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 + return input.matches("^" + escapeSpecialRegexChars(pattern) + "=.*$"); + } + + Pattern SPECIAL_REGEX_CHARS = Pattern.compile("[{}()\\[\\]><-=!.+*?^$\\\\|]"); + + String escapeSpecialRegexChars(String str) { + return SPECIAL_REGEX_CHARS.matcher(str).replaceAll("\\\\$0"); + } +} \ No newline at end of file diff --git a/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.qlref b/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.qlref new file mode 100644 index 000000000000..dca594b38d22 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.qlref @@ -0,0 +1 @@ +experimental/Security/CWE/CWE-730/RegexInjection.ql \ No newline at end of file diff --git a/java/ql/test/experimental/query-tests/security/CWE-730/options b/java/ql/test/experimental/query-tests/security/CWE-730/options new file mode 100644 index 000000000000..dd125ad0f4ef --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-730/options @@ -0,0 +1 @@ +// semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4 \ No newline at end of file From 13655b5d80cda15a22eb49941be2a3ddc50e6969 Mon Sep 17 00:00:00 2001 From: edvraa <80588099+edvraa@users.noreply.github.com> Date: Wed, 21 Apr 2021 13:08:35 +0300 Subject: [PATCH 2/7] Add RegExUtils --- .../Security/CWE/CWE-730/RegexInjection.ql | 29 +++++- .../security/CWE-730/RegexInjection.expected | 96 ++++++++++++------- .../security/CWE-730/RegexInjection.java | 54 ++++++++++- .../query-tests/security/CWE-730/options | 2 +- 4 files changed, 138 insertions(+), 43 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql b/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql index 58d0d81d6157..9a96805c7e5b 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql @@ -21,22 +21,39 @@ class RegexSink extends DataFlow::ExprNode { RegexSink() { exists(MethodAccess ma, Method m | m = ma.getMethod() | ( - ma.getArgument(0) = this.asExpr() and + m.getDeclaringType().hasQualifiedName("java.lang", "String") and ( - m.getDeclaringType().hasQualifiedName("java.lang", "String") and + ma.getArgument(0) = this.asExpr() and ( m.hasName("matches") or m.hasName("split") or m.hasName("replaceFirst") or m.hasName("replaceAll") ) - or - m.getDeclaringType().hasQualifiedName("java.util.regex", "Pattern") and + ) + or + m.getDeclaringType().hasQualifiedName("java.util.regex", "Pattern") and + ( + ma.getArgument(0) = this.asExpr() and ( m.hasName("compile") or m.hasName("matches") ) ) + or + m.getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "RegExUtils") and + ( + ma.getArgument(1) = this.asExpr() and + m.getParameterType(1).(Class).hasQualifiedName("java.lang", "String") and + ( + m.hasName("removeAll") or + m.hasName("removeFirst") or + m.hasName("removePattern") or + m.hasName("replaceAll") or + m.hasName("replaceFirst") or + m.hasName("replacePattern") + ) + ) ) ) } @@ -51,7 +68,9 @@ class RegExpSanitizationCall extends Sanitizer { sanitize = "(?:escape|saniti[sz]e)" and regexp = "regexp?" | - calleeName.regexpMatch("(?i)(" + sanitize + ".*" + regexp + ".*)" + "|(" + regexp + ".*" + sanitize + ".*)") + calleeName + .regexpMatch("(?i)(" + sanitize + ".*" + regexp + ".*)" + "|(" + regexp + ".*" + sanitize + + ".*)") ) } } diff --git a/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.expected b/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.expected index 521a45d3d47a..93961372d759 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.expected @@ -1,39 +1,63 @@ edges -| RegexInjection.java:11:22:11:52 | getParameter(...) : String | RegexInjection.java:14:26:14:47 | ... + ... | -| RegexInjection.java:18:22:18:52 | getParameter(...) : String | RegexInjection.java:21:24:21:30 | pattern | -| RegexInjection.java:25:22:25:52 | getParameter(...) : String | RegexInjection.java:28:31:28:37 | pattern | -| RegexInjection.java:32:22:32:52 | getParameter(...) : String | RegexInjection.java:35:29:35:35 | pattern | -| RegexInjection.java:39:22:39:52 | getParameter(...) : String | RegexInjection.java:42:34:42:40 | pattern | -| RegexInjection.java:49:22:49:52 | getParameter(...) : String | RegexInjection.java:52:28:52:34 | pattern | -| RegexInjection.java:56:22:56:52 | getParameter(...) : String | RegexInjection.java:59:28:59:34 | pattern | -| RegexInjection.java:63:22:63:52 | getParameter(...) : String | RegexInjection.java:66:36:66:42 | pattern : String | -| RegexInjection.java:66:32:66:43 | foo(...) : String | RegexInjection.java:66:26:66:52 | ... + ... | -| RegexInjection.java:66:36:66:42 | pattern : String | RegexInjection.java:66:32:66:43 | foo(...) : String | +| RegexInjection.java:13:22:13:52 | getParameter(...) : String | RegexInjection.java:16:26:16:47 | ... + ... | +| RegexInjection.java:20:22:20:52 | getParameter(...) : String | RegexInjection.java:23:24:23:30 | pattern | +| RegexInjection.java:27:22:27:52 | getParameter(...) : String | RegexInjection.java:30:31:30:37 | pattern | +| RegexInjection.java:34:22:34:52 | getParameter(...) : String | RegexInjection.java:37:29:37:35 | pattern | +| RegexInjection.java:41:22:41:52 | getParameter(...) : String | RegexInjection.java:44:34:44:40 | pattern | +| RegexInjection.java:51:22:51:52 | getParameter(...) : String | RegexInjection.java:54:28:54:34 | pattern | +| RegexInjection.java:58:22:58:52 | getParameter(...) : String | RegexInjection.java:61:28:61:34 | pattern | +| RegexInjection.java:65:22:65:52 | getParameter(...) : String | RegexInjection.java:68:36:68:42 | pattern : String | +| RegexInjection.java:68:32:68:43 | foo(...) : String | RegexInjection.java:68:26:68:52 | ... + ... | +| RegexInjection.java:68:36:68:42 | pattern : String | RegexInjection.java:68:32:68:43 | foo(...) : String | +| RegexInjection.java:90:22:90:52 | getParameter(...) : String | RegexInjection.java:93:40:93:46 | pattern | +| RegexInjection.java:97:22:97:52 | getParameter(...) : String | RegexInjection.java:100:42:100:48 | pattern | +| RegexInjection.java:104:22:104:52 | getParameter(...) : String | RegexInjection.java:107:44:107:50 | pattern | +| RegexInjection.java:111:22:111:52 | getParameter(...) : String | RegexInjection.java:114:41:114:47 | pattern | +| RegexInjection.java:118:22:118:52 | getParameter(...) : String | RegexInjection.java:121:43:121:49 | pattern | +| RegexInjection.java:133:22:133:52 | getParameter(...) : String | RegexInjection.java:136:45:136:51 | pattern | nodes -| RegexInjection.java:11:22:11:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| RegexInjection.java:14:26:14:47 | ... + ... | semmle.label | ... + ... | -| RegexInjection.java:18:22:18:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| RegexInjection.java:21:24:21:30 | pattern | semmle.label | pattern | -| RegexInjection.java:25:22:25:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| RegexInjection.java:28:31:28:37 | pattern | semmle.label | pattern | -| RegexInjection.java:32:22:32:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| RegexInjection.java:35:29:35:35 | pattern | semmle.label | pattern | -| RegexInjection.java:39:22:39:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| RegexInjection.java:42:34:42:40 | pattern | semmle.label | pattern | -| RegexInjection.java:49:22:49:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| RegexInjection.java:52:28:52:34 | pattern | semmle.label | pattern | -| RegexInjection.java:56:22:56:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| RegexInjection.java:59:28:59:34 | pattern | semmle.label | pattern | -| RegexInjection.java:63:22:63:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| RegexInjection.java:66:26:66:52 | ... + ... | semmle.label | ... + ... | -| RegexInjection.java:66:32:66:43 | foo(...) : String | semmle.label | foo(...) : String | -| RegexInjection.java:66:36:66:42 | pattern : String | semmle.label | pattern : String | +| RegexInjection.java:13:22:13:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:16:26:16:47 | ... + ... | semmle.label | ... + ... | +| RegexInjection.java:20:22:20:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:23:24:23:30 | pattern | semmle.label | pattern | +| RegexInjection.java:27:22:27:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:30:31:30:37 | pattern | semmle.label | pattern | +| RegexInjection.java:34:22:34:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:37:29:37:35 | pattern | semmle.label | pattern | +| RegexInjection.java:41:22:41:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:44:34:44:40 | pattern | semmle.label | pattern | +| RegexInjection.java:51:22:51:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:54:28:54:34 | pattern | semmle.label | pattern | +| RegexInjection.java:58:22:58:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:61:28:61:34 | pattern | semmle.label | pattern | +| RegexInjection.java:65:22:65:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:68:26:68:52 | ... + ... | semmle.label | ... + ... | +| RegexInjection.java:68:32:68:43 | foo(...) : String | semmle.label | foo(...) : String | +| RegexInjection.java:68:36:68:42 | pattern : String | semmle.label | pattern : String | +| RegexInjection.java:90:22:90:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:93:40:93:46 | pattern | semmle.label | pattern | +| RegexInjection.java:97:22:97:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:100:42:100:48 | pattern | semmle.label | pattern | +| RegexInjection.java:104:22:104:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:107:44:107:50 | pattern | semmle.label | pattern | +| RegexInjection.java:111:22:111:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:114:41:114:47 | pattern | semmle.label | pattern | +| RegexInjection.java:118:22:118:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:121:43:121:49 | pattern | semmle.label | pattern | +| RegexInjection.java:133:22:133:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:136:45:136:51 | pattern | semmle.label | pattern | #select -| RegexInjection.java:14:26:14:47 | ... + ... | RegexInjection.java:11:22:11:52 | getParameter(...) : String | RegexInjection.java:14:26:14:47 | ... + ... | $@ is user controlled. | RegexInjection.java:11:22:11:52 | getParameter(...) | This regular expression pattern | -| RegexInjection.java:21:24:21:30 | pattern | RegexInjection.java:18:22:18:52 | getParameter(...) : String | RegexInjection.java:21:24:21:30 | pattern | $@ is user controlled. | RegexInjection.java:18:22:18:52 | getParameter(...) | This regular expression pattern | -| RegexInjection.java:28:31:28:37 | pattern | RegexInjection.java:25:22:25:52 | getParameter(...) : String | RegexInjection.java:28:31:28:37 | pattern | $@ is user controlled. | RegexInjection.java:25:22:25:52 | getParameter(...) | This regular expression pattern | -| RegexInjection.java:35:29:35:35 | pattern | RegexInjection.java:32:22:32:52 | getParameter(...) : String | RegexInjection.java:35:29:35:35 | pattern | $@ is user controlled. | RegexInjection.java:32:22:32:52 | getParameter(...) | This regular expression pattern | -| RegexInjection.java:42:34:42:40 | pattern | RegexInjection.java:39:22:39:52 | getParameter(...) : String | RegexInjection.java:42:34:42:40 | pattern | $@ is user controlled. | RegexInjection.java:39:22:39:52 | getParameter(...) | This regular expression pattern | -| RegexInjection.java:52:28:52:34 | pattern | RegexInjection.java:49:22:49:52 | getParameter(...) : String | RegexInjection.java:52:28:52:34 | pattern | $@ is user controlled. | RegexInjection.java:49:22:49:52 | getParameter(...) | This regular expression pattern | -| RegexInjection.java:59:28:59:34 | pattern | RegexInjection.java:56:22:56:52 | getParameter(...) : String | RegexInjection.java:59:28:59:34 | pattern | $@ is user controlled. | RegexInjection.java:56:22:56:52 | getParameter(...) | This regular expression pattern | -| RegexInjection.java:66:26:66:52 | ... + ... | RegexInjection.java:63:22:63:52 | getParameter(...) : String | RegexInjection.java:66:26:66:52 | ... + ... | $@ is user controlled. | RegexInjection.java:63:22:63:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:16:26:16:47 | ... + ... | RegexInjection.java:13:22:13:52 | getParameter(...) : String | RegexInjection.java:16:26:16:47 | ... + ... | $@ is user controlled. | RegexInjection.java:13:22:13:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:23:24:23:30 | pattern | RegexInjection.java:20:22:20:52 | getParameter(...) : String | RegexInjection.java:23:24:23:30 | pattern | $@ is user controlled. | RegexInjection.java:20:22:20:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:30:31:30:37 | pattern | RegexInjection.java:27:22:27:52 | getParameter(...) : String | RegexInjection.java:30:31:30:37 | pattern | $@ is user controlled. | RegexInjection.java:27:22:27:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:37:29:37:35 | pattern | RegexInjection.java:34:22:34:52 | getParameter(...) : String | RegexInjection.java:37:29:37:35 | pattern | $@ is user controlled. | RegexInjection.java:34:22:34:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:44:34:44:40 | pattern | RegexInjection.java:41:22:41:52 | getParameter(...) : String | RegexInjection.java:44:34:44:40 | pattern | $@ is user controlled. | RegexInjection.java:41:22:41:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:54:28:54:34 | pattern | RegexInjection.java:51:22:51:52 | getParameter(...) : String | RegexInjection.java:54:28:54:34 | pattern | $@ is user controlled. | RegexInjection.java:51:22:51:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:61:28:61:34 | pattern | RegexInjection.java:58:22:58:52 | getParameter(...) : String | RegexInjection.java:61:28:61:34 | pattern | $@ is user controlled. | RegexInjection.java:58:22:58:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:68:26:68:52 | ... + ... | RegexInjection.java:65:22:65:52 | getParameter(...) : String | RegexInjection.java:68:26:68:52 | ... + ... | $@ is user controlled. | RegexInjection.java:65:22:65:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:93:40:93:46 | pattern | RegexInjection.java:90:22:90:52 | getParameter(...) : String | RegexInjection.java:93:40:93:46 | pattern | $@ is user controlled. | RegexInjection.java:90:22:90:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:100:42:100:48 | pattern | RegexInjection.java:97:22:97:52 | getParameter(...) : String | RegexInjection.java:100:42:100:48 | pattern | $@ is user controlled. | RegexInjection.java:97:22:97:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:107:44:107:50 | pattern | RegexInjection.java:104:22:104:52 | getParameter(...) : String | RegexInjection.java:107:44:107:50 | pattern | $@ is user controlled. | RegexInjection.java:104:22:104:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:114:41:114:47 | pattern | RegexInjection.java:111:22:111:52 | getParameter(...) : String | RegexInjection.java:114:41:114:47 | pattern | $@ is user controlled. | RegexInjection.java:111:22:111:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:121:43:121:49 | pattern | RegexInjection.java:118:22:118:52 | getParameter(...) : String | RegexInjection.java:121:43:121:49 | pattern | $@ is user controlled. | RegexInjection.java:118:22:118:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:136:45:136:51 | pattern | RegexInjection.java:133:22:133:52 | getParameter(...) : String | RegexInjection.java:136:45:136:51 | pattern | $@ is user controlled. | RegexInjection.java:133:22:133:52 | getParameter(...) | This regular expression pattern | diff --git a/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.java b/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.java index c203360e105b..43218202acf0 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.java +++ b/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.java @@ -6,6 +6,8 @@ import javax.servlet.http.HttpServletResponse; import javax.servlet.ServletException; +import org.apache.commons.lang3.RegExUtils; + public class RegexInjection extends HttpServlet { public boolean string1(javax.servlet.http.HttpServletRequest request) { String pattern = request.getParameter("pattern"); @@ -83,4 +85,54 @@ public boolean pattern5(javax.servlet.http.HttpServletRequest request) { String escapeSpecialRegexChars(String str) { return SPECIAL_REGEX_CHARS.matcher(str).replaceAll("\\\\$0"); } -} \ No newline at end of file + + public boolean apache1(javax.servlet.http.HttpServletRequest request) { + String pattern = request.getParameter("pattern"); + String input = request.getParameter("input"); + + return RegExUtils.removeAll(input, pattern).length() > 0; // BAD + } + + public boolean apache2(javax.servlet.http.HttpServletRequest request) { + String pattern = request.getParameter("pattern"); + String input = request.getParameter("input"); + + return RegExUtils.removeFirst(input, pattern).length() > 0; // BAD + } + + public boolean apache3(javax.servlet.http.HttpServletRequest request) { + String pattern = request.getParameter("pattern"); + String input = request.getParameter("input"); + + return RegExUtils.removePattern(input, pattern).length() > 0; // BAD + } + + public boolean apache4(javax.servlet.http.HttpServletRequest request) { + String pattern = request.getParameter("pattern"); + String input = request.getParameter("input"); + + return RegExUtils.replaceAll(input, pattern, "").length() > 0; // BAD + } + + public boolean apache5(javax.servlet.http.HttpServletRequest request) { + String pattern = request.getParameter("pattern"); + String input = request.getParameter("input"); + + return RegExUtils.replaceFirst(input, pattern, "").length() > 0; // BAD + } + + public boolean apache6(javax.servlet.http.HttpServletRequest request) { + String pattern = request.getParameter("pattern"); + String input = request.getParameter("input"); + + Pattern pt = (Pattern)(Object) pattern; + return RegExUtils.replaceFirst(input, pt, "").length() > 0; // GOOD, Pattern compile is the sink instead + } + + public boolean apache7(javax.servlet.http.HttpServletRequest request) { + String pattern = request.getParameter("pattern"); + String input = request.getParameter("input"); + + return RegExUtils.replacePattern(input, pattern, "").length() > 0; // BAD + } +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-730/options b/java/ql/test/experimental/query-tests/security/CWE-730/options index dd125ad0f4ef..73f1b5a3c3eb 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-730/options +++ b/java/ql/test/experimental/query-tests/security/CWE-730/options @@ -1 +1 @@ -// semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4 \ No newline at end of file +// semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/apache-commons-lang3-3.7 \ No newline at end of file From 452ec8c43f005b4ee7f84cbb6b702ce82f037d3b Mon Sep 17 00:00:00 2001 From: edvraa <80588099+edvraa@users.noreply.github.com> Date: Wed, 21 Apr 2021 13:12:53 +0300 Subject: [PATCH 3/7] comments --- .../Security/CWE/CWE-730/RegexInjection.ql | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql b/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql index 9a96805c7e5b..451e8fe816bd 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql @@ -17,6 +17,9 @@ import semmle.code.java.dataflow.FlowSources import semmle.code.java.dataflow.TaintTracking import DataFlow::PathGraph +/** + * A data flow sink for untrusted user input used to construct regular expressions. + */ class RegexSink extends DataFlow::ExprNode { RegexSink() { exists(MethodAccess ma, Method m | m = ma.getMethod() | @@ -61,6 +64,10 @@ class RegexSink extends DataFlow::ExprNode { abstract class Sanitizer extends DataFlow::ExprNode { } +/** + * A call to a function whose name suggests that it escapes regular + * expression meta-characters. + */ class RegExpSanitizationCall extends Sanitizer { RegExpSanitizationCall() { exists(string calleeName, string sanitize, string regexp | @@ -75,6 +82,9 @@ class RegExpSanitizationCall extends Sanitizer { } } +/** + * A taint-tracking configuration for untrusted user input used to construct regular expressions. + */ class RegexInjectionConfiguration extends TaintTracking::Configuration { RegexInjectionConfiguration() { this = "RegexInjectionConfiguration" } From 9774b24c4e297655148654047fa66d1fd133be34 Mon Sep 17 00:00:00 2001 From: edvraa <80588099+edvraa@users.noreply.github.com> Date: Thu, 22 Apr 2021 09:44:07 +0300 Subject: [PATCH 4/7] Use TypeString --- .../src/experimental/Security/CWE/CWE-730/RegexInjection.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql b/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql index 451e8fe816bd..c33b7ecf3af6 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql @@ -24,7 +24,7 @@ class RegexSink extends DataFlow::ExprNode { RegexSink() { exists(MethodAccess ma, Method m | m = ma.getMethod() | ( - m.getDeclaringType().hasQualifiedName("java.lang", "String") and + m.getDeclaringType() instanceof TypeString and ( ma.getArgument(0) = this.asExpr() and ( @@ -47,7 +47,7 @@ class RegexSink extends DataFlow::ExprNode { m.getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "RegExUtils") and ( ma.getArgument(1) = this.asExpr() and - m.getParameterType(1).(Class).hasQualifiedName("java.lang", "String") and + m.getParameterType(1).(Class) instanceof TypeString and ( m.hasName("removeAll") or m.hasName("removeFirst") or From 86444bfa09e2e48ec37801abf3b80d537dce4f92 Mon Sep 17 00:00:00 2001 From: edvraa <80588099+edvraa@users.noreply.github.com> Date: Thu, 22 Apr 2021 09:48:46 +0300 Subject: [PATCH 5/7] Use set literal expression --- .../Security/CWE/CWE-730/RegexInjection.ql | 24 +++++-------------- 1 file changed, 6 insertions(+), 18 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql b/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql index c33b7ecf3af6..ad75e993c30b 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql @@ -27,35 +27,23 @@ class RegexSink extends DataFlow::ExprNode { m.getDeclaringType() instanceof TypeString and ( ma.getArgument(0) = this.asExpr() and - ( - m.hasName("matches") or - m.hasName("split") or - m.hasName("replaceFirst") or - m.hasName("replaceAll") - ) + m.hasName(["matches", "split", "replaceFirst", "replaceAll"]) ) or m.getDeclaringType().hasQualifiedName("java.util.regex", "Pattern") and ( ma.getArgument(0) = this.asExpr() and - ( - m.hasName("compile") or - m.hasName("matches") - ) + m.hasName(["compile", "matches"]) ) or m.getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "RegExUtils") and ( ma.getArgument(1) = this.asExpr() and m.getParameterType(1).(Class) instanceof TypeString and - ( - m.hasName("removeAll") or - m.hasName("removeFirst") or - m.hasName("removePattern") or - m.hasName("replaceAll") or - m.hasName("replaceFirst") or - m.hasName("replacePattern") - ) + m.hasName([ + "removeAll", "removeFirst", "removePattern", "replaceAll", "replaceFirst", + "replacePattern" + ]) ) ) ) From ade238307f1416ad45af12f2a9c5ab949a04f427 Mon Sep 17 00:00:00 2001 From: edvraa <80588099+edvraa@users.noreply.github.com> Date: Thu, 22 Apr 2021 10:02:06 +0300 Subject: [PATCH 6/7] Add a test --- .../security/CWE-730/RegexInjection.expected | 52 ++++++++++--------- .../security/CWE-730/RegexInjection.java | 10 ++++ 2 files changed, 38 insertions(+), 24 deletions(-) diff --git a/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.expected b/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.expected index 93961372d759..1dfe806f10b3 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.expected @@ -9,12 +9,13 @@ edges | RegexInjection.java:65:22:65:52 | getParameter(...) : String | RegexInjection.java:68:36:68:42 | pattern : String | | RegexInjection.java:68:32:68:43 | foo(...) : String | RegexInjection.java:68:26:68:52 | ... + ... | | RegexInjection.java:68:36:68:42 | pattern : String | RegexInjection.java:68:32:68:43 | foo(...) : String | -| RegexInjection.java:90:22:90:52 | getParameter(...) : String | RegexInjection.java:93:40:93:46 | pattern | -| RegexInjection.java:97:22:97:52 | getParameter(...) : String | RegexInjection.java:100:42:100:48 | pattern | -| RegexInjection.java:104:22:104:52 | getParameter(...) : String | RegexInjection.java:107:44:107:50 | pattern | -| RegexInjection.java:111:22:111:52 | getParameter(...) : String | RegexInjection.java:114:41:114:47 | pattern | -| RegexInjection.java:118:22:118:52 | getParameter(...) : String | RegexInjection.java:121:43:121:49 | pattern | -| RegexInjection.java:133:22:133:52 | getParameter(...) : String | RegexInjection.java:136:45:136:51 | pattern | +| RegexInjection.java:84:22:84:52 | getParameter(...) : String | RegexInjection.java:90:26:90:47 | ... + ... | +| RegexInjection.java:100:22:100:52 | getParameter(...) : String | RegexInjection.java:103:40:103:46 | pattern | +| RegexInjection.java:107:22:107:52 | getParameter(...) : String | RegexInjection.java:110:42:110:48 | pattern | +| RegexInjection.java:114:22:114:52 | getParameter(...) : String | RegexInjection.java:117:44:117:50 | pattern | +| RegexInjection.java:121:22:121:52 | getParameter(...) : String | RegexInjection.java:124:41:124:47 | pattern | +| RegexInjection.java:128:22:128:52 | getParameter(...) : String | RegexInjection.java:131:43:131:49 | pattern | +| RegexInjection.java:143:22:143:52 | getParameter(...) : String | RegexInjection.java:146:45:146:51 | pattern | nodes | RegexInjection.java:13:22:13:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | | RegexInjection.java:16:26:16:47 | ... + ... | semmle.label | ... + ... | @@ -34,18 +35,20 @@ nodes | RegexInjection.java:68:26:68:52 | ... + ... | semmle.label | ... + ... | | RegexInjection.java:68:32:68:43 | foo(...) : String | semmle.label | foo(...) : String | | RegexInjection.java:68:36:68:42 | pattern : String | semmle.label | pattern : String | -| RegexInjection.java:90:22:90:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| RegexInjection.java:93:40:93:46 | pattern | semmle.label | pattern | -| RegexInjection.java:97:22:97:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| RegexInjection.java:100:42:100:48 | pattern | semmle.label | pattern | -| RegexInjection.java:104:22:104:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| RegexInjection.java:107:44:107:50 | pattern | semmle.label | pattern | -| RegexInjection.java:111:22:111:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| RegexInjection.java:114:41:114:47 | pattern | semmle.label | pattern | -| RegexInjection.java:118:22:118:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| RegexInjection.java:121:43:121:49 | pattern | semmle.label | pattern | -| RegexInjection.java:133:22:133:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| RegexInjection.java:136:45:136:51 | pattern | semmle.label | pattern | +| RegexInjection.java:84:22:84:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:90:26:90:47 | ... + ... | semmle.label | ... + ... | +| RegexInjection.java:100:22:100:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:103:40:103:46 | pattern | semmle.label | pattern | +| RegexInjection.java:107:22:107:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:110:42:110:48 | pattern | semmle.label | pattern | +| RegexInjection.java:114:22:114:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:117:44:117:50 | pattern | semmle.label | pattern | +| RegexInjection.java:121:22:121:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:124:41:124:47 | pattern | semmle.label | pattern | +| RegexInjection.java:128:22:128:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:131:43:131:49 | pattern | semmle.label | pattern | +| RegexInjection.java:143:22:143:52 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RegexInjection.java:146:45:146:51 | pattern | semmle.label | pattern | #select | RegexInjection.java:16:26:16:47 | ... + ... | RegexInjection.java:13:22:13:52 | getParameter(...) : String | RegexInjection.java:16:26:16:47 | ... + ... | $@ is user controlled. | RegexInjection.java:13:22:13:52 | getParameter(...) | This regular expression pattern | | RegexInjection.java:23:24:23:30 | pattern | RegexInjection.java:20:22:20:52 | getParameter(...) : String | RegexInjection.java:23:24:23:30 | pattern | $@ is user controlled. | RegexInjection.java:20:22:20:52 | getParameter(...) | This regular expression pattern | @@ -55,9 +58,10 @@ nodes | RegexInjection.java:54:28:54:34 | pattern | RegexInjection.java:51:22:51:52 | getParameter(...) : String | RegexInjection.java:54:28:54:34 | pattern | $@ is user controlled. | RegexInjection.java:51:22:51:52 | getParameter(...) | This regular expression pattern | | RegexInjection.java:61:28:61:34 | pattern | RegexInjection.java:58:22:58:52 | getParameter(...) : String | RegexInjection.java:61:28:61:34 | pattern | $@ is user controlled. | RegexInjection.java:58:22:58:52 | getParameter(...) | This regular expression pattern | | RegexInjection.java:68:26:68:52 | ... + ... | RegexInjection.java:65:22:65:52 | getParameter(...) : String | RegexInjection.java:68:26:68:52 | ... + ... | $@ is user controlled. | RegexInjection.java:65:22:65:52 | getParameter(...) | This regular expression pattern | -| RegexInjection.java:93:40:93:46 | pattern | RegexInjection.java:90:22:90:52 | getParameter(...) : String | RegexInjection.java:93:40:93:46 | pattern | $@ is user controlled. | RegexInjection.java:90:22:90:52 | getParameter(...) | This regular expression pattern | -| RegexInjection.java:100:42:100:48 | pattern | RegexInjection.java:97:22:97:52 | getParameter(...) : String | RegexInjection.java:100:42:100:48 | pattern | $@ is user controlled. | RegexInjection.java:97:22:97:52 | getParameter(...) | This regular expression pattern | -| RegexInjection.java:107:44:107:50 | pattern | RegexInjection.java:104:22:104:52 | getParameter(...) : String | RegexInjection.java:107:44:107:50 | pattern | $@ is user controlled. | RegexInjection.java:104:22:104:52 | getParameter(...) | This regular expression pattern | -| RegexInjection.java:114:41:114:47 | pattern | RegexInjection.java:111:22:111:52 | getParameter(...) : String | RegexInjection.java:114:41:114:47 | pattern | $@ is user controlled. | RegexInjection.java:111:22:111:52 | getParameter(...) | This regular expression pattern | -| RegexInjection.java:121:43:121:49 | pattern | RegexInjection.java:118:22:118:52 | getParameter(...) : String | RegexInjection.java:121:43:121:49 | pattern | $@ is user controlled. | RegexInjection.java:118:22:118:52 | getParameter(...) | This regular expression pattern | -| RegexInjection.java:136:45:136:51 | pattern | RegexInjection.java:133:22:133:52 | getParameter(...) : String | RegexInjection.java:136:45:136:51 | pattern | $@ is user controlled. | RegexInjection.java:133:22:133:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:90:26:90:47 | ... + ... | RegexInjection.java:84:22:84:52 | getParameter(...) : String | RegexInjection.java:90:26:90:47 | ... + ... | $@ is user controlled. | RegexInjection.java:84:22:84:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:103:40:103:46 | pattern | RegexInjection.java:100:22:100:52 | getParameter(...) : String | RegexInjection.java:103:40:103:46 | pattern | $@ is user controlled. | RegexInjection.java:100:22:100:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:110:42:110:48 | pattern | RegexInjection.java:107:22:107:52 | getParameter(...) : String | RegexInjection.java:110:42:110:48 | pattern | $@ is user controlled. | RegexInjection.java:107:22:107:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:117:44:117:50 | pattern | RegexInjection.java:114:22:114:52 | getParameter(...) : String | RegexInjection.java:117:44:117:50 | pattern | $@ is user controlled. | RegexInjection.java:114:22:114:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:124:41:124:47 | pattern | RegexInjection.java:121:22:121:52 | getParameter(...) : String | RegexInjection.java:124:41:124:47 | pattern | $@ is user controlled. | RegexInjection.java:121:22:121:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:131:43:131:49 | pattern | RegexInjection.java:128:22:128:52 | getParameter(...) : String | RegexInjection.java:131:43:131:49 | pattern | $@ is user controlled. | RegexInjection.java:128:22:128:52 | getParameter(...) | This regular expression pattern | +| RegexInjection.java:146:45:146:51 | pattern | RegexInjection.java:143:22:143:52 | getParameter(...) : String | RegexInjection.java:146:45:146:51 | pattern | $@ is user controlled. | RegexInjection.java:143:22:143:52 | getParameter(...) | This regular expression pattern | diff --git a/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.java b/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.java index 43218202acf0..2aace93aeb8c 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.java +++ b/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.java @@ -80,6 +80,16 @@ public boolean pattern5(javax.servlet.http.HttpServletRequest request) { return input.matches("^" + escapeSpecialRegexChars(pattern) + "=.*$"); } + public boolean pattern6(javax.servlet.http.HttpServletRequest request) { + String pattern = request.getParameter("pattern"); + String input = request.getParameter("input"); + + escapeSpecialRegexChars(pattern); + + // BAD: the pattern is not really sanitized + return input.matches("^" + pattern + "=.*$"); + } + Pattern SPECIAL_REGEX_CHARS = Pattern.compile("[{}()\\[\\]><-=!.+*?^$\\\\|]"); String escapeSpecialRegexChars(String str) { From 5eb96c1e4533729483037138d2719b729d79e521 Mon Sep 17 00:00:00 2001 From: edvraa <80588099+edvraa@users.noreply.github.com> Date: Tue, 27 Apr 2021 20:26:29 +0300 Subject: [PATCH 7/7] Remove Class cast --- java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql b/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql index ad75e993c30b..3b8b5dc759ac 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql @@ -39,7 +39,7 @@ class RegexSink extends DataFlow::ExprNode { m.getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "RegExUtils") and ( ma.getArgument(1) = this.asExpr() and - m.getParameterType(1).(Class) instanceof TypeString and + m.getParameterType(1) instanceof TypeString and m.hasName([ "removeAll", "removeFirst", "removePattern", "replaceAll", "replaceFirst", "replacePattern"