Merge pull request #8410 from joefarebrother/sensitive-logging

Java: Promote Sensitive Logging query

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

@@ -0,0 +1,26 @@
+/** Provides configurations for sensitive logging queries. */
+
+import java
+import semmle.code.java.dataflow.ExternalFlow
+import semmle.code.java.dataflow.TaintTracking
+import semmle.code.java.security.SensitiveActions
+import DataFlow
+
+/** A variable that may hold sensitive information, judging by its name. * */
+class CredentialExpr extends Expr {
+  CredentialExpr() {
+    exists(Variable v | this = v.getAnAccess() |
+      v.getName().regexpMatch([getCommonSensitiveInfoRegex(), "(?i).*(username).*"]) and
+      not v.isFinal()
+    )
+  }
+}
+
+/** A data-flow configuration for identifying potentially-sensitive data flowing to a log output. */
+class SensitiveLoggerConfiguration extends TaintTracking::Configuration {
+  SensitiveLoggerConfiguration() { this = "SensitiveLoggerConfiguration" }
+
+  override predicate isSource(DataFlow::Node source) { source.asExpr() instanceof CredentialExpr }
+
+  override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "logging") }
+}

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

@@ -14,5 +14,6 @@ public static void main(String[] args) {
         String password = "Pass@0rd";
 
         // GOOD: user password is never written to debug log
+        logger.debug("User password changed")
     }
 }

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

@@ -0,0 +1,20 @@
+/**
+ * @name Insertion of sensitive information into log files
+ * @description Writing sensitive information to log files can allow that
+ *              information to be leaked to an attacker more easily.
+ * @kind path-problem
+ * @problem.severity warning
+ * @precision medium
+ * @id java/sensitive-log
+ * @tags security
+ *       external/cwe/cwe-532
+ */
+
+import java
+import semmle.code.java.security.SensitiveLoggingQuery
+import PathGraph
+
+from SensitiveLoggerConfiguration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where cfg.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "This $@ is written to a log file.", source.getNode(),
+  "potentially sensitive information"

java/ql/src/change-notes/2022-03-11-sensitive-logging.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* The query "Insertion of sensitive information into log files" (`java/sensitive-logging`) has been promoted from experimental to the main query pack. This query was originally [submitted as an experimental query by @luchua-bc](https://github.com/github/codeql/pull/3090).

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

@@ -1,49 +0,0 @@
-/**
- * @name Insertion of sensitive information into log files
- * @description Writing sensitive information to log files can give valuable
- *              guidance to an attacker or expose sensitive user information.
- * @kind path-problem
- * @problem.severity warning
- * @precision medium
- * @id java/sensitiveinfo-in-logfile
- * @tags security
- *       external/cwe/cwe-532
- */
-
-import java
-import semmle.code.java.dataflow.ExternalFlow
-import semmle.code.java.dataflow.TaintTracking
-import semmle.code.java.security.SensitiveActions
-import DataFlow
-import PathGraph
-
-/**
- * Gets a regular expression for matching names of variables that indicate the value being held may contain sensitive information
- */
-private string getACredentialRegex() { result = "(?i)(.*username|url).*" }
-
-/** Variable keeps sensitive information judging by its name * */
-class CredentialExpr extends Expr {
-  CredentialExpr() {
-    exists(Variable v | this = v.getAnAccess() |
-      v.getName().regexpMatch([getCommonSensitiveInfoRegex(), getACredentialRegex()])
-    )
-  }
-}
-
-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) { sinkNode(sink, "logging") }
-
-  override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
-    TaintTracking::localTaintStep(node1, node2)
-  }
-}
-
-from LoggerConfiguration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
-where cfg.hasFlowPath(source, sink)
-select sink.getNode(), source, sink, "Outputting $@ to log.", source.getNode(),
-  "sensitive information"

java/ql/test/query-tests/security/CWE-532/SensitiveLogInfo.ql

@@ -0,0 +1,11 @@
+import java
+import TestUtilities.InlineFlowTest
+import semmle.code.java.security.SensitiveLoggingQuery
+
+class HasFlowTest extends InlineFlowTest {
+  override DataFlow::Configuration getTaintFlowConfig() {
+    result instanceof SensitiveLoggerConfiguration
+  }
+
+  override DataFlow::Configuration getValueFlowConfig() { none() }
+}

java/ql/test/query-tests/security/CWE-532/Test.java

@@ -0,0 +1,16 @@
+import org.apache.logging.log4j.Logger;
+
+class Test {
+    void test(String password) {
+        Logger logger = null;
+
+        logger.info("User's password is: " + password); // $ hasTaintFlow
+    }   
+
+    void test2(String authToken) {
+        Logger logger = null;
+
+        logger.error("Auth failed for: " + authToken); // $ hasTaintFlow 
+    }
+
+}

java/ql/test/query-tests/security/CWE-532/options

@@ -0,0 +1 @@
+//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/apache-log4j-1.2.17:${testdir}/../../../stubs/apache-log4j-2.14.1:${testdir}/../../../stubs/apache-commons-logging-1.2:${testdir}/../../../stubs/jboss-logging-3.4.2:${testdir}/../../../stubs/slf4j-2.0.0:${testdir}/../../../stubs/scijava-common-2.87.1:${testdir}/../../../stubs/flogger-0.7.1:${testdir}/../../../stubs/google-android-9.0.0