Merge branch 'main' into java/experimental/command-injection

java/ql/src/CHANGELOG.md

@@ -1,3 +1,9 @@
+## 0.6.3
+
+### Minor Analysis Improvements
+
+* The `java/summary/lines-of-code` query now only counts lines of Java code. The new `java/summary/lines-of-code-kotlin` counts lines of Kotlin code.
+
 ## 0.6.2
 
 ### Minor Analysis Improvements

java/ql/src/Security/CWE/CWE-022/ZipSlip.qhelp

@@ -3,17 +3,15 @@
   "qhelp.dtd">
 <qhelp>
 <overview>
-<p>Extracting files from a malicious zip archive (or another archive format)
-without validating that the destination file path
-is within the destination directory can cause files outside the destination directory to be
-overwritten, due to the possible presence of directory traversal elements (<code>..</code>) in
-archive paths.</p>
+<p>Extracting files from a malicious zip file, or similar type of archive,
+is at risk of directory traversal attacks if filenames from the archive are
+not properly validated.</p>
 
 <p>Zip archives contain archive entries representing each file in the archive. These entries
 include a file path for the entry, but these file paths are not restricted and may contain
 unexpected special elements such as the directory traversal element (<code>..</code>). If these
-file paths are used to determine an output file to write the contents of the archive item to, then
-the file may be written to an unexpected location. This can result in sensitive information being
+file paths are used to create a filesystem path, then a file operation may happen in an 
+unexpected location. This can result in sensitive information being
 revealed or deleted, or an attacker being able to influence behavior by modifying unexpected
 files.</p>
 

java/ql/src/Security/CWE/CWE-022/ZipSlip.ql

@@ -1,8 +1,8 @@
 /**
- * @name Arbitrary file write during archive extraction ("Zip Slip")
- * @description Extracting files from a malicious archive without validating that the
- *              destination file path is within the destination directory can cause files outside
- *              the destination directory to be overwritten.
+ * @name Arbitrary file access during archive extraction ("Zip Slip")
+ * @description Extracting files from a malicious ZIP file, or similar type of archive, without
+ *              validating that the destination file path is within the destination directory
+ *              can allow an attacker to unexpectedly gain access to resources.
  * @kind path-problem
  * @id java/zipslip
  * @problem.severity error

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

@@ -14,11 +14,14 @@
 
 import java
 import semmle.code.java.security.CommandLineQuery
+import semmle.code.java.security.ExternalProcess
 import LocalUserInputToArgumentToExecFlow::PathGraph
 
 from
   LocalUserInputToArgumentToExecFlow::PathNode source,
-  LocalUserInputToArgumentToExecFlow::PathNode sink
-where LocalUserInputToArgumentToExecFlow::flowPath(source, sink)
-select sink.getNode().asExpr(), source, sink, "This command line depends on a $@.",
-  source.getNode(), "user-provided value"
+  LocalUserInputToArgumentToExecFlow::PathNode sink, Expr e
+where
+  LocalUserInputToArgumentToExecFlow::flowPath(source, sink) and
+  argumentToExec(e, sink.getNode())
+select e, source, sink, "This command line depends on a $@.", source.getNode(),
+  "user-provided value"

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

@@ -14,6 +14,7 @@
 
 import java
 import semmle.code.java.security.CommandLineQuery
+import semmle.code.java.security.ExternalProcess
 
 /**
  * Strings that are known to be sane by some simple local analysis. Such strings

java/ql/src/Telemetry/AutomodelApplicationModeCharacteristics.qll

@@ -59,13 +59,13 @@ module ApplicationCandidatesImpl implements SharedCharacteristics::CandidateSig
       e.getType() instanceof NumberType
     )
     or
-    t instanceof AutomodelEndpointTypes::TaintedPathSinkType and
+    t instanceof AutomodelEndpointTypes::PathInjectionSinkType and
     e instanceof PathSanitizer::PathInjectionSanitizer
   }
 
   RelatedLocation asLocation(Endpoint e) { result = e.asExpr() }
 
-  predicate isKnownKind = AutomodelJavaUtil::isKnownKind/3;
+  predicate isKnownKind = AutomodelJavaUtil::isKnownKind/2;
 
   predicate isSink(Endpoint e, string kind) {
     exists(string package, string type, string name, string signature, string ext, string input |
@@ -79,7 +79,7 @@ module ApplicationCandidatesImpl implements SharedCharacteristics::CandidateSig
   predicate isNeutral(Endpoint e) {
     exists(string package, string type, string name, string signature |
       sinkSpec(e, package, type, name, signature, _, _) and
-      ExternalFlow::neutralModel(package, type, name, [signature, ""], _, _)
+      ExternalFlow::neutralModel(package, type, name, [signature, ""], "sink", _)
     )
   }
 

java/ql/src/Telemetry/AutomodelEndpointTypes.qll

@@ -40,18 +40,18 @@ class NegativeSinkType extends SinkType {
 }
 
 /** A sink relevant to the SQL injection query */
