Merge branch 'main' into post-release-prep/codeql-cli-2.15.1

java/ql/lib/change-notes/2023-10-17-new-models.md

@@ -0,0 +1,8 @@
+---
+category: minorAnalysis
+---
+* Added models for the following packages:
+
+  * com.alibaba.fastjson2
+  * javax.management
+  * org.apache.http.client.utils

java/ql/lib/ext/com.alibaba.fastjson2.model.yml

@@ -0,0 +1,6 @@
+extensions:
+  - addsTo:
+      pack: codeql/java-all
+      extensible: summaryModel
+    data:
+      - ["com.alibaba.fastjson2", "JSON", False, "parseArray", "(String,Class)", "", "Argument[0]", "ReturnValue.Element", "taint", "ai-manual"]

java/ql/lib/ext/javax.crypto.model.yml

@@ -0,0 +1,7 @@
+extensions:
+  - addsTo:
+      pack: codeql/java-all
+      extensible: sinkModel
+    data:
+      - ["javax.crypto", "Cipher", True, "init", "(int,Key,AlgorithmParameterSpec)", "", "Argument[2]", "encryption-iv", "manual"]
+      - ["javax.crypto", "Cipher", True, "init", "(int,Key,AlgorithmParameterSpec,SecureRandom)", "", "Argument[2]", "encryption-iv", "manual"]

java/ql/lib/ext/javax.crypto.spec.model.yml

@@ -0,0 +1,9 @@
+extensions:
+  - addsTo:
+      pack: codeql/java-all
+      extensible: summaryModel
+    data:
+      - ["javax.crypto.spec", "IvParameterSpec", True, "IvParameterSpec", "", "", "Argument[0]", "Argument[this]", "taint", "manual"]
+      - ["javax.crypto.spec", "GCMParameterSpec", True, "GCMParameterSpec", "", "", "Argument[1]", "Argument[this]", "taint", "manual"]
+      - ["javax.crypto.spec", "RC2ParameterSpec", True, "RC2ParameterSpec", "", "", "Argument[1]", "Argument[this]", "taint", "manual"]
+      - ["javax.crypto.spec", "RC5ParameterSpec", True, "RC5ParameterSpec", "", "", "Argument[3]", "Argument[this]", "taint", "manual"]

java/ql/lib/ext/javax.management.model.yml

@@ -0,0 +1,6 @@
+extensions:
+  - addsTo:
+      pack: codeql/java-all
+      extensible: summaryModel
+    data:
+      - ["javax.management", "ObjectName", True, "ObjectName", "(String)", "", "Argument[0]", "Argument[this]", "taint", "ai-manual"]

java/ql/lib/ext/org.apache.http.client.utils.model.yml

@@ -10,6 +10,7 @@ extensions:
       - ["org.apache.http.client.utils", "URIBuilder", True, "setHost", "(String)", "", "Argument[0]", "ReturnValue", "taint", "ai-manual"]
       - ["org.apache.http.client.utils", "URIBuilder", True, "setPath", "(String)", "", "Argument[0]", "Argument[this].SyntheticField[org.apache.http.client.utils.URIBuilder.path]", "taint", "ai-manual"]
       - ["org.apache.http.client.utils", "URIBuilder", True, "setPathSegments", "(List)", "", "Argument[0]", "Argument[this].SyntheticField[org.apache.http.client.utils.URIBuilder.path]", "taint", "ai-manual"]
+      - ["org.apache.http.client.utils", "URIBuilder", True, "setPort", "(int)", "", "Argument[this]", "ReturnValue", "value", "ai-manual"]
       - ["org.apache.http.client.utils", "URLEncodedUtils", True, "parse", "(URI,String)", "", "Argument[0]", "ReturnValue.Element", "taint", "ai-manual"]
       - ["org.apache.http.client.utils", "URIUtils", True, "createURI", "(String,String,int,String,String,String)", "", "Argument[1]", "ReturnValue", "taint", "hq-manual"]
       - ["org.apache.http.client.utils", "URIUtils", True, "extractHost", "(URI)", "", "Argument[0]", "ReturnValue", "taint", "hq-manual"]

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

