Opened 11 years ago

Closed 17 months 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)

crsf-origin-check.diff (7.4 KB) - added by davidben 11 years ago.
Updated patch

Download all attachments as: .zip

Change History (16)

comment:1 Changed 11 years ago by Luke Plant

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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.

Changed 11 years ago by davidben

Attachment: crsf-origin-check.diff added

Updated patch

comment:2 Changed 11 years ago by davidben

Cc: davidben 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.)

Last edited 20 months ago by Tim Graham (previous) (diff)

comment:3 Changed 11 years ago by Luke Plant

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.

Last edited 11 years ago by Luke Plant (previous) (diff)

comment:4 Changed 11 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:5 Changed 8 years ago by Collin Anderson

Cc: cmawebsite@… added

comment:8 Changed 23 months ago 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?

comment:9 Changed 20 months ago by Tim Graham

Owner: changed from nobody to Tim Graham
Status: newassigned
Version: 1.3master

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 Changed 20 months ago by Tim Graham

Cc: Adam Johnson 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?

Last edited 19 months ago by Tim Graham (previous) (diff)

comment:11 Changed 19 months ago 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 in version 3.0.0.

comment:12 Changed 19 months ago 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.

comment:13 Changed 19 months ago by Tim Graham

I posted those questions and more to django-developers.

comment:14 Changed 19 months ago by Tim Graham

Patch needs improvement: unset

PR is ready for some review.

comment:15 Changed 17 months ago by Tim Graham

Triage Stage: AcceptedReady for checkin

Reviewed by Florian Apolloner, Chris Jerdonek, and Adam Johnson.

comment:16 Changed 17 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In dba44a7a:

Refs #16010 -- Required CSRF_TRUSTED_ORIGINS setting to include the scheme.

comment:17 Changed 17 months ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In 2411b8b5:

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.

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