WebInvocationPrivilegeEvaluator Bean should support multiple SecurityFilterChains

[fast-reflexes]

Background

Having multiple WebSecurityConfigurerAdapter's which are ordered and each processes a limited set of paths is a very handy tool for creating a structured security configuration. The advantages are:

• May add a catch all that denies all access for any request not explicitly whitelisted by previous configs.
• With both a regular API with basic auth, internal API for UI which uses login via SAML2 and some parts of an app open, configuring it all in one adapter would be messy (if even possible) and therefore also error-prone.

We have had a setup with multiple adapters for a long time and it has worked very well. Spring does not disallow it and there are parts in the code which even suggests that this is an intended use (for example the property securityFilterChainBuilders in singleton WebSecurity which is a List of builders and not a single object). The role of method HttpSecurity.requestMatchers also encourages such use.

Bug

Nonetheless, parts of Spring Security does not take this into account. More specifically, the WebSecurity singleton has a property named filterSecurityInterceptor which is populated with the FilterSecurityInterceptor from the LAST WebSecurityConfigurerAdapter processed. This FilterSecurityInterceptor is then added to the bean DefaultWebInvocationPrivilegeEvaluator created via bean method privilegeEvaluator in WebSecurityConfiguration. This means that any use of property filterSecurityInterceptor in singleton WebSecurity, or any use of bean DefaultWebInvocationPrivilegeEvaluator will only take the last processed WebSecurityConfigurerAdapter into account, which does not seem correct. Luckily, it seems to me that the only use of property filterSecurityInterceptor is to create bean DefaultWebInvocationPrivilegeAdapter and the only use of this bean is in newly added filter ErrorPageSecurityFilter, which is also where we can see this bug at play. Nonetheless, the design seems flawed.

Note that this bug is NOT any of the following:

• ErrorPageSecurityFilter inherits from HttpFilter and not Filter causing ClassDefNotFound exception in some environments:
  • ErrorPageSecurityFilter prevents deployment to a Servlet 3.1 compatible container spring-boot#28790
  • https://stackoverflow.com/questions/70087604/why-do-i-have-classnotfoundexception-javax-servlet-http-httpfilter-for-a-war-af/70087906
• MockMvc does not exclude ErrorPageSecurityFilter, which should only be included for ERROR dispatches, from regular dispatches. This is due to MockFilterChain mistakenly including this filter no matter the dispatch type whereas the real ApplicationFilterChain used by Spring will exclude it if the dispatch type is not ERROR:
  • Page with permitAll is no longer accessible via auto-configured MockMvc spring-boot#28759
  • Spring Security 5.6.0 breaks multiple configurations in tests #10544

... however, in the case of the problems with MockMvc, the bug presented herein come into play as well (even though there is also a problem with MockMvc and their MockFilterChain not taking dispatcher type into account).

Reproduce

Reproduction of bug is shown with filter ErrorPageSecurityFilter.

Download project https://github.com/fast-reflexes/spring-boot-bug/tree/filterSecurityInterceptor

Project involves a security setup like

@EnableWebSecurity(debug = true)
@Order(0)
class Config: WebSecurityConfigurerAdapter() {

    override fun configure(http: HttpSecurity) {
        http
            .requestMatchers()
            .antMatchers("/error/**", "/test")
            .and()
            .authorizeRequests()
            .anyRequest().permitAll()
    }
}

@EnableWebSecurity(debug = true)
@Order(1)
class ClosedConfig: WebSecurityConfigurerAdapter() {

    override fun configure(http: HttpSecurity) {
        http
            .requestMatchers()
                .antMatchers("/non-existing")
                .and()
            .authorizeRequests()
                .anyRequest().hasRole("PRIVILEGED_USER")
    }
}

@EnableWebSecurity(debug = true)
@Order(2)
class CatchAllConfig: WebSecurityConfigurerAdapter() {

    override fun configure(http: HttpSecurity) {
        http
            .authorizeRequests()
            .anyRequest().denyAll()
    }

}

