diff --git a/java/ql/src/Security/CWE/CWE-113/ResponseSplitting.ql b/java/ql/src/Security/CWE/CWE-113/ResponseSplitting.ql index 6132406bf3a..26ef82702ee 100644 --- a/java/ql/src/Security/CWE/CWE-113/ResponseSplitting.ql +++ b/java/ql/src/Security/CWE/CWE-113/ResponseSplitting.ql @@ -31,10 +31,16 @@ class ResponseSplittingConfig extends TaintTracking::Configuration { or node.getType() instanceof BoxedType or - exists(MethodAccess ma | - ma.getMethod().hasQualifiedName("java.lang", "String", "replaceAll") and - ma.getArgument(0).(StringLiteral).getValue().matches("%[^%") and - node.asExpr() = ma + exists(MethodAccess ma, string methodName, CompileTimeConstantExpr target | + node.asExpr() = ma and + ma.getMethod().hasQualifiedName("java.lang", "String", methodName) and + target = ma.getArgument(0) and + ( + methodName = "replace" and target.getIntValue() = [10, 13] + or + methodName = "replaceAll" and + target.getStringValue().regexpMatch(".*([\n\r]|\\[\\^[^\\]\r\n]*\\]).*") + ) ) } } diff --git a/java/ql/test/query-tests/security/CWE-113/semmle/tests/ResponseSplitting.expected b/java/ql/test/query-tests/security/CWE-113/semmle/tests/ResponseSplitting.expected index 35cba00aa6b..011d9035f26 100644 --- a/java/ql/test/query-tests/security/CWE-113/semmle/tests/ResponseSplitting.expected +++ b/java/ql/test/query-tests/security/CWE-113/semmle/tests/ResponseSplitting.expected @@ -1,14 +1,20 @@ edges | ResponseSplitting.java:22:20:22:67 | new Cookie(...) : Cookie | ResponseSplitting.java:23:23:23:28 | cookie | | ResponseSplitting.java:22:39:22:66 | getParameter(...) : String | ResponseSplitting.java:22:20:22:67 | new Cookie(...) : Cookie | +| ResponseSplitting.java:53:14:53:48 | getParameter(...) : String | ResponseSplitting.java:59:27:59:27 | t : String | +| ResponseSplitting.java:59:27:59:27 | t : String | ResponseSplitting.java:59:27:59:57 | replaceFirst(...) | nodes | ResponseSplitting.java:22:20:22:67 | new Cookie(...) : Cookie | semmle.label | new Cookie(...) : Cookie | | ResponseSplitting.java:22:39:22:66 | getParameter(...) : String | semmle.label | getParameter(...) : String | | ResponseSplitting.java:23:23:23:28 | cookie | semmle.label | cookie | | ResponseSplitting.java:28:38:28:72 | getParameter(...) | semmle.label | getParameter(...) | | ResponseSplitting.java:29:38:29:72 | getParameter(...) | semmle.label | getParameter(...) | +| ResponseSplitting.java:53:14:53:48 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| ResponseSplitting.java:59:27:59:27 | t : String | semmle.label | t : String | +| ResponseSplitting.java:59:27:59:57 | replaceFirst(...) | semmle.label | replaceFirst(...) | subpaths #select | ResponseSplitting.java:23:23:23:28 | cookie | ResponseSplitting.java:22:39:22:66 | getParameter(...) : String | ResponseSplitting.java:23:23:23:28 | cookie | Response-splitting vulnerability due to this $@. | ResponseSplitting.java:22:39:22:66 | getParameter(...) | user-provided value | | ResponseSplitting.java:28:38:28:72 | getParameter(...) | ResponseSplitting.java:28:38:28:72 | getParameter(...) | ResponseSplitting.java:28:38:28:72 | getParameter(...) | Response-splitting vulnerability due to this $@. | ResponseSplitting.java:28:38:28:72 | getParameter(...) | user-provided value | | ResponseSplitting.java:29:38:29:72 | getParameter(...) | ResponseSplitting.java:29:38:29:72 | getParameter(...) | ResponseSplitting.java:29:38:29:72 | getParameter(...) | Response-splitting vulnerability due to this $@. | ResponseSplitting.java:29:38:29:72 | getParameter(...) | user-provided value | +| ResponseSplitting.java:59:27:59:57 | replaceFirst(...) | ResponseSplitting.java:53:14:53:48 | getParameter(...) : String | ResponseSplitting.java:59:27:59:57 | replaceFirst(...) | Response-splitting vulnerability due to this $@. | ResponseSplitting.java:53:14:53:48 | getParameter(...) | user-provided value | diff --git a/java/ql/test/query-tests/security/CWE-113/semmle/tests/ResponseSplitting.java b/java/ql/test/query-tests/security/CWE-113/semmle/tests/ResponseSplitting.java index d8bdd7e1731..b2ea8780e8e 100644 --- a/java/ql/test/query-tests/security/CWE-113/semmle/tests/ResponseSplitting.java +++ b/java/ql/test/query-tests/security/CWE-113/semmle/tests/ResponseSplitting.java @@ -48,4 +48,32 @@ public class ResponseSplitting extends HttpServlet { Cookie cookie2 = new Cookie("name", cookie.getName()); response.addCookie(cookie2); } + + public void sanitizerTests(HttpServletRequest request, HttpServletResponse response){ + String t = request.getParameter("contentType"); + + // GOOD: whitelist-based sanitization + response.setHeader("h", t.replaceAll("[^a-zA-Z]", "")); + + // BAD: not replacing all problematic characters + response.setHeader("h", t.replaceFirst("[^a-zA-Z]", "")); + + // GOOD: replace all line breaks + response.setHeader("h", t.replace('\n', ' ').replace('\r', ' ')); + + // FALSE NEGATIVE: replace only some line breaks + response.setHeader("h", t.replace('\n', ' ')); + + // FALSE NEGATIVE: replace only some line breaks + response.setHeader("h", t.replaceAll("\r", "")); + + // GOOD: replace all linebreaks with a simple regex + response.setHeader("h", t.replaceAll("\n", "").replaceAll("\r", "")); + + // GOOD: replace all linebreaks with a complex regex + response.setHeader("h", t.replaceAll("[\n\r]", "")); + + // GOOD: replace all linebreaks with a complex regex + response.setHeader("h", t.replaceAll("something|[a\nb\rc]+|somethingelse", "")); + } }