@@ -0,0 +1,20 @@
+/** Provides predicates to reason about disabling CSRF protection in Spring. */
+
+import java
+
+/** Holds if `call` disables CSRF protection in Spring. */
+predicate disablesSpringCsrfProtection(MethodAccess call) {
+  call.getMethod().hasName("disable") and
+  call.getReceiverType()
+      .hasQualifiedName("org.springframework.security.config.annotation.web.configurers",
+        "CsrfConfigurer<HttpSecurity>")
+  or
+  call.getMethod()
+      .hasQualifiedName("org.springframework.security.config.annotation.web.builders",
+        "HttpSecurity", "csrf") and
+  call.getArgument(0)
+      .(MemberRefExpr)
+      .getReferencedCallable()
+      .hasQualifiedName("org.springframework.security.config.annotation.web.configurers",
+        "AbstractHttpConfigurer", "disable")
+}

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

@@ -2,7 +2,7 @@
 
 import java
 import semmle.code.java.dataflow.TaintTracking
-import semmle.code.java.dataflow.DataFlow2
+private import semmle.code.java.dataflow.ExternalFlow
 
 /**
  * Holds if `array` is initialized only with constants.
@@ -99,7 +99,7 @@ private module ArrayUpdateFlow = DataFlow::Global<ArrayUpdateConfig>;
 private class StaticInitializationVectorSource extends DataFlow::Node {
   StaticInitializationVectorSource() {
     exists(StaticByteArrayCreation array | array = this.asExpr() |
-      not ArrayUpdateFlow::flow(DataFlow2::exprNode(array), _) and
+      not ArrayUpdateFlow::flow(DataFlow::exprNode(array), _) and
       // Reduce FPs from utility methods that return an empty array in an exceptional case
       not exists(ReturnStmt ret |
         array.getADimension().(CompileTimeConstantExpr).getIntValue() = 0 and
@@ -113,34 +113,7 @@ private class StaticInitializationVectorSource extends DataFlow::Node {
  * A sink that initializes a cipher with unsafe parameters.
  */
 private class EncryptionInitializationSink extends DataFlow::Node {
-  EncryptionInitializationSink() {
-    exists(MethodAccess ma, Method m | m = ma.getMethod() |
-      m.hasQualifiedName("javax.crypto", "Cipher", "init") and
-      m.getParameterType(2)
-          .(RefType)
-          .hasQualifiedName("java.security.spec", "AlgorithmParameterSpec") and
-      ma.getArgument(2) = this.asExpr()
-    )
-  }
-}
-
-/**
- * Holds if `fromNode` to `toNode` is a dataflow step
- * that creates cipher's parameters with initialization vector.
- */
-private predicate createInitializationVectorSpecStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
-  exists(ConstructorCall cc, RefType type |
-    cc = toNode.asExpr() and type = cc.getConstructedType()
-  |
-    type.hasQualifiedName("javax.crypto.spec", "IvParameterSpec") and
-    cc.getArgument(0) = fromNode.asExpr()
-    or
-    type.hasQualifiedName("javax.crypto.spec", ["GCMParameterSpec", "RC2ParameterSpec"]) and
-    cc.getArgument(1) = fromNode.asExpr()
-    or
-    type.hasQualifiedName("javax.crypto.spec", "RC5ParameterSpec") and
-    cc.getArgument(3) = fromNode.asExpr()
-  )
+  EncryptionInitializationSink() { sinkNode(this, "encryption-iv") }
 }
 
 /**
@@ -156,10 +129,6 @@ deprecated class StaticInitializationVectorConfig extends TaintTracking::Configu
   }
 
   override predicate isSink(DataFlow::Node sink) { sink instanceof EncryptionInitializationSink }
-
-  override predicate isAdditionalTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
-    createInitializationVectorSpecStep(fromNode, toNode)
-  }
 }
 
 /**
@@ -169,10 +138,6 @@ module StaticInitializationVectorConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) { source instanceof StaticInitializationVectorSource }
 
   predicate isSink(DataFlow::Node sink) { sink instanceof EncryptionInitializationSink }
-
-  predicate isAdditionalFlowStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
-    createInitializationVectorSpecStep(fromNode, toNode)
-  }
 }
 
 /** Tracks the flow from a static initialization vector to the initialization of a cipher */

java/ql/src/Security/CWE/CWE-352/SpringCSRFProtection.ql

