Merge pull request #804 from dicksonlabs/fix_forwarded_for

Improve request.location(), add request.paranoid_location()

README.md

@@ -123,12 +123,14 @@ The exact code will vary depending on the method you use for your geocodable str
 Request Geocoding by IP Address
 -------------------------------
 
-Geocoder adds a `location` method to the standard `Rack::Request` object so you can easily look up the location of any HTTP request by IP address. For example, in a Rails controller or a Sinatra app:
+Geocoder adds `location` and `paranoid_location` methods to the standard `Rack::Request` object so you can easily look up the location of any HTTP request by IP address. For example, in a Rails controller or a Sinatra app:
 
     # returns Geocoder::Result object
     result = request.location
 
-Note that this will usually return `nil` in your test and development environments because things like "localhost" and "0.0.0.0" are not an Internet IP addresses.
+**The `location` method is vulnerable to trivial IP address spoofing via HTTP headers.**  If that's a problem for your application, use `paranoid_location` instead, but be aware that `paranoid_location` will *not* try to trace a request's originating IP through proxy headers; you will instead get the location of the last proxy the request passed through, if any (excepting any proxies you have explicitly whitelisted in your Rack config).
+
+Note that these methods will usually return `nil` in your test and development environments because things like "localhost" and "0.0.0.0" are not an Internet IP addresses.
 
 See _Advanced Geocoding_ below for more information about `Geocoder::Result` objects.
 

lib/geocoder/request.rb

 module Geocoder
   module Request
 
+    # The location() method is vulnerable to trivial IP spoofing.
+    #   Don't use it in authorization/authentication code, or any
+    #   other security-sensitive application.  Use paranoid_location
+    #   instead.
     def location
-      @location ||= begin
-        detected_ip = env['HTTP_X_REAL_IP'] || (
-          env['HTTP_X_FORWARDED_FOR'] &&
-          env['HTTP_X_FORWARDED_FOR'].split(",").first.strip
-        )
-        detected_ip = IpAddress.new(detected_ip.to_s)
-        if detected_ip.valid? and !detected_ip.loopback?
-          real_ip = detected_ip.to_s
-        else
-          real_ip = self.ip
+      @location ||= Geocoder.search(geocoder_spoofable_ip, ip_address: true).first
+    end
+
+    # This paranoid_location() protects you from trivial IP spoofing.
+    #   For requests that go through a proxy that you haven't
+    #   whitelisted as trusted in your Rack config, you will get the
+    #   location for the IP of the last untrusted proxy in the chain,
+    #   not the original client IP.  You WILL NOT get the location
+    #   corresponding to the original client IP for any request sent
+    #   through a non-whitelisted proxy.
+    def paranoid_location
+      @location ||= Geocoder.search(ip, ip_address: true).first
+    end
+
+    # There's a whole zoo of nonstandard headers added by various
+    #   proxy softwares to indicate original client IP.
+    # ANY of these can be trivially spoofed!
+    #   (except REMOTE_ADDR, which should by set by your server,
+    #    and is included at the end as a fallback.
+    # Order does matter: we're following the convention established in
+    #   ActionDispatch::RemoteIp::GetIp::calculate_ip()
+    #   https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/middleware/remote_ip.rb
+    #   where the forwarded_for headers, possibly containing lists,
+    #   are arbitrarily preferred over headers expected to contain a
+    #   single address.
+    GEOCODER_CANDIDATE_HEADERS = ['HTTP_X_FORWARDED_FOR',
+                                  'HTTP_X_FORWARDED',
+                                  'HTTP_FORWARDED_FOR',
+                                  'HTTP_FORWARDED',
+                                  'HTTP_X_CLIENT_IP',
+                                  'HTTP_CLIENT_IP',
+                                  'HTTP_X_REAL_IP',
+                                  'HTTP_X_CLUSTER_CLIENT_IP',
+                                  'REMOTE_ADDR']
+
+    def geocoder_spoofable_ip
+
+      # We could use a more sophisticated IP-guessing algorithm here,
+      # in which we'd try to resolve the use of different headers by
+      # different proxies.  The idea is that by comparing IPs repeated
+      # in different headers, you can sometimes decide which header
+      # was used by a proxy further along in the chain, and thus
+      # prefer the headers used earlier.  However, the gains might not
+      # be worth the performance tradeoff, since this method is likely
+      # to be called on every request in a lot of applications.
+      GEOCODER_CANDIDATE_HEADERS.each do |header|
+        if @env.has_key? header
+          addrs = geocoder_split_ip_addresses(@env[header])
+          addrs = geocoder_reject_trusted_ip_addresses(addrs)
+          return addrs.first if addrs.any?
         end
