Add Log Injection Vulnerability

java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp

@@ -0,0 +1,49 @@
+
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+
+<p>If unsanitized user input is written to a log entry, a malicious user may be able to forge new log entries.</p>
+
+<p>Forgery can occur if a user provides some input creating the appearance of multiple
+  log entries. This can include unescaped new-line characters, or HTML or other markup.</p>
+</overview>
+
+<recommendation>
+<p>
+User input should be suitably sanitized before it is logged.
+</p>
+<p>
+If the log entries are plain text then line breaks should be removed from user input, using for example
+<code>String replace(char oldChar, char newChar)</code> or similar. Care should also be taken that user input is clearly marked
+in log entries, and that a malicious user cannot cause confusion in other ways.
+</p>
+<p>
+For log entries that will be displayed in HTML, user input should be HTML encoded before being logged, to prevent forgery and
+other forms of HTML injection.
+</p>
+
+</recommendation>
+
+<example>
+  <p>In the example, a username, provided by the user, is logged using <code>logger.warn</code> (from  <code>org.slf4j.Logger</code>). 
+    In the first case (<code>/bad</code> endpoint), the username is logged without any sanitization.
+    If a malicious user provides <code>Guest'%0AUser:'Admin</code> as a username parameter, 
+    the log entry will be split into two separate lines, where the first line will be <code>User:'Guest'</code> and the second one will be <code>User:'Admin'</code>.
+</p>
+<sample src="LogInjectionBad.java" />
+
+<p> In the second case (<code>/good</code> endpoint), <code>matches()</code> is used to ensure the user input only has alphanumeric characters.
+  If a malicious user provides `Guest'%0AUser:'Admin` as a username parameter, 
+  the log entry will not be split into two separate lines, resulting in a single line <code>User:'Guest'User:'Admin'</code>.</p>
+
+<sample src="LogInjectionGood.java" />
+</example>
+
+<references>
+<li>OWASP: <a href="https://owasp.org/www-community/attacks/Log_Injection">Log Injection</a>.</li>
+</references>
+</qhelp>

java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql

@@ -0,0 +1,67 @@
+/**
+ * @name Log Injection
+ * @description Building log entries from user-controlled data is vulnerable to
+ *              insertion of forged log entries by a malicious user.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id java/log-injection
+ * @tags security
+ *       external/cwe/cwe-117
+ */
+
+import java
+import DataFlow::PathGraph
+import experimental.semmle.code.java.Logging
+import semmle.code.java.dataflow.FlowSources
+
+/**
+ * A taint-tracking configuration for tracking untrusted user input used in log entries.
+ */
+private class LogInjectionConfiguration extends TaintTracking::Configuration {
+  LogInjectionConfiguration() { this = "Log Injection" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node sink) {
+    sink.asExpr() = any(LoggingCall c).getALogArgument()
+  }
+
+  override predicate isSanitizer(DataFlow::Node node) {
+    node.getType() instanceof BoxedType or node.getType() instanceof PrimitiveType
+  }
+
+  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+    guard instanceof StrCheckSanitizerGuard
+  }
+}
+
+/**
+ * Models any regex or equality check as a sanitizer guard.
+ * Assumes any check on the taint to be a valid sanitizing check.
+ */
+private class StrCheckSanitizerGuard extends DataFlow::BarrierGuard {
+  StrCheckSanitizerGuard() {
+    exists(Method m |
+      m.getDeclaringType().hasQualifiedName("java.util.regex", "Pattern") and
+      m.hasName("matches")
+      or
+      m.getDeclaringType() instanceof TypeString and
+      m.hasName([
+          "startsWith", "regionMatches", "matches", "equals", "equalsIgnoreCase", "endsWith",
+          "contentEquals", "contains"
+        ])
+    |
+      m.getAReference() = this
+    )
+  }
+
+  override predicate checks(Expr e, boolean branch) {
+    e = this.(MethodAccess).getQualifier() and branch = true
+  }
+}
+
+from LogInjectionConfiguration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where cfg.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "$@ flows to log entry.", source.getNode(),
+  "User-provided value"

java/ql/src/experimental/Security/CWE/CWE-117/LogInjectionBad.java

@@ -0,0 +1,24 @@
+package com.example.restservice;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.springframework.web.bind.annotation.GetMapping;
+import org.springframework.web.bind.annotation.RequestParam;
+import org.springframework.web.bind.annotation.RestController;
+
+@RestController
+public class LogInjection {
+
+    private final Logger log = LoggerFactory.getLogger(LogInjection.class);
+
+    // /bad?username=Guest'%0AUser:'Admin
+    @GetMapping("/bad")
+    public String bad(@RequestParam(value = "username", defaultValue = "name") String username) {
+        log.warn("User:'{}'", username);
+        // The logging call above would result in multiple log entries as shown below:
+        // User:'Guest'
+        // User:'Admin'
+        return username;
+    }
+}
+

java/ql/src/experimental/Security/CWE/CWE-117/LogInjectionGood.java

@@ -0,0 +1,25 @@
+package com.example.restservice;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.springframework.web.bind.annotation.GetMapping;
+import org.springframework.web.bind.annotation.RequestParam;
+import org.springframework.web.bind.annotation.RestController;
+
+@RestController
+public class LogInjection {
+
+    private final Logger log = LoggerFactory.getLogger(LogInjection.class);
+
+    // /good?username=Guest'%0AUser:'Admin
+    @GetMapping("/good")
+    public String good(@RequestParam(value = "username", defaultValue = "name") String username) {
+        // The regex check here, allows only alphanumeric characters to pass.
+        // Hence, does not result in log injection
+        if (username.matches("\w*")) {
+            log.warn("User:'{}'", username);
+
+            return username;
+        }
+    }
+}

java/ql/src/experimental/Security/CWE/CWE-532/SensitiveInfoLog.ql

@@ -10,6 +10,7 @@
 import java
 import semmle.code.java.dataflow.TaintTracking
 import semmle.code.java.security.SensitiveActions
+import experimental.semmle.code.java.Logging
 import DataFlow
 import PathGraph
 
@@ -27,38 +28,14 @@ class CredentialExpr extends Expr {
   }
 }
 
-/** Class of popular logging utilities * */
-class LoggerType extends RefType {
-  LoggerType() {
-    this.hasQualifiedName("org.apache.log4j", "Category") or //Log4J
-    this.hasQualifiedName("org.apache.logging.log4j", "Logger") or //Log4J 2
-    this.hasQualifiedName("org.slf4j", "Logger") or //SLF4j and Gradle Logging
-    this.hasQualifiedName("org.jboss.logging", "BasicLogger") or //JBoss Logging
-    this.hasQualifiedName("org.jboss.logging", "Logger") or //JBoss Logging (`org.jboss.logging.Logger` in some implementations like JBoss Application Server 4.0.4 did not implement `BasicLogger`)
-    this.hasQualifiedName("org.apache.commons.logging", "Log") or //Apache Commons Logging
-    this.hasQualifiedName("org.scijava.log", "Logger") //SciJava Logging
-  }
-}
-
-predicate isSensitiveLoggingSink(DataFlow::Node sink) {
-  exists(MethodAccess ma |
-    ma.getMethod().getDeclaringType() instanceof LoggerType and
-    (
-      ma.getMethod().hasName("debug") or
-      ma.getMethod().hasName("trace") or
-      ma.getMethod().hasName("debugf") or
-      ma.getMethod().hasName("debugv")
-    ) and //Check low priority log levels which are more likely to be real issues to reduce false positives
-    sink.asExpr() = ma.getAnArgument()
-  )
-}
-
 class LoggerConfiguration extends DataFlow::Configuration {
   LoggerConfiguration() { this = "Logger Configuration" }
 
   override predicate isSource(DataFlow::Node source) { source.asExpr() instanceof CredentialExpr }
 
-  override predicate isSink(DataFlow::Node sink) { isSensitiveLoggingSink(sink) }
+  override predicate isSink(DataFlow::Node sink) {
+    exists(LoggingCall c | sink.asExpr() = c.getALogArgument())
+  }
 
   override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
     TaintTracking::localTaintStep(node1, node2)

java/ql/src/experimental/semmle/code/java/Logging.qll

@@ -0,0 +1,30 @@
+/**
+ * Provides classes and predicates for working with loggers.
+ */
+
+import java
+
+/** Models a call to a logging method. */
+class LoggingCall extends MethodAccess {
+  LoggingCall() {
+    exists(RefType t, Method m |
+      t.hasQualifiedName("org.apache.log4j", "Category") or // Log4j 1
+      t.hasQualifiedName("org.apache.logging.log4j", ["Logger", "LogBuilder"]) or // Log4j 2      
+      t.hasQualifiedName("org.apache.commons.logging", "Log") or
+      // JBoss Logging (`org.jboss.logging.Logger` in some implementations like JBoss Application Server 4.0.4 did not implement `BasicLogger`)
+      t.hasQualifiedName("org.jboss.logging", ["BasicLogger", "Logger"]) or
+      t.hasQualifiedName("org.slf4j.spi", "LoggingEventBuilder") or
+      t.hasQualifiedName("org.slf4j", "Logger") or
+      t.hasQualifiedName("org.scijava.log", "Logger") or
+      t.hasQualifiedName("java.lang", "System$Logger") or
+      t.hasQualifiedName("java.util.logging","Logger")
+    |
+      m.getDeclaringType().(RefType).extendsOrImplements*(t) and
+      m.getReturnType() instanceof VoidType and
+      this = m.getAReference()
+    )
+  }
+
+  /** Returns an argument which would be logged by this call. */
+  Argument getALogArgument() { result = this.getArgument(_) }
+}