Address review comment

Handle more regex cases that cover line breaks
github · Apr 17, 2023 · f5702f5 · f5702f5
1 parent e167d3c
commit f5702f5
Show file tree

Hide file tree

Showing 2 changed files with 64 additions and 6 deletions.
diff --git 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
@@ -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 {