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

Draft of adding CSRF protection for AJAX requests #1017

Merged
merged 1 commit into from Aug 18, 2015

Conversation

Projects
None yet
3 participants
@MaxGabriel
Copy link
Member

MaxGabriel commented Jun 23, 2015

cf #1016

The code is missing tests and high-level documentation, but should be ready for review otherwise.

  • Thoughts on what the default name should be for the CSRF cookie and header? I'm not super familiar with what other frameworks use (I see Angular uses "XSRF" and Rails uses "csrf"). Yesod uses "CSRF" in its functions instead of "XSRF", which is a strike against using "XSRF" for the naming scheme, even if it ends up being more consistent with tools like Angular.
  • Is yesod-core the right home for these functions?

cc @gregwebs

@MaxGabriel

This comment has been minimized.

Copy link
Member Author

MaxGabriel commented Jun 23, 2015

Added a draft of the CSRF overview section. Need to verify the haddocks look sensible but am otherwise happy with that part.

Edit: Have verified Haddocks look correct.

@gregwebs

This comment has been minimized.

Copy link
Member

gregwebs commented Jun 23, 2015

For CSRF vs XSRF, you are doing it right by picking one and making it configurable

@MaxGabriel

This comment has been minimized.

Copy link
Member Author

MaxGabriel commented Jun 23, 2015

This current PR also requires manually calling checkCsrfHeader on requests that require it. Ideas on making that more automatic/seamless when accepting AJAX requests, the way that runFormPost handles CSRF tokens automatically? I was thinking of some sort of variant of requireJsonBody that also checked the CSRF header.

@gregwebs

This comment has been minimized.

Copy link
Member

gregwebs commented Jun 23, 2015

Hmm, I would like to turn this into a middleware (at least a yesod middleware). That way enabling it would just mean dropping in a middleware.

@MaxGabriel

This comment has been minimized.

Copy link
Member Author

MaxGabriel commented Jun 23, 2015

