github · dellalibera · Jul 2, 2020 · Jul 2, 2020 · Jul 3, 2020 · Jul 3, 2020
@@ -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 with characters that are interpreted
+when the log output is displayed. If the log is displayed as a plain text file, then new
+line characters can be used by a malicious user. If the log is displayed as HTML, then
+arbitrary HTML may be included to spoof log entries.</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 `logger.warn` (from `org.slf4j.Logger`). 
+In the first case (`/bad` endpoint), the username is logged without any sanitization.
+If a malicious user provides `Guest'%0AUser:'Admin` as a username parameter, 
+the log entry will be splitted in two separate lines, where the first line will be `User:'Guest'`, while the second one will be `User:'Admin'`.
+</p>
+<p> In the second case (`/good` endpoint), <code>replace</code> is used to ensure no line endings are present in the user input.
+If a malicious user provides `Guest'%0AUser:'Admin` as a username parameter, 
+the log entry will not be splitted in two separate lines, resulting in a single line `User:'Guest'User:'Admin'`.
+</p>
+
+<sample src="examples/LogInjection.java" />
+</example>
+
+<references>
+<li>OWASP: <a href="https://www.owasp.org/index.php/Log_Injection">Log Injection</a>.</li>
+</references>
+</qhelp>
@@ -0,0 +1,21 @@
+/**
+ * @name Log Injection
+ * @description Building log entries from user-controlled sources 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 semmle.code.java.dataflow.FlowSources
+import LogInjection::LogInjection
+import DataFlow::PathGraph
+
+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,84 @@
+/**
+ * Provides a taint-tracking configuration for reasoning about untrusted user input used in log entries.
+ */
+
+import java
+import semmle.code.java.dataflow.FlowSources
+
+module LogInjection {
+  string logLevel() {
+    result = "trace" or
+    result = "info" or
+    result = "warn" or
+    result = "error" or
+    result = "fatal"
-    result = "trace" or
-    result = "info" or
-    result = "warn" or
-    result = "error" or
-    result = "fatal"
+    result in ["trace", "info", "warn", "error", "fatal"]
-    result = "trace" or
-    result = "info" or
-    result = "warn" or
-    result = "error" or
-    result = "fatal"
+    result in ["trace", "info", "warn", "error", "fatal"]
+  }
+
+  /**
+   * A data flow source for user input used in log entries.
+   */
+  abstract class Source extends DataFlow::Node { }
+
+  /**
+   * A data flow sink for user input used in log entries.
+   */
+  abstract class Sink extends DataFlow::Node { }
+
+  /**
+   * A sanitizer for malicious user input used in log entries.
+   */
+  abstract class Sanitizer extends DataFlow::Node { }
+
+  /**
+   * A taint-tracking configuration for untrusted user input used in log entries.
+   */
+  class LogInjectionConfiguration extends TaintTracking::Configuration {
+    LogInjectionConfiguration() { this = "LogInjection" }
+
+    override predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+    override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+
+    override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
+  }
+
+  /**
+   * A source of remote user controlled input.
+   */
+  class RemoteSource extends Source {
+    RemoteSource() { this instanceof RemoteFlowSource }
+  }
+
+  /**
+   * A class of popular logging utilities.
+   */
+  class LoggingType extends RefType {
+    LoggingType() {
+      this.hasQualifiedName("org.apache.logging.log4j", "Logger") or
+      this.hasQualifiedName("org.slf4j", "Logger")
+    }
+  }
+
+  /**
+   * A method call to a logging mechanism.
+   */
+  class LoggingCall extends MethodAccess {
+    LoggingCall() {
+      this.getMethod().getDeclaringType() instanceof LoggingType and
+      this.getMethod().hasName(logLevel())
+    }
+  }
+
+  /**
+   * An argument to a logging mechanism.
+   */
+  class LoggingSink extends Sink {
+    LoggingSink() { this.asExpr() = any(LoggingCall console).getAnArgument() }
-    LoggingSink() { this.asExpr() = any(LoggingCall console).getAnArgument() }
+    LoggingSink() { this.asExpr() = any(LoggingCall lc).getAnArgument() }
-    LoggingSink() { this.asExpr() = any(LoggingCall console).getAnArgument() }
+    LoggingSink() { this.asExpr() = any(LoggingCall lc).getAnArgument() }
+  }
+
+  class TypeSanitizer extends Sanitizer {
+    TypeSanitizer() {
+      this.getType() instanceof PrimitiveType or this.getType() instanceof BoxedType
+    }
+  }
+}
@@ -0,0 +1,33 @@
+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) {
+        username = username.replace("\n", "");
+        log.warn("User:'{}'", username);
+        return username;
+        // User:'Guest'User:'Admin'
+    }
+
+    // /bad?username=Guest'%0AUser:'Admin
+    @GetMapping("/bad")
+    public String bad(@RequestParam(value = "username", defaultValue = "name") String username) {
+        log.warn("User:'{}'", username);
+        return username;
+        // User:'Guest'
+        // User:'Admin'
+
+    }
+
+}