Support Origin header checking in the CSRF middleware

[davidben]

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.

[Luke Plant]

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.

[davidben]

Updated patch

[davidben]

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.)

[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.

[Aymeric Augustin]

Change UI/UX from NULL to False.

[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?

[Tim Graham]

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.

[Tim Graham]

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 ​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?

[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.

[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.

[Tim Graham]

I posted those questions and more to ​django-developers.

[Tim Graham]

​PR is ready for some review.

[Tim Graham]

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

[Mariusz Felisiak <felisiak.mariusz@…>]

In dba44a7a:

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

[Mariusz Felisiak <felisiak.mariusz@…>]

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.