See bug in action

1. Start app with ./gradlew bootRun

2. Go to http://localhost:8080/non-existing. This endpoint is restricted so Spring will want to send an error with 403. A new error dispatch starts.

3. Check console and verify that Spring Security filter chain accepts the request, but when the ErrorPageSecurityFilter processes it, the request is considered unauthorized, because the DefaultWebInvocationPrivilegeEvaluator only considers the last WebSecurityConfigurerAdapter which is a catch-all that denies everything. Therefore, only a status code of 403 is sent and no error page. Nonetheless, the first WebSecurityConfigurerAdapter explicitly allows access to /error to anyone:

   ...
   2021-11-26 09:52:42.860 DEBUG 68724 --- [nio-8080-exec-1] o.s.security.web.FilterChainProxy        : Secured GET /error 
   2021-11-26 09:52:42.897 TRACE 68724 --- [nio-8080-exec-1] o.s.s.w.a.expression.WebExpressionVoter  : Voted to deny authorization
   2021-11-26 09:52:42.900 DEBUG 68724 --- [nio-8080-exec-1] a.DefaultWebInvocationPrivilegeEvaluator : filter invocation [/error] denied for AnonymousAuthenticationToken [Principal=anonymousUser, Credentials=[PROTECTED], Authenticated=true, Details=WebAuthenticationDetails [RemoteIpAddress=0:0:0:0:0:0:0:1, SessionId=3B7CB0BABC6A8360268E10BF4DB0BE8F], Granted Authorities=[ROLE_ANONYMOUS]]
   ...
   

Fix bug by changing last WebSecurityConfigurerAdapter to permit all

1. Change the last configuration to permitAll() instead of denyAll().

2. Run the previous test again.

3. Verify that the an actual error page is sent and output in console shows that access to /error is now permitted by the DefaultWebInvocationPrivilegeEvaluator:

   ...
   2021-11-26 10:54:51.356 DEBUG 69801 --- [nio-8080-exec-1] o.s.security.web.FilterChainProxy        : Secured GET /error
   2021-11-26 10:54:51.371 TRACE 69801 --- [nio-8080-exec-1] o.s.web.servlet.DispatcherServlet        : "ERROR" dispatch for GET "/error", parameters={}, headers={masked} in DispatcherServlet 'dispatcherServlet'
   2021-11-26 10:54:51.378 TRACE 69801 --- [nio-8080-exec-1] s.w.s.m.m.a.RequestMappingHandlerMapping : 2 matching mappings: [{ [/error], produces [text/html]}, { [/error]}]
   2021-11-26 10:54:51.379 TRACE 69801 --- [nio-8080-exec-1] s.w.s.m.m.a.RequestMappingHandlerMapping : Mapped to org.springframework.boot.autoconfigure.web.servlet.error.BasicErrorController#errorHtml(HttpServletRequest, HttpServletResponse)
   2021-11-26 10:54:51.392 TRACE 69801 --- [nio-8080-exec-1] o.s.web.method.HandlerMethod             : Arguments: [SecurityContextHolderAwareRequestWrapper[ org.springframework.security.web.context.HttpSessionSecurityContextRepository$SaveToSessionRequestWrapper@7fb8f2b5], org.springframework.security.web.context.HttpSessionSecurityContextRepository$SaveToSessionResponseWrapper@14b5cd81]
   2021-11-26 10:54:51.420 DEBUG 69801 --- [nio-8080-exec-1] o.s.w.s.v.ContentNegotiatingViewResolver : Selected 'text/html' given [text/html, text/html;q=0.8]
   2021-11-26 10:54:51.420 TRACE 69801 --- [nio-8080-exec-1] o.s.web.servlet.DispatcherServlet        : Rendering view [org.springframework.boot.autoconfigure.web.servlet.error.ErrorMvcAutoConfiguration$StaticView@1770d1b3] 
   2021-11-26 10:54:51.426 DEBUG 69801 --- [nio-8080-exec-1] o.s.web.servlet.DispatcherServlet        : Exiting from "ERROR" dispatch, status 403, headers={masked}
   ...
   

