1321870 - fix ratelimiting in webapp to ratelimit on the right header

Status: RESOLVED FIXED

(Socorro :: Webapp, task, P2)

Description

[Will Kahn-Greene [:willkg] ET needinfo? me]

We're using the wrong key for ratelimit:

https://github.com/mozilla/socorro/blob/9c3ba26e96b42836edbaf5f6d5fd53d41aca7201/webapp-django/crashstats/supersearch/views.py#L126

"ip" gets translated to using the REMOTE_ADDR header:

https://github.com/jsocol/django-ratelimit/blob/master/ratelimit/utils.py#L31

In our case, that's the ELB. Thus we're ratelimiting the total number of hits on those API endpoints rather than the number of hits for a given IP address.

I think we need to change it to look at the X-Forwarded-For header, but we need to be cautious that that header is coming explicitly and only from nginx and isn't coming from the client. Some details on what to think about here:

http://django-ratelimit.readthedocs.io/en/latest/security.html#client-ip-address

Making this a P2 since it's busted now and probably affects people, but it's been busted for ages so there's some urgency to fix it, but I don't think we have to do it right this second.

Comment 1

[Will Kahn-Greene [:willkg] ET needinfo? me]

As a side note, I didn't look at all the places we do ratelimiting in the webapp. We should fix key usage for all of them--not just the one I mentioned in the description.

Comment 2

[Peter Bengtsson [:peterbe]]

Turns out we have some code that does this:
https://github.com/mozilla/socorro/blob/222133aaaf7aa89f44e8ad0664f315de9351afa5/webapp-django/crashstats/crashstats/middleware.py#L9-L31
But we should audit that in alignment with the security recommendations and check that things work.

Comment 3

[Osmose [:osmose, :mkelly]]

I reviewed the code and the middleware from comment 2 seems to be correct in most environments...

...but not ELB. According their docs, they append the client IP to the _end_ of the X-Forwarded-For header, not the beginning like most others (nginx, the MDN docs) say they should: https://docs.aws.amazon.com/elasticloadbalancing/latest/classic/x-forwarded-headers.html

¯\_(ツ)_/¯

We can either update the middleware to conform to ELB's behavior, or we could make it configurable (either via another setting or a separate middleware) and configure Amazon-based environments to use the middleware.

willkg: If we're fine with writing code that only works in the local dev environment or AWS, then I think the first option is fine. What do you think?

Comment 4

[Will Kahn-Greene [:willkg] ET needinfo? me]

