Merge pull request #14724 from egregius313/egregius313/java/environment-variable-injection

Java: Environment variable injection query

java/ql/lib/ext/hudson.model.yml

@@ -32,8 +32,11 @@ extensions:
       - ["hudson", "FilePath", True, "write", "(String,String)", "", "Argument[0]", "file-content-store", "manual"]
       - ["hudson", "Launcher$ProcStarter", False, "cmds", "", "", "Argument[0]", "command-injection", "manual"]
       - ["hudson", "Launcher$ProcStarter", False, "cmdAsSingleString", "", "", "Argument[0]", "command-injection", "manual"]
+      - ["hudson", "Launcher$ProcStarter", False, "envs", "(String[])", "", "Argument[0]", "environment-injection", "manual"]
       - ["hudson", "Launcher", True, "launch", "", "", "Argument[0]", "command-injection", "manual"]
+      - ["hudson", "Launcher", True, "decorateByEnv", "(String[])", "", "Argument[0]", "environment-injection", "manual"]
       - ["hudson", "Launcher", True, "launchChannel", "", "", "Argument[0]", "command-injection", "manual"]
+      - ["hudson", "Launcher", True, "launchChannel", "", "", "Argument[3]", "environment-injection", "manual"]
       - ["hudson", "XmlFile", False, "XmlFile", "(XStream,File)", "", "Argument[1]", "path-injection", "ai-manual"]
   - addsTo:
       pack: codeql/java-all

java/ql/lib/ext/java.lang.model.yml

@@ -22,6 +22,8 @@ extensions:
       - ["java.lang", "Runtime", True, "exec", "(String,String[])", "", "Argument[0]", "command-injection", "ai-manual"]
       - ["java.lang", "Runtime", True, "exec", "(String,String[],File)", "", "Argument[0]", "command-injection", "ai-manual"]
       - ["java.lang", "Runtime", True, "exec", "(String,String[],File)", "", "Argument[2]", "command-injection", "ai-manual"]
+      # All implementations of `java.lang.Runtime::exec` take the environment variables as their second argument.
+      - ["java.lang", "Runtime", True, "exec", "", "", "Argument[1]", "environment-injection", "manual"]
       # These are potential vulnerabilities, but not for command-injection. No query for this kind of vulnerability currently exists.
       # - ["java.lang", "Runtime", False, "load", "(String)", "", "Argument[0]", "command-injection", "ai-manual"]
       # - ["java.lang", "Runtime", False, "loadLibrary", "(String)", "", "Argument[0]", "command-injection", "ai-manual"]

java/ql/lib/ext/org.apache.commons.exec.environment.model.yml

@@ -0,0 +1,6 @@
+extensions:
+  - addsTo:
+      pack: codeql/java-all
+      extensible: sinkModel
+    data:
+      - ["org.apache.commons.exec.environment", "EnvironmentUtils", True, "addVariableToEnvironment", "(Map,String)", "", "Argument[0]", "environment-injection", "manual"]

java/ql/lib/ext/org.apache.commons.exec.launcher.model.yml

@@ -0,0 +1,6 @@
+extensions:
+  - addsTo:
+      pack: codeql/java-all
+      extensible: sinkModel
+    data:
+      - ["org.apache.commons.exec.launcher", "CommandLauncher", True, "exec", "", "", "Argument[1]", "environment-injection", "manual"]

java/ql/lib/ext/org.apache.commons.exec.model.yml

@@ -9,3 +9,5 @@ extensions:
       - ["org.apache.commons.exec", "CommandLine", True, "addArguments", "(String,boolean)", "", "Argument[0]", "command-injection", "manual"]
       - ["org.apache.commons.exec", "CommandLine", True, "addArguments", "(String[])", "", "Argument[0]", "command-injection", "manual"]
       - ["org.apache.commons.exec", "CommandLine", True, "addArguments", "(String[],boolean)", "", "Argument[0]", "command-injection", "manual"]
+      - ["org.apache.commons.exec", "Executor", True, "execute", "(CommandLine,Map)", "", "Argument[1]", "environment-injection", "manual"]
+      - ["org.apache.commons.exec", "Executor", True, "execute", "(CommandLine,Map,ExecuteResultHandler)", "", "Argument[1]", "environment-injection", "manual"]

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

