Add allow_no_origin option to CSRF

[luhn]

Fixes #3508

[luhn]

Things to discuss:

• allow_no_origin — Name okay?
• allow_no_origin argument to set_default_csrf_options and check_csrf_origin — Technically not backwards compatible.

[mmerickel]

I think the name is fine. I thought about allow_unknown_origin or allow_missing_origin as well. This all seems good to me so far.

[luhn]

Great! Wrote up some docs.

[Deimos]

Code looks good, and I tested it on my app and confirmed that it fixes my issue - POST requests now work if the Referer header is disabled, but still require a correct CSRF token.

There were just a couple of tiny things in the docs/comments I noticed. Thanks again for implementing this so quickly, I really appreciate it!

[luhn]

Thanks for catching those! Changes made.

[digitalresistor]

This looks great, I would say the client should make sure to send Origin, especially when the spec requires it, but if someone wants to turn a knob to turn off a security feature, this isn't half bad.

[mmerickel]

Honestly I feel like the default for this should be True considering an Origin header isn't set by default when on the same domain. In that case aren't we currently just falling back to the Referer header? Or am I wrong here?

[mmerickel]

To clarify it seems to me like the current logic is:

same domain request -> no origin header sent, use referer
cross domain request -> origin header is almost always sent, fallback to referer

I think (without testing) that even if you turn off the privacy settings named by @Deimos, the Origin header will still be sent on cross domain requests, and this PR is really focused on fixing same domain requests. The common case here, to me, should be permissive as compliant user agents will probably still sent an Origin header on cross-domain requests even in private browsing situations because it hides the origin's path and only shows the domain. The spec also leaves room for "null" as the value of the Origin header, which we aren't supporting atm but I'd claim should be handled permissively if set, which it is not right now.

My proposed changes:

1. change default to True

2. handle Origin: null as permissive always, non-configurable

[Deimos]

Personally, I'd support the check being disabled by default, at least for same-domain requests. I think the really key thing to recognize is that the token-based method is already considered strong, and it is individually good protection. The Origin/Referer check is just a second (weak) layer that isn't truly reliable and can cause issues like this.

In the OWASP cheatsheet that I linked in the issue, it says:

We recommend token based CSRF defense (either stateful/stateless) as a primary defense to mitigate CSRF in your applications. Only for highly sensitive operations, we also recommend a user interaction based protection (either re-authentication/one-time token, detailed in section 6.5) along with token based mitigation.

As a defense-in-depth measure, consider implementing one mitigation from Defense in Depth Mitigations section (you can choose the mitigation that fits your ecosystem considering the issues mentioned under them). These defense-in-depth mitigation techniques are not recommended to be used by themselves (without token based mitigation) for mitigating CSRF in your applications.

The Origin check is one of the "Defense in Depth Mitigations". It's not an essential part, and wouldn't be considered proper protection on its own. The recommendation in the cheatsheet is:

Usually, a minor percentage of traffic does fall under above categories [for why the headers wouldn't be present] (1-2%) and no enterprise would want to lose even this minor % of traffic. One of the popular technique used across Internet to make this technique more usable is to accept the request if the Origin/referrer matches your configured list of domains "OR" a null value (Examples here. null value is to cover the edge cases mentioned above where these headers are not sent). Please note that, attackers can exploit this but people prefer to use this technique as a defense in depth measure because of minor effort involved in deploying it

[mmerickel]

I opened #3518, please let me know what you folks think.