diff --git a/java/ql/lib/semmle/code/java/frameworks/Regex.qll b/java/ql/lib/semmle/code/java/frameworks/Regex.qll index 4e83981d8574..790255c97031 100644 --- a/java/ql/lib/semmle/code/java/frameworks/Regex.qll +++ b/java/ql/lib/semmle/code/java/frameworks/Regex.qll @@ -2,6 +2,27 @@ private import semmle.code.java.dataflow.ExternalFlow +/** The class `java.util.regex.Pattern`. */ +class TypeRegexPattern extends Class { + TypeRegexPattern() { this.hasQualifiedName("java.util.regex", "Pattern") } +} + +/** The `quote` method of the `java.util.regex.Pattern` class. */ +class PatternQuoteMethod extends Method { + PatternQuoteMethod() { + this.getDeclaringType() instanceof TypeRegexPattern and + this.hasName("quote") + } +} + +/** The `LITERAL` field of the `java.util.regex.Pattern` class. */ +class PatternLiteralField extends Field { + PatternLiteralField() { + this.getDeclaringType() instanceof TypeRegexPattern and + this.hasName("LITERAL") + } +} + private class RegexModel extends SummaryModelCsv { override predicate row(string s) { s = diff --git a/java/ql/lib/semmle/code/java/regex/RegexFlowModels.qll b/java/ql/lib/semmle/code/java/regex/RegexFlowModels.qll index de0b5465fe49..20ba2c14dc8f 100644 --- a/java/ql/lib/semmle/code/java/regex/RegexFlowModels.qll +++ b/java/ql/lib/semmle/code/java/regex/RegexFlowModels.qll @@ -27,6 +27,12 @@ private class RegexSinkCsv extends SinkModelCsv { "com.google.common.base;Splitter;false;split;(CharSequence);;Argument[-1];regex-use[0];manual", "com.google.common.base;Splitter;false;splitToList;(CharSequence);;Argument[-1];regex-use[0];manual", "com.google.common.base;Splitter$MapSplitter;false;split;(CharSequence);;Argument[-1];regex-use[0];manual", + "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", + "org.apache.commons.lang3;RegExUtils;false;replaceAll;(String,String,String);;Argument[1];regex-use;manual", + "org.apache.commons.lang3;RegExUtils;false;replaceFirst;(String,String,String);;Argument[1];regex-use;manual", + "org.apache.commons.lang3;RegExUtils;false;replacePattern;(String,String,String);;Argument[1];regex-use;manual", ] } } diff --git a/java/ql/lib/semmle/code/java/security/regexp/RegexInjection.qll b/java/ql/lib/semmle/code/java/security/regexp/RegexInjection.qll new file mode 100644 index 000000000000..3c1e2e982296 --- /dev/null +++ b/java/ql/lib/semmle/code/java/security/regexp/RegexInjection.qll @@ -0,0 +1,48 @@ +/** Provides classes and predicates related to regex injection in Java. */ + +import java +private import semmle.code.java.dataflow.DataFlow +private import semmle.code.java.frameworks.Regex +private import semmle.code.java.regex.RegexFlowModels + +/** A data flow sink for untrusted user input used to construct regular expressions. */ +abstract class RegexInjectionSink extends DataFlow::ExprNode { } + +/** A sanitizer for untrusted user input used to construct regular expressions. */ +abstract class RegexInjectionSanitizer extends DataFlow::ExprNode { } + +/** A method call that takes a regular expression as an argument. */ +private class DefaultRegexInjectionSink extends RegexInjectionSink { + DefaultRegexInjectionSink() { + // we only select sinks where there is direct regex creation, not regex uses + sinkNode(this, ["regex-use[]", "regex-use[f1]", "regex-use[f-1]", "regex-use[-1]", "regex-use"]) + } +} + +/** + * A call to the `Pattern.quote` method, which gives metacharacters or escape sequences + * no special meaning. + */ +private class PatternQuoteCall extends RegexInjectionSanitizer { + PatternQuoteCall() { + exists(MethodAccess ma, Method m | m = ma.getMethod() | + ma.getArgument(0) = this.asExpr() and + m instanceof PatternQuoteMethod + ) + } +} + +/** + * Use of the `Pattern.LITERAL` flag with `Pattern.compile`, which gives metacharacters + * or escape sequences no special meaning. + */ +private class PatternLiteralFlag extends RegexInjectionSanitizer { + PatternLiteralFlag() { + exists(MethodAccess ma, Method m, PatternLiteralField field | m = ma.getMethod() | + ma.getArgument(0) = this.asExpr() and + m.getDeclaringType() instanceof TypeRegexPattern and + m.hasName("compile") and + ma.getArgument(1) = field.getAnAccess() + ) + } +} diff --git a/java/ql/lib/semmle/code/java/security/regexp/RegexInjectionQuery.qll b/java/ql/lib/semmle/code/java/security/regexp/RegexInjectionQuery.qll new file mode 100644 index 000000000000..e2453c08dcae --- /dev/null +++ b/java/ql/lib/semmle/code/java/security/regexp/RegexInjectionQuery.qll @@ -0,0 +1,17 @@ +/** Provides configurations to be used in queries related to regex injection. */ + +import java +import semmle.code.java.dataflow.FlowSources +import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.security.regexp.RegexInjection + +/** A taint-tracking configuration for untrusted user input used to construct regular expressions. */ +class RegexInjectionConfiguration extends TaintTracking::Configuration { + RegexInjectionConfiguration() { this = "RegexInjection" } + + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + + override predicate isSink(DataFlow::Node sink) { sink instanceof RegexInjectionSink } + + override predicate isSanitizer(DataFlow::Node node) { node instanceof RegexInjectionSanitizer } +} diff --git a/java/ql/src/Security/CWE/CWE-730/RegexInjection.java b/java/ql/src/Security/CWE/CWE-730/RegexInjection.java new file mode 100644 index 000000000000..673d5a2fa409 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-730/RegexInjection.java @@ -0,0 +1,22 @@ +import java.util.regex.Pattern; +import javax.servlet.http.HttpServlet; +import javax.servlet.http.HttpServletRequest; + +public class RegexInjectionDemo extends HttpServlet { + + public boolean badExample(javax.servlet.http.HttpServletRequest request) { + String regex = request.getParameter("regex"); + String input = request.getParameter("input"); + + // BAD: Unsanitized user input is used to construct a regular expression + return input.matches(regex); + } + + public boolean goodExample(javax.servlet.http.HttpServletRequest request) { + String regex = request.getParameter("regex"); + String input = request.getParameter("input"); + + // GOOD: User input is sanitized before constructing the regex + return input.matches(Pattern.quote(regex)); + } +} diff --git a/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.qhelp b/java/ql/src/Security/CWE/CWE-730/RegexInjection.qhelp similarity index 67% rename from java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.qhelp rename to java/ql/src/Security/CWE/CWE-730/RegexInjection.qhelp index 6cd80b52f755..3e239d07107b 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.qhelp +++ b/java/ql/src/Security/CWE/CWE-730/RegexInjection.qhelp @@ -15,25 +15,25 @@ 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. +such as Pattern.quote to escape meta-characters that have special meaning.

-The following example shows a HTTP request parameter that is used to construct a regular expression: +The following example shows an 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, +If a malicious user provides a regex whose worst-case performance is exponential, 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 second case, the user input is escaped using Pattern.quote before being included in the regular expression. This ensures that the user cannot insert characters which have a special meaning in regular expressions.

+ @@ -44,5 +44,8 @@ OWASP:
  • Wikipedia: ReDoS.
    • +
  • +Java API Specification: Pattern.quote. +
    •  diff --git a/java/ql/src/Security/CWE/CWE-730/RegexInjection.ql b/java/ql/src/Security/CWE/CWE-730/RegexInjection.ql new file mode 100644 index 000000000000..820e0949d220 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-730/RegexInjection.ql @@ -0,0 +1,23 @@ +/** + * @name Regular expression injection + * @description User input should not be used in regular expressions without first being escaped, + * 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 + * @security-severity 7.5 + * @precision high + * @id java/regex-injection + * @tags security + * external/cwe/cwe-730 + * external/cwe/cwe-400 + */ + +import java +import semmle.code.java.security.regexp.RegexInjectionQuery +import DataFlow::PathGraph + +from DataFlow::PathNode source, DataFlow::PathNode sink, RegexInjectionConfiguration c +where c.hasFlowPath(source, sink) +select sink.getNode(), source, sink, "This regular expression is constructed from a $@.", + source.getNode(), "user-provided value" diff --git a/java/ql/src/change-notes/2022-11-03-regex-injection.md b/java/ql/src/change-notes/2022-11-03-regex-injection.md new file mode 100644 index 000000000000..759aa2a86e1e --- /dev/null +++ b/java/ql/src/change-notes/2022-11-03-regex-injection.md @@ -0,0 +1,4 @@ +--- +category: newQuery +--- +* The query, `java/regex-injection`, has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @edvraa](https://github.com/github/codeql/pull/5704). diff --git a/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.java b/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.java deleted file mode 100644 index 387648a443e6..000000000000 --- a/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.java +++ /dev/null @@ -1,38 +0,0 @@ -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.ql b/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql deleted file mode 100644 index f60e5d9070bb..000000000000 --- a/java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql +++ /dev/null @@ -1,89 +0,0 @@ -/** - * @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 - -/** - * 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() | - ( - m.getDeclaringType() instanceof TypeString and - ( - ma.getArgument(0) = this.asExpr() and - m.hasName(["matches", "split", "replaceFirst", "replaceAll"]) - ) - or - m.getDeclaringType().hasQualifiedName("java.util.regex", "Pattern") and - ( - ma.getArgument(0) = this.asExpr() and - m.hasName(["compile", "matches"]) - ) - or - m.getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "RegExUtils") and - ( - ma.getArgument(1) = this.asExpr() and - m.getParameterType(1) instanceof TypeString and - m.hasName([ - "removeAll", "removeFirst", "removePattern", "replaceAll", "replaceFirst", - "replacePattern" - ]) - ) - ) - ) - } -} - -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 | - calleeName = this.asExpr().(Call).getCallee().getName() and - sanitize = "(?:escape|saniti[sz]e)" and - regexp = "regexp?" - | - calleeName - .regexpMatch("(?i)(" + sanitize + ".*" + regexp + ".*)" + "|(" + regexp + ".*" + sanitize + - ".*)") - ) - } -} - -/** - * A taint-tracking configuration for untrusted user input used to construct regular expressions. - */ -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, "This regular expression is constructed from a $@.", - source.getNode(), "user-provided value" 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 deleted file mode 100644 index a795f1591ada..000000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.expected +++ /dev/null @@ -1,73 +0,0 @@ -edges -| 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:68:36:68:42 | pattern : String | RegexInjection.java:71:14:71:23 | str : String | -| RegexInjection.java:71:14:71:23 | str : String | RegexInjection.java:72:12:72:14 | str : String | -| 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 | ... + ... | -| 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:71:14:71:23 | str : String | semmle.label | str : String | -| RegexInjection.java:72:12:72:14 | str : String | semmle.label | str : String | -| 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 | -subpaths -| RegexInjection.java:68:36:68:42 | pattern : String | RegexInjection.java:71:14:71:23 | str : String | RegexInjection.java:72:12:72:14 | str : String | RegexInjection.java:68:32:68:43 | foo(...) : String | -#select -| RegexInjection.java:16:26:16:47 | ... + ... | RegexInjection.java:13:22:13:52 | getParameter(...) : String | RegexInjection.java:16:26:16:47 | ... + ... | This regular expression is constructed from a $@. | RegexInjection.java:13:22:13:52 | getParameter(...) | user-provided value | -| RegexInjection.java:23:24:23:30 | pattern | RegexInjection.java:20:22:20:52 | getParameter(...) : String | RegexInjection.java:23:24:23:30 | pattern | This regular expression is constructed from a $@. | RegexInjection.java:20:22:20:52 | getParameter(...) | user-provided value | -| RegexInjection.java:30:31:30:37 | pattern | RegexInjection.java:27:22:27:52 | getParameter(...) : String | RegexInjection.java:30:31:30:37 | pattern | This regular expression is constructed from a $@. | RegexInjection.java:27:22:27:52 | getParameter(...) | user-provided value | -| RegexInjection.java:37:29:37:35 | pattern | RegexInjection.java:34:22:34:52 | getParameter(...) : String | RegexInjection.java:37:29:37:35 | pattern | This regular expression is constructed from a $@. | RegexInjection.java:34:22:34:52 | getParameter(...) | user-provided value | -| RegexInjection.java:44:34:44:40 | pattern | RegexInjection.java:41:22:41:52 | getParameter(...) : String | RegexInjection.java:44:34:44:40 | pattern | This regular expression is constructed from a $@. | RegexInjection.java:41:22:41:52 | getParameter(...) | user-provided value | -| RegexInjection.java:54:28:54:34 | pattern | RegexInjection.java:51:22:51:52 | getParameter(...) : String | RegexInjection.java:54:28:54:34 | pattern | This regular expression is constructed from a $@. | RegexInjection.java:51:22:51:52 | getParameter(...) | user-provided value | -| RegexInjection.java:61:28:61:34 | pattern | RegexInjection.java:58:22:58:52 | getParameter(...) : String | RegexInjection.java:61:28:61:34 | pattern | This regular expression is constructed from a $@. | RegexInjection.java:58:22:58:52 | getParameter(...) | user-provided value | -| RegexInjection.java:68:26:68:52 | ... + ... | RegexInjection.java:65:22:65:52 | getParameter(...) : String | RegexInjection.java:68:26:68:52 | ... + ... | This regular expression is constructed from a $@. | RegexInjection.java:65:22:65:52 | getParameter(...) | user-provided value | -| RegexInjection.java:90:26:90:47 | ... + ... | RegexInjection.java:84:22:84:52 | getParameter(...) : String | RegexInjection.java:90:26:90:47 | ... + ... | This regular expression is constructed from a $@. | RegexInjection.java:84:22:84:52 | getParameter(...) | user-provided value | -| RegexInjection.java:103:40:103:46 | pattern | RegexInjection.java:100:22:100:52 | getParameter(...) : String | RegexInjection.java:103:40:103:46 | pattern | This regular expression is constructed from a $@. | RegexInjection.java:100:22:100:52 | getParameter(...) | user-provided value | -| RegexInjection.java:110:42:110:48 | pattern | RegexInjection.java:107:22:107:52 | getParameter(...) : String | RegexInjection.java:110:42:110:48 | pattern | This regular expression is constructed from a $@. | RegexInjection.java:107:22:107:52 | getParameter(...) | user-provided value | -| RegexInjection.java:117:44:117:50 | pattern | RegexInjection.java:114:22:114:52 | getParameter(...) : String | RegexInjection.java:117:44:117:50 | pattern | This regular expression is constructed from a $@. | RegexInjection.java:114:22:114:52 | getParameter(...) | user-provided value | -| RegexInjection.java:124:41:124:47 | pattern | RegexInjection.java:121:22:121:52 | getParameter(...) : String | RegexInjection.java:124:41:124:47 | pattern | This regular expression is constructed from a $@. | RegexInjection.java:121:22:121:52 | getParameter(...) | user-provided value | -| RegexInjection.java:131:43:131:49 | pattern | RegexInjection.java:128:22:128:52 | getParameter(...) : String | RegexInjection.java:131:43:131:49 | pattern | This regular expression is constructed from a $@. | RegexInjection.java:128:22:128:52 | getParameter(...) | user-provided value | -| RegexInjection.java:146:45:146:51 | pattern | RegexInjection.java:143:22:143:52 | getParameter(...) : String | RegexInjection.java:146:45:146:51 | pattern | This regular expression is constructed from a $@. | RegexInjection.java:143:22:143:52 | getParameter(...) | user-provided value | 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 deleted file mode 100644 index dca594b38d22..000000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.qlref +++ /dev/null @@ -1 +0,0 @@ -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 deleted file mode 100644 index 73f1b5a3c3eb..000000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-730/options +++ /dev/null @@ -1 +0,0 @@ -// 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 diff --git a/java/ql/test/query-tests/security/CWE-730/RegexInjectionTest.expected b/java/ql/test/query-tests/security/CWE-730/RegexInjectionTest.expected new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.java b/java/ql/test/query-tests/security/CWE-730/RegexInjectionTest.java similarity index 65% rename from java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.java rename to java/ql/test/query-tests/security/CWE-730/RegexInjectionTest.java index 2aace93aeb8c..5c7a3ca05746 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-730/RegexInjection.java +++ b/java/ql/test/query-tests/security/CWE-730/RegexInjectionTest.java @@ -7,128 +7,119 @@ import javax.servlet.ServletException; import org.apache.commons.lang3.RegExUtils; +import com.google.common.base.Splitter; -public class RegexInjection extends HttpServlet { +public class RegexInjectionTest 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 + return input.matches("^" + pattern + "=.*$"); // $ hasRegexInjection } 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 + return input.split(pattern).length > 0; // $ hasRegexInjection } 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 + return input.split(pattern, 0).length > 0; // $ hasRegexInjection } 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 + return input.replaceFirst(pattern, "").length() > 0; // $ hasRegexInjection + } + + public boolean string5(javax.servlet.http.HttpServletRequest request) { + String pattern = request.getParameter("pattern"); + String input = request.getParameter("input"); + + return input.replaceAll(pattern, "").length() > 0; // $ hasRegexInjection } public boolean pattern1(javax.servlet.http.HttpServletRequest request) { String pattern = request.getParameter("pattern"); String input = request.getParameter("input"); - Pattern pt = Pattern.compile(pattern); + Pattern pt = Pattern.compile(pattern); // $ hasRegexInjection Matcher matcher = pt.matcher(input); - return matcher.find(); // BAD + return matcher.find(); } 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 + return Pattern.compile(pattern).matcher(input).matches(); // $ hasRegexInjection } public boolean pattern3(javax.servlet.http.HttpServletRequest request) { String pattern = request.getParameter("pattern"); String input = request.getParameter("input"); - return Pattern.matches(pattern, input); // BAD + return Pattern.compile(pattern, 0).matcher(input).matches(); // $ hasRegexInjection } 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; + return Pattern.matches(pattern, input); // $ hasRegexInjection } 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) + "=.*$"); + return input.matches("^" + foo(pattern) + "=.*$"); // $ hasRegexInjection } - 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) { - return SPECIAL_REGEX_CHARS.matcher(str).replaceAll("\\\\$0"); + String foo(String str) { + return str; } 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 + return RegExUtils.removeAll(input, pattern).length() > 0; // $ hasRegexInjection } 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 + return RegExUtils.removeFirst(input, pattern).length() > 0; // $ hasRegexInjection } 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 + return RegExUtils.removePattern(input, pattern).length() > 0; // $ hasRegexInjection } 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 + return RegExUtils.replaceAll(input, pattern, "").length() > 0; // $ hasRegexInjection } 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 + return RegExUtils.replaceFirst(input, pattern, "").length() > 0; // $ hasRegexInjection } public boolean apache6(javax.servlet.http.HttpServletRequest request) { @@ -136,13 +127,40 @@ 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) { String pattern = request.getParameter("pattern"); String input = request.getParameter("input"); - return RegExUtils.replacePattern(input, pattern, "").length() > 0; // BAD + return RegExUtils.replacePattern(input, pattern, "").length() > 0; // $ hasRegexInjection + } + + // test `Pattern.quote` sanitizer + public boolean quoteTest(javax.servlet.http.HttpServletRequest request) { + String pattern = request.getParameter("pattern"); + String input = request.getParameter("input"); + + return input.matches(Pattern.quote(pattern)); // Safe + } + + // test `Pattern.LITERAL` sanitizer + public boolean literalTest(javax.servlet.http.HttpServletRequest request) { + String pattern = request.getParameter("pattern"); + String input = request.getParameter("input"); + + return Pattern.compile(pattern, Pattern.LITERAL).matcher(input).matches(); // Safe + } + + public Splitter guava1(javax.servlet.http.HttpServletRequest request) { + String pattern = request.getParameter("pattern"); + return Splitter.onPattern(pattern); // $ hasRegexInjection + } + + public Splitter guava2(javax.servlet.http.HttpServletRequest request) { + String pattern = request.getParameter("pattern"); + // sink is `Pattern.compile` + return Splitter.on(Pattern.compile(pattern)); // $ hasRegexInjection } } diff --git a/java/ql/test/query-tests/security/CWE-730/RegexInjectionTest.ql b/java/ql/test/query-tests/security/CWE-730/RegexInjectionTest.ql new file mode 100644 index 000000000000..368b5170bfc4 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-730/RegexInjectionTest.ql @@ -0,0 +1,20 @@ +import java +import TestUtilities.InlineExpectationsTest +import semmle.code.java.security.regexp.RegexInjectionQuery + +class RegexInjectionTest extends InlineExpectationsTest { + RegexInjectionTest() { this = "RegexInjectionTest" } + + override string getARelevantTag() { result = "hasRegexInjection" } + + override predicate hasActualResult(Location location, string element, string tag, string value) { + tag = "hasRegexInjection" and + exists(DataFlow::PathNode source, DataFlow::PathNode sink, RegexInjectionConfiguration c | + c.hasFlowPath(source, sink) + | + location = sink.getNode().getLocation() and + element = sink.getNode().toString() and + value = "" + ) + } +} diff --git a/java/ql/test/query-tests/security/CWE-730/options b/java/ql/test/query-tests/security/CWE-730/options index 2f7d22dc61ca..884cb21114c3 100644 --- a/java/ql/test/query-tests/security/CWE-730/options +++ b/java/ql/test/query-tests/security/CWE-730/options @@ -1 +1 @@ -// semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/servlet-api-2.4:${testdir}/../../../stubs/guava-30.0 \ No newline at end of file +// semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/servlet-api-2.4:${testdir}/../../../stubs/guava-30.0:${testdir}/../../../stubs/servlet-api-2.4:${testdir}/../../../stubs/apache-commons-lang3-3.7