Opened 5 years ago

Closed 5 years ago

#24496 closed New feature (fixed)

Check CSRF Referer against CSRF_COOKIE_DOMAIN

Reported by: Matt Robenolt Owned by: Tim Graham <timograham@…>
Component: CSRF Version: master
Severity: Normal Keywords: csrf 1.9
Cc: django@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Carl Meyer)

Right now, if you try to share a CSRF token across a subdomain without https, everything works great since the Referer header isn't validated.

But over https, we want to be a bit more strict and make sure that the Referer is from another secure site, and also that the Referer matches where we think it should be coming from. The canonical source for where we think it should be from is CSRF_COOKIE_DOMAIN.

If we set our CSRF_COOKIE_DOMAIN to .example.com, that means our CSRF validation should work for www.example.com and foo.example.com. Not just strictly the domain the request is coming from.

Attachments (3)

24496.diff (4.6 KB) - added by Joshua Kehn 5 years ago.
Cookie Domain Flow.png (34.3 KB) - added by Joshua Kehn 5 years ago.
New CSRF_COOKIE_DOMAIN flow
Existing Flow (with patch).png (31.6 KB) - added by Joshua Kehn 5 years ago.
Existing (with patch) flow

Download all attachments as: .zip

Change History (33)

comment:1 Changed 5 years ago by Tim Graham

Could you provide a reference for "we want to be a bit more strict ..."? For example, is this part of an RFC or something that other frameworks implement?

comment:2 Changed 5 years ago by Matt Robenolt

I think the comment in this file explains it best:

# Suppose user visits http://example.com/
# An active network attacker (man-in-the-middle, MITM) sends a
# POST form that targets https://example.com/detonate-bomb/ and
# submits it via JavaScript.
#
# The attacker will need to provide a CSRF cookie and token, but
# that's no problem for a MITM and the session-independent
# nonce we're using. So the MITM can circumvent the CSRF
# protection. This is true for any HTTP connection, but anyone
# using HTTPS expects better! For this reason, for
# https://example.com/ we need additional protection that treats
# http://example.com/ as completely untrusted. Under HTTPS,
# Barth et al. found that the Referer header is missing for
# same-domain requests in only about 0.2% of cases or less, so
# we can use strict Referer checking.

https://github.com/django/django/blob/668d53cd125175eb708cc0af143f47b42cd42153/django/middleware/csrf.py#L135-L149

comment:3 Changed 5 years ago by Carl Meyer

Tim, the "we want to be more strict" is something Django already implements (checking REFERER against the request host on an HTTPS request). Matt's feature request is for that check to be against CSRF_COOKIE_DOMAIN rather than the request host, to allow for cross-subdomain requests. I think this is reasonable, and probably the way it should have worked from the beginning.

comment:4 Changed 5 years ago by Matt Robenolt

I've opened a discussion on the mailing list as suggested by timgraham since this is related to security: https://groups.google.com/forum/#!topic/django-developers/tEEw02RhV0M

comment:5 Changed 5 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

comment:6 Changed 5 years ago by Chris Streeter

Cc: django@… added

comment:7 Changed 5 years ago by Aymeric Augustin

Looks reasonable to me.

comment:8 Changed 5 years ago by Tim Graham

Patch needs improvement: set

Marking as "patch needs improvement" per Luke's comments.

comment:9 Changed 5 years ago by Tim Graham

Patch needs improvement: unset

comment:10 Changed 5 years ago by Tim Graham

Patch needs improvement: set

comment:11 Changed 5 years ago by Tim Graham

Patch needs improvement: unset

comment:12 Changed 5 years ago by Tim Graham

Needs documentation: set

comment:13 Changed 5 years ago by Seth Gottlieb

Submitted a pull request to mattrobenolt that added some documentation for his fix.

If we decide to go with Troy Grosfield's suggestion of adding a CSRF_WHITELIST_ORIGINS setting (which I like), I can document that instead.

comment:14 Changed 5 years ago by Joshua Kehn

Description: modified (diff)
Needs documentation: unset
Owner: changed from nobody to Joshua Kehn
Status: newassigned
Summary: Check CSRF Referer against CSRF_COOKIE_DOMAINCheck CSRF Referer against CSRF_TRUSTED_ORIGINS

Based on the mailing list discussion here https://groups.google.com/forum/#!topic/django-developers/eQeaNzSlSbw I'm updating the ticket description and including a PR with tests and updated documentation that allows expanding the allowed HTTP Referer domains via a setting called CSRF_TRUSTED_ORIGINS.

