spring-projects / spring-security

Spring Security
http://spring.io/projects/spring-security
Apache License 2.0
8.86k stars 5.92k forks source link

A way to re-enable CSRF for OAuth2 bearer token requests #8668

Open lightoze opened 4 years ago

lightoze commented 4 years ago

We have user-facing services which are accessed via OAuth2 proxy, so they are configured as resource server and bearer token is added to request headers by the proxy (from client session).

Currently OAuth2ResourceServerConfigurer.registerDefaultCsrfOverride is called unconditionally and it makes it impossible to use conventional CSRF configuration.

On reactive side there seems to be a solution (not tested though): when custom requireCsrfProtectionMatcher is specified, the override is not applied. Similar behaviour could be implemented for servlet security by introducing specifiedRequireCsrfProtectionMatcher flag on CsrfConfigurer.

jzheaux commented 4 years ago

Thanks for the report, @lightoze. Can you tell me more about your use case?

I'm asking because, since your proxy is the thing that has the client's session, I'd expect that to be where the CSRF token is generated/stored and where CSRF denials would be performed.

lightoze commented 4 years ago

In this setup both the proxy and each individual service have their own session. When CSRF is to be used, each service will use it's own session. If we ever want to have cross-service requests possible (not likely at the moment) we'll implement custom shared token repository, but decision when to use CSRF and when not to will be still on individual service side.

jzheaux commented 4 years ago

Okay, thanks @lightoze for the extra background.

Similar behaviour could be implemented for servlet security by introducing specifiedRequireCsrfProtectionMatcher flag on CsrfConfigurer.

Perhaps, though I wonder if applications already setting requreCsrfProtectionMatcher would be surprised to see the ignore setting no longer getting applied.

I'll keep this ticket open while I continue to research ways to accommodate your use case. In the meantime, I believe it will work to set the matcher directly on the filter using an ObjectPostProcessor:

.csrf(csrf -> csrf
        .withObjectPostProcessor(new ObjectPostProcessor<CsrfFilter>() {
            @Override 
            public <O extends CsrfFilter> O postProcess(O object) {
                object.setRequireCsrfProtectionMatcher(CsrfFilter.DEFAULT_CSRF_MATCHER);
                return object;
            }
        })
    )
lightoze commented 4 years ago

@jzheaux Thanks for the workaround!

zokkr commented 4 years ago

Hi @jzheaux With my current project, I have a similar requirement: I am not using the JWT in an authorization header, but instead (to have it in a secure place) I set it as HttpOnly and SameSite=Strict cookie and afterwards resolve it like this:

  public String cookieTokenExtractor(HttpServletRequest request) {
    String header = request.getHeader(HttpHeaders.AUTHORIZATION);
    if (header != null) {
      return header.replace("Bearer ", "");
    }
    Cookie cookie = WebUtils.getCookie(request, "access_token");
    return cookie != null ? cookie.getValue() : null;
  }

This leads to the need of having csrf validation in place. I modified your workaround (thanks for that one!) a bit, so it doesn't unnecessarily check GET requests to this one:

        .csrf(csrf -> csrf
            .withObjectPostProcessor(new ObjectPostProcessor<CsrfFilter>() {
              @Override
              public <O extends CsrfFilter> O postProcess(O object) {
                object.setRequireCsrfProtectionMatcher(request -> {
                  try {
                    return cookieTokenExtractor(request) != null;
                  }
                  catch (OAuth2AuthenticationException ex) {
                    return false;
                  }
                });
                return object;
              }
            })
        )

but basically reimplementing the BearerTokenRequestMatcher really only feels like a workaround. It would be great to have a configuration option to enable csrf also for requests that contain a JWT. If it defaults to being disabled, existing users would not feel a disruption, too.

OtenMoten commented 1 year ago

Hi @jzheaux With my current project, I have a similar requirement: I am not using the JWT in an authorization header, but instead (to have it in a secure place) I set it as HttpOnly and SameSite=Strict cookie and afterwards resolve it like this:

  public String cookieTokenExtractor(HttpServletRequest request) {
    String header = request.getHeader(HttpHeaders.AUTHORIZATION);
    if (header != null) {
      return header.replace("Bearer ", "");
    }
    Cookie cookie = WebUtils.getCookie(request, "access_token");
    return cookie != null ? cookie.getValue() : null;
  }