-        Geocoder.search(real_ip).first
       end
-      @location
+
+      @env['REMOTE_ADDR']
+    end
+
+    private
+
+    def geocoder_split_ip_addresses(ip_addresses)
+      ip_addresses ? ip_addresses.strip.split(/[,\s]+/) : []
+    end
+
+    # use Rack's trusted_proxy?() method to filter out IPs that have
+    #   been configured as trusted; includes private ranges by
+    #   default.  (we don't want every lookup to return the location
+    #   of our own proxy/load balancer)
+    def geocoder_reject_trusted_ip_addresses(ip_addresses)
+      ip_addresses.reject { |ip| trusted_proxy?(ip) }
     end
   end
 end

test/fixtures/freegeoip_74_200_247_60

+{
+  "city": "Nuevo Laredo",
+  "region_code": "TAM",
+  "region_name": "Tamaulipas",
+  "metrocode": "0",
+  "zipcode": "",
+  "country_name": "Mexico",
+  "country_code": "MX",
+  "ip": "74.200.247.60",
+  "latitude": "27.5",
+  "longitude": "-99.517"
+}

test/unit/request_test.rb

@@ -3,33 +3,58 @@ $: << File.join(File.dirname(__FILE__), "..")
 require 'test_helper'
 
 class RequestTest < GeocoderTestCase
-  class MockRequest
+  class MockRequest < Rack::Request
     include Geocoder::Request
-    attr_accessor :env, :ip
-    def initialize(env={}, ip="")
-      @env = env
-      @ip  = ip
+    def initialize(headers={}, ip="")
+      super_env = headers
+      super_env.merge!({'REMOTE_ADDR' => ip}) unless ip == ""
+      super(super_env)
     end
   end
   def test_http_x_real_ip
     req = MockRequest.new({"HTTP_X_REAL_IP" => "74.200.247.59"})
     assert req.location.is_a?(Geocoder::Result::Freegeoip)
   end
+  def test_http_x_client_ip
+    req = MockRequest.new({"HTTP_X_CLIENT_IP" => "74.200.247.59"})
+    assert req.location.is_a?(Geocoder::Result::Freegeoip)
+  end
+  def test_http_x_cluster_client_ip
+    req = MockRequest.new({"HTTP_X_CLUSTER_CLIENT_IP" => "74.200.247.59"})
+    assert req.location.is_a?(Geocoder::Result::Freegeoip)
+  end
   def test_http_x_forwarded_for_without_proxy
     req = MockRequest.new({"HTTP_X_FORWARDED_FOR" => "74.200.247.59"})
     assert req.location.is_a?(Geocoder::Result::Freegeoip)
   end
   def test_http_x_forwarded_for_with_proxy
-    req = MockRequest.new({"HTTP_X_FORWARDED_FOR" => "74.200.247.59, 74.200.247.59"})
+    req = MockRequest.new({"HTTP_X_FORWARDED_FOR" => "74.200.247.59, 74.200.247.60"})
+    assert req.geocoder_spoofable_ip == '74.200.247.59'
+    assert req.ip == '74.200.247.60'
     assert req.location.is_a?(Geocoder::Result::Freegeoip)
+    assert_equal "US", req.location.country_code
+  end
+  def test_paranoid_http_x_forwarded_for_with_proxy
+    req = MockRequest.new({"HTTP_X_FORWARDED_FOR" => "74.200.247.59, 74.200.247.60"})
+    assert req.geocoder_spoofable_ip == '74.200.247.59'
+    assert req.ip == '74.200.247.60'
+    assert req.paranoid_location.is_a?(Geocoder::Result::Freegeoip)
+    assert_equal "MX", req.paranoid_location.country_code
   end
   def test_with_request_ip
     req = MockRequest.new({}, "74.200.247.59")
     assert req.location.is_a?(Geocoder::Result::Freegeoip)
   end
-
   def test_with_loopback_x_forwarded_for
     req = MockRequest.new({"HTTP_X_FORWARDED_FOR" => "127.0.0.1"}, "74.200.247.59")
     assert_equal "US", req.location.country_code
   end
+  def test_http_x_forwarded_for_with_misconfigured_proxies
+    req = MockRequest.new({"HTTP_X_FORWARDED_FOR" => ","}, "74.200.247.59")
+    assert req.location.is_a?(Geocoder::Result::Freegeoip)
+  end
+  def test_non_ip_in_proxy_header
+    req = MockRequest.new({"HTTP_X_FORWARDED_FOR" => "Albequerque NM"})
+    assert req.location.is_a?(Geocoder::Result::Freegeoip)
+  end
 end