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

bug 1456165: remove csrfmiddlewaretoken from wiki.document view responses #4757

Merged
merged 1 commit into from Apr 30, 2018

Conversation

Projects
None yet
2 participants
@escattone
Member

escattone commented Apr 27, 2018

This PR removes csrfmiddlewaretoken from the responses of all endpoints that require caching in the CloudFront CDN, which ends up being just the wiki.document endpoint. This provides a possible solution to bug 1456165, since it avoids setting a new csrftoken cookie on wiki.document page loads due to the fact that the CDN behavior can't forward the existing one (it can't forward the existing csrftoken cookie without negating caching altogether for that behavior, since CloudFront does not provide separate configuration for caching and forwarding).

@escattone escattone requested a review from jwhitlock Apr 27, 2018

@jwhitlock

This comment has been minimized.

Member

jwhitlock commented Apr 30, 2018

I'm trying to decide if we should always remove CSRF, or just for anonymous users. If we try to restrict this to anonymous users:

  1. Wiki doc behavior would change to cache on the CSRF token and session cookies
  2. Anon requests would get neither the CSRF or the session cookie by default.
  3. Logged-in users would get both a session and CSRF cookie.

This might work going forward. However, if an anon user that visited MDN in the last year shows up with a CSRF cookie, then the CDN would give them a per-user cache. We'd need to add a middleware that actively deletes CSRF cookies for anonymous users, and maybe add it to logout.

OK, now for the risks:

  • There's a known attack pattern where an attacker 1) logs out a user, 2) shows them a look-alike login form, 3) logs in the user with a man-in-the-middle attack, getting the user's account credentials. The folks at https://security.stackexchange.com/q/62769/177031 are mostly concerned about this. Our POST logout mitigates this attack - the POST would have to come from developer.mozilla.org, and include the session cookie. Further, they would have to deflect login from GitHub to somewhere else, and OAuth2 gives us a few protections here.
  • A user could force POSTs to logout users as they visit MDN. Again, this requires the POST to come from developer.mozilla.org, and I'm not sure to what end they would be doing that. Some people suggest a denial-of-service attack, but for MDN it would be a denial-of-editing attack. It seems they would use the ability to do in-page POSTs for other purposes.
  • A user could force POSTs to watch a page. Again, I'm not creative enough to see where that attack would be going. I do think we could present a login link to anon users and a watch form to logged in users, but it is probably OK.

OK, that's my preliminary thoughts. Now to actually try to code out, read it closely, etc.

@escattone

This comment has been minimized.

Member

escattone commented Apr 30, 2018

@jwhitlock That's a promising idea, restricting this approach to anonymous users. That would require:

  • adding csrftoken to the list of cookies that are forwarded and used for caching to the */docs/* CDN behavior
  • modifying the templates used for document pages such that the csrf_token is only called for logged-in users (not strictly necessary)
  • adding middleware that would modify the response such that the csrftoken cookie is always deleted for anonymous users (critical, since we're adding csrftoken to the list of cookies forwarded/cached-on, and this ensures that the CDN remains useful, that all anonymous users share the same cache)
@escattone

This comment has been minimized.

Member

escattone commented Apr 30, 2018

Just for the record, @jwhitlock and I discussed the idea (via a Vidyo meeting) of restricting this approach to anonymous users, and realized that we'd have to handle the special case of Admin login, which depends on an anonymous user getting a CSRF token.

@jwhitlock

This works locally for me, and a CSRF token is not added when I visit wiki pages.

I did get an unexpected CSRF token when logging in - it appears the django-allauth GitHub callback is adding it. More investigation is needed, but it should still be OK for our CDN interaction.

👍, I'd like to give it a try on staging. I think we can revisit CSRF and the admin login after Django 1.11 ships.

@@ -5,7 +5,6 @@
<p>{{ _('This article needs these reviews:') }}</p>
<form method="post" action="{{ url('wiki.quick_review', document.slug) }}">
{% csrf_token %}

This comment has been minimized.

@jwhitlock

jwhitlock Apr 30, 2018

Member

Good catch! I forgot about this one...

@jwhitlock jwhitlock merged commit 5560418 into mozilla:master Apr 30, 2018

1 check passed

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

@escattone escattone deleted the escattone:csrf-exemptions-1456165 branch May 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment