Merge remote-tracking branch 'origin/codeql-cli-2.16.1' into henrymercer/2.16.0-mergeback

java/ql/src/CHANGELOG.md

@@ -1,3 +1,13 @@
+## 0.8.7
+
+### New Queries
+
+* Added the `java/exec-tainted-environment` query, to detect the injection of environment variables names or values from remote input.
+
+### Minor Analysis Improvements
+
+* A manual neutral summary model for a callable now blocks all generated summary models for that callable from having any effect.
+
 ## 0.8.6
 
 ### Deprecated Queries

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/released/0.8.7.md

@@ -0,0 +1,9 @@
+## 0.8.7
+
+### New Queries
+
+* Added the `java/exec-tainted-environment` query, to detect the injection of environment variables names or values from remote input.
+
+### Minor Analysis Improvements
+
+* A manual neutral summary model for a callable now blocks all generated summary models for that callable from having any effect.

java/ql/src/codeql-pack.release.yml

@@ -1,2 +1,2 @@
 ---
-lastReleaseVersion: 0.8.6
+lastReleaseVersion: 0.8.7

java/ql/src/experimental/Security/CWE/CWE-020/Log4jJndiInjection.ql

@@ -19,6 +19,7 @@ import java
 import semmle.code.java.dataflow.TaintTracking
 import semmle.code.java.dataflow.FlowSources
 import semmle.code.java.dataflow.ExternalFlow
+private import semmle.code.java.security.Sanitizers
 import Log4jInjectionFlow::PathGraph
 
 private class ActivateModels extends ActiveExperimentalModels {
@@ -33,11 +34,7 @@ class Log4jInjectionSink extends DataFlow::Node {
 /**
  * A node that sanitizes a message before logging to avoid log injection.
  */
-class Log4jInjectionSanitizer extends DataFlow::Node {
-  Log4jInjectionSanitizer() {
-    this.getType() instanceof BoxedType or this.getType() instanceof PrimitiveType
-  }
-}
+class Log4jInjectionSanitizer extends DataFlow::Node instanceof SimpleTypeSanitizer { }
 
 /**
  * A taint-tracking configuration for tracking untrusted user input used in log entries.

java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql

@@ -18,6 +18,7 @@ import semmle.code.java.dataflow.ExternalFlow
 import semmle.code.java.dataflow.FlowSources
 import JFinalController
 import semmle.code.java.security.PathSanitizer
+private import semmle.code.java.security.Sanitizers
 import InjectFilePathFlow::PathGraph
 
 private class ActivateModels extends ActiveExperimentalModels {
@@ -56,7 +57,7 @@ module InjectFilePathConfig implements DataFlow::ConfigSig {
   }
 
   predicate isBarrier(DataFlow::Node node) {
-    exists(Type t | t = node.getType() | t instanceof BoxedType or t instanceof PrimitiveType)
+    node instanceof SimpleTypeSanitizer
     or
     node instanceof PathInjectionSanitizer
   }

java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExec.qll

@@ -2,6 +2,7 @@ import java
 import semmle.code.java.frameworks.javaee.ejb.EJBRestrictions
 import semmle.code.java.dataflow.DataFlow
 import semmle.code.java.dataflow.FlowSources
+private import semmle.code.java.security.Sanitizers
 
 module ExecCmdFlowConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) {
@@ -20,8 +21,7 @@ module ExecCmdFlowConfig implements DataFlow::ConfigSig {
     node instanceof AssignToNonZeroIndex or
     node instanceof ArrayInitAtNonZeroIndex or
     node instanceof StreamConcatAtNonZeroIndex or
-    node.getType() instanceof PrimitiveType or
-    node.getType() instanceof BoxedType
+    node instanceof SimpleTypeSanitizer
   }
 }
 
@@ -41,10 +41,7 @@ module ExecUserFlowConfig implements DataFlow::ConfigSig {
     )
   }
 
-  predicate isBarrier(DataFlow::Node node) {
-    node.getType() instanceof PrimitiveType or
-    node.getType() instanceof BoxedType
-  }
+  predicate isBarrier(DataFlow::Node node) { node instanceof SimpleTypeSanitizer }
 }
 
 /** Tracks flow of unvalidated user input that is used in Runtime.Exec */

java/ql/src/experimental/Security/CWE/CWE-089/MyBatisAnnotationSqlInjection.ql

@@ -17,6 +17,7 @@ import MyBatisCommonLib
 import MyBatisAnnotationSqlInjectionLib
 import semmle.code.java.dataflow.FlowSources
 import semmle.code.java.dataflow.TaintTracking
+private import semmle.code.java.security.Sanitizers
 import MyBatisAnnotationSqlInjectionFlow::PathGraph
 
 private module MyBatisAnnotationSqlInjectionConfig implements DataFlow::ConfigSig {
@@ -24,11 +25,7 @@ private module MyBatisAnnotationSqlInjectionConfig implements DataFlow::ConfigSi
 
   predicate isSink(DataFlow::Node sink) { sink instanceof MyBatisAnnotatedMethodCallArgument }
 
-  predicate isBarrier(DataFlow::Node node) {
-    node.getType() instanceof PrimitiveType or
-    node.getType() instanceof BoxedType or
-    node.getType() instanceof NumberType
-  }
+  predicate isBarrier(DataFlow::Node node) { node instanceof SimpleTypeSanitizer }
 
   predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
     exists(MethodCall ma |

java/ql/src/experimental/Security/CWE/CWE-089/MyBatisMapperXmlSqlInjection.ql

@@ -17,6 +17,7 @@ import MyBatisCommonLib
 import MyBatisMapperXmlSqlInjectionLib
 import semmle.code.xml.MyBatisMapperXML
 import semmle.code.java.dataflow.FlowSources
+private import semmle.code.java.security.Sanitizers
 import MyBatisMapperXmlSqlInjectionFlow::PathGraph
 
 private module MyBatisMapperXmlSqlInjectionConfig implements DataFlow::ConfigSig {
@@ -24,11 +25,7 @@ private module MyBatisMapperXmlSqlInjectionConfig implements DataFlow::ConfigSig
 
   predicate isSink(DataFlow::Node sink) { sink instanceof MyBatisMapperMethodCallAnArgument }
 
-  predicate isBarrier(DataFlow::Node node) {
-    node.getType() instanceof PrimitiveType or
-    node.getType() instanceof BoxedType or
-    node.getType() instanceof NumberType
-  }
+  predicate isBarrier(DataFlow::Node node) { node instanceof SimpleTypeSanitizer }
 
   predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
     exists(MethodCall ma |

java/ql/src/experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql

@@ -14,6 +14,7 @@
 import java
 import semmle.code.java.dataflow.TaintTracking
 import semmle.code.java.dataflow.FlowSources
+import semmle.code.java.security.Sanitizers
 import ClientSuppliedIpUsedInSecurityCheckLib
 import ClientSuppliedIpUsedInSecurityCheckFlow::PathGraph
 
@@ -38,9 +39,7 @@ module ClientSuppliedIpUsedInSecurityCheckConfig implements DataFlow::ConfigSig
       not aa.getIndexExpr().(CompileTimeConstantExpr).getIntValue() = 0
     )
     or
-    node.getType() instanceof PrimitiveType
-    or
-    node.getType() instanceof BoxedType
+    node instanceof SimpleTypeSanitizer
   }
 }
 

java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll

@@ -5,6 +5,7 @@ private import semmle.code.java.dataflow.FlowSources
 private import semmle.code.java.dataflow.StringPrefixes
 private import semmle.code.java.frameworks.javaee.ejb.EJBRestrictions
 private import experimental.semmle.code.java.frameworks.SpringResource
+private import semmle.code.java.security.Sanitizers
 
 private class ActiveModels extends ActiveExperimentalModels {
   ActiveModels() { this = "unsafe-url-forward" }
@@ -128,12 +129,7 @@ private class SpringModelAndViewSink extends UnsafeUrlForwardSink {
   }
 }
 
-private class PrimitiveSanitizer extends UnsafeUrlForwardSanitizer {
-  PrimitiveSanitizer() {
-    this.getType() instanceof PrimitiveType or
-    this.getType() instanceof BoxedType or
-    this.getType() instanceof NumberType
-  }
+private class PrimitiveSanitizer extends UnsafeUrlForwardSanitizer instanceof SimpleTypeSanitizer {
 }
 
 private class SanitizingPrefix extends InterestingPrefix {

java/ql/src/qlpack.yml

@@ -1,5 +1,5 @@
 name: codeql/java-queries
-version: 0.8.6
+version: 0.8.7
 groups:
   - java
   - queries

java/ql/src/utils/modelgenerator/internal/CaptureModelsSpecific.qll

@@ -5,6 +5,7 @@
 private import java as J
 private import semmle.code.java.dataflow.internal.DataFlowPrivate
 private import semmle.code.java.dataflow.internal.ContainerFlow as ContainerFlow
+private import semmle.code.java.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl
 private import semmle.code.java.dataflow.internal.ModelExclusions
 private import semmle.code.java.dataflow.DataFlow as Df
 private import semmle.code.java.dataflow.SSA as Ssa
@@ -21,6 +22,8 @@ class Type = J::Type;
 
 class Unit = J::Unit;
 
+class Callable = J::Callable;
+
 private J::Method superImpl(J::Method m) {
   result = m.getAnOverride() and
   not exists(result.getAnOverride()) and
@@ -35,15 +38,19 @@ private predicate isInfrequentlyUsed(J::CompilationUnit cu) {
 /**
  * Holds if it is relevant to generate models for `api`.
  */
-private predicate isRelevantForModels(J::Callable api) {
+private predicate isRelevantForModels(Callable api) {
   not isUninterestingForModels(api) and
-  not isInfrequentlyUsed(api.getCompilationUnit())
+  not isInfrequentlyUsed(api.getCompilationUnit()) and
+  // Disregard all APIs that have a manual model.
+  not api = any(FlowSummaryImpl::Public::SummarizedCallable sc | sc.applyManualModel()).asCallable() and
+  not api =
+    any(FlowSummaryImpl::Public::NeutralSummaryCallable sc | sc.hasManualModel()).asCallable()
 }
 
 /**
  * Holds if it is relevant to generate models for `api` based on data flow analysis.
  */
-predicate isRelevantForDataFlowModels(J::Callable api) {
+predicate isRelevantForDataFlowModels(Callable api) {
   isRelevantForModels(api) and
   (not api.getDeclaringType() instanceof J::Interface or exists(api.getBody()))
 }
@@ -56,7 +63,7 @@ predicate isRelevantForTypeBasedFlowModels = isRelevantForModels/1;
  * In the Standard library and 3rd party libraries it the Callables that can be called
  * from outside the library itself.
  */
-class TargetApiSpecific extends J::Callable {
+class TargetApiSpecific extends Callable {
   TargetApiSpecific() {
     this.isPublic() and
     this.fromSource() and
@@ -66,6 +73,15 @@ class TargetApiSpecific extends J::Callable {
     ) and
     isRelevantForModels(this)
   }
+
+  /**
+   * Gets the callable that a model will be lifted to, if any.
+   */
+  Callable lift() {
+    exists(Method m | m = superImpl(this) and m.fromSource() | result = m)
+    or
+    not exists(superImpl(this)) and result = this
+  }
 }
 
 private string isExtensible(J::RefType ref) {
@@ -79,9 +95,7 @@ private string typeAsModel(J::RefType type) {
 }
 
 private J::RefType bestTypeForModel(TargetApiSpecific api) {
-  if exists(superImpl(api))
-  then superImpl(api).fromSource() and result = superImpl(api).getDeclaringType()
-  else result = api.getDeclaringType()
+  result = api.lift().getDeclaringType()
 }
 
 /**
@@ -195,7 +209,7 @@ string returnNodeAsOutput(DataFlowImplCommon::ReturnNodeExt node) {
 /**
  * Gets the enclosing callable of `ret`.
  */
-J::Callable returnNodeEnclosingCallable(DataFlowImplCommon::ReturnNodeExt ret) {
+Callable returnNodeEnclosingCallable(DataFlowImplCommon::ReturnNodeExt ret) {
   result = DataFlowImplCommon::getNodeEnclosingCallable(ret).asCallable()
 }
 

java/ql/src/utils/modelgenerator/internal/CaptureSummaryFlowQuery.qll

@@ -73,10 +73,11 @@ string captureFlow(DataFlowTargetApi api) {
 }
 
 /**
- * Gets the neutral summary for `api`, if any.
- * A neutral model is generated, if there does not exist any summary model.
+ * Gets the neutral summary model for `api`, if any.
+ * A neutral summary model is generated, if we are not generating
+ * a summary model that applies to `api`.
  */
 string captureNoFlow(DataFlowTargetApi api) {
-  not exists(captureFlow(api)) and
+  not exists(DataFlowTargetApi api0 | exists(captureFlow(api0)) and api0.lift() = api.lift()) and
   result = ModelPrinting::asNeutralSummaryModel(api)
 }