Merge pull request #7054 from atorralba/atorralba/promote-log-injection

Java: Promote Log Injection from experimental

Author: atorralba

java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll

@@ -95,6 +95,7 @@ private module Frameworks {
   private import semmle.code.java.frameworks.JaxWS
   private import semmle.code.java.frameworks.JoddJson
   private import semmle.code.java.frameworks.JsonJava
+  private import semmle.code.java.frameworks.Logging
   private import semmle.code.java.frameworks.Objects
   private import semmle.code.java.frameworks.Optional
   private import semmle.code.java.frameworks.Stream

java/ql/lib/semmle/code/java/frameworks/Logging.qll

@@ -0,0 +1,36 @@
+/** Provides classes and predicates related to Log Injection vulnerabilities. */
+
+import java
+import semmle.code.java.dataflow.DataFlow
+import semmle.code.java.dataflow.ExternalFlow
+
+/** A data flow sink for unvalidated user input that is used to log messages. */
+abstract class LogInjectionSink extends DataFlow::Node { }
+
+/**
+ * A node that sanitizes a message before logging to avoid log injection.
+ */
+abstract class LogInjectionSanitizer extends DataFlow::Node { }
+
+/**
+ * A unit class for adding additional taint steps.
+ *
+ * Extend this class to add additional taint steps that should apply to the `LogInjectionConfiguration`.
+ */
+class LogInjectionAdditionalTaintStep extends Unit {
+  /**
+   * Holds if the step from `node1` to `node2` should be considered a taint
+   * step for the `LogInjectionConfiguration` configuration.
+   */
+  abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
+}
+
+private class DefaultLogInjectionSink extends LogInjectionSink {
+  DefaultLogInjectionSink() { sinkNode(this, "logging") }
+}
+
+private class DefaultLogInjectionSanitizer extends LogInjectionSanitizer {
+  DefaultLogInjectionSanitizer() {
+    this.getType() instanceof BoxedType or this.getType() instanceof PrimitiveType
+  }
+}

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

@@ -0,0 +1,22 @@
+/** Provides taint tracking configurations to be used in queries related to the Log Injection vulnerability. */
+
+import java
+import semmle.code.java.dataflow.FlowSources
+import semmle.code.java.security.LogInjection
+
+/**
+ * A taint-tracking configuration for tracking untrusted user input used in log entries.
+ */
+class LogInjectionConfiguration extends TaintTracking::Configuration {
+  LogInjectionConfiguration() { this = "LogInjectionConfiguration" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof LogInjectionSink }
+
+  override predicate isSanitizer(DataFlow::Node node) { node instanceof LogInjectionSanitizer }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
+    any(LogInjectionAdditionalTaintStep c).step(node1, node2)
+  }
+}

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

@@ -29,16 +29,15 @@ other forms of HTML injection.
 </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>). 
+  <p>In the first 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>
+<p> In the second example (<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 logged at all, preventing the injection.</p>
 
 <sample src="LogInjectionGood.java" />
 </example>

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

@@ -0,0 +1,21 @@
+/**
+ * @name Log Injection
+ * @description Building log entries from user-controlled data may allow
+ *              insertion of forged log entries by malicious users.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 7.8
+ * @precision medium
+ * @id java/log-injection
+ * @tags security
+ *       external/cwe/cwe-117
+ */
+
+import java
+import semmle.code.java.security.LogInjectionQuery
+import DataFlow::PathGraph
+
+from LogInjectionConfiguration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where cfg.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "This $@ flows to a log entry.", source.getNode(),
+  "user-provided value"

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

@@ -16,9 +16,9 @@ public class LogInjection {
     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*")) {
+        if (username.matches("\\w*")) {
             log.warn("User:'{}'", username);
-            
+
             return username;
         }
     }

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

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* The query "Log Injection" (`java/log-injection`) has been promoted from experimental to the main query pack. Its results will now appear by default. The query was originally [submitted as an experimental query by @porcupineyhairs and @dellalibera](https://github.com/github/codeql/pull/5099).