To recap (because I think I understand this, but I'm not positive), Socorro has a middleware that stomps on REMOTE_ADDR with the first value of X-Forwarded-For header if such exists. It doesn't exist in the local development environment. It does exist in our AWS-based server environments and is populated by the ELB.

The ELB puts the client IP address as the last value in a comma-separated list of values in the X-Forwarded-For header.

It seems like it's "working" right now because there's only one value in the X-Forwarded-For header that the ELB put there and since there's only one value, the first value is the last value is the client's IP address. Thus rate limiting is working on the client's IP address and not on the ELB.

Does that sound right?

Given that, I vote we fix the middleware and update the comment to talk about the ELB so that it's more correct and not accidentally correct.

Comment 5

[Osmose [:osmose, :mkelly]]

(In reply to Will Kahn-Greene [:willkg] ET needinfo? me from comment #4)
> To recap (because I think I understand this, but I'm not positive), Socorro
> has a middleware that stomps on REMOTE_ADDR with the first value of
> X-Forwarded-For header if such exists. It doesn't exist in the local
> development environment. It does exist in our AWS-based server environments
> and is populated by the ELB.
> 
> The ELB puts the client IP address as the last value in a comma-separated
> list of values in the X-Forwarded-For header.

Yes

> It seems like it's "working" right now because there's only one value in the
> X-Forwarded-For header that the ELB put there and since there's only one
> value, the first value is the last value is the client's IP address. Thus
> rate limiting is working on the client's IP address and not on the ELB.

If it seems like it's working, this is probably the case. Specifically, the ELB will pass-through client-supplied IPs, so anyone wanting to get around the rate limiting can do so right now by spoofing IPs in the header (this is because we are using the first value in our middleware).

> Given that, I vote we fix the middleware and update the comment to talk
> about the ELB so that it's more correct and not accidentally correct.

Righto!

Comment 6

[[github robot]]

Commits pushed to master at https://github.com/mozilla-services/socorro

https://github.com/mozilla-services/socorro/commit/33342491e38039c6311ee9a7c4b51a8c88699bc6
Fix bug 1321870: Use last IP instead of first from X-Forwarded-For.

https://github.com/mozilla-services/socorro/commit/f90974c282e70ca4bf0e19951fd57b28811d29b8
Merge pull request #4304 from Osmose/remote-host-elb

Fix bug 1321870: Use last IP instead of first from X-Forwarded-For.

Comment 7

[Brian Pitts]

I believe we are currently rate-limiting the ELB, not the client.

The standard behavior for X-Forwarded-For is to append the connecting client's IP address to the end. Thus is true both for ELBs and for our nginx config.

Let's say we have

client: 1.1.1.1
ELB: 2.2.2.2

When the ELB receives the request, X-Forwarded-For is not present. It sets it to the client address.

X-Forwarded-For: 1.1.1.1

Now when nginx receives the request, that single IP address is in the header. It appends the ELB's address.

X-Forwarded-For: 1.1.1.1,2.2.2.2

That is what the webapp will see.

However, the code in the middleware will extract the last IP address. This is why I say we are rate-limiting the ELB and not the client.

https://github.com/mozilla-services/socorro/blob/33342491e38039c6311ee9a7c4b51a8c88699bc6/webapp-django/crashstats/crashstats/middleware.py#L34

>>> real_ip='1.1.1.1,2.2.2.2'
>>> real_ip.split(',')[-1].strip()
'2.2.2.2

So, should we just switch to extracting the first IP address instead? No, because that can't be trusted. The client can include a value there that the ELB and nginx will happily pass along. For instance, if the client sent 

X-Forwarded-For: 9.9.9.9

to the ELB, the ELB would append to it and send

X-Forwarded-For: 9.9.9.9,1.1.1.1

to nginx, and nginx would send

X-Forwarded-For: 9.9.9.9,1.1.1.1,2.2.2.2

to the webapp. That would let clients avoid rate-limiting by passing arbitrary values.

I see two potential fixes.

One is to fix the value that the webapp receives for remote_addr, so that it does no have to deal with x-forwarded-for at all. We can do that using nginx's ngx_http_realip_module (http://nginx.org/en/docs/http/ngx_http_realip_module.html) in a way that will make remote_addr always be the ip address that connected to the ELB. Once that is done we could just disable/remove the SetRemoteAddrFromForwardedFor middleware

The other is to make the webapp aware of the number of proxies in between it and the client, and have it use that many from the right in the x-forwarded-for header as the client ip. This is what normandy does since bug #1268214. For example, we have two proxies (ELB and Nginx) in between clients and the webapp. So


>>> num_proxies=2
>>> real_ip='1.1.1.1,2.2.2.2'
>>> real_ip.split(',')[-num_proxies].strip()
'1.1.1.1'
>>> real_and_fake_ip='9.9.9.9,1.1.1.1,2.2.2.2'
>>> real_ip.split(',')[-num_proxies].strip()
'1.1.1.1

Comment 8

[Osmose [:osmose, :mkelly]]

(In reply to Brian Pitts from comment #7)
> One is to fix the value that the webapp receives for remote_addr, so that it
> does no have to deal with x-forwarded-for at all. We can do that using
> nginx's ngx_http_realip_module
> (http://nginx.org/en/docs/http/ngx_http_realip_module.html) in a way that
> will make remote_addr always be the ip address that connected to the ELB.
> Once that is done we could just disable/remove the
> SetRemoteAddrFromForwardedFor middleware

I like this one the best. Can we do this one?

Comment 9

[Brian Pitts]

Let's give it a shot. It would help us test the change if we had an easy way to verify what the webapp sees for remote_addr and x-forwarded-for. Maybe we could add displaying those to the sitestatus page, at least temporarily?

Comment 10

[Will Kahn-Greene [:willkg] ET needinfo? me]

Brian: I can do that. I made bug #1475993 for it.

Comment 11

[Brian Pitts]

In https://github.com/mozilla-services/cloudops-deployment/pull/2261 I made the nginx change that I had discussed earlier. Details are there; but tl;dr is that rate limiting will work now.

Comment 12

[Will Kahn-Greene [:willkg] ET needinfo? me]

The X-Real-IP changes landed on stage. Is this working as we want it now? If so, can we close this out?

Comment 13

[Brian Pitts]

Yes, I just verified that if I stop nginx from passing X-Forwarded-For, REMOTE_ADDR is still set correctly.