aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorJeremy Kemper <jeremy@bitsweat.net>2008-03-28 21:38:01 +0000
committerJeremy Kemper <jeremy@bitsweat.net>2008-03-28 21:38:01 +0000
commita61b63d42056b119e061f7ebf31985887d569c79 (patch)
tree326e6264702c422002d38235496ada5716137475
parent18049864cbe4bbdfd7eb0e96238877bfe74d79fd (diff)
downloadrails-a61b63d42056b119e061f7ebf31985887d569c79.tar.gz
rails-a61b63d42056b119e061f7ebf31985887d569c79.tar.bz2
rails-a61b63d42056b119e061f7ebf31985887d569c79.zip
Avoid remote_ip spoofing
git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@9124 5ecf4fe2-1ee6-0310-87b1-e25e094e27de
-rw-r--r--actionpack/CHANGELOG2
-rwxr-xr-xactionpack/lib/action_controller/request.rb41
-rw-r--r--actionpack/test/controller/request_test.rb25
3 files changed, 53 insertions, 15 deletions
diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG
index 40921b8526..5dab228866 100644
--- a/actionpack/CHANGELOG
+++ b/actionpack/CHANGELOG
@@ -1,5 +1,7 @@
*SVN*
+* Avoid remote_ip spoofing. [Brian Candler]
+
* Added support for regexp flags like ignoring case in the :requirements part of routes declarations #11421 [NeilW]
* Fixed that ActionController::Base#read_multipart would fail if boundary was exactly 10240 bytes #10886 [ariejan]
diff --git a/actionpack/lib/action_controller/request.rb b/actionpack/lib/action_controller/request.rb
index 4889d296ef..745161def8 100755
--- a/actionpack/lib/action_controller/request.rb
+++ b/actionpack/lib/action_controller/request.rb
@@ -124,26 +124,41 @@ module ActionController
end
alias xhr? :xml_http_request?
+ # Which IP addresses are "trusted proxies" that can be stripped from
+ # the right-hand-side of X-Forwarded-For
+ TRUSTED_PROXIES = /^127\.0\.0\.1$|^(10|172\.(1[6-9]|2[0-9]|30|31)|192\.168)\./i
+
# Determine originating IP address. REMOTE_ADDR is the standard
# but will fail if the user is behind a proxy. HTTP_CLIENT_IP and/or
- # HTTP_X_FORWARDED_FOR are set by proxies so check for these before
- # falling back to REMOTE_ADDR. HTTP_X_FORWARDED_FOR may be a comma-
- # delimited list in the case of multiple chained proxies; the first is
- # the originating IP.
- #
- # Security note: do not use if IP spoofing is a concern for your
- # application. Since remote_ip checks HTTP headers for addresses forwarded
- # by proxies, the client may send any IP. remote_addr can't be spoofed but
- # also doesn't work behind a proxy, since it's always the proxy's IP.
+ # HTTP_X_FORWARDED_FOR are set by proxies so check for these if
+ # REMOTE_ADDR is a proxy. HTTP_X_FORWARDED_FOR may be a comma-
+ # delimited list in the case of multiple chained proxies; the last
+ # address which is not trusted is the originating IP.
+
def remote_ip
- return @env['HTTP_CLIENT_IP'] if @env.include? 'HTTP_CLIENT_IP'
+ if TRUSTED_PROXIES !~ @env['REMOTE_ADDR']
+ return @env['REMOTE_ADDR']
+ end
+
+ if @env.include? 'HTTP_CLIENT_IP'
+ if @env.include? 'HTTP_X_FORWARDED_FOR'
+ # We don't know which came from the proxy, and which from the user
+ raise ActionControllerError.new(<<EOM)
+IP spoofing attack?!
+HTTP_CLIENT_IP=#{@env['HTTP_CLIENT_IP'].inspect}
+HTTP_X_FORWARDED_FOR=#{@env['HTTP_X_FORWARDED_FOR'].inspect}
+EOM
+ end
+ return @env['HTTP_CLIENT_IP']
+ end
if @env.include? 'HTTP_X_FORWARDED_FOR' then
- remote_ips = @env['HTTP_X_FORWARDED_FOR'].split(',').reject do |ip|
- ip.strip =~ /^unknown$|^(10|172\.(1[6-9]|2[0-9]|30|31)|192\.168)\./i
+ remote_ips = @env['HTTP_X_FORWARDED_FOR'].split(',')
+ while remote_ips.size > 1 && TRUSTED_PROXIES =~ remote_ips.last.strip
+ remote_ips.pop
end
- return remote_ips.first.strip unless remote_ips.empty?
+ return remote_ips.last.strip
end
@env['REMOTE_ADDR']
diff --git a/actionpack/test/controller/request_test.rb b/actionpack/test/controller/request_test.rb
index 2f72f9017a..4d645f56e0 100644
--- a/actionpack/test/controller/request_test.rb
+++ b/actionpack/test/controller/request_test.rb
@@ -13,9 +13,17 @@ class RequestTest < Test::Unit::TestCase
assert_equal '1.2.3.4', @request.remote_ip
@request.env['HTTP_CLIENT_IP'] = '2.3.4.5'
+ assert_equal '1.2.3.4', @request.remote_ip
+
+ @request.remote_addr = '192.168.0.1'
assert_equal '2.3.4.5', @request.remote_ip
@request.env.delete 'HTTP_CLIENT_IP'
+ @request.remote_addr = '1.2.3.4'
+ @request.env['HTTP_X_FORWARDED_FOR'] = '3.4.5.6'
+ assert_equal '1.2.3.4', @request.remote_ip
+
+ @request.remote_addr = '127.0.0.1'
@request.env['HTTP_X_FORWARDED_FOR'] = '3.4.5.6'
assert_equal '3.4.5.6', @request.remote_ip
@@ -35,10 +43,23 @@ class RequestTest < Test::Unit::TestCase
assert_equal '3.4.5.6', @request.remote_ip
@request.env['HTTP_X_FORWARDED_FOR'] = '127.0.0.1,3.4.5.6'
- assert_equal '127.0.0.1', @request.remote_ip
+ assert_equal '3.4.5.6', @request.remote_ip
@request.env['HTTP_X_FORWARDED_FOR'] = 'unknown,192.168.0.1'
- assert_equal '1.2.3.4', @request.remote_ip
+ assert_equal 'unknown', @request.remote_ip
+
+ @request.env['HTTP_X_FORWARDED_FOR'] = '9.9.9.9, 3.4.5.6, 10.0.0.1, 172.31.4.4'
+ assert_equal '3.4.5.6', @request.remote_ip
+
+ @request.env['HTTP_CLIENT_IP'] = '8.8.8.8'
+ e = assert_raises(ActionController::ActionControllerError) {
+ @request.remote_ip
+ }
+ assert_match /IP spoofing attack/, e.message
+ assert_match /HTTP_X_FORWARDED_FOR="9.9.9.9, 3.4.5.6, 10.0.0.1, 172.31.4.4"/, e.message
+ assert_match /HTTP_CLIENT_IP="8.8.8.8"/, e.message
+
+ @request.env.delete 'HTTP_CLIENT_IP'
@request.env.delete 'HTTP_X_FORWARDED_FOR'
end