github · aschackmull · Mar 24, 2021 · Feb 2, 2021 · Mar 2, 2021 · Mar 3, 2021
@@ -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>
@@ -0,0 +1,38 @@
+/**
+ * @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
+  }
+}
+
+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"
@@ -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;
+    }
+}
+
@@ -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;
+        }
+    }
+}
@@ -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)

@@ -0,0 +1,35 @@
+/**
+ * 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("com.google.common.flogger", "LoggingApi") or
+      t.hasQualifiedName("java.lang", "System$Logger") or
+      t.hasQualifiedName("java.util.logging", "Logger") or
+      t.hasQualifiedName("android.util", "Log")
+    |
+      (
+        m.getDeclaringType().getASourceSupertype*() = t or
+        m.getDeclaringType().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(_) }
+}