Restore original remote_ip algorithm. #7980

Merged
merged 1 commit into from Jan 2, 2013

Projects

None yet

10 participants

@steveklabnik
Ruby on Rails member

We incorrectly were using the first address, rather than the last one.

Fixes #7979

/cc @indirect @gazay

@steveklabnik
Ruby on Rails member

I am 99% sure this is right, the tests were new, so I wasn't sure how to 'revert' them. Please check this request well before merging. ;)

@indirect
Ruby on Rails member

I put some notes into the extended commit messages, but the upshot is that it works correctly now, it ignores private IPv6 IPs now (which it didn't do before), and the code is much shorter. Yay. :)

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Oct 18, 2012
actionpack/lib/action_dispatch/middleware/remote_ip.rb
@@ -105,22 +99,15 @@ def to_s
@ip = calculate_ip
end
- private
+ private
@rafaelfranca
rafaelfranca Oct 18, 2012

Opps. Take care to not change the code conventions.

@indirect
indirect Oct 18, 2012

In my defense, he changed my outdented protected to an indented private. :) I'll restore it to what it was before the commit I am reverting here.

@rafaelfranca rafaelfranca commented on an outdated diff Oct 18, 2012
actionpack/test/dispatch/request_test.rb
end
- test "remote ip v6 with user specified trusted proxies Regexp" do
- @trusted_proxies = /^fe80:0000:0000:0000:0202:b3ff:fe1e:8329$/i
-
- request = stub_request 'REMOTE_ADDR' => '2001:0db8:85a3:0000:0000:8a2e:0370:7334',
- 'HTTP_X_FORWARDED_FOR' => 'fe80:0000:0000:0000:0202:b3ff:fe1e:8329'
- assert_equal '2001:0db8:85a3:0000:0000:8a2e:0370:7334', request.remote_ip
-
- request = stub_request 'HTTP_X_FORWARDED_FOR' => 'fe80:0000:0000:0000:0202:b3ff:fe1e:8329, 2001:0db8:85a3:0000:0000:8a2e:0370:7334'
- assert_equal nil, request.remote_ip
- end
+ # test "remote ip when the remote ip middleware returns nil" do
@rafaelfranca
rafaelfranca Oct 18, 2012

Commented tests.

@rafaelfranca
Ruby on Rails member

I did some comments inline. The feature seems fine but we need to remove the comment tests (if they are not needed) or uncomment they (if they are needed)

@indirect
Ruby on Rails member
@gazay
@indirect
Ruby on Rails member

Ohhh, I understand what the problem is now. Many proxies don't actually modify the X-Forwarded-For header. Instead, they add another X-Forwarded-For header. According to the HTTP spec, that means that the values should be concatenated, joined by a comma. If you're using a ruby server that understands repeated headers, the correct IP will then be the last value. If the proxy conforms to the W3C spec that you linked, the correct IP will be the first value.

Um. How can we resolve this? FWIW, I've only ever seen the "adds a second header" proxy type in the wild (I know HAProxy does this).

@indirect
Ruby on Rails member

In addition, other blog posts I have read on the subject seem to indicate that the last IP is the correct one in real-world scenarios.

@indirect
Ruby on Rails member

And indeed, the Apache bug on this subject also seems to imply we should be using the last IP address in the concatenated list.

@gazay
@indirect
Ruby on Rails member

It seems like an option on the middleware is the way to go if this is a common need, and a replacement middleware that removes the calls to reverse is the way to go if its not common. WDYT, people who can merge this?

@steveklabnik
Ruby on Rails member

Ugh.

@indirect
Ruby on Rails member

@steveklabnik inorite? 😭

@rafaelfranca
Ruby on Rails member

I don't like the idea to add Yet Another Configuration Option, but I don't know if this is not common

@steveklabnik
Ruby on Rails member

I always prefer what is correct to what is not. It's not our job to fix bugs in other projects, we should follow the spec.

Then again, I don't have anything currently deployed in production that depends on this in either direction ;)

@indirect
Ruby on Rails member
@ehoch

Figured I'd weigh in here. I use Google Page Speed Service in front of Heroku. Not the craziest, most unheard of Rails environment. There, the client's IP is the FIRST in the X-Forwarded-For csv.

Can we make this configurable?

@indirect
Ruby on Rails member

