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

CSRF token mask from breach-mitigation-rails gem #16570

Merged
merged 1 commit into from Aug 20, 2014

Conversation

Projects
None yet
7 participants
@bradleybuda
Contributor

bradleybuda commented Aug 19, 2014

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.

Auth token mask from breach-mitigation-rails gem
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.
@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Aug 19, 2014

Member

Very nice @bradleybuda 👍

Member

jeremy commented Aug 19, 2014

Very nice @bradleybuda 👍

@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz Aug 19, 2014

Member

Looks good to me!

Member

NZKoz commented Aug 19, 2014

Looks good to me!

@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz Aug 19, 2014

Member

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 valid_authenticity_token??

Member

NZKoz commented Aug 19, 2014

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 valid_authenticity_token??

@@ -478,7 +479,7 @@ def teardown
def test_should_not_warn_if_form_authenticity_param_matches_form_authenticity_token
ActionController::Base.logger = @logger
SecureRandom.stubs(:base64).returns(@token)
@controller.stubs(:valid_authenticity_token?).returns(:true)

This comment has been minimized.

@jeremy

jeremy Aug 19, 2014

Member

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?

@jeremy

jeremy Aug 19, 2014

Member

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?

This comment has been minimized.

@bradleybuda

bradleybuda Aug 19, 2014

Contributor

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.

@bradleybuda

bradleybuda Aug 19, 2014

Contributor

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.

This comment has been minimized.

@jeremy

jeremy Aug 20, 2014

Member

@bradleybuda - OK! Aiming to include in 4.2 beta release, so we'll merge now and update tests + changelog for next beta ❤️

@jeremy

jeremy Aug 20, 2014

Member

@bradleybuda - OK! Aiming to include in 4.2 beta release, so we'll merge now and update tests + changelog for next beta ❤️

jeremy added a commit that referenced this pull request Aug 20, 2014

Merge pull request #16570 from bradleybuda/breach-mitigation-mask-csr…
…f-token

CSRF token mask from breach-mitigation-rails gem

@jeremy jeremy merged commit 79d50ce into rails:master Aug 20, 2014

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details
def compare_with_real_token(token, session)
# Borrow a constant-time comparison from Rack
Rack::Utils.secure_compare(token, real_csrf_token(session))

This comment has been minimized.

@tarcieri

tarcieri Oct 31, 2014

Contributor

👍

@tarcieri

tarcieri Oct 31, 2014

Contributor

👍

This comment has been minimized.

@vipulnsward

vipulnsward Oct 31, 2014

Member

shouldn't we use ActiveSupport::SecurityUtils.secure_compare(token, real_csrf_token(session)) now?

@vipulnsward

vipulnsward Oct 31, 2014

Member

shouldn't we use ActiveSupport::SecurityUtils.secure_compare(token, real_csrf_token(session)) now?

This comment has been minimized.

@vipulnsward

vipulnsward Oct 31, 2014

Member

😬 old PR, din't see.

@vipulnsward

vipulnsward Oct 31, 2014

Member

😬 old PR, din't see.

@dragonsinth

This comment has been minimized.

Show comment
Hide comment
@dragonsinth

dragonsinth Jan 13, 2015

Q: why is strict base64 used instead of base64url with lenient decoding? This is causing us problems interoperating between systems because we use base64url for CSRF tokens. We moved to base64url to avoid having to separately url-encode the CSRF value in certain contexts. base64url is just a lot easier to plumb since it's intrinsically safe.

Q: why is strict base64 used instead of base64url with lenient decoding? This is causing us problems interoperating between systems because we use base64url for CSRF tokens. We moved to base64url to avoid having to separately url-encode the CSRF value in certain contexts. base64url is just a lot easier to plumb since it's intrinsically safe.

This comment has been minimized.

Show comment
Hide comment
@bradleybuda

bradleybuda Aug 17, 2016

Contributor

I don't believe there was any good reason for that choice, though it's been a while since I looked at this code. Any encoding function that is invertible and bijective should be fine here; we just need to get the bytes in to and out of portable strings.

However, changing the encoding function could cause backwards-incompatibility blips when users upgrade; any outstanding CSRF values might be rejected and have to be reissued. A minor issue I think.

Contributor

bradleybuda replied Aug 17, 2016

I don't believe there was any good reason for that choice, though it's been a while since I looked at this code. Any encoding function that is invertible and bijective should be fine here; we just need to get the bytes in to and out of portable strings.

However, changing the encoding function could cause backwards-incompatibility blips when users upgrade; any outstanding CSRF values might be rejected and have to be reissued. A minor issue I think.

This comment has been minimized.

Show comment
Hide comment
@dragonsinth

dragonsinth Aug 17, 2016

I would love for someone to move on this:

#18496

I think it should be ok to change since the upgraded lenient base64url decoder will accept strict inputs. New CSRF tokens wouldn't be readable with older code.

dragonsinth replied Aug 17, 2016

I would love for someone to move on this:

#18496

I think it should be ok to change since the upgraded lenient base64url decoder will accept strict inputs. New CSRF tokens wouldn't be readable with older code.

@Alamoz

This comment has been minimized.

Show comment
Hide comment
@Alamoz

Alamoz Mar 20, 2015

Are you going to revisit #11729 and rebase with this merge as @jeremy suggested?

Alamoz commented Mar 20, 2015

Are you going to revisit #11729 and rebase with this merge as @jeremy suggested?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment