Skip to content

CSRF Token handler with BREACH detection, several concerns are unclear from the documentation #12652

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

Closed
anschnapp opened this issue Feb 9, 2023 · 5 comments
Assignees
Labels
in: docs An issue in Documentation or samples status: duplicate A duplicate of another issue type: enhancement A general enhancement

Comments

@anschnapp
Copy link

anschnapp commented Feb 9, 2023

Expected Behavior
I could read the documentation (migration guide and or normal documentation) and have a good understanding how the new csrf protection works. When it is meaningful to use it and when not. How to proper set it up to have a real effect.

Current Behavior

There are several concerns about the new default csrf behaviour which seems not to be addressed in the documentation or in he code:

  1. If the new default handler XorServerCsrfTokenRequestAttributeHandler is used together with the repository CookieServerCsrfTokenRepository then the prevention strategy does not make any difference to me. Because the token will be send in 2 versions for every request (in due to the stateless repository). One Version which is "hardened" and the original plain version. If i got the BREACH attack correctly the plain version would be vulnerable here. Because of the known length and of being static for potential many requests (of the same users). And an advanced attacker could use the hijacked plain version to also generate a valid (new) XOR version.

  2. I'm wondering if the correct usage of XorServerCsrfTokenRequestAttributeHandler would also involve an update of the csrf token on a regular basis. Which seems at least not be the case right now for single page applications which communicate on their own per REST to the BE. If there will not be a regular update of this representation then i guess it would not help agains the BREACH attack. Because it's then again a secret static within many requests which goes over the wire.

Context

I'm currently confused how to go on after the migration to the new spring security version (inside a spring boot 3 update)
Do I get additional security benefits by switching (we use it together with CookieServerCsrfTokenRepository). Do I have to adjust the workflow how FE is "refreshing" csrf tokens.

EDIT:

  • Remove concern about breaking user sessions, seems like chapter "I am using AngularJS or another Javascript framework" from the migration guide (only the 5.8.1 preparation before 6.0) handle this case.
@anschnapp anschnapp added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Feb 9, 2023
@dadouam
Copy link

dadouam commented Feb 9, 2023

I agree that more information on this new default could be helpful, I had to dig a little into Stackoverflow and here to have a better understanding on this BREACH attack and the CSRF-related changes in 6.0.

About your 2nd point, considering I'm also making a "REST" application that does not generate any HTML pages using Spring Boot (pure SPA), here is what I understood (sources: https://breachattack.com/ and https://www.youtube.com/watch?v=hw4BRoO45SU and https://resources.infosecinstitute.com/topic/the-breach-attack/):

  • For the BREACH attack to work, the response body must reflect in some way the user input. For example, the attacker could control a query parameter that appears in the response body somewhere. Usually, response bodies in such scenarios would be server-rendered web pages.
  • By adjusting the value of this reflected input, the attacker can guess that certain patterns of characters are repeated because of the way compression works (this is why the attack is mitigated when server compression is disabled), this bypasses TLS even if the content remains encrypted.
  • In order for it to work, there must be some repetition of text.
    • Before 6.0, my understanding (because I don't do server-rendered pages) is that Spring Security included the CSRF token somewhere on web pages, either as a hidden input field, meta field, etc...
    • The value of this token never changed for the session duration.
    • So this token was potentially vulnerable to the BREACH attack as long as the attacker can find a way to inject text into the webpage.
    • Now that the attacker knows the CSRF value even though the content is still encrypted, the attacker can execute malicious requests on behalf of the victim.
  • This is why XorCsrfTokenRequestAttributeHandler was introduced in 6.0:
    • the raw value of the token never changes and is kept in places that are not subject to http body compression (user session, HTTP headers, including cookies)
    • when the token needs to be made available to the client (server-rendered page, SPA fetch request...): the returned value is the XOR version, two requests never provide the same "token". Spring Security will compare this XOR token with the raw token on subsequent CSRF-protected requests.
    • this mitigates the BREACH attack when the token is included as part of the HTML response because there is no longer a possibility of repetition.

Therefore, I don't think standard REST APIs scenarios are at risk. Unless I'm terribly wrong, the BREACH attack needs a secret value (CSRF, bank account number, ...) to be present in the response body and repeated across requests. I never send the CSRF token as part of my JSON request/response body so I can't think of a way that reusing the same XORed token can be a problem here.

I did "upgrade" to this new default though, along with the deferred token. I just had to create an additional endpoint /csrf to:

  • trigger the token loading at least once
  • initialize my SPA with the value of this token in browser memory (can't rely on the raw value from cookies anymore unless you opt out from the XOR handler)
  • handle edge cases via a custom AccessDeniedHandler to let the front-end know if it should receive a fresh token again.

Though I believe the old default is still relevant for REST APIs because I have a hard time imagining a scenario where a SPA can be vulnerable to this attack.

@anschnapp
Copy link
Author

Ok many thanks @dadouam for the explanation and the great links! (except of one, they were new to me)

I guess I know understand it better, after reading your comments and the resources you linked.

Back to my 2 topics, my current understanding would be:

1.) Using XorServerCsrfTokenRequestAttributeHandler with CookieServerCsrfTokenRepository is save because the values (also the fixed secret) are not included in the compacted body (instead they are cookie and custom header attributes).
However it also gives not a better security to use XorServerCsrfTokenRequestAttributeHandler here because if the value is not rendered inside the body we don't have to harden it.

2.) For single page apps there is no need to update the token. It's not "guessable" by BREACH attacks because it's not rendered in the body. (For server rendering pages there is a "native" update process).

Would be nice if someone could confirm if i'm correct this time ;)

But even if so, I think that documentation could be really improved here. It's quite difficult to get this right.

@sjohnr sjohnr self-assigned this Feb 14, 2023
@sjohnr
Copy link
Member

sjohnr commented Feb 15, 2023

Thanks for the feedback, @anschnapp and for the links and response, @dadouam!

Many of the concerns you're bringing up are questions, not necessarily enhancement suggestions. As you are no doubt aware, we prefer to use Stack Overflow for questions. I would encourage you to update this issue with a link to any re-posted questions (so that other people can find them), because much of what you're asking is relevant and may not have an answer on SO.

However, you've made some statements in your questions that I'd like to address (at least in part), because in this case I think the dialogue can help us produce better documentation. I also agree that we need better documentation because we've gotten fairly consistent feedback on this topic, so discussing what can specifically be improved is valuable.

I'm wondering if the correct usage of XorServerCsrfTokenRequestAttributeHandler would also involve an update of the csrf token on a regular basis.

Hopefully the response and resources provided by @dadouam help answer this question. Regardless, I think this is definitely a topic that we need to address because it seems a common point of confusion (and for good reason).

In brief, I will say (or perhaps reiterate) that the front-end need not refresh the token repeatedly, as this is not purpose of BREACH protection in Spring Security. The idea behind the support is that whatever workflow you were using before should continue to work. (However, the AngularJS case noted above in your edit does require specific configuration to work around.)

the values (also the fixed secret) are not included in the compacted body (instead they are cookie and custom header attributes).

This is not necessarily true in all cases. One application may only expose a REST-like interface (we'll call it non-html) and another may expose both html and non-html responses. Only you can determine which scenario you're in, but the defaults are there to protect you through defense in-depth in the second case (especially if you're not aware you fall into the second case).

However it also gives not a better security to use XorServerCsrfTokenRequestAttributeHandler here because if the value is not rendered inside the body we don't have to harden it.

Same as above, this is not necessarily true for all users or in all cases. It may, however, be true for your application and it's really up to you to weigh tradeoffs and make this decision.

For single page apps there is no need to update the token. It's not "guessable" by BREACH attacks because it's not rendered in the body.

Again, same as above. The framework provides defense in-depth by default, which you are able to turn off or configure according to your requirements if the above statement is true for you.


If you have feedback or any of the points above don't sound correct, please feel free to add additional comments. I'd like to specifically hear if my responses feel like they could be expanded and become relevant and useful documentation, or if you have any other suggestions.

Having said that, I'm going to close this as a duplicate of gh-12462. Any feedback you (continue to) provide can be folded into that issue.

@sjohnr sjohnr closed this as completed Feb 15, 2023
@sjohnr sjohnr added status: duplicate A duplicate of another issue in: docs An issue in Documentation or samples and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 15, 2023
@anschnapp
Copy link
Author

@sjohnr thanks for all your explanation which you have posted, even if i used the wrong platform for the questions!

To be honest i was quite indecisive where to place it. I decided for the enhancement because i thought the root cause of my questions are missing descriptions in the documentation and i hoped this ticket could be the starting point to find an improvement here. I will take a look at the "duplicated by" ticket you posted. And add some suggestions there (if it's not addressed already)

Next time if i have questions of any kind i will prefer stack overflow (with the right tagging).

@sjohnr
Copy link
Member

sjohnr commented Feb 15, 2023

Not a problem @anschnapp! The main thing to keep in mind if you do want to submit enhancements is to include specific details about what can be improved, and not include questions. But in this case, the questions indicate that our documentation needs to be enhanced, so I agree it was probably worth a conversation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: docs An issue in Documentation or samples status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants