Opened 4 years ago

Last modified 4 weeks ago

#16010 new New feature

Support Origin header checking in the CSRF middleware

Reported by: davidben Owned by: nobody
Component: CSRF Version: 1.3
Severity: Normal Keywords:
Cc: davidben, cmawebsite@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
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 4 years ago.
Updated patch

Download all attachments as: .zip

Change History (6)

comment:1 Changed 4 years ago by lukeplant

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

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 4 years ago by davidben

Updated patch

comment:2 Changed 4 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.)

(Also I apparently still cannot consistently spell csrf right... I blame 'crlf'.)

comment:3 Changed 4 years ago 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 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 4 years ago by lukeplant (previous) (diff)

comment:4 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:5 Changed 4 weeks ago by collinanderson

  • Cc cmawebsite@… added
Note: See TracTickets for help on using tickets.
Back to Top