Chain of events

1. During initialization bean method springFilterSecurityChain is executed in file WebSecurityConfiguration. This method creates the Spring Security filter chain filter.
2. For each WebSecurityConfigurerAdapter given, its init method is executed. The HttpSecurity object is fetched for each config and queued as a builder in securityFilterChainBuilders property of type List in WebSecurity via method addSecurityFilterChainBuilder. This method returns the WebSecurity object itself. In the same init call, the WebSecurity.postBuildAction property is set to a Runnable which adds the FilterSecurityInterceptor of the current config as the filterSecurityInterceptor property of the WebSecurity singleton itself. Since this method call sets property postBuildAction in WebSecurity, it overwrites the previous Runnable that this property was set to earlier. The postBuildAction Runnable is not executed yet but the final assigned property is a Runnable which involves the HttpSecurity object of the last WebSecurityConfigurerAdapter processed.
3. After all builders are added to the WebSecurity List property securityFilterChainBuilders with builders (and the postBuildAction is overwritten each time), each config is processed and built. In method configure of AbstractInterceptUrlConfigurer, a FilterSecurityInterceptor is created for and attached to each configuration.
4. After all the configs are processed and built, the postBuildAction in WebSecurity is executed and attaches the FilterSecurityInterceptor of the last processed config as the filterSecurityInterceptor of the singleton WebSecurity object. The FilterSecurityInterceptor's of the other configs are, via the overwritten postBuildAction Runnable (and the fact that the securityInterceptor of the WebSecurity class is a single object and not a list of objects) ignored.
5. In file WebSecurityConfiguration, bean method privilegeEvaluator is executed, constructing a WebInvocationPrivilegeEvaluator bean in the shape of a DefaultWebInvocationPrivilegeEvaluator which is constructed from method getPrivilegeEvaluator in file WebSecurity. This method uses the filterSecurityIinterceptor set in WebSecurity, which corresponds to the FilterSecurityInterceptor of the last processed WebSecurityConfigurerAdapter.
6. Now, in the reproductions you saw in the logs that Spring Security processes the request and then dispatches an errors message with path /error. You will see a new invocation in Spring Security with this requested path, and since we have a config that matches and allows the /error path, Spring Security will allow it. The ApplicationFilterChain will then call the new ErrorPageSecurityFilter, because it is an ERROR dispatch.
7. In ErrorPageSecurityFilter, the method doFilter will execute, which will retrieve the WebInvocationPrivilegeEvaluator bean, which is the bean from step 5. Once retrieved, this bean will use the available authentication along with its FilterSecurityInterceptor, from the last processed WebSecurityConfigurerAdapter, to retrieve meta data security expressions and determine whether access can be given. If access is given, the initial dispatch is allowed, rendering a full error page. If access is denied, the ErrorPageSecurityFilter will send a response containing the intended http status code for the dispatch, but with nothing more.
8. Now consider the security setup above where /error paths are allowed to everyone and with a last catch-all security config which denies all. In such a setup, the security filter chain will allow access to the error page, but the ErrorPageSecurityFilter will, erroneously, deny access to the error page even though the configuration indicates that it should be allowed.

Problems

• DefaultWebInvocationPrivilegeEvaluator's isAllowed method should not be used since it doesn't make use of the full security config specified. Luckily, it seems this method is only used by the ErrorPageSecurityFilter. WebSecurity's property filterSecurityInterceptor should not be used either since it ignores part of the security config. Seems it is only used to create the WebInvocationPrivilegeEvaluator bean and since its isAllowed method is used only by ErrorPageSecurityFilter, its impact seems contained. Even if this problem is solved, I don't see how the concerned functionality of the given classes can continue to exist in their current form for any future use since they clearly don't capture the full given security config given to Spring.
• If this behaviour is intended, it should be clearly advised against multiple WebSecurityConfigurerAdapter's and an exception should be thrown if multiple of these are present. Since I think multiple WebSecurityConfigurerAdapter's have a real use-case, I suggest this bug should be fixed instead.

The problem is not per se, very big in the prescribed case, but the logic is flawed and if more components use the filterSecurityInterceptor in WebSecurity or the DefaultWebInvocationPrivilegeEvaluator in the future, it could be worse. Therefore, it is important to either fix this bug or force users to only use one WebSecurityConfigurerAdapter (not recommended imho).

Expected behaviour

Expected behaviour is that all use of property filterSecurityInterceptor in WebSecurity singleton, and all use of bean DefaultWebInvocationPrivilegeEvaluator should take the entire security config into account, not only the part coming from the last processed WebSecurityConfigurerAdapter. Alternatively, Spring should reject multiple WebSecurityConfigurerAdapter's. Since the latter seems counterintuitive and there is a good usecase for multiple WebSecurityConfigurerAdapters, I'd rather see this as a bug and that the relevant parts of WebSecurity and DefaultWebInvocationPrivilegeEvaluator be rewritten.

To conclude, in this specific case, the entire error page should be allowed by ErrorPageSecurityFilter in BOTH cases, even when the last WebSecurityConfigurerAdapter is a catch-all which denies all access simply because a previous one explicitly allowed access to the /error path to everyone.

A ticket has been filed in Spring Boot repo to to alert users of this behaviour in ErrorPageSecurityFilter: spring-projects/spring-boot#28818

[marcusdacoregio]

Hi @fast-reflexes, thanks so much for the detailed response.

The WebInvocationPrivilegeEvaluator API is behaving as intended. We are gonna provide another implementation that relies on a RequestMatcher. I went ahead and changed the title to best reflect the changes.

[fast-reflexes]

Hi @marcusdacoregio

Could you elaborate on this :) When you say that the API works as intended, do you mean:

• Everything in this example works as it should, access to /error SHOULD be denied despite it being white-listed in the config? If this is the case, I'd love to hear the rationale behind it :)
• The WebInvocationPrivilegeEvaluator works as it should but ErrorPageSecurityFilter is really looking for an other functionality so it shouldn't use WebInvocationPrivilegeEvaluator but something else?
• WebInvocationPrivilegeEvaluator AND ErrorPageSecurityFilter work as they should but WebSecurity should provide a FilterSecurityInterceptor that take the full config into account, which it currently doesn't.

No matter which, I think it's odd for ANY use case that WebInvocationPrivilegeEvaluator only takes the last config into account so I'd LOVE to hear a use-case with multiple security configs where it has the intended effect while at the same time being correct :) I put so much time into this so I'd like to follow up a little bit :)

I'm a bit puzzled as to how you can not see this as a bug... all in all, you think that the current functionality is correct? Could you elaborate on what grounds you think it should deny access?

[marcusdacoregio]

Please ignore the line about the API. I mean that that implementation is behaving as intended since it was designed for a single filter chain.

We are in talks with the Spring Boot team and will come up with a solution that works for everyone.
I'll let you know the outcome once we work out the issue.

[fast-reflexes]

@marcusdacoregio great! Thanks! I'd love to check it out but I don't understand how to get that version.. can't find it in Maven repo nor JCenter, can't find any branch in your repo called that.. I tried to add it as constraints in my Gradle file but to not avail.. could you give me a hint? :)

[darrenpm]

@fast-reflexes Thanks for logging the issues, I stumbled across the same problem today and it was a relief to come find your detailed explanations.

I would have expected the snapshot builds to be in here, but the changes aren't in the most recent build
https://repo.spring.io/ui/native/snapshot/org/springframework/security/spring-security-web/5.7.0-SNAPSHOT

The Maven config for the snapshot repo is:

<repository>
  <id>spring-snapshots</id>
  <name>Spring Snapshot Repository</name>
  <url>https://repo.spring.io/snapshot</url>
</repository>

