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

Why use referrer header for CSRF protection when you have sychronizer tokens? #364

Closed
knod opened this issue Apr 9, 2019 · 9 comments
Closed
Labels

Comments

@knod
Copy link

knod commented Apr 9, 2019

I'm new to the security game. I'd understood that a sychronizer token and a referrer header were doing basically the same thing, and that a sychronizer token is more robust. What does a referrer header add that a sychronizer token doesn't address?

@azmeuk azmeuk added the csrf label Jul 29, 2020
@gsmethells
Copy link

gsmethells commented Jun 25, 2021

I'll add a reference to CSRF and the synchronizer token pattern from OWASP and second the question.

@davidism
Copy link
Member

davidism commented Jun 25, 2021

Because that's how the code was written years ago. Given how vague this issue is worded, I would have to re-research our current implementation and compare it to the suggested alternative to even understand if it would be necessary or feasible to change. If someone wants to put in such work and suggest a way forward, fine, otherwise I'll probably close this.

@gsmethells
Copy link

gsmethells commented Jun 25, 2021

If request.is_secure is False (due to being behind nginx or a load-balancer that does SSL termination), then it appears that same_origin() will not be called. It feels like it should either be all in or not.

       if request.is_secure and current_app.config['WTF_CSRF_SSL_STRICT']:
            if not request.referrer:
                self._error_response('The referrer header is missing.')

            good_referrer = f'https://{request.host}/'

            if not same_origin(request.referrer, good_referrer):
                self._error_response('The referrer does not match the host.')

I realize that opting out is as easy as setting current_app.config['WTF_CSRF_SSL_STRICT'] to False but to opt-in currently requires that the request have the https protocol when hitting Flask (from my understanding of request.is_secure).

I lean towards doing the same_origin() check always, but it has a variant here that prevents it from always being checked (SSL terminated prior to hitting gunicorn engine running Flask app, for example).

@gsmethells
Copy link

gsmethells commented Jun 25, 2021

Doing some research on OWASP, further down the same page linked earlier, OWASP does recommend checking the Referer header but only if the Origin header is not present. The current code does not make any such distinction and an addition to match the OWASP recommendation would be a worthy improvement.

In addition, what Target origin to compare either of those to is considered by OWASP to be non-trivial because "the application server is frequently sitting behind one or more proxies and the original URL is different from the URL the app server actually receives".

In fact, their third recommendation to use the X-Forwarded-Host header value probably hits the mark on the head. In the case of nginx proxying for gunicorn, X-Forwarded-Host is the most appropriate value and would be better than

good_referrer = f'https://{request.host}/'

unless request.host already handles this case within Flask? If not, then the current code does not make this distinction either or offer the caller a configuration manner for steering towards use of the X-Forwarded-Host header value. This would also be a worthy change to the current code, IMHO.

If there is a more appropriate source than OWASP to quote from, then I am not aware of it, but am open to suggestions.

@gsmethells
Copy link

Mozilla links to the above HTTP header values:

@davidism
Copy link
Member

davidism commented Jun 25, 2021

If request.is_secure is False (due to being behind nginx or a load-balancer that does SSL termination)

You need to configure your app with ProxyFix then. The app should know how it's proxied. That seems to address your findings. They seem orthogonal to the original question though.

@gsmethells
Copy link

gsmethells commented Jun 28, 2021

    # Proxy a Flask app nicely:  https://web.archive.org/web/20190128010140/http://flask.pocoo.org/snippets/35/
    proxy_set_header          Host  $http_host;
    proxy_set_header          X-Forwarded-Proto  https;
    proxy_set_header          X-Forwarded-For  $proxy_add_x_forwarded_for;
    proxy_set_header          X-Real-IP  $remote_addr;
    proxy_set_header          X-Scheme  $scheme;
    proxy_set_header          REMOTE_ADDR  $remote_addr;

For now we run nginx with this but will be moving to GCP and not reverse proxying with nginx any longer. We'll see how we resolve that, but that still does not touch on the Origin header vs Referer header topic.

@gsmethells
Copy link

gsmethells commented Jun 28, 2021

As far as the orthogonality to the original question, variability is a concern in security software. The less variability the better. And assumptions can be killer. If only the first item below from OWASP guidance concerning CSRF is followed and nothing more, then the user can easily be in for an unpleasant surprise regarding what protections are or are not being magically provided by their framework. It'll take some digging for them to find all the pieces that must be put together.

In any case, focusing in on the original question alone and ignore any dangers from variance, OWASP guidance is to:

  1. Check if your framework has built-in CSRF protection and use it
  2. For stateful software use the synchronizer token pattern
  3. For stateless software use double submit cookies
  4. Implement at least one mitigation from Defense in Depth Mitigations section

where (4) includes:

  1. Consider SameSite Cookie Attribute for session cookies
  2. Consider implementing user interaction based protection for highly sensitive operations
  3. Consider the use of custom request headers
  4. Consider verifying the origin with standard headers

hence, the answer to this ticket is that both "synchronizer token pattern" and "verifying the origin" are necessary to meet OWASP security standards if the SameSite Cookie Attribute is not very strict (sorry, couldn't help the pun). Though, to be honest, I have read of security experts poo-pooing the security of Same Origin so likely the more mitigations the better.

So, if there are no other topics other than Origin header vs Referer header which I am still curious about, then this ticket should be closed based upon that research.

@gsmethells
Copy link

Here is a link to proxy setups and ProxyFix for the curious reader who trundles here at a later date.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

No branches or pull requests

4 participants