@@ -12,11 +12,8 @@
  */
 
 import java
+import semmle.code.java.security.SpringCsrfProtection
 
 from MethodAccess call
-where
-  call.getMethod().hasName("disable") and
-  call.getReceiverType()
-      .hasQualifiedName("org.springframework.security.config.annotation.web.configurers",
-        "CsrfConfigurer<HttpSecurity>")
+where disablesSpringCsrfProtection(call)
 select call, "CSRF vulnerability due to protection being disabled."

java/ql/src/change-notes/2023-10-16-spring-disabled-csrf-protection-improved.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The query `java/spring-disabled-csrf-protection` has been improved to detect more ways of disabling CSRF in Spring.

java/ql/test/kotlin/library-tests/java-kotlin-collection-type-generic-methods/test.expected

@@ -165,6 +165,8 @@ methodWithDuplicate
 | List | add | int |
 | List | addAll | Collection<? extends E> |
 | List | addAll | int |
+| List | addFirst | E |
+| List | addLast | E |
 | List | contains | Object |
 | List | containsAll | Collection<?> |
 | List | copyOf | Collection<? extends E> |
@@ -213,6 +215,8 @@ methodWithDuplicate
 | List<String> | add | int |
 | List<String> | addAll | Collection<? extends String> |
 | List<String> | addAll | int |
+| List<String> | addFirst | String |
+| List<String> | addLast | String |
 | List<String> | contains | Object |
 | List<String> | containsAll | Collection<?> |
 | List<String> | copyOf | Collection<? extends E> |

java/ql/test/library-tests/frameworks/apache-collections/options

@@ -1 +1 @@
-//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/apache-commons-collections4-4.4
+//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/apache-commons-collections4-4.4 --release 11

java/ql/test/library-tests/sensitive-actions/test.expected

@@ -67,5 +67,3 @@ sensitiveDataMethod
 | Test.java:37:18:37:31 | aaCryptAccntaa |
 | Test.java:39:18:39:33 | aaCryptTrustedaa |
 | Test.java:41:18:41:40 | aaCryptRefreshaaTokenaa |
-| file:///modules/java.base/java/lang/invoke/MemberName.class:0:0:0:0 | isTrustedFinalField |
-| file:///modules/java.base/java/lang/reflect/Field.class:0:0:0:0 | isTrustedFinal |

java/ql/test/library-tests/sensitive-actions/test.ql

@@ -9,4 +9,4 @@ query predicate sensitiveVariable(Variable v) {
   v.getName().regexpMatch(getCommonSensitiveInfoRegex())
 }
 
-query predicate sensitiveDataMethod(SensitiveDataMethod m) { any() }
+query predicate sensitiveDataMethod(SensitiveDataMethod m) { m.fromSource() }

java/ql/test/library-tests/types/record-classes/RecordClasses.ql

@@ -2,6 +2,7 @@ import java
 
 from Record r, boolean isFinal, boolean isStatic, string superTypes
 where
+  r.fromSource() and
   (if r.isFinal() then isFinal = true else isFinal = false) and
   (if r.isStatic() then isStatic = true else isStatic = false) and
   superTypes = concat(RefType superType | superType = r.getASupertype() | superType.toString(), ",")

java/ql/test/query-tests/security/CWE-352/CONSISTENCY/typeParametersInScope.expected

@@ -0,0 +1 @@
+| Type new Customizer<CsrfConfigurer<HttpSecurity>>(...) { ... } uses out-of-scope type variable B. Note the Java extractor is known to sometimes do this; the Kotlin extractor should not. |

java/ql/test/query-tests/security/CWE-352/SpringCsrfProtectionTest.expected

@@ -0,0 +1,2 @@
+testFailures
+failures

java/ql/test/query-tests/security/CWE-352/SpringCsrfProtectionTest.java

@@ -0,0 +1,10 @@
+import org.springframework.security.config.annotation.web.builders.HttpSecurity;
+import org.springframework.security.config.annotation.web.configurers.AbstractHttpConfigurer;
+
+public class SpringCsrfProtectionTest {
+    protected void test(HttpSecurity http) throws Exception {
+        http.csrf(csrf -> csrf.disable()); // $ hasSpringCsrfProtectionDisabled
+        http.csrf().disable(); // $ hasSpringCsrfProtectionDisabled
+        http.csrf(AbstractHttpConfigurer::disable); // $ hasSpringCsrfProtectionDisabled
+    }
+}

