CSRF token mask from breach-mitigation-rails gem #16570
Conversation
This merges in the code from the breach-mitigation-rails gem that masks authenticity tokens on each request by XORing them with a random set of bytes. The masking is used to make it impossible for an attacker to steal a CSRF token from an SSL session by using techniques like the BREACH attack. The patch is pretty simple - I've copied over the [relevant code](https://github.com/meldium/breach-mitigation-rails/blob/master/lib/breach_mitigation/masking_secrets.rb) and updated the tests to pass, mostly by adjusting stubs and mocks.
|
Very nice @bradleybuda 👍 |
|
Looks good to me! |
|
Perhaps a CHANGELOG entry would be nice, indicate that we're masking the tokens now to mitigate BREACH and that if you were somehow manually generating your tokens before, you'll need to override |
There was a problem hiding this comment.
Trying to reason through how these stub changes affect coverage. Difficult to reason through. Are we covering the old cases as well as demonstrating the new ones we need to?
There was a problem hiding this comment.
The coverage remains on the old code, but the new stuff isn't fully tested. I'll add some additional cases and update the PR.
There was a problem hiding this comment.
@bradleybuda - OK! Aiming to include in 4.2 beta release, so we'll merge now and update tests + changelog for next beta ❤️
…f-token CSRF token mask from breach-mitigation-rails gem
There was a problem hiding this comment.
shouldn't we use ActiveSupport::SecurityUtils.secure_compare(token, real_csrf_token(session)) now?
This merges in the code from the breach-mitigation-rails gem that masks authenticity tokens on each request by XORing them with a random set of bytes. The masking is used to make it impossible for an attacker to steal a CSRF token from an SSL session by using techniques like the BREACH attack.
The patch is pretty simple - I've copied over the relevant code and updated the tests to pass, mostly by adjusting stubs and mocks. @NZKoz suggested that this would be a useful thing to have in Rails itself - we (and many others) have been using it in production for over a year with no issues.
The "length-hiding" code from the breach-mitigation-rails gem is not included here, as it is more complex, more brittle, and provides less protection against the attack - I don't think it's worth including.