@@ -0,0 +1,46 @@
+/** Modules to reason about the tainting of environment variables */
+
+private import semmle.code.java.dataflow.ExternalFlow
+private import semmle.code.java.dataflow.FlowSources
+private import semmle.code.java.Maps
+private import semmle.code.java.JDK
+
+private module ProcessBuilderEnvironmentConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    exists(MethodCall mc | mc = source.asExpr() |
+      mc.getMethod().hasQualifiedName("java.lang", "ProcessBuilder", "environment")
+    )
+  }
+
+  predicate isSink(DataFlow::Node sink) { sink.asExpr() = any(MapMutation mm).getQualifier() }
+}
+
+private module ProcessBuilderEnvironmentFlow = DataFlow::Global<ProcessBuilderEnvironmentConfig>;
+
+/**
+ * A node that acts as a sanitizer in configurations related to environment variable injection.
+ */
+abstract class ExecTaintedEnvironmentSanitizer extends DataFlow::Node { }
+
+/**
+ * A taint-tracking configuration that tracks flow from unvalidated data to an environment variable for a subprocess.
+ */
+module ExecTaintedEnvironmentConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source instanceof ThreatModelFlowSource }
+
+  predicate isBarrier(DataFlow::Node barrier) { barrier instanceof ExecTaintedEnvironmentSanitizer }
+
+  predicate isSink(DataFlow::Node sink) {
+    sinkNode(sink, "environment-injection")
+    or
+    // sink is a key or value added to a `ProcessBuilder::environment` map.
+    exists(MapMutation mm | mm.getAnArgument() = sink.asExpr() |
+      ProcessBuilderEnvironmentFlow::flowToExpr(mm.getQualifier())
+    )
+  }
+}
+
+/**
+ * Taint-tracking flow for unvalidated data to an environment variable for a subprocess.
+ */
+module ExecTaintedEnvironmentFlow = TaintTracking::Global<ExecTaintedEnvironmentConfig>;

java/ql/src/Security/CWE/CWE-078/ExecTaintedEnvironment.qhelp

@@ -0,0 +1,44 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Passing unvalidated user input into the environment variables of a subprocess can allow an attacker to execute malicious code.</p>
+</overview>
+
+<recommendation>
+<p>If possible, use hard-coded string literals to specify the environment variable or its value. 
+Instead of passing the user input directly to the
+process or library function, examine the user input and then choose
+among hard-coded string literals.</p>
+
+<p>If the applicable environment variables cannot be determined at
+compile time, then add code to verify that the user input string is
+safe before using it.</p>
+</recommendation>
+
+<example>
+<p>In the following (BAD) example, the environment variable <code>PATH</code> is set to the value of the user input <code>path</code> without validation.</p>
+
+<sample src="ExecTaintedEnvironmentValue.java" />
+
+<p>In the following (BAD) example, an environment variable is set with a name that is derived from the user input <code>var</code> without validation.</p>
+
+<sample src="ExecTaintedEnvironmentName.java" />
+<p>In the following (GOOD) example, the user's input is validated before being used to set the environment variable.</p>
+
+<sample src="ExecTaintedEnvironmentValidated.java" />
+
+<p>In the following (GOOD) example, the user's input is checked and used to determine an environment variable to add.</p>
+
+<sample src="ExecTaintedEnvironmentChecked.java" />
+</example>
+<references>
+  <li>
+    The Java Tutorials: <a href="https://docs.oracle.com/javase/tutorial/essential/environment/env.html">Environment Variables</a>.
+  </li>
+  <li>
+    OWASP: <a href="https://owasp.org/www-community/attacks/Command_Injection">Command injection</a>.
+  </li>
+</references>
+</qhelp>

java/ql/src/Security/CWE/CWE-078/ExecTaintedEnvironment.ql

@@ -0,0 +1,24 @@
+/**
+ * @name Building a command with an injected environment variable
+ * @description Passing environment variables containing externally controlled
+ *              strings to a command line is vulnerable to malicious changes to the
+ *              environment of a subprocess.
+ * @problem.severity error
+ * @kind path-problem
+ * @security-severity 9.8
+ * @precision medium
+ * @id java/exec-tainted-environment
+ * @tags security
+ *    external/cwe/cwe-078
+ *    external/cwe/cwe-088
+ *    external/cwe/cwe-454
+ */
+
+import java
+import semmle.code.java.security.TaintedEnvironmentVariableQuery
+import ExecTaintedEnvironmentFlow::PathGraph
+
+from ExecTaintedEnvironmentFlow::PathNode source, ExecTaintedEnvironmentFlow::PathNode sink
+where ExecTaintedEnvironmentFlow::flowPath(source, sink)
+select sink.getNode(), source, sink,
+  "This command will be execute with a tainted environment variable."

java/ql/src/Security/CWE/CWE-078/ExecTaintedEnvironmentChecked.java

@@ -0,0 +1,7 @@
+Map<String, String> env = builder.environment();
+String debug = request.getParameter("debug");
+
+// GOOD: Checking the value and not tainting the variable added to the environment
+if (debug != null) {
+    env.put("PYTHONDEBUG", "1");
+}

java/ql/src/Security/CWE/CWE-078/ExecTaintedEnvironmentName.java

