Opened 13 years ago
Closed 4 years ago
#16010 closed New feature (fixed)
Support Origin header checking in the CSRF middleware
Reported by: | davidben | Owned by: | Tim Graham |
---|---|---|---|
Component: | CSRF | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | davidben, cmawebsite@…, Adam Johnson | 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
The Origin header has been implemented now in WebKit-based browsers (see http://www.browserscope.org/), and Mozilla also has a ticket for it (https://bugzilla.mozilla.org/show_bug.cgi?id=446344). This patch implements the check when the browser supports it without relying on it for CSRF checking. It should provide better protection against cross-subdomain CSRF attacks when it can be used.
Attachments (1)
Change History (16)
comment:1 by , 13 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 13 years ago
Cc: | added |
---|
Hrm, I don't know if using same_origin in both places would work. We'd need to special-case 'null', so it wouldn't be any cleaner. I guess a string comparison isn't quite right if there is disagreement over what is the "default port" for the protocol, but same_origin doesn't take care of that either. I imagine people aren't going to grow more interesting ones than http/80 and https/443; that bit is only there because changing the web to explicitly serialize a default port will break things.
The refactoring also means that request.get_host() needs to work in all the unit tests. I put something in, but I'm not sure if that's the cleanest way to do it. (I kept the HTTP_HOST lines in the relevant tests in to be explicit that they care about the value.)
comment:3 by , 13 years ago
I'm hesitating on this, because:
1) I think we need to worry about CSRF_COOKIE_DOMAIN - this can be set to allow cross-subdomain POSTs, but with this addition that would break.
2) Which brings us to the strict referer checking to HTTPS, which doesn't allow for CSRF_COOKIE_DOMAIN either, but probably should - and probably hasn't come up so far because fewer people are using HTTPS. But I'm hesitating on that too, because of possible unforeseen security implications in the context of HTTPS.
So, I think to start with we should make the Origin checking allow wild-card subdomains if CSRF_COOKIE_DOMAIN does, and docs for that setting should be updated accordingly. Sorry for not mentioning that earlier, it didn't occur to me. For now, we'll leave the referer checking as it is, and as it is documented i.e. a simple strict referer check for HTTPS.
comment:5 by , 10 years ago
Cc: | added |
---|
comment:8 by , 4 years ago
More and more of my users are encountering problems with Django's CSRF protection because it requires an HTTP referer. It's still a tiny number, but it went from 0 in the last 10 years, to 3 in the last 6 months.
The referrer/origin checking seems unnecessary if CSRF_COOKIE_SECURE is enabled.
In any case, is there a problem with essentially replacing this line:
https://github.com/django/django/blob/master/django/middleware/csrf.py#L240
with this:
referer = request.META.get('HTTP_ORIGIN', request.META.get('HTTP_REFERER'))
and updating the error messages, tests, and documentation appropriately?
comment:9 by , 4 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Version: | 1.3 → master |
I created a PR that brings the patch from 10 years ago current. I still need to investigate Luke's comment about CSRF_COOKIE_DOMAIN
.
comment:10 by , 4 years ago
Cc: | added |
---|
While trying to make origin checking reuse the lists of hosts that referer checking uses, I noted that the values in the CSRF_COOKIE_DOMAIN
and CSRF_TRUSTED_ORIGINS
settings don't include the URL scheme (which the HTTP_ORIGIN header includes), and I'm not sure it's appropriate to discard the HTTP_ORIGIN
header's scheme in the comparison.
I'm not sure if we need new settings but I see that django-cors-headers has some:
CORS_ALLOWED_ORIGINS = [ "https://example.com", "https://sub.example.com", "http://localhost:8080", "http://127.0.0.1:9000" ]
as well as:
CORS_ALLOWED_ORIGIN_REGEXES = [ r"^https://\w+\.example\.com$", ]
and:
CORS_ALLOW_ALL_ORIGINS = (True or False)
Adam, maybe you have a suggestion here?
comment:11 by , 4 years ago
Ah yes - the scheme, and port (if non-default), are part of the Origin. Discarding them is not appropriate due to the security concern of leaking HTTPS data to an HTTP origin. I moved django-cors-headers to require schemes in version 3.0.0.
comment:12 by , 4 years ago
So we could add another setting CSRF_ALLOWED_ORIGINS
(which I sketched out as another commit in my draft PR) that includes the schemes. Unfortunately the name is very similar and not well differentiated from CSRF_TRUSTED_ORIGINS
setting that already exists. That setting could possibly be deprecated in favor of CSRF_ALLOWED_ORIGINS
and another new setting: CSRF_ALLOWED_ORIGIN_REGEXES
, to accommodate the "allow all subdomains" use case.
Do you think an CSRF_ALLOW_ALL_ORIGINS
setting is needed?
There's also a question of backward compatibility. Since CSRF_ALLOWED_ORIGINS
is empty by default, only same-origin requests will be allowed unless the new settings are set. I can't think of a useful deprecation path here, but perhaps a system check to flag an empty CSRF_ALLOWED_ORIGINS
if CSRF_TRUSTED_ORIGINS
isn't empty could be helpful in giving a heads up.
comment:15 by , 4 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Reviewed by Florian Apolloner, Chris Jerdonek, and Adam Johnson.
Thanks, good suggestion.
With this patch, we've now got some common code between the strict referer checking and the origin checking. Your 'good_origin' line should be pulled out, and used by both branches.
Both branches should also probably use the 'same_origin' utility. This is required for the referer checking, since 1) the referer includes more than the origin, and 2) a simple startswith check is not good enough, due to the possibility of something like
http://www.target.com.attacker.com
. With the same origin checking, using 'same_origin' is probably overkill, but it would keep things consistent.