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

Fix web UI when using session-based CSRF #6207

Merged
merged 7 commits into from Feb 19, 2019

Conversation

Projects
None yet
6 participants
@seawolf42
Copy link
Contributor

seawolf42 commented Sep 23, 2018

Note: Before submitting this pull request, please review our contributing guidelines.

Description

Closes #6206

Pass the actual CSRF token value instead of the name of the cookie in which to find the value, and use this actual value in the X-CSRFToken header.

@seawolf42

This comment has been minimized.

Copy link
Contributor Author

seawolf42 commented Sep 23, 2018

I believe there will need to be similar changes to the coreapi source, but I am not sure how to adapt it to behave similarly.

@seawolf42 seawolf42 changed the title include raw token in template, pass token value without getCookie() call Fix web UI when using session-based CSRF Sep 23, 2018

@encode encode deleted a comment from codecov-io Oct 2, 2018

@tomchristie

This comment has been minimized.

Copy link
Member

tomchristie commented Oct 2, 2018

On first sight I'd assume this would resolve the issue when using CSRF_USE_SESSIONS=True but break it for the default case?

@seawolf42

This comment has been minimized.

Copy link
Contributor Author

seawolf42 commented Oct 2, 2018

It shouldn't break it for the default case, as the same token value that was being sent by default is still being sent.

In the case of CSRF_USE_SESSIONS=True: The csrf cookie is not present, so instead of depending on it to get the value to inject into the response, the template includes the actual value to use, and the JS code reads it directly.

In the case of CSRF_USE_SESSIONS=False (the default): The csrf cookie is present, but instead of depending on it to get the value to inject into the response, the template includes the actual value to use, and the JS code reads it directly.

In both cases, the response blob still contains the CSRF token the same way it did before; it's just being read from a raw value instead of being read from a cookie which may or may not be present.

@seawolf42

This comment has been minimized.

Copy link
Contributor Author

seawolf42 commented Oct 7, 2018

@tomchristie are you referring to the {% if request %} condition in the base.html file? I'm not familiar enough with the architecture to know if/when that would be false, so I could use an assist there.

@carltongibson carltongibson self-requested a review Oct 17, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 19, 2018

Codecov Report

Merging #6207 into master will decrease coverage by 0.32%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6207      +/-   ##
==========================================
- Coverage   96.18%   95.85%   -0.33%     
==========================================
  Files         128      129       +1     
  Lines       17645    17862     +217     
  Branches     1460     1476      +16     
==========================================
+ Hits        16972    17122     +150     
- Misses        465      538      +73     
+ Partials      208      202       -6
Impacted Files Coverage Δ
tests/test_templates.py 100% <100%> (ø) ⬆️
rest_framework/renderers.py 76.86% <0%> (-7.99%) ⬇️
tests/test_schemas.py 94.03% <0%> (-0.16%) ⬇️
tests/test_templatetags.py 100% <0%> (ø) ⬆️
tests/test_viewsets.py 100% <0%> (ø) ⬆️
tests/test_utils.py 100% <0%> (ø) ⬆️
rest_framework/decorators.py 98.29% <0%> (ø) ⬆️
...st_framework/management/commands/generateschema.py 0% <0%> (ø)
rest_framework/fields.py 95.18% <0%> (ø) ⬆️
tests/test_fields.py 98.41% <0%> (+0.01%) ⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d976ac5...c71e472. Read the comment docs.

@seawolf42

This comment has been minimized.

Copy link
Contributor Author

seawolf42 commented Oct 19, 2018

@carltongibson I updated a unit test as we discussed yesterday to show that at least the base template includes the csrfToken data. I would also like to add a test for the positive case, something like:

def test_base_template_with_context(mocker):
    csrf_token_mock = mocker.patch('django.template.defaulttags.csrf_token', return_value='<TOKEN>')
    user = User.objects.create(username='abc')
    Token.objects.create(user=user)
    factory = RequestFactory()
    request = factory.get('/')
    request.user = user
    result = render(request, 'rest_framework/base.html')
    csrf_token_mock.assert_called_once()
    assert re.search(r'\bcsrfToken: "<TOKEN>"', result.content)

... but the project doesn't include pytest-mock nor does it include unittest.mock. Do you have any reason why I couldn't add either of these to the project, or is there a better way to test this?

@carltongibson

This comment has been minimized.

Copy link
Collaborator

carltongibson commented Oct 22, 2018

Hi @seawolf42.

Similar comes up on Django re CSRF_COOKIE_HTTPONLY
https://code.djangoproject.com/ticket/29879. (You can't read the cookie value there either.)

unittest.mock is part of the standard library. You can use it. 👍

@seawolf42

This comment has been minimized.

Copy link
Contributor Author

seawolf42 commented Oct 23, 2018

@carltongibson but mock is not part of the standard library for python < 3... 😄

I'll take a look at 29879.

seawolf42 added some commits Oct 23, 2018

@seawolf42

This comment has been minimized.

Copy link
Contributor Author

seawolf42 commented Oct 23, 2018

@carltongibson disregard, I figured out a test I could do without needing to mock anything.

@carltongibson

This comment has been minimized.

Copy link
Collaborator

carltongibson commented Oct 23, 2018

but mock is not part of the standard library for python < 3

I'd be happy for you to skip the test on PY2. (We're dropping it for 3.10 anyhow... c.f. #6230)

@seawolf42

This comment has been minimized.

Copy link
Contributor Author

seawolf42 commented Oct 23, 2018

@carltongibson I ended up writing a test that didn't require any mocks in the first place, so that has gone away. The only open question now is whether I need to also make changes to the core API itself as part of this PR or whether the note in the docs is enough.

@carltongibson

This comment has been minimized.

Copy link
Collaborator

carltongibson commented Oct 23, 2018

OK. Thanks! I will have a look tomorrow. Good effort!

@carltongibson carltongibson added this to the 3.9.1 Release milestone Oct 24, 2018

@carltongibson
Copy link
Collaborator

carltongibson left a comment

OK. A few comments, but I think the general approach is reasonable.

Show resolved Hide resolved docs/topics/api-clients.md Outdated
@@ -286,7 +286,7 @@ <h1>{{ name }}</h1>
<script>
window.drf = {
csrfHeaderName: "{{ csrf_header_name|default:'X-CSRFToken' }}",
csrfCookieName: "{{ csrf_cookie_name|default:'csrftoken' }}"
csrfToken: "{% if request %}{{ csrf_token }}{% endif %}"

This comment has been minimized.

@carltongibson

carltongibson Oct 24, 2018

Collaborator

Do you mean {% csrf_token %}? c.f. #3703 &co.

This comment has been minimized.

@seawolf42

seawolf42 Oct 24, 2018

Author Contributor

How very odd... I am sure I used it this way when I was first figuring things out, clearly I was mistaken. OK, I'll have to re-work one test too, will take care of that in the next day or two.

This comment has been minimized.

@seawolf42

seawolf42 Oct 24, 2018

Author Contributor

Oh, no, I do want {{ csrf_token }}. I don't want the entire HTML tag, just the value of the token.

This comment has been minimized.

@carltongibson

carltongibson Oct 24, 2018

Collaborator

Good, and TemplateHTMLRenderer will create a RequestContext, so csrf_token will be in the context. Fine. 👍

@@ -247,7 +247,7 @@ <h4 class="modal-title" id="myModalLabel">{{ error_title }}</h4>
<script>
window.drf = {
csrfHeaderName: "{{ csrf_header_name|default:'X-CSRFToken' }}",
csrfCookieName: "{{ csrf_cookie_name|default:'csrftoken' }}"
csrfToken: "{{ csrf_token }}"

This comment has been minimized.

@carltongibson

carltongibson Oct 24, 2018

Collaborator

Again {% csrf_token %}? And do we need the if request check?

This comment has been minimized.

@seawolf42

seawolf42 Oct 24, 2018

Author Contributor

We do need that check, because there is currently a unit test for rendering with no request and in that case the csrf_token call throws an exception (see https://code.djangoproject.com/ticket/29785, which you closed as "by design").

This comment has been minimized.

@seawolf42

seawolf42 Oct 24, 2018

Author Contributor

OK, read the comments out of order. Here we don't need the {% if request %} check because (presumably) there will always be a request when you load an admin page. In the base.html case, however, the check is only there because of the exception thrown if there is no request, this is to satisfy test_templates.test_base_template_with_no_context().

@carltongibson
Copy link
Collaborator

carltongibson left a comment

OK. I think this looks good to me.

(Just await some other 👀)

@seawolf42

This comment has been minimized.

Copy link
Contributor Author

seawolf42 commented Oct 24, 2018

Excellent, thanks for the help!

@carltongibson

This comment has been minimized.

Copy link
Collaborator

carltongibson commented Oct 24, 2018

@seawolf42 Thanks for your effort!

@auvipy

auvipy approved these changes Nov 13, 2018

@rpkilby rpkilby requested a review from tomchristie Nov 13, 2018

@rpkilby

This comment has been minimized.

Copy link
Member

rpkilby commented Nov 13, 2018

I'm not incredibly familiar with how Django's CSRF mechanism works since I've never really dug into it.

Given my ignorance on the subject, my concern would be that this PR fixes the problem, but possibly introduces some CSRF-related vulnerability. I assume there's a reason the token is passed in a cookie instead of it's value being embedded in the template?

@seawolf42

This comment has been minimized.

Copy link
Contributor Author

seawolf42 commented Nov 13, 2018

@rpkilby that's a reasonable concern. I'll explain as best I can that this is actually OK.

Background: http://kylebebak.github.io/post/csrf-protection - read the section on "Double-Submit Cookie", the mechanism Django uses.

The key takeaway is that there are two items that matter: the CSRF token as the server knows it and the CSRF token that the request contains. They are compared, and if they're the same, the request passes. The security of this hinges on the assumption that at least one of those values cannot be randomly changed by the user. Traditionally this is handled by having a cookie value that is set by the server that comes in on every request, and a token that the client sees in a Django form and sends back as a form field. There are alternate ways to retrieve the second value, but the cookie (if using cookies) must be trusted to have only been set either by the server or by JS sent from the server so that it can be trusted as an authoritative value.

DRF has been just looking at the cookie and sending that value as the second value rather than getting it to the client as part of page loads. If the cookie is trustworthy, and the JS can see the cookie, then using it doesn't break security (since the same-origin policy means if the JS can see it the JS came from the server, which can be trusted).

Enter session-based CSRF. Note that this was introduced specifically to increase security by solving situations where cookies assigned by a subdomain in a hosting environment may be readable by other applications in that same environment (so mysite.appspot.com might be hackable by yoursite.appspot.com if you can read the cookie I've set).

In this mechanism, the authoritative value is never sent to the client in any form; it's stored in the session instead. Now reading the value from the cookie doesn't work any longer because the cookie doesn't exist. The way to get the token to the server instead now hinges on it being in the page the server gave me, and I then read the value and send it back. The only cookie that requires protection is the session cookie, reducing the attack footprint because session cookies get extra browser protections.

Note again how traditional Django handles this: instead of reading the cookie value, the client receives the token on every form as a hidden field and the server receives that back as part of the form submission, then compares that value to the cookie that was set earlier. Switching to session-based CSRF doesn't break this because the authoritative value is in the session rather than the cookie but all that matters is the hidden field value is compared to that authoritative value to pass/fail the request.

In the DRF case, this PR changes the web UI to receive the CSRF token that same way... as part of the incoming page. No other aspect of the process is impacted, so the assumption here is that the Django forms way of doing things is the best practice, and we're just emulating it.

@seawolf42

This comment has been minimized.

Copy link
Contributor Author

seawolf42 commented Nov 13, 2018

As a side note, the switch to session-base CSRF essentially switches from "Double-Submit Cookie" to "Synchronizer Token" in the link above. This PR enables that switch with the web UI.

@rpkilby

This comment has been minimized.

Copy link
Member

rpkilby commented Nov 14, 2018

Hi @seawolf42 - thanks for the comprehensive writeup. I haven't had time to read through the linked articles in depth, but looking over your response, the implementation in the PR sounds 👍 to me.

DRF has been just looking at the cookie and sending that value as the second value rather than getting it to the client as part of page loads. If the cookie is trustworthy, and the JS can see the cookie, then using it doesn't break security (since the same-origin policy means if the JS can see it the JS came from the server, which can be trusted).

So my understanding here is that it doesn't matter whether the token is read from the response body (e.g., embedded in a form or in an inlined script) or read from the cookie, since the same origin policy prevents an external site from reading the contents. The token wouldn't unintentionally be exposed.

And it looks like the PR is compatible with both the cookie and session-based CSRF mechanisms.

Would it make sense to add some simple sanity tests for both settings? something like..

@override_settings(CSRF_USE_SESSIONS=False) 
def test_cookie_csrf():
    response = self.client.get(url)

    # CSRF token should be provided in page and as a cookie
    assert 'csrftoken' in response.client.cookies
    assert re.search(r'\bcsrfToken: ""', response.body.decode('utf-8'))
   

@override_settings(CSRF_USE_SESSIONS=True) 
def test_session_csrf():
    response = self.client.get(url)

    # CSRF cookie should not be set
    assert 'csrftoken' not in response.client.cookies

    # CSRF token should be provided in page and through session cookie
    assert 'session' in response.client.cookies
    assert re.search(r'\bcsrfToken: ""', response.body.decode('utf-8'))

These aren't full e2e tests, but should at least ensure that the necessary parts are provided in the response (right?).

@rpkilby

This comment has been minimized.

Copy link
Member

rpkilby commented Nov 14, 2018

I would still appreciate a review from @tomchristie or someone else with more experience in this area, but the PR looks good to me.

@carltongibson
Copy link
Collaborator

carltongibson left a comment

Hi @seawolf42. Right, 3.9.2 is it. 🙂

Thanks for the effort here, and welcome aboard! 🏆

@carltongibson carltongibson merged commit eb31801 into encode:master Feb 19, 2019

1 check passed

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

@seawolf42 seawolf42 deleted the seawolf42:fix/support-session-csrf branch Feb 19, 2019

@seawolf42

This comment has been minimized.

Copy link
Contributor Author

seawolf42 commented Feb 19, 2019

Awesome, thanks!

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.