I guess the upside is that it's always going to be first or last? Unless you combine Google Page Speed and HAProxy, then it'll be in the middle. -_-

I'm ready to implement an option that lets you tell the middleware whether you want the first or last IP. I'm not sure which option should be the default, though, since the "spec correct" one is first and the "works like Apache etc" one is last. :|

@gazay

I think that default should be last IP (your implementation), because long time nobody noticed that, and my pull was opened about 8 months before merged in. So I think first IP is like exception.

I can help you with implementation

@indirect
Ruby on Rails member
@steveklabnik
Ruby on Rails member

Yes, @gazay let me know if you have some commits and I can give you access to my repo.

@gazay

Great, thanks guys!

@gazay

So, I'm ready to push some commits. I did the same option usage as with ip_spoofing_check. And I wrote some tests to check different order usage

And btw I think that we shouldn't filter values from X_FORWARDED_FOR and give to user unexpected IP of unknown proxy server. We should give him only first or last ip from list as we decided here. I changed code in remote_ip.rb and fix tests for it. What do you think @indirect, @steveklabnik?

@steveklabnik
Ruby on Rails member

You're added to my repo.

@gazay

I've added commits. I'm not sure about name of option - x_forwarded_for_last_ip, but that's all I could think of

@indirect
Ruby on Rails member
@indirect
Ruby on Rails member
@gazay

About filtering – I see the problem, you right, I'll change it back. About naming - last_ip or last_client_ip not really clear when someone will read this code or try to change option. It's like if ip_spoofing_check will be just ip_check - not sure what this option allow to check. Maybe last_forwardable_ip?

@indirect
Ruby on Rails member

Ahh, I see. I was thinking the option should apply to REMOTE_ADDR too, but it seems that that header will only ever have a list of IPs due to duplicated headers, and so reverse is always the correct direction.

How about calling the option "last_forwarded_ip"? That includes both the important part of the header name, and literally describes what you will get if you turn the option on -- the last IP from the proxy's list of forwarded IPs.

@gazay

Ok, I'll amend it to my commit.

@gazay

I've decided not to use force push right now, before end of conversation, so here new commit. I changed option name and added entries to Changelog and rails guides.

@gazay

So what do you think @steveklabnik, @indirect, @rafaelfranca?

@indirect
Ruby on Rails member

I am 👍 on merging this. We need an option to make remote_ip usable depending on what kind of proxy the rails app is behind, and this seems good to me.

@steveklabnik
Ruby on Rails member

I am just a lowly implementor.

@indirect
Ruby on Rails member
@steveklabnik
Ruby on Rails member

Rebased and sqaushed.

@rafaelfranca rafaelfranca commented on the diff Nov 18, 2012
actionpack/lib/action_dispatch/middleware/remote_ip.rb
end
def to_s
@ip ||= calculate_ip
end
- private
+ protected
@rafaelfranca
rafaelfranca Nov 18, 2012

Why this change? I think we don't need protected, private is fine

@indirect
indirect Nov 18, 2012

Last time I left it private, and you complained that I was only changing the indentation. :) This is just the way it was in my original implementation, I have no preference between them.

@rafaelfranca rafaelfranca commented on an outdated diff Nov 18, 2012
actionpack/CHANGELOG.md
@@ -1,5 +1,15 @@
## Rails 4.0.0 (unreleased) ##
+* Added option `config.action_dispatch.last_forwarded_ip` to enable different
+ order for getting `remote_ip` from `X_FORWARDED_FOR` header.
+ Set last valid IP from `X_FORWARDED_FOR` header as `client_ip` by default.
@rafaelfranca
rafaelfranca Nov 18, 2012

I think we need to make this explanation more clear. Maybe with one example of the behavior

@rafaelfranca rafaelfranca commented on an outdated diff Nov 18, 2012
railties/lib/rails/application.rb
@@ -354,7 +354,12 @@ def default_middleware_stack #:nodoc:
middleware.use ::Rails::Rack::Logger, config.log_tags # must come after Rack::MethodOverride to properly log overridden methods
middleware.use ::ActionDispatch::ShowExceptions, config.exceptions_app || ActionDispatch::PublicExceptions.new(Rails.public_path)
middleware.use ::ActionDispatch::DebugExceptions, app
- middleware.use ::ActionDispatch::RemoteIp, config.action_dispatch.ip_spoofing_check, config.action_dispatch.trusted_proxies
+ middleware.use(
+ ::ActionDispatch::RemoteIp,
+ config.action_dispatch.ip_spoofing_check,
+ config.action_dispatch.trusted_proxies,
+ config.action_dispatch.last_forwarded_ip
@rafaelfranca
rafaelfranca Nov 18, 2012

The name of the configuration doesn't tell too much. What mean last_forwarded_ip = false?

@rafaelfranca
Ruby on Rails member

The implementation is good to me, but I think we need a better name for the configuration and a better explanation. Also I think we need to expand the guides to tell me when I need to change that value

@steveklabnik
Ruby on Rails member

Seems legit.

@gingerlime

Hope I can butt-in on this. I've tried to look at this closely, and I believe actually both @indirect and @gazay are correct :)

However, @indirect's approach is probably safer. i.e. whilst the leftmost IP can be the correct IP for the client, it can also easily be spoofed this way.

I think I also identified an existing potential vulnerability in both master and stable, particularly to rails apps with no proxy (most prominent example is nginx/passenger probably).

I've written a (rather lengthy) blog post about this, which I hope explains things clearly... including how both can be correct.

Given that my ruby/rails experience is somewhat limited, I want to be careful with broad statements, but would be great if you guys can take a look, and I'm happy to discuss / explain / discover I'm completely wrong.

@indirect
Ruby on Rails member

@rafaelfranca how about an option name like use_last_forwarded_ip? I think stalling this patch until the guide is implemented is a very bad idea, because the current behaviour is wrong in the default case. This is an attempt to restore the correct default behaviour, and provide an option for people like @gazay who need the opposite.

I also believe that @gingerlime is correct, and the current implementation is a (small) security vulnerability, since it allows IP address spoofing.

@indirect
Ruby on Rails member

@gingerlime if it makes you feel any better, there are very few Rails apps in production that don't have any kind of proxy set up. Apps on Heroku Cedar (not Heroku Bamboo) are the only ones I am aware of.

So I guess Heroku might want to know about this potential IP spoofing vulnerability. /cc @hone

@rafaelfranca
Ruby on Rails member

use_last_forwarded_ip seems better.

We need to treat documentation as part of the code. A full pull request needs code, tests and documentation.

Although the code and tests are good the documentation is not.

Since that code is only on master and the Rails 4.0.0 is not released yet we don't need to hurry.

About the potential IP spoofing I need to call @NZKoz. Could you look at #7980 (comment) and http://blog.gingerlime.com/2012/rails-ip-spoofing-vulnerabilities-and-protection/

@gingerlime

@indirect thanks. Yep. It seems so. Unicorn/Thin/Mongrel et al should all rely on an external proxy so should be fine. I was under the impression that nginx/passenger is quite a popular combination though (??), and it seems to be vulnerable too.

If you consider those web servers that don't handle X-Forwarded-For properly, they might also be susceptible to some form of spoofing, but depends on the specifics of how they handle those headers and which webserver sits in front. I didn't get a chance to play with anything other than Unicorn and Passenger personally to test this unfortunately.

I know my opinion doesn't count for much, being a complete outsider to the rails project, but I believe that there shouldn't be a need for this use_last_forwarded_ip option (whatever you decide to call it). I might be wrong, but I believe that if you define your proxies correctly, the current algorithm will result in the leftmost IP address being equal to the rightmost one (after those proxies were removed). And if they're not equal, the chances are that this is a result of either some spoofing or misconfiguration.

@indirect
Ruby on Rails member

@gingerlime ahh, you make a very good point. I didn't connect the dots during the earlier discussion, but the option shouldn't be needed at all if you can explicitly list the proxies whose addresses you don't want to count. @gazay, would that work in your situation?

Given the potential for IP spoofing vulnerabilities, I think we might want to ultimately revert to the original behaviour without an option, and suggest in the comments/docs that anyone who needs something custom should remove this middleware and insert their own instead.

@NZKoz
Ruby on Rails member

Yes, while we read from X-Forwarded-For in the implementation of remote_ip then you'll always be potentially vulnerable to spoofing attacks. I'd suggest just adding something like:

  request.forwarded_ips # => ['x.x.x.x', 'y.y.y.y', 'z.z.z.z']

Then the config just becomes a question of first or last from that method.

FWIW, if you're using remote_ip in a security setting you're completely crazy ;)

