-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Fix CSRF protection provided by @EnableWebSocketSecurity / Stomp #12378
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
Hi @emopti-jrufer! Thanks for reporting this issue. Would you be able to put together a minimal, reproducible sample with a unit test that demonstrates the behavior? Ideally any external dependencies (if any) would be mocked, but I'm not personally familiar with how to do so. If it's not possible to put together a sample, it may be a bit of time before I could take a look at this. |
@sjohnr I created a sample that reproduces the issues. https://github.com/emopti-jrufer/spring-security-issue-12378-sample. |
I faced the exact same Issue. Please work on that asap as basically STOMP Websockets are broken and can't be used. The 2 fixes ( |
Thanks @emopti-jrufer for the sample! I was able to downgrade your sample to Spring Security 5.8.1 so I can do some testing prior to upgrading to 6.0 and can definitely see the issue. I unfortunately am not very familiar with websocket/stomp support so I missed this. @OlliL sorry to hear about your troubles. Thanks for verifying the workaround! Based on my testing, I believe the workaround can be simplified to no longer need the @Configuration
@EnableWebSecurity
public class SecurityConfiguration {
@Bean
public SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception {
CsrfTokenRequestAttributeHandler requestHandler = new CsrfTokenRequestAttributeHandler();
requestHandler.setCsrfRequestAttributeName(null); // <-- opt out of deferred tokens in 6.0
http
.authorizeHttpRequests((authorize) -> authorize
.requestMatchers("/csrf").permitAll()
.anyRequest().authenticated()
)
.csrf((csrf) -> csrf
.csrfTokenRequestHandler(requestHandler)
)
.formLogin((formLogin) -> formLogin
.successHandler(new BasicAuthenticationSuccessHandler())
);
return http.build();
}
// ...
} @emopti-jrufer can you verify that the above configuration removes the need for a custom This one looks like a tricky issue to resolve given the 5.8/6.0 work for |
I did face the same problem and struggled for some days, thanks @emopti-jrufer for your initial solution! It did work for me as well. I just tried the solution of @sjohnr removing the custom |
@sjohnr the solution you provided from a code point is simpler to implement but there is a small downside where the HTTP upgrade It should be noted that the simpler solution does not stop using deferred tokens but rather triggers them to be resolved right away. |
I can also confirm that its working with the remark @emopti-jrufer explained.... Since I already created an example to show the issue I link it here: https://github.com/OlliL/spring-boot-stomp - the fixes are already applied. There is a simple HTML Page served which opens a Websocket-Connection to the running Spring Boot Container to show the issue.... I know its no longer needed but the code was already created by me :) |
That is a great point, @emopti-jrufer, thanks for pointing that out!
Thanks @OlliL! Thanks for verifying @kaans! |
@emopti-jrufer (and others) thanks for your patience! I have created a branch on @Bean
public ChannelInterceptor csrfChannelInterceptor() {
return new XorCsrfChannelInterceptor();
} Would you have a few minutes to try this out in a real application? You can check out my branch and install snapshots to maven local as follows[1]:
[1]: On Mac/Linux. Steps for Windows should be similar. If a 6.0 baseline would be better for you, let me know and I can prepare a branch. |
@sjohnr |
Ok @emopti-jrufer no problem. The reason I mention it is that we recommend upgrading to Spring Security 5.8 first using the 5.8 migration guide. Branch of 6.0.x: gh-12378-6.0.x Following similar steps as above will (currently) build |
@sjohnr I am assuming you will be setting |
That's correct! And good point, I did not do that on this branch but will do so before merging. Would you like me to apply that change so you can test once more, or are you good with the testing as is? |
I am good as is. |
Uhm.... so I updated to 6.0.2 and removed the EagerCsrfTokenHandshakeInterceptor from my StompEndpoint registry as well as my csrfTokenRequestHandler from my csrf configuration. But even if I didn't - CSRF stopped working.... Judging from XorCsrfTokenUtils.getTokenValue It feels like there is some base64 encoding expected?! I wonder what am I missing CSRF Config:
Websocket Config:
EditI currently have no idea why the server does not emit an Xored CSRF Token but expects one back - could it be an issue with the CookieCsrfTokenRepository I'm using? So right now I had to explicitly define that bean to get it working -
Any hint what could have caused this issue would be appreciated. |
@OlliL, I've updated the migration guide for 5.8 to include the upgrade steps to prepare for 6.0. I also added cleanup steps to the 6.0 migration guide. You should no longer need the workaround with
In your case, it sounds like you would also benefit from reviewing the info in the 5.8 migration guide on using CookieCsrfTokenRepository. This outlines how to deal with deferred CSRF tokens. |
@sjohnr the thing is - while using The default So I currently see no other way except supplying the This is my SecurityConfiguration - The This is my WebSocketConfiguration as well as my WebSocketSecurityConfiguration. All the "hacks" where removed but thats the current setup I need to get it working with the current spring-security. |
I see. Thanks @OlliL! There are a number of different permutations possible depending on both your client-side framework and app code, in addition to using websockets. I wasn't considering the fact that you're using an Angular-style integration that obtains the raw token from the cookie itself. The CookieCsrfTokenRepository example could be modified to set a response header to obtain the hashed token. Note that there is also an option listed in the migration guide demonstrating an example Having said that, it sounds like you found the correct configuration. I'm not sure how much more specific the migration guide can be (your case is probably a common one but still one of many). If you feel there's room for improvement in the docs, feel free to submit an issue with suggestions and I'll happily take a look! |
@OlliL @sjohnr I am using Angular as our client-side framework. I ended up with a hybrid approach to use The solution could be a lot simpler if it was natively in the framework. For example I can share can the implementation or even contribute the framework if it would beneficial to others. |
I believe this is true because of
Can you elaborate on this point? What would be changed in the framework to make this work better for you? Keep in mind that the framework has to support both session and cookie-based CSRF tokens. Also bear in mind that the design consideration for deferred tokens is that the application itself should choose when the token is loaded. I don’t think the framework can make that choice for you. |
@sjohnr
My first attempt at trying eliminate the need for the extra
However all of these options have an issue where the csrf cookie was being set 2 times in the response. One for the clearing of the token in Possible solutions:
Could clarify the following for me? When referring to "loaded" are talking about retrieving the token or generating the new value?
This is referring to the locked down nature of the framework where certain internal apis are unavailable and classes are not extensible. Note: The debug statement in |
@emopti-jrufer appreciate the extra details! The behavior you're trying to puzzle out now is actually how to solve for deferred tokens in Spring Security 6, which the 5.8 migration guide covers in detail. If you follow the guide and run into issues with deferred tokens, you have 3 options for handling deferred tokens:
If you had followed the upgrade path from 5.7 to 5.8 and performed these steps in order to see which one worked for you (before trying to enable BREACH protection in a later step), you should find that Option 3 solves your problem. This is because in your case, you're using Next, when you try the steps in the next section to enable BREACH protection, you would find you have two options for handling it:
In your case, you have already found that Option 1 solves your problem. But because it wasn't combined with the solution from the deferred token section, you've struggled to get it working the way you'd like it to. I recognize that this is a bit of a "choose your own adventure" book, but that's because there are so many possible ways you can choose to integrate the frontend with Spring Security. Your case is one among many. Having said all that, here is what I believe should work for you: @Configuration
@EnableWebSecurity
public class SecurityConfiguration {
@Bean
public SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception {
CookieCsrfTokenRepository csrfTokenRepository = CookieCsrfTokenRepository.withHttpOnlyFalse();
XorCsrfTokenRequestAttributeHandler delegate = new XorCsrfTokenRequestAttributeHandler();
delegate.setCsrfRequestAttributeName(null);
// @formatter:off
http
.authorizeHttpRequests((authorize) -> authorize
.anyRequest().authenticated()
)
.csrf((csrf) -> csrf
.csrfTokenRequestHandler(delegate::handle)
.csrfTokenRepository(csrfTokenRepository)
)
.formLogin(Customizer.withDefaults());
// @formatter:on
return http.build();
}
} This solution should not require any calls to the At this point, I'm not really sure how any of this relates to web socket support. I've included the explanation and example above as an exercise in discovering what (if anything) is missing from our documentation that would help users like yourself (who come into this a bit later than you did and can benefit from better docs). If you feel I'm missing something, please correct me where I'm wrong. I feel like the main issue here is just following the migration guide top-to-bottom. If this is done, users will hopefully have a better experience. Can you think of anything I'm not considering? |
I also missed answering your questions, sorry about that.
Both. The application chooses to trigger loading the token via
All of the components you mentioned can be extended through delegation. You can use |
@sjohnr Is there a reason this information would be documented in a migration guide? Isn't this information still relevant in the latest version? We started on Spring 6 so following a migration guide for prior versions seems counterintuitive. That said I will try out your suggestions and if needed open separate issues or post on Stack Overflow. |
Thanks for the question, @emopti-jrufer! So as not to encourage a forum-like discussion here, I'll keep my answer brief. Since there were numerous highly impactful CSRF-related changes in Spring Security 6, I wanted to address upgrade issues first. Any positive feedback (this is usually rare 😉) would be used to make improvements on the main documentation for CSRF as time allows. Documentation improvements are a theme we're focusing on, so now is a good time to address it. We're also always looking for individuals who would be interested in making improvements to our documentation, as it tends to be an easy way to get involved with open source without a huge time commitment. If you're interested in helping, let us know by chiming in on an issue! I'll open a new issue for CSRF documentation enhancements when I get some time this week. |
@sjohnr Sry for using this as a forum-like discussion, but I need to know if the configuration is the right approach for an reactive
I'm asking as there's no way to call |
@HJK181, is this what you're looking for? https://docs.spring.io/spring-security/reference/5.8/migration/reactive.html#_i_am_using_angularjs_or_another_javascript_framework |
Shame on me, did not see the reactive menu entry on the left-hand side, thx a lot. |
Does this mean the websockets are broken in the 5.x branch? Any chance to have this ported to 5.x? |
@ptahchiev not that I'm aware of. If you find anything, please open an issue. Please note that this support was added to 5.8 (see gh-12562). |
To disable CSRF with stomp spring boot 3 just add this code in your configuration class, the bean name must be csrfChannelInterceptor :
|
CSRF protection provided by
@EnableWebSocketSecurity
is broken. I have identified 2 things that prevent theCsrfChannelInterceptor
from validating the CSRF token from the CONNECT headers.First problem encounted was the stack trace below. The
CsrfChannelInterceptor
was trying to access theCsrfToken
when processing the CONNECT message. The changes to made in 475b3bb and d94677f to defer the loading of the token caused the deferred supplier to try to access the http upgradeHttpServletRequest
after it has gone out of scope.Attempting to come up with a fix for the first problem I added an additional HandshakeInterceptor that forced eager loading the CsrfToken while they upgrade HttpServeletRequest was still in scope. I added the source below for clarity.
This overcame the first problem but then the
CsrfChannelInterceptor
started throwingInvalidCsrfTokenException
. After investigating it became apparent theCsrfChannelInterceptor
was only design to work with tokens that were not masked. SinceXorCsrfTokenRequestAttributeHandler
is now the default implementation forCsrfTokenRequestHandler
theCsrfChannelInterceptor
needs to be modified to have the same or similar logic as theCsrfFilter#doFilterInternal
.The text was updated successfully, but these errors were encountered: