Merge pull request #14515 from atorralba/atorralba/java/spring-csrf-improv

Java: Improve java/spring-disabled-csrf-protection

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/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/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/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> {
+
+}