PR is open: https://github.com/django/django/pull/5218

A patch has been added to this ticket as well.

Last edited 5 years ago by Joshua Kehn (previous) (diff)

comment:15 Changed 5 years ago by Carl Meyer

The new patch looks good, thanks Josh! After giving it some more thought overnight, I think we should actually combine the two proposed patches for this ticket. CSRF_TRUSTED_ORIGINS is broader in scope for true cross-origin requests from different domains, but using CSRF_COOKIE_DOMAIN as the default (if set) is just sensible, and easier if you have quite a few subdomains that should all be trusted (since we don't have any sort of wildcarding in CSRF_TRUSTED_ORIGINS).

comment:16 Changed 5 years ago by Joshua Kehn

Updated PR based on comments, I'll update the attached patch as well after this comment.

Regarding including the CSRF_COOKIE_DOMAIN, wouldn't that be required anyways for the request to pass ALLOWED_HOSTS? I'm thinking that should be checked after the CSRF_TRUSTED_ORIGINS check fails, correct? I'm attaching two flows.

Changed 5 years ago by Joshua Kehn

Attachment: 24496.diff added

comment:17 Changed 5 years ago by Joshua Kehn

Existing flow (with proposed patch): http://l.kehn.io/image/0z2X3j2k082c

CSRF_COOKIE_DOMAIN flow: http://l.kehn.io/image/3p3J142b3j3H

Changed 5 years ago by Joshua Kehn

Attachment: Cookie Domain Flow.png added

New CSRF_COOKIE_DOMAIN flow

Changed 5 years ago by Joshua Kehn

Existing (with patch) flow

comment:18 Changed 5 years ago by Carl Meyer

I think the logic should look more like "construct a list of valid referers, where that list consists of X plus CSRF_TRUSTED_ORIGINS, where X is CSRF_COOKIE_DOMAIN if set and otherwise request.get_host()".

In other words, it's basically the same logic as in your current PR, except that if CSRF_COOKIE_DOMAIN is set, it replaces request.get_host() entirely. But you'll also need the other changes in Matt's pull request in order to get the correct host matching (where initial dot allows subdomains).

I would probably start with Matt's PR and layer the addition of CSRF_TRUSTED_ORIGINS on top of that.

We could even merge them as separate fixes? Maybe I was wrong to suggest doing them both in this ticket.

comment:19 Changed 5 years ago by Joshua Kehn

I think the domain check complicates matters enough to warrant a separate fix. I've essentially repurposed this ticket to accommodate the CSRF_TRUSTED_ORIGINS additions so I'd suggest creating a new ticket.

I don't know about the get_host() vs. CSRF_COOKIE_DOMAIN check. Let's say CSRF_COOKIE_DOMAIN is set to .example.com and ALLOWED_HOSTS is set to ['api.example.com', 'api-dev.example.com']. To allow cross origin requests also set CSRF_TRUSTED_ORIGINS to ['dashboard.example.com'].

At this point Django would allow a request referred from badhost.example.com, since it's part of the CSRF_TRUSTED_ORIGINS + CSRF_COOKIE_DOMAIN. Is that acceptable?

comment:20 in reply to:  19 Changed 5 years ago by Carl Meyer

Replying to joshkehn:

I think the domain check complicates matters enough to warrant a separate fix. I've essentially repurposed this ticket to accommodate the CSRF_TRUSTED_ORIGINS additions so I'd suggest creating a new ticket.

I think instead we should revert the changes to ticket title and description here, and open a new ticket for CSRF_TRUSTED_ORIGINS, since this ticket was originally about CSRF_COOKIE_DOMAIN and that's its history. Sorry I led you down the wrong path yesterday.

I don't know about the get_host() vs. CSRF_COOKIE_DOMAIN check. Let's say CSRF_COOKIE_DOMAIN is set to .example.com and ALLOWED_HOSTS is set to ['api.example.com', 'api-dev.example.com']. To allow cross origin requests also set CSRF_TRUSTED_ORIGINS to ['dashboard.example.com'].

At this point Django would allow a request referred from badhost.example.com, since it's part of the CSRF_TRUSTED_ORIGINS + CSRF_COOKIE_DOMAIN. Is that acceptable?

Yes, that's fine. ALLOWED_HOSTS is not relevant here. If CSRF_COOKIE_DOMAIN is set to .example.com, that's a declaration that all subdomains of .example.com are trusted to submit unsafe requests (well, they still have to pass the CSRF token check, but they're trusted enough to pass the referrer check).

comment:21 Changed 5 years ago by Joshua Kehn

Right, my question was with CSRF_TRUSTED_ORIGINS explicitly set wouldn't someone reasonably assume that it's a filter on top of CSRF_COOKIE_DOMAIN?

Example: Set CSRF_COOKIE_DOMAIN to .example.com for convenience of configuration so that the cookie is shared among any subdomain. Set CSRF_TRUSTED_ORIGINS to only allow certain referers to pass the CSRF checks.

My suggestion would be to only use CSRF_COOKIE_DOMAIN if CSRF_TRUSTED_ORIGINS is empty and make that a separate branch of logic.

Carl, I'm not sure if there's an easy way to split out into a new ticket. Do you have one? If not, I can manually do it a bit later today.

comment:22 in reply to:  21 Changed 5 years ago by Carl Meyer

Replying to joshkehn:

Right, my question was with CSRF_TRUSTED_ORIGINS explicitly set wouldn't someone reasonably assume that it's a filter on top of CSRF_COOKIE_DOMAIN?

Example: Set CSRF_COOKIE_DOMAIN to .example.com for convenience of configuration so that the cookie is shared among any subdomain. Set CSRF_TRUSTED_ORIGINS to only allow certain referers to pass the CSRF checks.

My suggestion would be to only use CSRF_COOKIE_DOMAIN if CSRF_TRUSTED_ORIGINS is empty and make that a separate branch of logic.

It's a reasonable point, but I don't think so. Any host you are willing to share the CSRF cookie with is implicitly trusted to make use of it. Sharing the cookie is actually already a much deeper level of trust than that provided by CSRF_TRUSTED_ORIGINS alone, because it allows that host to get past both parts of the CSRF check, referrer and token. CSRF_TRUSTED_ORIGINS is a technique for adding _additional_ hosts who can bypass the referrer check, it's not a mechanism for restriction.

With your proposed behavior, it would be impossible to both have many wildcarded subdomains AND an external CORS-using host. That would be pretty frustrating.

Certainly the interaction of the two settings should be documented clearly.

Carl, I'm not sure if there's an easy way to split out into a new ticket. Do you have one? If not, I can manually do it a bit later today.

No, there's no secret technique for that. Shouldn't be too hard; I wouldn't try to copy all the comments or anything. Just open a new ticket and say "some previous discussion in #24496", and re-title your pull request so it gets linked up with the new ticket.

comment:23 Changed 5 years ago by Carl Meyer

Description: modified (diff)
Needs documentation: set
Summary: Check CSRF Referer against CSRF_TRUSTED_ORIGINSCheck CSRF Referer against CSRF_COOKIE_DOMAIN

Resetting the title/summary of this ticket to again be specific to the CSRF_COOKIE_DOMAIN fix. Also resetting the "needs docs" flag, since I think that's all that the original PR for that (https://github.com/django/django/pull/4337) is waiting on.

Sorry again for causing confusion and extra work by mixing these two slightly different issues together in one ticket.

comment:24 Changed 5 years ago by Joshua Kehn

Owner: Joshua Kehn deleted
Status: assignednew

Done, thanks Carl. New ticket #25334, PR and commit updated.

Last edited 5 years ago by Joshua Kehn (previous) (diff)

comment:25 Changed 5 years ago by Tim Graham

Needs documentation: unset
Triage Stage: AcceptedReady for checkin

Pending some release notes and a versionchanged annotation.

comment:26 Changed 5 years ago by Tim Graham

Needs documentation: set
Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

Patch now doesn't merge cleanly either.

comment:27 Changed 5 years ago by Tim Graham

Keywords: 1.9 added

comment:28 Changed 5 years ago by Tim Graham

Needs documentation: unset
Patch needs improvement: unset

comment:29 Changed 5 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:30 Changed 5 years ago by Tim Graham <timograham@…>

Owner: set to Tim Graham <timograham@…>
Resolution: fixed
Status: newclosed

In b0c56b89:

Fixed #24496 -- Added CSRF Referer checking against CSRF_COOKIE_DOMAIN.

Thanks Seth Gottlieb for help with the documentation and
Carl Meyer and Joshua Kehn for reviews.

Note: See TracTickets for help on using tickets.
Back to Top