with extra property:
<spring-security.version>5.7.0-SNAPSHOT</spring-security.version>

@marcusdacoregio I think this has to be included in 5.6.x as it's very much broken when used with Spring Boot 2.6.x

[fast-reflexes]

Thanks a lot @darrenpm !

SInce I'm using Gradle, I had to adapt it a bit:

repositories {
    maven(url = "https://repo.spring.io/snapshot" )
}

... then ...

ext["spring-security.version"] = "5.7.0-SNAPSHOT"

After this, when I run

./gradlew dependencies |grep security

Version 5.7.0-SNAPSHOT seems to be listed everywhere...

Nonetheless, the problems are not solved, neither for my private projects, nor the one in the repo I supplied. I don't know if I'm doing something wrong though :( Here is an updated branch which still fails using the 5.7.0-SNAPSHOT:

https://github.com/fast-reflexes/spring-boot-bug/tree/filterSecurityInterceptor-5.7.0-SNAPSHOT

[darrenpm]

Yeah I can confirm that build doesn't fix the problem for me.
But the issue is that the latest build of 5.7.0-SNAPSHOT doesn't contain @marcusdacoregio's fixes, it's from a few days before his changes were merged.
For example he added a class RequestMatcherDelegatingWebInvocationPrivilegeEvaluator which is not in the latest build in the snapshot repo: spring-security-web-5.7.0-20211209.200017-32.jar
Not sure where a build containing his fixes would be?

[darrenpm]

@fast-reflexes can you test with 6.0.0-SNAPSHOT, looks like the changes are in there.

[fast-reflexes]

@darrenpm Seems it requires Java 17 and we have used Java 11 before .. and the transition does not seem trivial so I have to come back later on that ;)

[darrenpm]

Yeah, same for me. I think this needs to be backported to 5.6

[marcusdacoregio]

Hey everyone. Thanks for trying it.

The changes are not in 5.7.0-SNAPSHOT because the CI failed and we didn't notice it. I'm running it again and you may be able to use it some minutes after this run is complete https://github.com/spring-projects/spring-security/actions/runs/1611007169.

@darrenpm thank you for helping @fast-reflexes on how to configure a different version for Spring Security.

Yeah, same for me. I think this needs to be backported to 5.6

I do share the same thought. @rwinch is on PTO for now, but I'll talk to him to see if we can backport this to be included in the next 5.6.x release.

[fast-reflexes]

@marcusdacoregio I'm glad to say that everything works as expected with 5.7.0-SNAPSHOT in all my examples. Also, in Spring Boot 2.6.2, the ErrorPageSecurityFilter has been upgraded so that it doesn't allow being invoked on non-error dispatches which fixes the previous problem with MockMvc.

Nonetheless, there is another problem which has surfaced now, it seems that when the ErrorPageSecurityFilter has to process some expressions containing rules for which IP addresses may access a certain resource, it produces a

java.lang.UnsupportedOperationException: public abstract java.lang.String javax.servlet.ServletRequest.getRemoteAddr() is not supported

...when it tries to access the remote address on the request. The odd thing is that the checks from inside the security chain itself works out without exceptions so seems to be some other problem (probably unrelated to the problem of this ticket) related to this new filter (and possible some of the Spring Security classes it uses). I'll see if I have the time to put together a new minimal demo showing this.

Thanks again all! ➕

[marcusdacoregio]

Thanks @fast-reflexes.

That java.lang.UnsupportedOperationException is probably related to the DummyRequest that is created by FilterInvocation, which does not override getRemoteAddr(). If you can investigate where this call is happening and file a ticket, we can discuss whether it's a bug or not.

[fast-reflexes]

Great @marcusdacoregio ! I filed one: #10664

:)

[marcusdacoregio]

Hey folks, just a heads up that the fix has been backported to 5.5.x and 5.6.x, therefore, it is available in the next releases of these versions, before that, you can use the SNAPSHOT versions.

Stay tuned and, if possible, try the versions as soon as possible.
Thanks!