From 07779ea879e682ee3d3a4728fbf3f98540c9310a Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Tue, 23 Mar 2021 14:40:08 -0700 Subject: [PATCH] middleware: Do not trust X-Forwarded-For; use X-Real-Ip, set from nginx. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `X-Forwarded-For` header is a list of proxies' IP addresses; each proxy appends the remote address of the host it received its request from to the list, as it passes the request down. A naïve parsing, as SetRemoteAddrFromForwardedFor did, would thus interpret the first address in the list as the client's IP. However, clients can pass in arbitrary `X-Forwarded-For` headers, which would allow them to spoof their IP address. `nginx`'s behavior is to treat the addresses as untrusted unless they match an allowlist of known proxies. By setting `real_ip_recursive on`, it also allows this behavior to be applied repeatedly, moving from right to left down the `X-Forwarded-For` list, stopping at the right-most that is untrusted. Rather than re-implement this logic in Django, pass the first untrusted value that `nginx` computer down into Django via `X-Real-Ip` header. This allows consistent IP addresses in logs between `nginx` and Django. Proxied calls into Tornado (which don't use UWSGI) already passed this header, as Tornado logging respects it. --- .../files/nginx/zulip-include-common/proxy | 1 + .../files/nginx/zulip-include-frontend/app | 4 +++ .../accept-loadbalancer.conf.template.erb | 8 ++++-- zerver/middleware.py | 26 +++++++++++-------- zproject/computed_settings.py | 2 +- 5 files changed, 27 insertions(+), 14 deletions(-) diff --git a/puppet/zulip/files/nginx/zulip-include-common/proxy b/puppet/zulip/files/nginx/zulip-include-common/proxy index 017a169824..be2b698162 100644 --- a/puppet/zulip/files/nginx/zulip-include-common/proxy +++ b/puppet/zulip/files/nginx/zulip-include-common/proxy @@ -4,5 +4,6 @@ proxy_http_version 1.1; proxy_set_header Connection ""; proxy_set_header Host $host; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; +proxy_set_header X-Real-Ip $remote_addr; proxy_next_upstream off; proxy_redirect off; diff --git a/puppet/zulip/files/nginx/zulip-include-frontend/app b/puppet/zulip/files/nginx/zulip-include-frontend/app index 16631b6f7a..9717be02ef 100644 --- a/puppet/zulip/files/nginx/zulip-include-frontend/app +++ b/puppet/zulip/files/nginx/zulip-include-frontend/app @@ -52,6 +52,7 @@ location /api/v1/events { # Send everything else to Django via uWSGI location / { include uwsgi_params; + uwsgi_param HTTP_X_REAL_IP $remote_addr; uwsgi_pass django; } @@ -65,12 +66,14 @@ location /thumbnail { include /etc/nginx/zulip-include/api_headers; include uwsgi_params; + uwsgi_param HTTP_X_REAL_IP $remote_addr; uwsgi_pass django; } location /avatar { include /etc/nginx/zulip-include/api_headers; include uwsgi_params; + uwsgi_param HTTP_X_REAL_IP $remote_addr; uwsgi_pass django; } @@ -79,6 +82,7 @@ location /api/ { include /etc/nginx/zulip-include/api_headers; include uwsgi_params; + uwsgi_param HTTP_X_REAL_IP $remote_addr; uwsgi_pass django; } diff --git a/puppet/zulip/templates/accept-loadbalancer.conf.template.erb b/puppet/zulip/templates/accept-loadbalancer.conf.template.erb index d970102414..8ffe2704b3 100644 --- a/puppet/zulip/templates/accept-loadbalancer.conf.template.erb +++ b/puppet/zulip/templates/accept-loadbalancer.conf.template.erb @@ -1,7 +1,11 @@ # Configuration for making Zulip app frontends accept the -# X-Forwarded-For header used by the Zulip proxy. The accepted IP -# addresses here are set in /etc/zulip/zulip.conf. +# X-Forwarded-For header used by proxies. The trusted IP addresses +# here are set by `loadbalancer.ips` in /etc/zulip/zulip.conf. +# +# This causes us to update $remote_addr, which we use in logging, and +# also pass down to Zulip as X-Real-Ip. real_ip_header X-Forwarded-For; +real_ip_recursive on; <% @loadbalancers.each do |host| -%> set_real_ip_from <%= host %>; <% diff --git a/zerver/middleware.py b/zerver/middleware.py index 64d2046483..ad8afe4a3d 100644 --- a/zerver/middleware.py +++ b/zerver/middleware.py @@ -519,25 +519,29 @@ class HostDomainMiddleware(MiddlewareMixin): return None -class SetRemoteAddrFromForwardedFor(MiddlewareMixin): - """ - Middleware that sets REMOTE_ADDR based on the HTTP_X_FORWARDED_FOR. +class SetRemoteAddrFromRealIpHeader(MiddlewareMixin): + """Middleware that sets REMOTE_ADDR based on the X-Real-Ip header. + + This middleware is similar to Django's old + SetRemoteAddrFromForwardedFor middleware. We use X-Real-Ip, and + not X-Forwarded-For, because the latter is a list of proxies, some + number of which are trusted by us, and some of which could be + arbitrarily set by the user. nginx has already parsed which are + which, and has set X-Real-Ip to the first one, going right to + left, which is untrusted. + + Since we are always deployed behind nginx, we can trust the + X-Real-Ip which is so set. In development, we fall back to the + REMOTE_ADDR supplied by the server. - This middleware replicates Django's former SetRemoteAddrFromForwardedFor middleware. - Because Zulip sits behind a NGINX reverse proxy, if the HTTP_X_FORWARDED_FOR - is set in the request, then it has properly been set by NGINX. - Therefore HTTP_X_FORWARDED_FOR's value is trusted. """ def process_request(self, request: HttpRequest) -> None: try: - real_ip = request.META["HTTP_X_FORWARDED_FOR"] + real_ip = request.META["HTTP_X_REAL_IP"] except KeyError: return None else: - # HTTP_X_FORWARDED_FOR can be a comma-separated list of IPs. - # For NGINX reverse proxy servers, the client's IP will be the first one. - real_ip = real_ip.split(",")[0].strip() request.META["REMOTE_ADDR"] = real_ip diff --git a/zproject/computed_settings.py b/zproject/computed_settings.py index 511000b139..c0e307b262 100644 --- a/zproject/computed_settings.py +++ b/zproject/computed_settings.py @@ -172,7 +172,7 @@ MIDDLEWARE = ( # With the exception of it's dependencies, # our logging middleware should be the top middleware item. "zerver.middleware.TagRequests", - "zerver.middleware.SetRemoteAddrFromForwardedFor", + "zerver.middleware.SetRemoteAddrFromRealIpHeader", "zerver.middleware.RequestContext", "zerver.middleware.LogRequests", "zerver.middleware.JsonErrorHandler",