Insecure algorithm for determining remote/scheme/host

[vfaronov]

Long story short

aiohttp.web.BaseRequest has attributes scheme, host and, since 2.3, remote. Their values are determined by looking at request headers that can be manipulated by a remote user. Under most configurations, the user can set them to any desired values. This is especially bad for the remote attribute, because the user’s IP address is often relied upon for access controls.

(I’m reporting this in the open rather than privately, because remote is not yet released, while scheme and host do not seem like much of a security problem.)

Expected behaviour

This involves the headers Forwarded and X-Forwarded-For. The last element (comma-separated) in these headers can be trusted as long as a trusted proxy is configured to append it.

The same applies to the headers X-Forwarded-Host and X-Forwarded-Proto, with the exception that they are usually single values (to be replaced by the proxy), not comma-separated lists.

Actual behaviour

aiohttp takes the first element from Forwarded/X-Forwarded-For, without knowing which of these headers (if any) are controlled by a trusted proxy.

In most deployments where aiohttp sits behind nginx, the Forwarded header is not controlled by the proxy. Nobody is aware that it needs to be controlled. It is not mentioned in the example nginx configuration from aiohttp docs. In such deployments, an external user can trivially force scheme, host and remote to any desired values by sending a header like:

Forwarded: for=10.0.0.1;host=example.net;proto=https

In fact, it’s impossible to configure current versions of nginx to correctly append a Forwarded header (without writing some C or possibly Lua code).

But even if aiohttp was sitting behind a proxy that correctly controlled all of the involved headers, nothing would change, because the proxy would append a comma-separated element to the remote user’s Forwarded header, while aiohttp is looking at the first element — which is still controlled by the user.

Steps to reproduce

Run this server program:

from aiohttp import web
async def handle(request):
    info = (request.scheme, request.host, request.remote)
    return web.Response(text=repr(info))
app = web.Application()
app.router.add_get('/', handle)
web.run_app(app)

behind nginx with the following configuration (derived from the example):

daemon off;
error_log stderr;
pid /tmp/nginx1.pid;
events {
}
http {
  server {
    listen 12345;
    server_name example.com;
    access_log /tmp/nginx1.access.log;
    location / {
      proxy_set_header Host $http_host;
      proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
      proxy_redirect off;
      proxy_buffering off;
      proxy_pass http://localhost:8080;
    }
  }
}

and send requests to it with curl:

$ curl -s localhost:12345/ -H 'Forwarded: for=10.0.0.1;host=example.net;proto=https'
('https', 'example.net', '10.0.0.1')

Your environment

aiohttp Git master, Python 3.5, Linux

[asvetlov]

Sorry, I don't follow what do you propose?

[vfaronov]

@asvetlov

1. Remove remote.
2. Make scheme and host look at the last element of Forwarded, not the first.
3. Think about some sort of configuration options to indicate which headers can be trusted. Then remote can be restored (again, looking at the last element, not the first).

For example, see Gunicorn’s secure_scheme_headers and forwarded_allow_ips settings. (Their default for secure_scheme_headers is too broad, but they have privately indicated an interest in changing it in a future version.)

[vfaronov]

Actually, rather than hardcoding “last element”, it might be better to provide, again, an option to choose it — like the num_proxies parameter to Werkzeug’s ProxyFix — for cases when aiohttp is not immediately behind the “front-end” proxy.

[asvetlov]

People will reimplement remote prop anyway, and most likely their implementation will be less safe than provided by aiohttp. Be could disable forwarded headers processing by default though.
Last element is not satisfactory in case of chain of two reverse proxies in front of aiohttp server.

I like idea of additional opt-in application parameters for configuring corresponding header.

[vfaronov]

@asvetlov The parameter could combine headers and indices, like this:

# Use the first element from Forwarded
app.trusted_proxy_headers = {'Forwarded': 0}

# Use the second-last element from X-Forwarded-For,
# as well as any X-Forwarded-Proto
app.trusted_proxy_headers = {'X-Forwarded-For': -2,
                             'X-Forwarded-Proto': True}

[asvetlov]

I've decided to drop headers lookup for mentioned properties at all but support scheme, host and remote in Request.clone().
The approach allows making middleware for setting proper values.
We'll publish such middleware as separate project under aio-libs umbrella soon.
Unfortunately it is a breaking change but the security issue status forces backward compatibility breakage.

[thomaspsk]

@asvetlov I am a huge fan of the work you do, but this is the second time in the span of a week and half where you have introduced a breaking change in a minor version (first yarl, now aiohttp). The yarl issue affected us while doing a deployment which brought down our production environment. This time, we at noticed the issue in a testing environment (luckily), when as a direct response to the yarl issue, we upgraded to the latest version of aiohttp.

We host clusters of aiohttp services behind Amazon ALBs, which redirect all traffic internally via HTTP, not HTTPS. We use the scheme to determine which protocol was used to send URLs back to clients. This is important when running in dev environments vs. production. We do not support HTTP in production, which is where this becomes breaking. Checking the scheme in prod now returns URLs prefixed with HTTP scheme, instead of HTTPS.

While it's not a big issue for us to adapt to, breaking changes should not be introduced in minor versions. Therefore we don't go digging through change logs to see what we need to change when performing a minor upgrade.

I reiterate, I love the work you do and am super thankful for everything you have provided to the Python community. But please please please be careful with versioning!

I hope you understand,
Thomas

[asvetlov]

@thomaspsk thank you for feedback.
yarl breakage was unwitting but remote is deliberate decision (sorry for that but previous implementation introduced an important enough security issue).

We have created https://github.com/wikibusiness/aiohttp-remotes for helping users to respect headers like X-Forwarded-* etc.

For your case the closest (but not secure) way is adding a middleware:

from aiohttp-remotes import setup, XForwardedRelaxed
await setup(XForwardedRelaxed())

The library will be moved into aio-libs organization just after writing documentation.

Sorry for caused inconvenience.

[asvetlov]

P.S.
Next release will be aiohttp 3.0 -- major with many backward incompatible changes. Migration should be not very hard though.
After that I'll try to avoid breaking changes (except security issues maybe) up to aiohttp 4.0