Skip to content
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 filter in Play doesn't allow custom token generator #1734

Closed
brikis98 opened this issue Sep 24, 2013 · 9 comments
Closed

CSRF filter in Play doesn't allow custom token generator #1734

brikis98 opened this issue Sep 24, 2013 · 9 comments
Milestone

Comments

@brikis98
Copy link

For the last year or so, we've been using the CSRF filter originally created by @jto. This filter allowed the user to specify a custom token generator, which is important if your deployment includes other (non-play) apps that read your token and expect it to use a specific format.

We're upgrading to Play 2.2 and it looks like the CSRFAction class does not support a pluggable generator: it is just hard-coded to use Crypto.generateSignedToken. Would there be any objections to us submitting a pull request to add tokenGenerator: => String as a constructor parameter of CSRFAction and CSRFFilter? We could set the default value to Crypto.generateSignedToken, so it would be backwards compatible, while still allowing an injection point for this custom behavior.

@jroper
Copy link
Member

jroper commented Sep 24, 2013

Hi @brikis98,

I'll look at restoring this functionality today.

@jroper
Copy link
Member

jroper commented Sep 24, 2013

Note that the reason we switched to signed tokens is to address the BREACH security vulnerability.

@brikis98
Copy link
Author

Thanks @jroper. Can you provide a bit more info about how the signed token helps deal with BREACH?

@jroper
Copy link
Member

jroper commented Sep 24, 2013

The signed token randomises the token value on each request, making it impossible to character by character ascertain what it is using a BREACH style exploit. You're vulnerable if you're using SSL with HTTP level gzip encoding.

@jroper
Copy link
Member

jroper commented Sep 24, 2013

@brikis98
Copy link
Author

Hm, perhaps I'm misreading the code, but it like a new token isn't generated on each request, but only if the request doesn't already have one:

} else if (getTokenFromHeader(request, tokenName, cookieName).isEmpty && createIfNotFound(request)) {

      // No token in header and we have to create one if not found, so create a new token
      val newToken = Crypto.generateSignedToken
}

@jroper
Copy link
Member

jroper commented Sep 24, 2013

The raw token itself is generated once per session. However, the token is resigned on each use, see:

https://github.com/playframework/playframework/blob/master/framework/src/play-filters-helpers/src/main/scala/play/filters/csrf/csrf.scala#L58

Hence the signed token is random per request, but the raw token it's generated from is static.

@brikis98
Copy link
Author

Ah, gotcha. Thanks for the detailed explanation.

@jroper
Copy link
Member

jroper commented Sep 24, 2013

@brikis98 I've implemented this, feel free to review to make sure it has what you need. Note that I've also implemented a feature to disable signing for your use case:

#1737

jroper added a commit to jroper/playframework that referenced this issue Oct 1, 2013
* Fixes playframework#1734, custom token generator feature reinstatement
* Fixes playframework#1728, ensured CSRFFilter can be instantiated without a running
  application
* Added a csrf.sign.tokens conifguration option to switch between
  default CSRF token providers, either signed or unsigned.
* Abstracted tests so they can be run on many different permutations of
  configuration
* Added documentation about all the different configuration options

This commit breaks binary compatibility, the CSRFFilter constructor
parameters are now not lazy, and CSRFFilter is no longer a case class,
so many of the methods it used to provide are no longer there.  This was
deemed necessary because the intended use of CSRFFilter, ie:

    object Global extends WithFilters(CSRFFilter()) with GlobalSettings

was not possible with the old constructor.  The constructor is however
still source compatible for most use cases.

Since that constructor is intentionally breaking binary compatibility,
new parameters that were added for custom token generation and
configuration signing were added without consideration for binary
compatibility, only source compatibility.
@jroper jroper closed this as completed in 01a2445 Oct 3, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants