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

[johnraycp]

This is a change to the CsrfFilter class to add a random value that the CSRF token is XORed with. I've isolated the change to just the CsrfFilter. The code maybe isn't as clean as my prior pull request but it's simpler and doesn't require changing any interfaces.

NOTE: On a server using SSL and gzip compression this does not protect sensitive data which can be retrieved via a GET request to a page. However, it does stop an attacker from submitting forms or viewing sensitive data returned from a form submit.

Related #4001 and #4042

[jgrandja]

Hi @johnraycp. Thanks for taking the time in submitting this PR.

Although the CSRF token may be exposed to BREACH when returned in a response body, for example, a hidden parameter in a form, it still does not generally solve BREACH overall. As @rwinch noted in the comment, there may be other sensitive data exposed in a form, such as bank information or SIN #.

The ideal solution IMO would allow the "masking" of all sensitive data on the outgoing response, not just the CSRF token. This feature would obviously require more of an effort but may very well provide a full-feature for solving or at least mitigating BREACH.

Disabling HTTP Compression altogether or Referrer Check Mitigation or HTTP Chunked Encoding Mitigation are other possible solutions but this would require changes at the Web Server level and are less-intrusive.

I did look at your proposed changes and wanted to point out that there are a few failing tests as a result of the changes.

You mentioned in your comment that other frameworks have implemented the "masking" of secrets. Can you please provide me more information on which frameworks have implemented this as I'd be curious to look at their implementation.

[johnraycp]

If you're willing to accept this change let me know and I'll fix the tests.

Other examples I've seen using this technique are:

• Rails - CSRF token mask from breach-mitigation-rails gem rails/rails#16570
• Facebook - https://www.facebook.com/notes/protect-the-graph/preventing-a-breach-attack/1455331811373632/
• PHP add-on - https://defuse.ca/mitigating-breach-tls-attack-in-php.htm

The PHP add-on also has an obfuscate function that randomly adds the Unicode Character 'ZERO WIDTH SPACE' (U+200B) before and after each character being encoded. So the single character 'a' ends up like

`
​​​​​a​​​​​

`

The obfuscate idea could work for displaying text to a user on the page but won't work for the CSRF token. Perhaps bank information or SIN # should be fixed separately as a new ThymeLeaf obfuscate tag?

[jgrandja]

Thanks for sending the info @johnraycp.

I read the following re: Facebook:

Facebook's defense in depth keeps the CSRF token under wraps even in the most hostile circumstances. Many other proposed solutions, such as adding random padding to payloads, would merely delay an exploit rather than prevent it. Turning off compression would slow Facebook down dramatically, especially for mobile clients. Some websites could get away with watching the referrer and only enabling compression for trusted referrers. Facebook has a significant presence on third-party websites, so this would again hurt browsing speed on a huge number of websites where Facebook components are embedded.

Unfortunately, there is currently no silver bullet that can prevent this attack everywhere. As a result, each website owner—especially those managing user accounts—should evaluate their defense against BREACH and CSRF attacks to make sure they are taking sufficient precautions.

As the bold highlight indicates, the method of defence goes a lot further than what any framework can help solve or mitigate as this needs to go deeper into the IT infrastructure setup, network layer security and even constant monitoring of these types of attacks to be really tight on security.

Nevertheless, as @rwinch indicated and I agree as well, fixing the CSRF support would be a step in the right direction.

Looking at the changes further, my preference would be to contain these changes in an implementation of CsrfTokenRepository instead of the CsrfFilter itself. It would be a similar implementation as HttpSessionCsrfTokenRepository that stores the CsrfToken in the session and would also contain the XOR logic when the token is generated and compared with. This new implementation of CsrfTokenRepository can then be set on the CsrfFilter without any changes to the filter itself.
What are your thoughts on this strategy?

[johnraycp]

Unfortunately the CsrfFilter does a String equals on the tokens. Since the whole point of this enhancement is to return a different token value each time the comparison always fails. So at least a minor change to the CsrfFilter is required to call a function to see if the token String in a request matches the token. I had submitted a request with a compare() method add to the CsrfToken interface but it was rejected because it modified an interface.

[jgrandja]

@rwinch and I discussed on what the best possible strategy to go with that would be completely passive and the default method strategy in Java 8 looks like the way to go.

For example, we could add the following default method to CsrfToken

public interface CsrfToken {
    …
    default boolean matches(CsrfToken token) {
        return getToken().equals(token.getToken());
    }
}

We can then override the default method in our implementation of CsrfToken to handle the XOR logic.

Spring Security 5.0 will support Java 8 so we will have to wait until that release to implement this. So for now I will close this PR but leave the issue open so we keep track of this feature for 5.0 release.

We appreciate your input and welcome more contributions.
Thank you.