A Yesod middleware seems right for this, sounds good (Wai middleware is doable, but would it need a function passed in to extract the CSRF token from the request; not sure if that's worthwhile).

@gregwebs

This comment has been minimized.

Copy link
Member

gregwebs commented Jun 24, 2015

You are welcome to use a Yesod middleware. If you went the wai route, you would probably take advantage of the vault parameter for request-local storage rather than passing in a function.

@MaxGabriel MaxGabriel force-pushed the MaxGabriel:ajaxCsrfProtection branch from 46421a3 to 91ec997 Jul 1, 2015

@MaxGabriel

This comment has been minimized.

Copy link
Member Author

MaxGabriel commented Jul 1, 2015

Getting pretty close. Added Yesod Middleware and tests. The Yesod Middleware isn't well organized or documented yet, so working on cleaning up that.

@MaxGabriel MaxGabriel force-pushed the MaxGabriel:ajaxCsrfProtection branch from a7fa940 to d256ead Jul 6, 2015

@MaxGabriel

This comment has been minimized.

Copy link
Member Author

MaxGabriel commented Jul 6, 2015

This should be ready for review. I'm happy with the explanation of CSRF attacks and the prevention mechanism. The middleware makes CSRF protection very simple to add.

I'm not sure how this fits into yesod-form's CSRF protection yet; this middleware could handle that case if you used Javascript to add the CSRF header when a request was made from a form. If you didn't have Javascript enabled, that wouldn't work, though. Maybe on version of the middleware could skip CSRF protection for forms somehow? Anyway, I'm investigating this.

@gregwebs

This comment has been minimized.

Copy link
Member

gregwebs commented Jul 6, 2015

forms should be sent as form-data. This CSRF protection would then only be on for JSON data (content type of application/json).

@MaxGabriel

This comment has been minimized.

Copy link
Member Author

MaxGabriel commented Jul 15, 2015

Yeah, it looks like I can't just prevent CSRF protection from applying to application/x-www-form-urlencoded and multipart/form-data because functions like requireJsonBody don't require an application/json content type, so those functions would be vulnerable to CSRF by sending a JSON body with a form content type. I could potentially change those functions to require that content type (though that's a breaking change).

I can have it only run CSRF protection on application/json, but it might end up biting people when they use javascript to submit a different content type and silently don't have CSRF protection.

I can also just require users to add the CSRF header to their forms using e.g. jQuery. This makes it take extra work to support users with Javascript disabled, though. Maybe that's ok.

@gregwebs

This comment has been minimized.

Copy link
Member

gregwebs commented Jul 15, 2015

This makes it take extra work to support users with Javascript disabled, though. Maybe that's ok.

Doesn't one have to use the existing Yesod forms that inject a hidden attribute to support disabled javascript?

@MaxGabriel

This comment has been minimized.

Copy link
Member Author

MaxGabriel commented Jul 16, 2015

@gregwebs Yeah the current yesod-forms library works with disabled Javascript, but the middleware will reject the request before the handler can run the form to check the CSRF token.

@snoyberg

This comment has been minimized.

Copy link
Member

snoyberg commented Jul 19, 2015

@gregwebs @MaxGabriel For the record, this is the prime example of a PR you guys should feel free to move ahead with without my input :)

@MaxGabriel

This comment has been minimized.

Copy link
Member Author

MaxGabriel commented Aug 17, 2015

I'm not sure what I was thinking before, to handle the case where Javascript was disabled I just needed to check the form parameters for CSRF tokens as well, which I thought would be hard for some reason but was quite easy.

I made a few more modifications like using constant time comparison, as yesod-form suggests.

Anyway, I think this PR is ready for final review. I know you said to go ahead without your input @snoyberg but since it's a security thing I wouldn't mind more review; for a long time this PR wasn't doing things like constant time comparison.

@snoyberg

This comment has been minimized.

Copy link
Member

snoyberg commented Aug 17, 2015

LGTM 👍

checkCsrfHeaderOrParam headerName paramName = do
validHeader <- hasValidCsrfHeaderNamed headerName
validParam <- hasValidCsrfParamNamed paramName
unless (validHeader || validParam) (permissionDenied "Invalid CSRF token in header.")

This comment has been minimized.

Copy link
@gregwebs

gregwebs Aug 17, 2015

Member

The error message here only mentions one case.

res <- request (defaultRequest { requestMethod = "POST" })
assertStatus 403 res
it "403s write requests with the wrong CSRF header" $ runner $ do
res <- request (defaultRequest { requestMethod = "POST", requestHeaders = [(defaultCsrfHeaderName, "foo")] })

This comment has been minimized.

Copy link
@gregwebs

gregwebs Aug 17, 2015

Member

it would be good to see this parallel the above test more closely (run setCookieValue and append "foo" to the value)

@gregwebs

This comment has been minimized.

Copy link
Member

gregwebs commented Aug 17, 2015

looks fantastic! I only had 2 comments for making minor improvements. Looks like this also need to be rebased to the lastest master.

@MaxGabriel MaxGabriel force-pushed the MaxGabriel:ajaxCsrfProtection branch from 0a6d2b1 to 132abce Aug 17, 2015

@MaxGabriel

This comment has been minimized.

Copy link
Member Author

MaxGabriel commented Aug 17, 2015

Cool, I made changes based on @gregwebs's comments and rebased against master. I'll rebase into one commit and merge later tonight.

@gregwebs

This comment has been minimized.

Copy link
Member

gregwebs commented Aug 17, 2015

After merging

  • add to scaffold

@MaxGabriel MaxGabriel force-pushed the MaxGabriel:ajaxCsrfProtection branch from 132abce to 33982b2 Aug 17, 2015

MaxGabriel added a commit that referenced this pull request Aug 18, 2015

Merge pull request #1017 from MaxGabriel/ajaxCsrfProtection
Adding CSRF protection for AJAX requests

@MaxGabriel MaxGabriel merged commit 0f55dcc into yesodweb:master Aug 18, 2015

1 check passed

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

@snoyberg snoyberg removed the in progress label Aug 18, 2015

@MaxGabriel

This comment has been minimized.

Copy link
Member Author

MaxGabriel commented Aug 19, 2015

Cool, I'm adding it to this PR to the yesod-scaffolding yesodweb/yesod-scaffold#76

Working on adding support to yesod-test for using CSRF tokens this way; mostly done with that.

@MaxGabriel

This comment has been minimized.

Copy link
Member Author

MaxGabriel commented Aug 21, 2015

So based on some testing, it seems like lookupPostParam will consume the response body when JSON is sent to the server, so the response body is empty when the handler tries to parse JSON from it.

Haven't investigated much; initial ideas are to only use lookupPostParam if the header doesn't have the CSRF token, or to only use lookupPostParam on the content types forms use.

Edit: It looks like the root cause of this is yesod-test adding the "application/x-www-form-urlencoded" Content-Type to requests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.