add check_origin option and support an origin of null

[mmerickel]

1. Change allow_no_origin to default to True.

2. Always allow Origin: null.

3. Support a list of origins in the Origin header, per the RFC, and use the last one for validation.

I think probably the most controversial part of this PR would be handling Origin: null. Are people okay the way it is or should I tie its behavior to allow_no_origin as well? I can see arguments both directions and would be fine with either solution. I am slightly leaning toward tying it to allow_no_origin even though that's not what the PR currently does.

See discussion in #3512 as well.

updated description

1. Allow csrf_trusted_origins setting to specify a value of null to allow the special Origin: null header.

2. Support a list of origins and use the last one for validation.

3. Add a check_origin option to set_default_csrf_options to turn off origin checks entirely - default is to keep origin checks on.

[stevepiercy]

Minor grammar fix. Otherwise LGTM.

[mmerickel]

@stevepiercy I force-pushed since you reviewed I think - or your comment is missing for some other reason.

[stevepiercy]

Review take 2!

[luhn]

I think this makes sense to disable by default.

Maybe kinda radical, but should we remove origin checking feature entirely? As discussed, CSRF tokens are very strong protection by default. Very few people will even know this is an option, will anybody enable it?

[mmerickel]

Maybe kinda radical, but should we remove origin checking feature entirely? As discussed, CSRF tokens are very strong protection by default. Very few people will even know this is an option, will anybody enable it?

I'd ask @dstufft what he thinks about that as he's the one who added the feature in the first place. Some options might be to turn it on only when pyramid.csrf_trusted_origins is set and / or support some special values like * or same-origin or probably better would be to add some check_origin=True flag to set_default_csrf_options. I personally do not value the feature very highly (I'm agree with @luhn that the token itself is a very strong solution, especially combined with the default-lax cookies we already added) so am open to some tweaks.

[digitalresistor]

Having had some time to chew on this today, I am a slight -0 on making this change, but ultimately don't think it matters much.

Browsers are sending the Origin header by default on POST requests, even if not required by the RFC, and ultimately I do think that is a good thing. But maybe the CSRF layer doing the check is the wrong place to do it? And anyone that wants the Origin header check to exist should do so in their own view deriver instead?

[mmerickel]

I need to do a little bit of experimentation to get a better sense of when the Origin header is not sent. I know it's not sent on a lot of cross-origin GET requests. It also seems like it's not sent on a lot of same-origin requests which was the main issue identified in #3512. I do want to hear what @dstufft thinks as well, as he added the origin checks in the first place.

[dstufft]

So, the reason why origin checking exists is because token based auth has a "flaw" depending exactly on how it's implemented. Basically anything cookie based is going to treat http://example.com/ and https://example.com/ as the same origin-- which is generally bad, particularly since things like CookieCSRFStoragePolicy mean that a MITM attacker can set the CSRF token to a known value or can read it and then execute cross site requests.

The origin checking only applies to HTTPS sites, and it exists entirely to ensure that HTTPS and HTTP versions of sites are treated as different origins.

For whatever it's worth, Django has a similar check (though theirs doesn't use the Origin header at all, only Referrer though it probably should support Origin header as well) with no option to disable it. There's also been research to suggest that less than 0.2% of HTTPS traffic is sent without a Referer header (most Referer header stripping is done at the network level which generally can't affect HTTPS connections anyways). So personally I'd say that if you're going to provide an option to disable the origin check, the origin check should still be on by default, but I'm also of the opinion that I wouldn't personally provide the option at all-- I just don't think it's a configuration I'd ever worry about supporting (and indeed the Mozilla page for network.http.sendRefererHeader explicity calls out that setting it to 0 will cause some sites to stop working).

A better privacy solution is to reduce the information provided in the Referrer/Origin headers to only send them on the same site requests and to only provide the actual origin information but not the specific URL.

[dstufft]

Also you should certainly tie Origin: null to allow_no_origin, since Allowing null I believe sidesteps a lot of the security gain of the origin check.

[mmerickel]

This is all pretty compelling to me. I'll modify the default back to false. I'm hesitant to remove the feature entirely now that it's in and feels easy to maintain but I'm fine removing it if others want to pile on.

As far as null goes, I'm tempted to just allow that value in pyramid.csrf_trusted_origins so a user can decide what to do via that setting instead as it's not technically "no origin".

Thank you for getting back about this @dstufft.

[mmerickel]

I should say that it does still seem to me that we should make the origin check itself separate from csrf token checking. I'm going to add a knob to turn off csrf origin checks entirely because if your storage policy is setting secure (https-only) tokens then the origin protections don't seem necessary.

[mmerickel]

Sorry you should probably view this file with ?w=1, dedented this whole block.

[digitalresistor]

I am a huge 👍 on leaving it enabled by default. So I can support this change :-)