github · atorralba · Jan 11, 2022 · Oct 29, 2021 · Oct 29, 2021 · Nov 2, 2021
diff --git a/java/change-notes/2021-11-04-log-injection-query.md b/java/change-notes/2021-11-04-log-injection-query.md
@@ -0,0 +1,2 @@
+lgtm,codescanning
+* 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).
@@ -92,6 +92,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

@@ -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
+  }
+}
@@ -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)
+  }
+}
@@ -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>

@@ -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 high
+ * @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"
@@ -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;
         }
     }

@@ -2,7 +2,6 @@ import java
 import DataFlow
 import semmle.code.java.frameworks.Networking
 import semmle.code.java.security.QueryInjection
-import experimental.semmle.code.java.Logging
 
 /**
  * A data flow source of the client ip obtained according to the remote endpoint identifier specified

@@ -11,9 +11,9 @@
  */
 
 import java
+import semmle.code.java.dataflow.ExternalFlow
 import semmle.code.java.dataflow.TaintTracking
 import semmle.code.java.security.SensitiveActions
-import experimental.semmle.code.java.Logging
 import DataFlow
 import PathGraph
 
@@ -36,9 +36,7 @@ class LoggerConfiguration extends DataFlow::Configuration {
 
   override predicate isSource(DataFlow::Node source) { source.asExpr() instanceof CredentialExpr }
 
-  override predicate isSink(DataFlow::Node sink) {
-    exists(LoggingCall c | sink.asExpr() = c.getALogArgument())
-  }
+  override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "logging") }
 
   override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
     TaintTracking::localTaintStep(node1, node2)
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		lgtm,codescanning
		* 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).