@@ -0,0 +1,10 @@
+public void doGet(HttpServletRequest request, HttpServletResponse response) {
+    String attr = request.getParameter("attribute");
+    String value = request.getParameter("value");
+
+    Map<String, String> env = processBuilder.environment();
+    // BAD: attr and value are tainted and being added to the environment
+    env.put(attr, value);
+
+    processBuilder.start();
+}

java/ql/src/Security/CWE/CWE-078/ExecTaintedEnvironmentValidated.java

@@ -0,0 +1,9 @@
+String opt = request.getParameter("opt");
+String value = request.getParameter("value");
+
+Map<String, String> env = processBuilder.environment();
+
+// GOOD: opt and value are checked before being added to the environment
+if (permittedJavaOptions.contains(opt) && validOption(opt, value)) {
+    env.put(opt, value);
+}

java/ql/src/Security/CWE/CWE-078/ExecTaintedEnvironmentValue.java

@@ -0,0 +1,9 @@
+public void doGet(HttpServletRequest request, HttpServletResponse response) {
+    String path = request.getParameter("path");
+
+    Map<String, String> env = processBuilder.environment();
+    // BAD: path is tainted and being added to the environment
+    env.put("PATH", path);
+
+    processBuilder.start();
+}

java/ql/src/change-notes/2024-01-09-environment-variable-injection-query.md

@@ -0,0 +1,5 @@
+---
+category: newQuery
+---
+* Added the `java/exec-tainted-environment` query, to detect the injection of environment variables names or values from remote input.
+

java/ql/test/query-tests/security/CWE-078/ExecRelative.expected

@@ -1 +1,2 @@
+| TaintedEnvironment.java:39:35:39:55 | new String[] | Command with a relative path 'ls' is executed. |
 | Test.java:50:46:50:49 | "ls" | Command with a relative path 'ls' is executed. |

java/ql/test/query-tests/security/CWE-078/ExecTaintedEnvironment.ql

@@ -0,0 +1,12 @@
+import java
+import semmle.code.java.dataflow.FlowSources
+import semmle.code.java.security.TaintedEnvironmentVariableQuery
+import TestUtilities.InlineFlowTest
+
+private class TestSource extends RemoteFlowSource {
+  TestSource() { this.asExpr().(MethodCall).getMethod().hasName("source") }
+
+  override string getSourceType() { result = "test source" }
+}
+
+import TaintFlowTest<ExecTaintedEnvironmentConfig>

java/ql/test/query-tests/security/CWE-078/TaintedEnvironment.java

@@ -0,0 +1,41 @@
+import java.lang.ProcessBuilder;
+import java.lang.Runtime;
+import java.util.Map;
+
+public class TaintedEnvironment {
+    public Object source() {
+        return null;
+    }
+
+    public void buildProcess() throws java.io.IOException {
+        String s = (String) source();
+        ProcessBuilder pb = new ProcessBuilder();
+
+        pb.environment().put("foo", s); // $hasTaintFlow
+
+        pb.environment().put(s, "foo"); // $hasTaintFlow
+
+        Map<String, String> extra = Map.of("USER", s);
+
+        pb.environment().putAll(extra); // $hasTaintFlow
+
+        pb.environment().putIfAbsent("foo", s); // $hasTaintFlow
+        pb.environment().putIfAbsent(s, "foo"); // $hasTaintFlow
+
+        pb.environment().replace("foo", s); // $hasTaintFlow
+        pb.environment().replace(s, "foo"); // $hasTaintFlow
+        pb.environment().replace("foo", "bar", s); // $hasTaintFlow
+
+        Map<String, String> env = pb.environment();
+
+        env.put("foo", s); // $hasTaintFlow
+
+        pb.start();
+    }
+
+    public void exec() throws java.io.IOException {
+        String kv = (String) source();
+
+        Runtime.getRuntime().exec(new String[] { "ls" }, new String[] { kv }); // $hasTaintFlow
+    }
+}

shared/mad/codeql/mad/ModelValidation.qll

@@ -26,9 +26,9 @@ module KindValidation<KindValidationConfigSig Config> {
       this =
         [
           // shared
-          "code-injection", "command-injection", "file-content-store", "html-injection",
-          "js-injection", "ldap-injection", "log-injection", "path-injection", "request-forgery",
-          "sql-injection", "url-redirection",
+          "code-injection", "command-injection", "environment-injection", "file-content-store",
+          "html-injection", "js-injection", "ldap-injection", "log-injection", "path-injection",
+          "request-forgery", "sql-injection", "url-redirection",
           // Java-only currently, but may be shared in the future
           "bean-validation", "fragment-injection", "groovy-injection", "hostname-verification",
           "information-leak", "intent-redirection", "jexl-injection", "jndi-injection",