[Django] #16010: Support Origin header checking in the CSRF middleware
Django
-------------------------+------------------------------
Reporter: davidben | Owner: nobody
Type: New feature | Status: new
Milestone: | Component: contrib.csrf
Version: 1.3 | Severity: Normal
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Easy pickings: 0
-------------------------+------------------------------
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.
--
Ticket URL: <http://code.djangoproject.com/ticket/16010>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Type: New feature | Status: new
Milestone: | Component: contrib.csrf
Version: 1.3 | Severity: Normal
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 1 | Easy pickings: 0
---------------------------------------+------------------------------
Changes (by lukeplant):
* needs_better_patch: => 1
* stage: Unreviewed => Accepted
* needs_tests: => 0
* needs_docs: => 0
Comment:
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.
--
Ticket URL: <http://code.djangoproject.com/ticket/16010#comment:1>
Django
Type: New feature | Status: new
Milestone: | Component: contrib.csrf
Version: 1.3 | Severity: Normal
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 1 | Easy pickings: 0
---------------------------------------+------------------------------
* cc: davidben (added)
Comment:
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.)
(Also I apparently still cannot consistently spell csrf right... I blame
'crlf'.)
--
Ticket URL: <http://code.djangoproject.com/ticket/16010#comment:2>
Django
Type: New feature | Status: new
Milestone: | Component: contrib.csrf
Version: 1.3 | Severity: Normal
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 1 | Easy pickings: 0
---------------------------------------+------------------------------
Comment (by lukeplant):
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. 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.
--
Ticket URL: <http://code.djangoproject.com/ticket/16010#comment:3>
Django
Reporter: davidben | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
-----------------------------+------------------------------------
Changes (by collinanderson):
* cc: cmawebsite@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/16010#comment:5>
Django
Reporter: davidben | Owner: auvipy
Type: New feature | Status: assigned
Component: CSRF | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-----------------------------+------------------------------------
* status: new => assigned
* owner: nobody => auvipy
* version: 1.3 => master
--
Ticket URL: <https://code.djangoproject.com/ticket/16010#comment:6>
Django
Reporter: davidben | Owner:
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-----------------------------+------------------------------------
Changes (by auvipy):
* owner: auvipy =>
* status: assigned => new
Comment:
New/Core contributors should look after this. I'm working on other parts
of django, so better let others work on this whoever gave the ability and
interest to work with security
--
Ticket URL: <https://code.djangoproject.com/ticket/16010#comment:7>
Django
Reporter: davidben | Owner: (none)
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-----------------------------+------------------------------------
Comment (by Matt Johnson):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/16010#comment:8>
Django
Reporter: davidben | Owner: Tim Graham
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* owner: nobody => Tim Graham
* status: new => assigned
* version: 1.3 => master
Comment:
I created a [https://github.com/django/django/pull/13829 PR] that brings
the patch from 10 years ago current. I still need to investigate Luke's
comment about `CSRF_COOKIE_DOMAIN`.
--
Ticket URL: <https://code.djangoproject.com/ticket/16010#comment:9>
Django
Reporter: davidben | Owner: Tim Graham
Type: New feature | Status: assigned
Component: CSRF | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-----------------------------+--------------------------------------
Changes (by Tim Graham):
* cc: Adam Johnson (added)
Comment:
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
[https://pypi.org/project/django-cors-headers/ djanog-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?
--
Ticket URL: <https://code.djangoproject.com/ticket/16010#comment:10>
Django
Reporter: davidben | Owner: Tim Graham
Type: New feature | Status: assigned
Component: CSRF | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-----------------------------+--------------------------------------
Comment (by Adam Johnson):
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 [https://github.com/adamchainz/django-cors-
headers/blob/master/HISTORY.rst#300-2019-05-10 in version 3.0.0].
--
Ticket URL: <https://code.djangoproject.com/ticket/16010#comment:11>
Django
Reporter: davidben | Owner: Tim Graham
Type: New feature | Status: assigned
Component: CSRF | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-----------------------------+--------------------------------------
Comment (by Tim Graham):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16010#comment:12>
Django
Reporter: davidben | Owner: Tim Graham
Type: New feature | Status: assigned
Component: CSRF | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-----------------------------+--------------------------------------
Comment (by Tim Graham):
I posted those questions and more to [https://groups.google.com/g/django-
developers/c/W_RiCsguaSU/ django-developers].
--
Ticket URL: <https://code.djangoproject.com/ticket/16010#comment:13>
Django
Reporter: davidben | Owner: Tim Graham
Type: New feature | Status: assigned
Component: CSRF | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+--------------------------------------
* needs_better_patch: 1 => 0
Comment:
[https://github.com/django/django/pull/13829 PR] is ready for some review.
--
Ticket URL: <https://code.djangoproject.com/ticket/16010#comment:14>
Django
Reporter: davidben | Owner: Tim Graham
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Accepted => Ready for checkin
Comment:
Reviewed by Florian Apolloner, Chris Jerdonek, and Adam Johnson.
--
Ticket URL: <https://code.djangoproject.com/ticket/16010#comment:15>
Django
Reporter: davidben | Owner: Tim Graham
Type: New feature | Status: assigned
Component: CSRF | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+---------------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"dba44a7a7a3581ec722e06fa0f9f33dfc00ed5cd" dba44a7a]:
{{{
#!CommitTicketReference repository=""
revision="dba44a7a7a3581ec722e06fa0f9f33dfc00ed5cd"
Refs #16010 -- Required CSRF_TRUSTED_ORIGINS setting to include the
scheme.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/16010#comment:16>
Django
Reporter: davidben | Owner: Tim Graham
Component: CSRF | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+---------------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"2411b8b5eb65fe3d7bcc1ee1f59e2433520c7df6" 2411b8b5]:
{{{
#!CommitTicketReference repository=""
revision="2411b8b5eb65fe3d7bcc1ee1f59e2433520c7df6"
Fixed #16010 -- Added Origin header checking to CSRF middleware.
Thanks David Benjamin for the original patch, and Florian
Apolloner, Chris Jerdonek, and Adam Johnson for reviews.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/16010#comment:17>