Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow disabling the Origin check in CSRF protection #3508

Closed
Deimos opened this issue Sep 10, 2019 · 16 comments · Fixed by #3512
Closed

Allow disabling the Origin check in CSRF protection #3508

Deimos opened this issue Sep 10, 2019 · 16 comments · Fixed by #3512

Comments

@Deimos
Copy link

Deimos commented Sep 10, 2019

Feature Request

Is your feature request related to an issue? Please describe.
A decent number of users use privacy-oriented settings or extensions that can prevent their browser from sending referrer info. Lynx-based browsers seem to do this by default as well. Because of this, Pyramid's CSRF protection prevents those users from being able to use any views with CSRF protection, since the Origin check for CSRF always fails.

Describe the solution you'd like
I'd like an easy way to disable the Origin part of the check (maybe an argument on config.Configurator.set_default_csrf_options?). The Origin check is recommended but not truly necessary, and between the token check and SameSite cookies I'm comfortable disabling it.

Describe alternatives you've considered
I think I could probably effectively disable the check by finding somewhere to catch the BadCSRFOrigin exception and ignore it, but it seems a little tricky and a way to just disable the check cleanly would be nicer.

I haven't dug through the relevant Pyramid code much, so if there's already an easy way to ignore this check that I missed, please let me know.

@mmerickel
Copy link
Member

It was my impression that the header was required on https post requests but I can't actually find anything to back that up right now. Some followup questions:

  • Is the origin header "null" (as documented in privacy-sensitive browsing contexts) or is it missing entirely?
  • Is the request not https?

@Deimos
Copy link
Author

Deimos commented Sep 19, 2019

The requests are https. If I change Firefox's network.http.sendRefererHeader setting through about:config to the 0 setting, there's no Origin or Referer header sent at all, and Pyramid will fail the CSRF protection. If it's on 2 (the default), it sends a Referer header, but no Origin.

@mmerickel
Copy link
Member

The word "Origin" doesn't appear once in the linked page. Surely the origin header is controlled by something else?

@Deimos
Copy link
Author

Deimos commented Sep 19, 2019

Pyramid falls back to using Referer when Origin isn't present:

pyramid/src/pyramid/csrf.py

Lines 301 to 303 in a232b69

origin = request.headers.get("Origin")
if origin is None:
origin = request.referrer

@Deimos
Copy link
Author

Deimos commented Sep 19, 2019

(And it doesn't look like Firefox usually sends the Origin header at all, or maybe only for cross-domain requests, but it doesn't look like it's sending it during normal usage on my site)

@mmerickel
Copy link
Member

Yes, Origin header is only for cross-domain requests, and we're specifically concerned with mutations, not HEAD/OPTIONS/GET requests (which it also doesn't send the Origin header with).

@mmerickel
Copy link
Member

So is that the issue? That it's not a cross-domain request and it doesn't contain a referrer? I suppose I see that could be an issue.

@mmerickel
Copy link
Member

If that's the case, one workaround would be to pass a callback to set_default_csrf_options.

def allow_if_missing_origin(request):
    if not request.referrer and not request.headers.get('Origin'):
        return False
    return True
config.set_default_csrf_options(..., callback=allow_if_missing_origin)

@Deimos
Copy link
Author

Deimos commented Sep 19, 2019

That's the problem I'm having, yes - anyone that's disabled sending the Referer header is unable to do anything on the site that uses a POST request (logging in, posting comments, etc.) due to the CSRF check failing.

Thanks, the callback looks like a good workaround. I'll try that out. Edit: oh, not if it totally disables CSRF for that request.

I do think it would be nice to have a built-in option to easily disable that part of the CSRF check though, since that's more of a recommendation than an essential behavior, and can cause issues like this with people who were trying to improve their privacy. As the OWASP cheatsheet about it says:

If neither of these headers are present, you can either accept or block the request. We recommend blocking. Alternatively, you might want to log all such instances, monitor their use cases/behavior, and then start blocking requests only after you get enough confidence.

I think blocking by default is still a good idea, but a simple option to bypass it would be appreciated.

@luhn
Copy link
Contributor

luhn commented Sep 19, 2019

@mmerickel, if I'm sight-reading your workaround correct, CSRF would be disabled entirely when no Referer or Origin is present, which definitely is undesirable.

Thanks for the link to OWASP. That convinces me personally that this option is necessary. Should be a pretty easy PR, I'm willing to throw one together.

@mmerickel
Copy link
Member

Fair point. I think I’d accept an option to make the origin / referrer optional or maybe we could support some form of allowed_origins = * or something? I’m thinking that I do not want to turn off origin checking entirely and I’d like the feature to be specific to the case when the origin is unspecified.

@luhn
Copy link
Contributor

luhn commented Sep 19, 2019

Rather than disabling Origin/Refer checking, maybe the option just decides a pass or fail when both Origin and Refer are absent, otherwise the behavior is as-is.

@luhn
Copy link
Contributor

luhn commented Sep 19, 2019

On second look I may have repeated what you just said. 😬

@mmerickel
Copy link
Member

haha we’re on the same page then!

@luhn
Copy link
Contributor

luhn commented Sep 20, 2019

@Deimos, can you confirm the PR addresses the issue? #3512

@Deimos
Copy link
Author

Deimos commented Sep 20, 2019

I'll test it out tomorrow, thanks for working on this so quickly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants