Add line break sanitizers

java/ql/lib/change-notes/2022-10-06-log-injection-sanitizers.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Added sanitizers that recognize line breaks to the query `java/log-injection`.

java/ql/lib/semmle/code/java/security/LogInjection.qll

@@ -1,8 +1,9 @@
 /** Provides classes and predicates related to Log Injection vulnerabilities. */
 
 import java
-import semmle.code.java.dataflow.DataFlow
+private import semmle.code.java.dataflow.DataFlow
 private import semmle.code.java.dataflow.ExternalFlow
+private import semmle.code.java.controlflow.Guards
 
 /** A data flow sink for unvalidated user input that is used to log messages. */
 abstract class LogInjectionSink extends DataFlow::Node { }
@@ -31,6 +32,90 @@ private class DefaultLogInjectionSink extends LogInjectionSink {
 
 private class DefaultLogInjectionSanitizer extends LogInjectionSanitizer {
   DefaultLogInjectionSanitizer() {
-    this.getType() instanceof BoxedType or this.getType() instanceof PrimitiveType
+    this.getType() instanceof BoxedType or
+    this.getType() instanceof PrimitiveType or
+    this.getType() instanceof NumericType
   }
 }
+
+private class LineBreaksLogInjectionSanitizer extends LogInjectionSanitizer {
+  LineBreaksLogInjectionSanitizer() {
+    logInjectionSanitizer(this.asExpr())
+    or
+    this = DataFlow::BarrierGuard<logInjectionGuard/3>::getABarrierNode()
+  }
+}
+
+/**
+ * Holds if the return value of `ma` is sanitized against log injection attacks
+ * by removing line breaks from it.
+ */
+private predicate logInjectionSanitizer(MethodAccess ma) {
+  exists(CompileTimeConstantExpr target, CompileTimeConstantExpr replacement |
+    ma.getMethod().getDeclaringType() instanceof TypeString and
+    target = ma.getArgument(0) and
+    replacement = ma.getArgument(1) and
+    not replacement.getStringValue().matches(["%\n%", "%\r%"])
+  |
+    ma.getMethod().hasName("replace") and
+    not replacement.getIntValue() = [10, 13] and
+    (
+      target.getIntValue() = [10, 13] or // 10 == '\n', 13 == '\r'
+      target.getStringValue() = ["\n", "\r"]
+    )
+    or
+    ma.getMethod().hasName("replaceAll") and
+    (
+      // Replace anything not in an allow list
+      target.getStringValue().matches("[^%]") and
+      not target.getStringValue().matches(["%\n%", "%\r%"])
+      or
+      // Replace line breaks
+      target.getStringValue() = ["\n", "\r"]
+    )
+  )
+}
+
+/**
+ * Holds if `g` guards `e` in branch `branch` against log injection attacks
+ * by checking if there are line breaks in `e`.
+ */
+private predicate logInjectionGuard(Guard g, Expr e, boolean branch) {
+  exists(MethodAccess ma, CompileTimeConstantExpr target |
+    ma = g and
+    target = ma.getArgument(0)
+  |
+    ma.getMethod().getDeclaringType() instanceof TypeString and
+    ma.getMethod().hasName("contains") and
+    target.getStringValue() = ["\n", "\r"] and
+    e = ma.getQualifier() and
+    branch = false
+    or
+    ma.getMethod().hasName("matches") and
+    (
+      ma.getMethod().getDeclaringType() instanceof TypeString and
+      e = ma.getQualifier()
+      or
+      ma.getMethod().getDeclaringType().hasQualifiedName("java.util.regex", "Pattern") and
+      e = ma.getArgument(1)
+    ) and
+    (
+      // Allow anything except line breaks
+      (
+        not target.getStringValue().matches("%[^%]%") and
+        not target.getStringValue().matches(["%\n%", "%\r%"])
+        or
+        target.getStringValue().matches(["%[^%\n%]%", "%[^%\r%]%"])
+      ) and
+      branch = true
+      or
+      // Disallow line breaks
+      (
+        not target.getStringValue().matches(["%[^%\n%]%", "%[^%\r%]%"]) and
+        // Assuming a regex containing line breaks is correctly matching line breaks in a string
+        target.getStringValue().matches(["%\n%", "%\r%"])
+      ) and
+      branch = false
+    )
+  )
+}

java/ql/test/query-tests/security/CWE-117/LogInjectionTest.java

@@ -1,5 +1,6 @@
 import java.util.ResourceBundle;
 import java.util.logging.LogRecord;
+import java.util.regex.Pattern;
 import com.google.common.flogger.LoggingApi;
 import org.apache.commons.logging.Log;
 import org.apache.log4j.Category;
@@ -19,6 +20,114 @@ public class LogInjectionTest {
         return null;
     }
 
+    public void testSanitizers() {
+        String source = (String) source();
+        Logger logger = null;
+        logger.debug(source.replace("\n", "")); // Safe
+        logger.debug(source.replace("\n", "\n")); // $ hasTaintFlow
+        logger.debug(source.replace("\n", "\r")); // $ hasTaintFlow
+        logger.debug(source.replace("\r", "")); // Safe
+        logger.debug(source.replace("\r", "\n")); // $ hasTaintFlow
+        logger.debug(source.replace("\r", "\r")); // $ hasTaintFlow
+        logger.debug(source.replace("something_else", "")); // $ hasTaintFlow
+        logger.debug(source.replace('\n', '_')); // Safe
+        logger.debug(source.replace('\n', '\n')); // $ hasTaintFlow
+        logger.debug(source.replace('\n', '\r')); // $ hasTaintFlow
+        logger.debug(source.replace('\r', '_')); // Safe
+        logger.debug(source.replace('\r', '\n')); // $ hasTaintFlow
+        logger.debug(source.replace('\r', '\r')); // $ hasTaintFlow
+        logger.debug(source.replace('-', '_')); // $ 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("[^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
+    }
+
+    public void testGuards() {
+        String source = (String) source();
+        Logger logger = null;
+
+        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 {
+            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 {
+            logger.debug(source); // $ hasTaintFlow
+        }
+
+        if (Pattern.matches(".*", source)) {
+            logger.debug(source); // Safe (assuming not DOTALL)
+        } else {
+            logger.debug(source); // $ hasTaintFlow
+        }
+
+        if (source.matches("[^\n\r]*")) {
+            logger.debug(source); // Safe
+        } else {
+            logger.debug(source); // $ hasTaintFlow
+        }
+
+        if (Pattern.matches("[^\n\r]*", source)) {
+            logger.debug(source); // Safe
+        } else {
+            logger.debug(source); // $ hasTaintFlow
+        }
+
+        if (source.matches("[^a-zA-Z]*")) {
+            logger.debug(source); // $ hasTaintFlow
+        } else {
+            logger.debug(source); // $ hasTaintFlow
+        }
+
+        if (Pattern.matches("[^a-zA-Z]*", source)) {
+            logger.debug(source); // $ hasTaintFlow
+        } else {
+            logger.debug(source); // $ hasTaintFlow
+        }
+
+        if (source.matches("[\n]*")) {
+            logger.debug(source); // $ hasTaintFlow
+        } else {
+            logger.debug(source); // $ MISSING: $ hasTaintFlow
+        }
+
+        if (Pattern.matches("[\n]*", source)) {
+            logger.debug(source); // $ hasTaintFlow
+        } else {
+            logger.debug(source); // $ MISSING: $ hasTaintFlow
+        }
+
+    }
+
     public void test() {
         {
             Category category = null;