java/ql/test/query-tests/security/CWE-352/SpringCsrfProtectionTest.ql

@@ -0,0 +1,18 @@
+import java
+import semmle.code.java.security.SpringCsrfProtection
+import TestUtilities.InlineExpectationsTest
+
+module SpringCsrfProtectionTest implements TestSig {
+  string getARelevantTag() { result = "hasSpringCsrfProtectionDisabled" }
+
+  predicate hasActualResult(Location location, string element, string tag, string value) {
+    tag = "hasSpringCsrfProtectionDisabled" and
+    exists(MethodAccess call | disablesSpringCsrfProtection(call) |
+      call.getLocation() = location and
+      element = call.toString() and
+      value = ""
+    )
+  }
+}
+
+import MakeTest<SpringCsrfProtectionTest>

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

@@ -0,0 +1 @@
+semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/springframework-5.3.8

java/ql/test/stubs/apache-mina-sshd-2.8.0/org/apache/sshd/common/util/threads/CloseableExecutorService.java

@@ -9,4 +9,6 @@ import org.apache.sshd.common.Closeable;
 public interface CloseableExecutorService extends Closeable, ExecutorService
 {
     default boolean awaitTermination(Duration p0){ return false; }
+
+    default void close() { }
 }

java/ql/test/stubs/springframework-5.3.8/org/springframework/security/config/annotation/web/builders/HttpSecurity.java

@@ -3,9 +3,11 @@ package org.springframework.security.config.annotation.web.builders;
 import org.springframework.security.config.annotation.AbstractConfiguredSecurityBuilder;
 import org.springframework.security.config.annotation.SecurityBuilder;
 import org.springframework.security.config.annotation.web.HttpSecurityBuilder;
+import org.springframework.security.config.annotation.web.builders.HttpSecurity.RequestMatcherConfigurer;
 import org.springframework.security.web.DefaultSecurityFilterChain;
 import org.springframework.security.web.util.matcher.RequestMatcher;
 import org.springframework.security.config.Customizer;
+import org.springframework.security.config.annotation.web.configurers.CsrfConfigurer;
 import org.springframework.security.config.annotation.web.configurers.ExpressionUrlAuthorizationConfigurer;
 import org.springframework.security.config.annotation.web.AbstractRequestMatcherRegistry;
 
@@ -35,6 +37,14 @@ public final class HttpSecurity extends AbstractConfiguredSecurityBuilder<Defaul
 		return null;
 	}
 
+	public CsrfConfigurer<HttpSecurity> csrf() {
+		return null;
+	}
+
+	public HttpSecurity csrf(Customizer<CsrfConfigurer<HttpSecurity>> csrfCustomizer) {
+		return null;
+	}
+
 	public final class MvcMatchersRequestMatcherConfigurer extends RequestMatcherConfigurer {
 	}
 

java/ql/test/stubs/springframework-5.3.8/org/springframework/security/config/annotation/web/configurers/AbstractHttpConfigurer.java

@@ -5,4 +5,6 @@ import org.springframework.security.config.annotation.web.HttpSecurityBuilder;
 import org.springframework.security.web.DefaultSecurityFilterChain;
 
 public abstract class AbstractHttpConfigurer<T extends AbstractHttpConfigurer<T, B>, B extends HttpSecurityBuilder<B>>
-		extends SecurityConfigurerAdapter<DefaultSecurityFilterChain, B> {}
+		extends SecurityConfigurerAdapter<DefaultSecurityFilterChain, B> {
+	public B disable() { return null; }
+}

java/ql/test/stubs/springframework-5.3.8/org/springframework/security/config/annotation/web/configurers/CsrfConfigurer.java

@@ -0,0 +1,8 @@
+package org.springframework.security.config.annotation.web.configurers;
+
+import org.springframework.security.config.annotation.web.HttpSecurityBuilder;
+
+public class CsrfConfigurer<H extends HttpSecurityBuilder<H>>
+        extends AbstractHttpConfigurer<CsrfConfigurer<H>, H> {
+
+}