diff --git a/java/ql/lib/semmle/code/java/security/LogInjection.qll b/java/ql/lib/semmle/code/java/security/LogInjection.qll index f7cf673154d2..e60e6ed9a7f3 100644 --- a/java/ql/lib/semmle/code/java/security/LogInjection.qll +++ b/java/ql/lib/semmle/code/java/security/LogInjection.qll @@ -68,10 +68,10 @@ private predicate logInjectionSanitizer(MethodAccess ma) { ( // Replace anything not in an allow list target.getStringValue().matches("[^%]") and - not target.getStringValue().matches(["%\n%", "%\r%"]) + not target.getStringValue().matches("%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%") or // Replace line breaks - target.getStringValue() = ["\n", "\r"] + target.getStringValue() = ["\n", "\r", "\\n", "\\r", "\\R"] ) ) } @@ -103,17 +103,17 @@ private predicate logInjectionGuard(Guard g, Expr e, boolean branch) { // Allow anything except line breaks ( not target.getStringValue().matches("%[^%]%") and - not target.getStringValue().matches(["%\n%", "%\r%"]) + not target.getStringValue().matches("%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%") or - target.getStringValue().matches(["%[^%\n%]%", "%[^%\r%]%"]) + target.getStringValue().matches("%[^%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%]%") ) and branch = true or // Disallow line breaks ( - not target.getStringValue().matches(["%[^%\n%]%", "%[^%\r%]%"]) and + not target.getStringValue().matches("%[^%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%]%") and // Assuming a regex containing line breaks is correctly matching line breaks in a string - target.getStringValue().matches(["%\n%", "%\r%"]) + target.getStringValue().matches("%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%") ) and branch = false ) diff --git a/java/ql/test/query-tests/security/CWE-117/LogInjectionTest.java b/java/ql/test/query-tests/security/CWE-117/LogInjectionTest.java index 1f0561c08061..4bcdb197e152 100644 --- a/java/ql/test/query-tests/security/CWE-117/LogInjectionTest.java +++ b/java/ql/test/query-tests/security/CWE-117/LogInjectionTest.java @@ -43,11 +43,21 @@ public void testSanitizers() { logger.debug(source.replaceAll("\r", "")); // Safe logger.debug(source.replaceAll("\r", "\n")); // $ hasTaintFlow logger.debug(source.replaceAll("\r", "\r")); // $ hasTaintFlow + logger.debug(source.replaceAll("\\n", "")); // Safe + logger.debug(source.replaceAll("\\n", "\n")); // $ hasTaintFlow + logger.debug(source.replaceAll("\\n", "\r")); // $ hasTaintFlow + logger.debug(source.replaceAll("\\r", "")); // Safe + logger.debug(source.replaceAll("\\r", "\n")); // $ hasTaintFlow + logger.debug(source.replaceAll("\\r", "\r")); // $ hasTaintFlow + logger.debug(source.replaceAll("\\R", "")); // Safe + logger.debug(source.replaceAll("\\R", "\n")); // $ hasTaintFlow + logger.debug(source.replaceAll("\\R", "\r")); // $ hasTaintFlow logger.debug(source.replaceAll("[^a-zA-Z]", "")); // Safe logger.debug(source.replaceAll("[^a-zA-Z]", "\n")); // $ hasTaintFlow logger.debug(source.replaceAll("[^a-zA-Z]", "\r")); // $ hasTaintFlow logger.debug(source.replaceAll("[^a-zA-Z\n]", "")); // $ hasTaintFlow logger.debug(source.replaceAll("[^a-zA-Z\r]", "")); // $ hasTaintFlow + logger.debug(source.replaceAll("[^a-zA-Z\\R]", "")); // $ hasTaintFlow } public void testGuards() { @@ -66,6 +76,18 @@ public void testGuards() { logger.debug(source); // Safe } + if (source.matches(".*\\n.*")) { + logger.debug(source); // $ hasTaintFlow + } else { + logger.debug(source); // Safe + } + + if (Pattern.matches(".*\\n.*", source)) { + logger.debug(source); // $ hasTaintFlow + } else { + logger.debug(source); // Safe + } + if (source.matches(".*\r.*")) { logger.debug(source); // $ hasTaintFlow } else { @@ -78,6 +100,30 @@ public void testGuards() { logger.debug(source); // Safe } + if (source.matches(".*\\r.*")) { + logger.debug(source); // $ hasTaintFlow + } else { + logger.debug(source); // Safe + } + + if (Pattern.matches(".*\\r.*", source)) { + logger.debug(source); // $ hasTaintFlow + } else { + logger.debug(source); // Safe + } + + if (source.matches(".*\\R.*")) { + logger.debug(source); // $ hasTaintFlow + } else { + logger.debug(source); // Safe + } + + if (Pattern.matches(".*\\R.*", source)) { + logger.debug(source); // $ hasTaintFlow + } else { + logger.debug(source); // Safe + } + if (source.matches(".*")) { logger.debug(source); // Safe (assuming not DOTALL) } else { @@ -102,6 +148,18 @@ public void testGuards() { logger.debug(source); // $ hasTaintFlow } + if (source.matches("[^\\R]*")) { + logger.debug(source); // Safe + } else { + logger.debug(source); // $ hasTaintFlow + } + + if (Pattern.matches("[^\\R]*", source)) { + logger.debug(source); // Safe + } else { + logger.debug(source); // $ hasTaintFlow + } + if (source.matches("[^a-zA-Z]*")) { logger.debug(source); // $ hasTaintFlow } else {