-class SqlSinkType extends SinkType {
-  SqlSinkType() { this = "sql" }
+class SqlInjectionSinkType extends SinkType {
+  SqlInjectionSinkType() { this = "sql-injection" }
 }
 
 /** A sink relevant to the tainted path injection query. */
-class TaintedPathSinkType extends SinkType {
-  TaintedPathSinkType() { this = "tainted-path" }
+class PathInjectionSinkType extends SinkType {
+  PathInjectionSinkType() { this = "path-injection" }
 }
 
 /** A sink relevant to the SSRF query. */
 class RequestForgerySinkType extends SinkType {
-  RequestForgerySinkType() { this = "ssrf" }
+  RequestForgerySinkType() { this = "request-forgery" }
 }
 
 /** A sink relevant to the command injection query. */

java/ql/src/Telemetry/AutomodelFrameworkModeCharacteristics.qll

@@ -48,7 +48,7 @@ module FrameworkCandidatesImpl implements SharedCharacteristics::CandidateSig {
 
   RelatedLocation asLocation(Endpoint e) { result = e.asParameter() }
 
-  predicate isKnownKind = AutomodelJavaUtil::isKnownKind/3;
+  predicate isKnownKind = AutomodelJavaUtil::isKnownKind/2;
 
   predicate isSink(Endpoint e, string kind) {
     exists(string package, string type, string name, string signature, string ext, string input |
@@ -60,7 +60,7 @@ module FrameworkCandidatesImpl implements SharedCharacteristics::CandidateSig {
   predicate isNeutral(Endpoint e) {
     exists(string package, string type, string name, string signature |
       sinkSpec(e, package, type, name, signature, _, _) and
-      ExternalFlow::neutralModel(package, type, name, [signature, ""], _, _)
+      ExternalFlow::neutralModel(package, type, name, [signature, ""], "sink", _)
     )
   }
 

java/ql/src/Telemetry/AutomodelJavaUtil.qll

@@ -27,31 +27,17 @@ class DollarAtString extends string {
  * Holds for all combinations of MaD kinds (`kind`) and their human readable
  * descriptions.
  */
-predicate isKnownKind(
-  string kind, string humanReadableKind, AutomodelEndpointTypes::EndpointType type
-) {
-  kind = "read-file" and
-  humanReadableKind = "read file" and
-  type instanceof AutomodelEndpointTypes::TaintedPathSinkType
+predicate isKnownKind(string kind, AutomodelEndpointTypes::EndpointType type) {
+  kind = "path-injection" and
+  type instanceof AutomodelEndpointTypes::PathInjectionSinkType
   or
-  kind = "create-file" and
-  humanReadableKind = "create file" and
-  type instanceof AutomodelEndpointTypes::TaintedPathSinkType
+  kind = "sql-injection" and
+  type instanceof AutomodelEndpointTypes::SqlInjectionSinkType
   or
-  kind = "sql" and
-  humanReadableKind = "mad modeled sql" and
-  type instanceof AutomodelEndpointTypes::SqlSinkType
-  or
-  kind = "open-url" and
-  humanReadableKind = "open url" and
-  type instanceof AutomodelEndpointTypes::RequestForgerySinkType
-  or
-  kind = "jdbc-url" and
-  humanReadableKind = "jdbc url" and
+  kind = "request-forgery" and
   type instanceof AutomodelEndpointTypes::RequestForgerySinkType
   or
   kind = "command-injection" and
-  humanReadableKind = "command injection" and
   type instanceof AutomodelEndpointTypes::CommandInjectionSinkType
 }
 

java/ql/src/Telemetry/AutomodelSharedCharacteristics.qll

@@ -50,7 +50,7 @@ signature module CandidateSig {
   /**
    * Defines what MaD kinds are known, and what endpoint type they correspond to.
    */
-  predicate isKnownKind(string kind, string humanReadableLabel, EndpointType type);
+  predicate isKnownKind(string kind, EndpointType type);
 
   /**
    * Holds if `e` is a flow sanitizer, and has type `t`.
@@ -276,7 +276,11 @@ module SharedCharacteristics<CandidateSig Candidate> {
       string madKind;
       Candidate::EndpointType endpointType;
 
-      KnownSinkCharacteristic() { Candidate::isKnownKind(madKind, this, endpointType) }
+      KnownSinkCharacteristic() {
+        Candidate::isKnownKind(madKind, endpointType) and
+        // bind "this" to a unique string differing from that of the SinkType classes
+        this = madKind + "-characteristic"
+      }
 
       override predicate appliesToEndpoint(Candidate::Endpoint e) { Candidate::isSink(e, madKind) }
 

java/ql/src/change-notes/2023-06-02-unsafe-deserialization-serialkiller.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The query `java/unsafe-deserialization` has been updated to take into account `SerialKiller`, a library used to prevent deserialization of arbitrary classes.

java/ql/src/change-notes/2023-06-16-zipslip-rename.md

@@ -0,0 +1,4 @@
+---
+category: fix
+---
+* The query "Arbitrary file write during archive extraction ("Zip Slip")" (`java/zipslip`) has been renamed to "Arbitrary file access during archive extraction ("Zip Slip")."

java/ql/src/change-notes/2023-06-23-apache-commons-lang.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* New models have been added for `org.apache.commons.lang`.

java/ql/src/change-notes/released/0.6.3.md

@@ -1,4 +1,5 @@
----
-category: minorAnalysis
----
+## 0.6.3
+
+### Minor Analysis Improvements
+
 * The `java/summary/lines-of-code` query now only counts lines of Java code. The new `java/summary/lines-of-code-kotlin` counts lines of Kotlin code.

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

@@ -1,2 +1,2 @@
 ---
-lastReleaseVersion: 0.6.2
+lastReleaseVersion: 0.6.3

java/ql/src/experimental/Security/CWE/CWE-078/ExecTainted.ql

@@ -15,7 +15,11 @@
 import java
 import semmle.code.java.security.CommandLineQuery
 import RemoteUserInputToArgumentToExecFlow::PathGraph
-import JSchOSInjection
+private import semmle.code.java.dataflow.ExternalFlow
+
+private class ActivateModels extends ActiveExperimentalModels {
+  ActivateModels() { this = "jsch-os-injection" }
+}
 
 // This is a clone of query `java/command-line-injection` that also includes experimental sinks.
 from

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

@@ -1,20 +0,0 @@
-/**
- * Provides classes for JSch OS command injection detection
- */
-
-import java
-
-/** The class `com.jcraft.jsch.ChannelExec`. */
-private class JSchChannelExec extends RefType {
-  JSchChannelExec() { this.hasQualifiedName("com.jcraft.jsch", "ChannelExec") }
-}
-
-/** A method to set an OS Command for the execution. */
-private class ChannelExecSetCommandMethod extends Method, ExecCallable {
-  ChannelExecSetCommandMethod() {
-    this.hasName("setCommand") and
-    this.getDeclaringType() instanceof JSchChannelExec
-  }
-
-  override int getAnExecutedArgument() { result = 0 }
-}

java/ql/src/qlpack.yml

@@ -1,5 +1,5 @@
 name: codeql/java-queries
-version: 0.6.3-dev
+version: 0.6.4-dev
 groups: 
   - java
   - queries

java/ql/src/utils/flowtestcasegenerator/testHeader.qlfrag

@@ -1,2 +1,3 @@
 import java
 import TestUtilities.InlineFlowTest
+import DefaultFlowTest