Improve CVE-2023-34035 detection

[dreis2211]

⚡ UPDATE ⚡: A proposed solution is available in the latest 6.2 snapshot build. Please see this comment for details. I would love your feedback.

UPDATE: Thanks again, @dreis2211 for the report.

As reported, there are some false positives with the validation put in place in response to CVE-2023-34035. A fix was released in 5.8.6, 6.0.6, and 6.1.3 to improve the validation.

In all cases, a new repo is now available with several more examples across all affected maintenance releases of Spring Security. The readme indicates which samples are vulnerable and which aren't.

• requestMatchers should not count servlets without mappings #13666
• requestMatchers servlet validation error should include information about servlet paths #13667
• Add MvcRequestMatcher reference documentation #13726
• AbstractRequestMatcherRegistry#requestMatchers should pick MvcRequestMatcher when using MockMvc #13849
• Simplify MvcRequestMatcher construction #13562
• Resolve RequestMatcher at request-time #13850

---

The original post follows:

Hi,

with the work on #13551 a regression has been introduced that prevents the startup of applications under certain (unfortunately not very uncommon) circumstances.

There are multiple already mentioned in the issue, another one I'm facing is that Undertow always registers a default servlet even if there are no mappings leading to two registrations being returned here:

spring-security/config/src/main/java/org/springframework/security/config/annotation/web/AbstractRequestMatcherRegistry.java

Line 322 in df239b6

Assert.isTrue(registrations.size() == 1,

For simpler reproduction, simply check out https://github.com/dreis2211/spring-security-13568 and try to startup the app.
It should yield a stacktrace showing this:

Caused by: java.lang.IllegalArgumentException: This method cannot decide whether these patterns are Spring MVC patterns or not. If this endpoint is a Spring MVC endpoint, please use requestMatchers(MvcRequestMatcher); otherwise, please use requestMatchers(AntPathRequestMatcher).
	at org.springframework.util.Assert.isTrue(Assert.java:122) ~[spring-core-6.0.11.jar:6.0.11]
	at org.springframework.security.config.annotation.web.AbstractRequestMatcherRegistry.requestMatchers(AbstractRequestMatcherRegistry.java:204) ~[spring-security-config-6.1.2.jar:6.1.2]
	at org.springframework.security.config.annotation.web.AbstractRequestMatcherRegistry.requestMatchers(AbstractRequestMatcherRegistry.java:248) ~[spring-security-config-6.1.2.jar:6.1.2]
	at com.example.springsecuritybugs.SecurityConfiguration.lambda$securityFilterChain$0(SecurityConfiguration.java:14) ~[main/:na]
	at org.springframework.security.config.annotation.web.builders.HttpSecurity.authorizeHttpRequests(HttpSecurity.java:1466) ~[spring-security-config-6.1.2.jar:6.1.2]

I think the assertion needs to be reverted for the time being and revisited at a later point. Or making it a warning log instead of the assertion, should this be really a case that you want to prevent in the future with a proper non-patch release.

Cheers,
Christoph

[dreis2211]

So after digging a bit deeper I have found a workaround

The following example:

@Bean
public SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception {
	http.authorizeHttpRequests((requests) -> requests
		.requestMatchers("/test1").permitAll()
		.anyRequest().authenticated()
	);
	return http.build();
}

Would need to be turned into:

@Bean
public SecurityFilterChain securityFilterChain(HttpSecurity http, HandlerMappingIntrospector introspector) throws Exception {
	MvcRequestMatcher.Builder mvcMatcherBuilder = new MvcRequestMatcher.Builder(introspector);
	http.authorizeHttpRequests((requests) -> requests
		.requestMatchers(mvcMatcherBuilder.pattern("/test1")).permitAll()
		.anyRequest().authenticated()
	);
	return http.build();
}

But again. This is a breaking change one doesn't expect in a patch release. Also, the MvcRequestMatcher.Builder facility is not really mentioned in the docs apart from using it, when you need to set a custom servletPath.

Should the assertion stick for whatever valid reason or turned into a log statement, the message is somewhat confusing because there are no methods inside AbstractRequestMatcherRegistry that really take either an MvcRequestMatcher or AntPathRequestMatcher, just the interface RequestMatcher. So it took me quite long to figure out - given the lack of docs - how to actually workaround the problem. Especially since building the different variants means using different things (AntPathRequestMatcher has a simple static method, MvcRequestMatcher providing the mentioned builder).

Having all of that said, I don't think using the MvcRequestMatcher.Builder is a cool API that should be the way to go here in the future. Spring Security configuration is already complex enough once your application becomes somewhat more sophisticated and turning a simple requestMatchers("/test1") into a variant where the user needs to build matchers via the builder, seems a bit bloated. I'd rather have the methods mvcMatchers() & antMatchers() back, which got deprecated and removed.

In summary: I think the assertion should just be removed and also not turned into a log message, warning people to migrate to something else. requestMatchers should simply do what it does and use mvc matchers in the end.

Cheers

[albertus82]

https://spring.io/security/cve-2023-34035 explains well what has changed and how to fix this error, however I agree that a patch release should not introduce breaking changes like this.

[dreis2211]

Oh, I've missed that announcement. Thanks for sharing....

But even then:

An application is not vulnerable if any of the following is true:
...
The application uses requestMatchers(String) only for Spring MVC endpoints

Which is the case in the reproducer app I shared. I wouldn't expect a breaking change for those applications.

[vitalyster]

So, if the app is only uses MVC we can safely wait for proper non-breaking change fix?