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

Increase the protection provided by the CSRF checks #2500

Merged
merged 2 commits into from
Apr 15, 2016

Conversation

dstufft
Copy link
Contributor

@dstufft dstufft commented Apr 15, 2016

Instead of only checking on the CSRF on POST requests, this will instead have the new, automatic CSRF checking occur on any HTTP method which is not defined as safe by RFC 2616.

# should never appear in an URL as doing so is a security issue. We also
# explicitly check for request.POST here as we do not support sending form
# encoded data over anything but a request.POST.
if request.method == "POST":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

request.POST can be used for any request that is uploaded using a form-encoding (multipart or urlencoded). A better check here would probably be if token in request.POST which will fail on any request where request.POST cannot be parsed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is perfectly acceptable for a PUT request to have a body that is form encoded, thus a PUT request could have the CSRF token in request.POST.

This would be a b/w incompatible change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally did this because I was told that WebOb will copy the body on PUT requests if we unconditionally accessed request.POST and that was one of the reasons that this couldn't be done. However looking at the code now in WebOb it appears it will only do that if the Content-Type matches, so I think that is probably reasonable then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had forgotten that was changed in WebOb, it used to do it unconditionally, but it was fixed because it would replace it with a FakeCGIBody, even on non form data.

@dstufft dstufft force-pushed the improve-csrf branch 2 times, most recently from bad5333 to 9028dd1 Compare April 15, 2016 22:20
@digitalresistor digitalresistor added this to the 1.7 milestone Apr 15, 2016
@digitalresistor
Copy link
Member

👍

dstufft added 2 commits April 15, 2016 18:31
Previously `check_csrf_token` would allow passing in a CSRF token in through a
the URL of a request. However this is a security issue because a CSRF token
must not be allowed to leak, and URLs regularly get copy/pasted or otherwise
end up leaking to the outside world.
Instead of only protecting against unsafe POST requests, have the automatic
CSRF protect on all methods which are not defined as "safe" by RFC2616.
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

Successfully merging this pull request may close these issues.

3 participants