This leads to the need of having csrf validation in place. I modified your workaround (thanks for that one!) a bit, so it doesn't unnecessarily check GET requests to this one:

        .csrf(csrf -> csrf
            .withObjectPostProcessor(new ObjectPostProcessor<CsrfFilter>() {
              @Override
              public <O extends CsrfFilter> O postProcess(O object) {
                object.setRequireCsrfProtectionMatcher(request -> {
                  try {
                    return cookieTokenExtractor(request) != null;
                  }
                  catch (OAuth2AuthenticationException ex) {
                    return false;
                  }
                });
                return object;
              }
            })
        )

but basically reimplementing the BearerTokenRequestMatcher really only feels like a workaround. It would be great to have a configuration option to enable csrf also for requests that contain a JWT. If it defaults to being disabled, existing users would not feel a disruption, too.

TL;DR

In a stateful JWT environment, the need for CSRF protection is often reduced due to inherent features such as token storage in HttpOnly cookies, adherence to the same origin policy, use of the Bearer token scheme, and stateful JWT verification. Additional mechanisms such as the default CSRF countermeasures in modern browsers and proper implementation of CORS further mitigate the risk. While these factors together provide a robust defence against CSRF attacks, it is important to recognise that security is a multifaceted discipline.

Personal statement: There's no need for CSRF in a stateful environment.

Detailed explanation

Cross-Site Request Forgery (CSRF) is an attack that tricks the victim into submitting a malicious request. This attack is specifically designed to change the state of requests, not steal data, as the attacker has no way of seeing the response to the fraudulent request. In the context of web applications that use JWT (JSON Web Tokens) for session management, the need for CSRF protection may be questioned. Here's why CSRF may not be necessary in a stateful JWT environment:

As you can see, there are many mechanics which protecting against man-in-the-middle attacks when using a stateless environment. Thus, the need for CSRF is not present.

ujhazib commented 1 year ago

The proposed workaround above has one minor flaw. In case of a CSRF token is invalid, the http status should be 403, but spring returns 401 in this case.

jzheaux commented 11 months ago

I'd like to take a moment to refresh my understanding of the situation here. When the token is stored in a cookie or in the session, then Spring Security's default CSRF defense is needed and should not be shut off by resource server configuration.

There are a few ways that we could do this:

  1. Stop overriding CSRF configuration, requiring the application to disable (not passive)
  2. Add a flag to resource server configuration, something like OAuthResourceServerConfiguration#requireCsrf (passive, but mixes two concerns)
  3. Add a flag to CSRF configuration that allows replacing any existing overrides with an overarching one, something like ignoringRequestMatchers(Consumer<List<RequestMatcher>> ignoringRequestMatchers) where the final list could be mutated or ignoreOnlyRequestMatcher(RequestMatcher) (passive)
  4. Override only for known-good BearerTokenResolver instances, e.g. those that don't use the session, a cookie, or HTTP Basic to pass the token. (not passive)

Option 4 could be made passive possibly by introducing a sub-interface to BearerTokenResolver called UsesBrowserCredentialsBearerTokenResolver or by introducing a default method in BearerTokenResolver like usesBrowserCredentials that returns false by default (forgive the names).

The first three options leave it up to the application to decide, so I don't want to only do that. I currently like a combination of options 3 and 4 the best since option 3 still gives the application full control should it feel that Spring Security is making the wrong decision and option 4 passively makes ResourceServerConfigurer's decision-making a bit smarter while remaining passive.

ilopezv commented 3 weeks ago

In my particular opinion, locate where the CsrfConfigurer contribution was being made by BearerTokenRequestMatcher was very tricky.

I think it could be an option to do this:

  1. define a flag in CsrfConfigurer (Option3) in order to stablish a way for user interaction and follow single responsibility principle
  2. modify this new flag throw registerDefaultCsrfOverride in OAuth2ResourceServerConfigurer (Option 2)

At startup OAuth2ResourceServerConfigurer will disable CSRF for OAuth2 scenarios but if developers want to re-enable it throw CsrfConfigurer will take precedence over OAuth2ResourceServerConfigurer

If none of the options works for you, I propose to update Spring Security documentation telling that in OAuth2 scenarios CSRF will be disabled by default.

By the way, something didn´t work as spected with MockMvc and SpringBootTest because in the default scenario CSRF filter applies all the times, in my particular case I only found this problem in runtime, nor in integrated test, probably because there is no real token and instead of that there is a @WithMockUser