From d08ee76b16d1d71cea4b4d5a32da44b477ee3bcb Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 16 Oct 2023 15:49:25 +0200 Subject: [PATCH 1/2] Java: Improve java/spring-disabled-csrf-protection --- .../java/security/SpringCsrfProtection.qll | 20 +++++++++++++++++++ .../CWE/CWE-352/SpringCSRFProtection.ql | 7 ++----- ...pring-disabled-csrf-protection-improved.md | 4 ++++ .../CWE-352/SpringCsrfProtectionTest.expected | 2 ++ .../CWE-352/SpringCsrfProtectionTest.java | 10 ++++++++++ .../CWE-352/SpringCsrfProtectionTest.ql | 18 +++++++++++++++++ .../test/query-tests/security/CWE-352/options | 1 + .../annotation/web/builders/HttpSecurity.java | 10 ++++++++++ .../configurers/AbstractHttpConfigurer.java | 4 +++- .../web/configurers/CsrfConfigurer.java | 8 ++++++++ 10 files changed, 78 insertions(+), 6 deletions(-) create mode 100644 java/ql/lib/semmle/code/java/security/SpringCsrfProtection.qll create mode 100644 java/ql/src/change-notes/2023-10-16-spring-disabled-csrf-protection-improved.md create mode 100644 java/ql/test/query-tests/security/CWE-352/SpringCsrfProtectionTest.expected create mode 100644 java/ql/test/query-tests/security/CWE-352/SpringCsrfProtectionTest.java create mode 100644 java/ql/test/query-tests/security/CWE-352/SpringCsrfProtectionTest.ql create mode 100644 java/ql/test/query-tests/security/CWE-352/options create mode 100644 java/ql/test/stubs/springframework-5.3.8/org/springframework/security/config/annotation/web/configurers/CsrfConfigurer.java diff --git a/java/ql/lib/semmle/code/java/security/SpringCsrfProtection.qll b/java/ql/lib/semmle/code/java/security/SpringCsrfProtection.qll new file mode 100644 index 00000000000..bc25f167327 --- /dev/null +++ b/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") + 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") +} diff --git a/java/ql/src/Security/CWE/CWE-352/SpringCSRFProtection.ql b/java/ql/src/Security/CWE/CWE-352/SpringCSRFProtection.ql index 9bca9dc3ed9..2ce5d5797ba 100644 --- a/java/ql/src/Security/CWE/CWE-352/SpringCSRFProtection.ql +++ b/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") +where disablesSpringCsrfProtection(call) select call, "CSRF vulnerability due to protection being disabled." diff --git a/java/ql/src/change-notes/2023-10-16-spring-disabled-csrf-protection-improved.md b/java/ql/src/change-notes/2023-10-16-spring-disabled-csrf-protection-improved.md new file mode 100644 index 00000000000..94462f0f8c3 --- /dev/null +++ b/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. diff --git a/java/ql/test/query-tests/security/CWE-352/SpringCsrfProtectionTest.expected b/java/ql/test/query-tests/security/CWE-352/SpringCsrfProtectionTest.expected new file mode 100644 index 00000000000..a74f2c23cda --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-352/SpringCsrfProtectionTest.expected @@ -0,0 +1,2 @@ +testFailures +failures \ No newline at end of file diff --git a/java/ql/test/query-tests/security/CWE-352/SpringCsrfProtectionTest.java b/java/ql/test/query-tests/security/CWE-352/SpringCsrfProtectionTest.java new file mode 100644 index 00000000000..7e1e92e4392 --- /dev/null +++ b/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 + } +} diff --git a/java/ql/test/query-tests/security/CWE-352/SpringCsrfProtectionTest.ql b/java/ql/test/query-tests/security/CWE-352/SpringCsrfProtectionTest.ql new file mode 100644 index 00000000000..df22aadd4e9 --- /dev/null +++ b/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 diff --git a/java/ql/test/query-tests/security/CWE-352/options b/java/ql/test/query-tests/security/CWE-352/options new file mode 100644 index 00000000000..595ccc6b812 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-352/options @@ -0,0 +1 @@ +semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/springframework-5.3.8 \ No newline at end of file diff --git a/java/ql/test/stubs/springframework-5.3.8/org/springframework/security/config/annotation/web/builders/HttpSecurity.java b/java/ql/test/stubs/springframework-5.3.8/org/springframework/security/config/annotation/web/builders/HttpSecurity.java index 7e4f1dceed4..3dbe33cdeb9 100644 --- a/java/ql/test/stubs/springframework-5.3.8/org/springframework/security/config/annotation/web/builders/HttpSecurity.java +++ b/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 csrf() { + return null; + } + + public HttpSecurity csrf(Customizer> csrfCustomizer) { + return null; + } + public final class MvcMatchersRequestMatcherConfigurer extends RequestMatcherConfigurer { } diff --git a/java/ql/test/stubs/springframework-5.3.8/org/springframework/security/config/annotation/web/configurers/AbstractHttpConfigurer.java b/java/ql/test/stubs/springframework-5.3.8/org/springframework/security/config/annotation/web/configurers/AbstractHttpConfigurer.java index 7a1b56d5f3f..7125e82d437 100644 --- a/java/ql/test/stubs/springframework-5.3.8/org/springframework/security/config/annotation/web/configurers/AbstractHttpConfigurer.java +++ b/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, B extends HttpSecurityBuilder> - extends SecurityConfigurerAdapter {} + extends SecurityConfigurerAdapter { + public B disable() { return null; } +} diff --git a/java/ql/test/stubs/springframework-5.3.8/org/springframework/security/config/annotation/web/configurers/CsrfConfigurer.java b/java/ql/test/stubs/springframework-5.3.8/org/springframework/security/config/annotation/web/configurers/CsrfConfigurer.java new file mode 100644 index 00000000000..f4fcab29568 --- /dev/null +++ b/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> + extends AbstractHttpConfigurer, H> { + +} From 4ecda9cccd202a99de8bbaefdd699753972ee790 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Tue, 17 Oct 2023 10:18:19 +0200 Subject: [PATCH 2/2] Add consistency check exception --- .../security/CWE-352/CONSISTENCY/typeParametersInScope.expected | 1 + 1 file changed, 1 insertion(+) create mode 100644 java/ql/test/query-tests/security/CWE-352/CONSISTENCY/typeParametersInScope.expected diff --git a/java/ql/test/query-tests/security/CWE-352/CONSISTENCY/typeParametersInScope.expected b/java/ql/test/query-tests/security/CWE-352/CONSISTENCY/typeParametersInScope.expected new file mode 100644 index 00000000000..687ce674721 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-352/CONSISTENCY/typeParametersInScope.expected @@ -0,0 +1 @@ +| Type new Customizer>(...) { ... } uses out-of-scope type variable B. Note the Java extractor is known to sometimes do this; the Kotlin extractor should not. |