@indirect
Ruby on Rails member
@gingerlime

@NZKoz I just re-read your comment:

while we read from X-Forwarded-For in the implementation of remote_ip then you'll always be potentially vulnerable to spoofing attacks

This is not entirely accurate. If you trust your proxy server(s), and by trust I also mean you trust them to handle X-Forwarded-For appropriately (and barring any vulnerabilities those might have). Then you should be able to trust a portion of the header's content, which should produce the IP address of the device connected to your outer-most trusted proxy.

If you don't use any (trusted) proxy servers, then you should simply ignore the X-Forwarded-For header completely.

@steveklabnik
Ruby on Rails member

So: what exactly needs to be done to move this forward?

@indirect
Ruby on Rails member

...better...docs?

@indirect
Ruby on Rails member
@steveklabnik
Ruby on Rails member

I can see that too. @NZKoz what do you think?

@NZKoz
Ruby on Rails member

sounds good to me

@indirect
Ruby on Rails member

Great. I'll try to get that implemented today.

@steveklabnik
Ruby on Rails member

Ping! @indirect did you get a chance to play with this?

@indirect
Ruby on Rails member

Oh man. Totally fell off my list. Today or tomorrow, I hope.

@steveklabnik
Ruby on Rails member

No worries bro, know you've been busy.

@jfirebaugh

I agree with reverting to last IP and documenting the trusted proxy option.

What I would like to see is an option to ignore HTTP_CLIENT_IP. In an EC2+ELB environment that header is effectively garbage and should be ignored (ELB only uses X-Forwarded-For, not Client-IP). The fact that it is used to try to detect spoofing results in legitimate users getting locked out. If not an option, then at least extract some finer-grained methods so I can monkey patch more easily.

def client_ip
  @env['HTTP_CLIENT_IP']
end

def forwarded_ips
  ips_from('HTTP_X_FORWARDED_FOR')
end

def remote_addrs
  ips_from('REMOTE_ADDR').reverse
end

def candidate_ips
  [client_ip, forwarded_ips, remote_addrs].flatten.compact
end

def remote_ip
  remove_proxies(candidate_ips).first || remote_addrs.first
end
@indirect
Ruby on Rails member
@indirect
Ruby on Rails member

@jfirebaugh after editing this, I'm not sure why you want to be able to monkeypatch this at all. You can completely resolve your stated use-case by adding a middleware farther out that strips the "garbage" header, and then disable the spoof checking using the option that is provided.

I personally would like to remove the spoof checking, because AFAIK there aren't any proxies that set both Client-Ip and X-Forwarded-For, but I'm sure that's a totally separate PR and discussion. :)

@jfirebaugh

Figuring out the correct IP is the responsibility of the RemoteIp middleware, right? Folks shouldn't have to write a second middleware that deals in the same protocol details as RemoteIp in order to get RemoteIp to work correctly in what is a very common environment.

I'm all for removing the spoof checking as well, but I think there needs to be a configuration option to say which of Client-Ip and X-Forwarded-For is accurate.

@indirect
Ruby on Rails member
@indirect
Ruby on Rails member

Okay. I have coded and documented the fuck out of this thing. :shipit: /cc @steveklabnik

@rafaelfranca
Ruby on Rails member

needs rebase

@indirect
Ruby on Rails member

@rafaelfranca bro, I just rebased this 4 minutes ago. It needs another one?!

@indirect indirect Restore original remote_ip algorithm.
Proxy servers add X-Forwarded-For headers, resulting in a list of IPs. We
remove trusted IP values, and then take the last given value, assuming that
it is the most likely to be the correct, unfaked value. See [1] for a very
thorough discussion of why that is the best option we have at the moment.

[1]: http://blog.gingerlime.com/2012/rails-ip-spoofing-vulnerabilities-and-protection/

Fixes #7979
75dcdbc
@indirect
Ruby on Rails member

Huh, stupid CHANGELOG. :P Rebased! Again! QUICK SOMEBODY PUSH THE MERGE BUTTON

@guilleiguaran guilleiguaran merged commit c79fb2a into rails:master Jan 2, 2013
@steveklabnik
Ruby on Rails member

:metal:

Thanks guys. ❤️

@steveklabnik steveklabnik deleted the steveklabnik:issue_7979 branch Jan 3, 2013
@amoose

o_O

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment