Fix Issue 4001: CSRF tokens are vulnerable to a BREACH attack

[johnraycp]

This is to fix issue 4001. The changes are:

• add an isValid() method to the CsrfToken interface to compare a String value from a HTTP request to a CsrfToken object.
• Modify DefaultCsrfToken
  • The constructor now generate it's own CSRF token instead of having it passed in
  • getToken() always return a different value.
  • Implement the isValid() method to check if a value is valid.
• The remainder of the changes were needed because of the DefaultCsrfToken constructor change and that isValid() must be called instead of the caller doing it's own string compare.

[pivotal-issuemaster]

@johnraycp Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-issuemaster]

@johnraycp Thank you for signing the Contributor License Agreement!

[hth]

I agree, currently CSRF generates the same token. But by generating a new token for every request, it does not solve the vulnerability. I can use any of these tokens and POST a new and different form which was not requested from server. Server would not stop the form submission as the token is still valid. The solution has to be somewhat like

  A request - X1 Token
  B request - X2 Token

Cannot interchange the tokens. B request should not be able to use X1 Token. And vice versa. A token and request has to be tightly coupled.

[johnraycp]

Over at the website http://breachattack.com/ in the mitigation section there are a few different solutions. You're talking about #3 "Randomizing secrets per request". My code is for #4 "Masking secrets (effectively randomizing by XORing with a random secret per request)".

#3 is slightly more secure but would require spring to keep a list of CSRF tokens.
#4 only stores one token in the session.

The main thing you want to do to prevent a BREACH attack is to send a random CSRF string with each request. It's fine if you can use the CSRF token from one request in a different one. Also #4 seems to be the solution that other frameworks are implementing. I assume they don't do #3 because there is no easy way to determine which CSRF tokens might still be used and which can't because the user can open pages in different tabs. So you end up have to keep a potentially large list of tokens in memory.

[hth]

👍

[rwinch]

Thanks for the PR @johnraycp!

BREACH is a general problem of being able to discover sensitive data (not just the CSRF token). For example, a user can easily discover a Bank Account Number from the request.

To solve BREACH with "masking sensitive data", users would need to perform an XOR with all of the sensitive data on their site. Spring Security does not know all the sensitive information for a particular domain, so it cannot provide support for this.

Instead of masking, it is recommended users disable HTTP compression for external requests (by looking at the referrer header). This allows performance to be maintained for users while protecting completely against the attack.

While the PR doesn't solve the risks of BREACH in general, I can see fixing for CSRF support as a valuable step in the right direction for Spring Security. However, I cannot see merging this particular implementation into Spring Security because we cannot make not passive changes to our APIs (i.e. adding a new method to an interface is not passive).

If we can come up with a passive way of implementing a fix without too much difficulty, I would consider putting a fix into Spring Security for BREACH. Otherwise, we will stick with the recommendation of disabling compression for external requests since this mitigates BREACH for all sensitive data.