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

Java: Promote Log Injection from experimental

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

@@ -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/Security/CWE/CWE-117/LogInjection.ql

@@ -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/LogInjectionGood.java

@@ -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/change-notes/2021-11-04-log-injection-query.md

@@ -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).

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

@@ -1,38 +0,0 @@
-/**
- * @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"

java/ql/src/experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll

@@ -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

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

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

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

@@ -1,40 +0,0 @@
-/**
- * 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")
-    |
-      (
-        m.getDeclaringType().getASourceSupertype*() = t or
-        m.getDeclaringType().extendsOrImplements*(t)
-      ) and
-      m.getReturnType() instanceof VoidType and
-      this = m.getAReference()
-    )
-    or
-    exists(RefType t, Method m | t.hasQualifiedName("android.util", "Log") |
-      m.hasName(["d", "e", "i", "v", "w", "wtf"]) and
-      m.getDeclaringType() = t and
-      this = m.getAReference()
-    )
-  }
-
-  /** Returns an argument which would be logged by this call. */
-  Argument getALogArgument() { result = this.getArgument(_) }
-}