[tor-commits] [bridgedb/master] Add config option to skip loopback addresses in X-Forwarded-For parsing.

isis at torproject.org isis at torproject.org
Tue Feb 13 22:26:49 UTC 2018


commit bf7ec9eeb61908bd3bc39efc68281638504cd3cf
Author: Isis Lovecruft <isis at torproject.org>
Date:   Tue Feb 13 21:11:21 2018 +0000

    Add config option to skip loopback addresses in X-Forwarded-For parsing.
---
 bridgedb.conf                                  |  5 ++++
 bridgedb/distributors/common/http.py           | 24 +++++++++++++----
 bridgedb/distributors/moat/server.py           | 36 +++++++++++++++++++-------
 bridgedb/parse/addr.py                         | 22 ++++++++++++++++
 bridgedb/test/moat_helpers.py                  |  3 +++
 bridgedb/test/test_distributors_common_http.py | 18 +++++++++++++
 bridgedb/test/test_distributors_moat_server.py |  1 +
 7 files changed, 94 insertions(+), 15 deletions(-)

diff --git a/bridgedb.conf b/bridgedb.conf
index 724c802..7700917 100644
--- a/bridgedb.conf
+++ b/bridgedb.conf
@@ -345,6 +345,11 @@ MOAT_HTTP_PORT = None
 # the *last* entry from its X-Forwarded-For header as the client's IP.
 MOAT_USE_IP_FROM_FORWARDED_HEADER = True
 
+# If True, there is a misconfigured proxy relaying incoming messages
+# to us: take the *last* entry in the X-Forwarded-For header which is
+# not a loopback address (127.0.0.1/8) as the client's IP.
+MOAT_SKIP_LOOPBACK_ADDRESSES = True
+
 # How many clusters do we group IPs in when distributing bridges based on IP?
 # Note that if PROXY_LIST_FILES is set (below), what we actually do here
 # is use one higher than the number here, and the extra cluster is used
diff --git a/bridgedb/distributors/common/http.py b/bridgedb/distributors/common/http.py
index 86c26b8..3bd7b41 100644
--- a/bridgedb/distributors/common/http.py
+++ b/bridgedb/distributors/common/http.py
@@ -21,6 +21,7 @@ import logging
 import os
 
 from bridgedb.parse.addr import isIPAddress
+from bridgedb.parse.addr import isLoopback
 
 
 #: The fully-qualified domain name for any and all web servers we run.
@@ -54,7 +55,7 @@ def getFQDN():
     """
     return SERVER_PUBLIC_FQDN
 
-def getClientIP(request, useForwardedHeader=False):
+def getClientIP(request, useForwardedHeader=False, skipLoopback=False):
     """Get the client's IP address from the ``'X-Forwarded-For:'``
     header, or from the :api:`request <twisted.web.server.Request>`.
 
@@ -62,6 +63,9 @@ def getClientIP(request, useForwardedHeader=False):
     :param request: A ``Request`` for a :api:`twisted.web.resource.Resource`.
     :param bool useForwardedHeader: If ``True``, attempt to get the client's
         IP address from the ``'X-Forwarded-For:'`` header.
+    :param bool skipLoopback: If ``True``, and ``useForwardedHeader`` is
+        also ``True``, then skip any loopback addresses (127.0.0.1/8) when
+        parsing the X-Forwarded-For header.
     :rtype: ``None`` or :any:`str`
     :returns: The client's IP address, if it was obtainable.
     """
@@ -70,10 +74,20 @@ def getClientIP(request, useForwardedHeader=False):
     if useForwardedHeader:
         header = request.getHeader("X-Forwarded-For")
         if header:
-            ip = header.split(",")[-1].strip()
-            if not isIPAddress(ip):
-                logging.warn("Got weird X-Forwarded-For value %r" % header)
-                ip = None
+            index = -1
+            ip = header.split(",")[index].strip()
+            if skipLoopback:
+                logging.info(("Parsing X-Forwarded-For again, ignoring "
+                              "loopback addresses..."))
+                while isLoopback(ip):
+                    index -= 1
+                    ip = header.split(",")[index].strip()
+            if not skipLoopback and isLoopback(ip):
+               logging.warn("Accepting loopback address: %s" % ip)
+            else:
+                if not isIPAddress(ip):
+                    logging.warn("Got weird X-Forwarded-For value %r" % header)
+                    ip = None
     else:
         ip = request.getClientIP()
 
diff --git a/bridgedb/distributors/moat/server.py b/bridgedb/distributors/moat/server.py
index 4137f51..509d471 100644
--- a/bridgedb/distributors/moat/server.py
+++ b/bridgedb/distributors/moat/server.py
@@ -134,9 +134,17 @@ class JsonAPIResource(resource.Resource):
     """A resource which conforms to the `JSON API spec <http://jsonapi.org/>`__.
     """
 
-    def __init__(self, useForwardedHeader=True):
+    def __init__(self, useForwardedHeader=True, skipLoopback=False):
+        """Create a JSON API resource, containing either error(s) or data.
+
+        :param bool useForwardedHeader: If ``True``, obtain the client's IP
+            address from the ``X-Forwarded-For`` HTTP header.
+        :param bool skipLoopback: Skip loopback addresses when parsing the
+            X-Forwarded-For header.
+        """
         resource.Resource.__init__(self)
         self.useForwardedHeader = useForwardedHeader
+        self.skipLoopback = skipLoopback
 
     def getClientIP(self, request):
         """Get the client's IP address from the ``'X-Forwarded-For:'``
@@ -148,7 +156,7 @@ class JsonAPIResource(resource.Resource):
         :rtype: ``None`` or :any:`str`
         :returns: The client's IP address, if it was obtainable.
         """
-        return getClientIP(request, self.useForwardedHeader)
+        return getClientIP(request, self.useForwardedHeader, self.skipLoopback)
 
     def formatDataForResponse(self, data, request):
         """Format a dictionary of ``data`` into JSON and add necessary response
@@ -233,8 +241,8 @@ class CustomErrorHandlingResource(resource.Resource):
 class JsonAPIDataResource(JsonAPIResource):
     """A resource which returns some JSON API data."""
 
-    def __init__(self, useForwardedHeader=True):
-        JsonAPIResource.__init__(self, useForwardedHeader)
+    def __init__(self, useForwardedHeader=True, skipLoopback=False):
+        JsonAPIResource.__init__(self, useForwardedHeader, skipLoopback)
 
     def checkRequestHeaders(self, request):
         """The JSON API specification requires servers to respond with certain HTTP
@@ -288,8 +296,8 @@ class CaptchaResource(JsonAPIDataResource):
     """A CAPTCHA."""
 
     def __init__(self, hmacKey=None, publicKey=None, secretKey=None,
-                 useForwardedHeader=True):
-        JsonAPIDataResource.__init__(self, useForwardedHeader)
+                 useForwardedHeader=True, skipLoopback=False):
+        JsonAPIDataResource.__init__(self, useForwardedHeader, skipLoopback)
         self.hmacKey = hmacKey
         self.publicKey = publicKey
         self.secretKey = secretKey
@@ -301,7 +309,7 @@ class CaptchaFetchResource(CaptchaResource):
     isLeaf = True
 
     def __init__(self, hmacKey=None, publicKey=None, secretKey=None,
-                 captchaDir="captchas", useForwardedHeader=True):
+                 captchaDir="captchas", useForwardedHeader=True, skipLoopback=False):
         """DOCDOC
 
         :param bytes hmacKey: The master HMAC key, used for validating CAPTCHA
@@ -319,6 +327,8 @@ class CaptchaFetchResource(CaptchaResource):
         :param str captchaDir: The directory where the cached CAPTCHA images
         :param bool useForwardedHeader: If ``True``, obtain the client's IP
             address from the ``X-Forwarded-For`` HTTP header.
+        :param bool skipLoopback: Skip loopback addresses when parsing the
+            X-Forwarded-For header.
         """
         CaptchaResource.__init__(self, hmacKey, publicKey, secretKey,
                                  useForwardedHeader)
@@ -477,7 +487,7 @@ class CaptchaCheckResource(CaptchaResource):
 
     def __init__(self, distributor, schedule, N=1,
                  hmacKey=None, publicKey=None, secretKey=None,
-                 useForwardedHeader=True):
+                 useForwardedHeader=True, skipLoopback=False):
         """Create a new resource for checking CAPTCHA solutions and returning
         bridges to a client.
 
@@ -490,6 +500,8 @@ class CaptchaCheckResource(CaptchaResource):
         :param int N: The number of bridges to hand out per query.
         :param bool useForwardedHeader: Whether or not we should use the the
             X-Forwarded-For header instead of the source IP address.
+        :param bool skipLoopback: Skip loopback addresses when parsing the
+            X-Forwarded-For header.
         """
         CaptchaResource.__init__(self, hmacKey, publicKey, secretKey,
                                  useForwardedHeader)
@@ -748,6 +760,7 @@ def addMoatServer(config, distributor):
              MOAT_BRIDGES_PER_ANSWER
              MOAT_TRANSPORT_PREFERENCE_LIST
              MOAT_USE_IP_FROM_FORWARDED_HEADER
+             MOAT_SKIP_LOOPBACK_ADDRESSES
              MOAT_ROTATION_PERIOD
              MOAT_GIMP_CAPTCHA_HMAC_KEYFILE
              MOAT_GIMP_CAPTCHA_RSA_KEYFILE
@@ -760,6 +773,7 @@ def addMoatServer(config, distributor):
     captcha = None
     fwdHeaders = config.MOAT_USE_IP_FROM_FORWARDED_HEADER
     numBridges = config.MOAT_BRIDGES_PER_ANSWER
+    skipLoopback = config.MOAT_SKIP_LOOPBACK_ADDRESSES
 
     logging.info("Starting moat servers...")
 
@@ -785,9 +799,11 @@ def addMoatServer(config, distributor):
     meek = CustomErrorHandlingResource()
     moat = CustomErrorHandlingResource()
     fetch = CaptchaFetchResource(hmacKey, publicKey, secretKey,
-                                 config.GIMP_CAPTCHA_DIR, fwdHeaders)
+                                 config.GIMP_CAPTCHA_DIR,
+                                 fwdHeaders, skipLoopback)
     check = CaptchaCheckResource(distributor, sched, numBridges,
-                                 hmacKey, publicKey, secretKey, fwdHeaders)
+                                 hmacKey, publicKey, secretKey,
+                                 fwdHeaders, skipLoopback)
 
     moat.putChild("fetch", fetch)
     moat.putChild("check", check)
diff --git a/bridgedb/parse/addr.py b/bridgedb/parse/addr.py
index bf5a8a5..90bc1d5 100644
--- a/bridgedb/parse/addr.py
+++ b/bridgedb/parse/addr.py
@@ -435,6 +435,28 @@ def isValidIP(ip):
         return False
     return True
 
+def isLoopback(ip):
+    """Determine if ``ip`` is a loopback or localhost address (127.0.0.1/8).
+
+    :type: ``str`` or ``ipaddr.IPAddress``
+    :param ip: An IP address.
+    :rtype: bool
+    :returns: ``True`` if the IP was a loopback or localhost address; ``False``
+        otherwise.
+    """
+    try:
+        if isinstance(ip, basestring):
+            ip = ipaddr.IPAddress(ip)
+
+        if ip.is_loopback:
+            return True
+        else:
+            return False
+    except Exception as err:
+        logging.debug("Error attempting to parse IP address: %s", ip)
+
+    return False
+
 def normalizeEmail(emailaddr, domainmap, domainrules, ignorePlus=True):
     """Normalise an email address according to the processing rules for its
     canonical originating domain.
diff --git a/bridgedb/test/moat_helpers.py b/bridgedb/test/moat_helpers.py
index c7f7718..07b44a0 100644
--- a/bridgedb/test/moat_helpers.py
+++ b/bridgedb/test/moat_helpers.py
@@ -42,6 +42,7 @@ MOAT_HTTPS_PORT = None
 MOAT_HTTP_IP = None
 MOAT_HTTP_PORT = None
 MOAT_USE_IP_FROM_FORWARDED_HEADER = True
+MOAT_SKIP_LOOPBACK_ADDRESSES = True
 MOAT_N_IP_CLUSTERS = 4
 MOAT_ROTATION_PERIOD = "3 hours"
 MOAT_GIMP_CAPTCHA_HMAC_KEYFILE = 'moat_captcha_hmac_key'
@@ -63,6 +64,7 @@ MOAT_HTTP_PORT = %r
 MOAT_BRIDGES_PER_ANSWER = %r
 MOAT_TRANSPORT_PREFERENCE_LIST = %r
 MOAT_USE_IP_FROM_FORWARDED_HEADER = %r
+MOAT_SKIP_LOOPBACK_ADDRESSES = %r
 MOAT_N_IP_CLUSTERS = %r
 MOAT_ROTATION_PERIOD = %r
 MOAT_GIMP_CAPTCHA_HMAC_KEYFILE = %r
@@ -82,6 +84,7 @@ MOAT_GIMP_CAPTCHA_RSA_KEYFILE = %r
        MOAT_BRIDGES_PER_ANSWER,
        MOAT_TRANSPORT_PREFERENCE_LIST,
        MOAT_USE_IP_FROM_FORWARDED_HEADER,
+       MOAT_SKIP_LOOPBACK_ADDRESSES,
        MOAT_N_IP_CLUSTERS,
        MOAT_ROTATION_PERIOD,
        MOAT_GIMP_CAPTCHA_HMAC_KEYFILE,
diff --git a/bridgedb/test/test_distributors_common_http.py b/bridgedb/test/test_distributors_common_http.py
index 60759e4..ff3da41 100644
--- a/bridgedb/test/test_distributors_common_http.py
+++ b/bridgedb/test/test_distributors_common_http.py
@@ -91,6 +91,24 @@ class GetClientIPTests(unittest.TestCase):
         clientIP = server.getClientIP(request, useForwardedHeader=True)
         self.assertEqual(clientIP, None)
 
+    def test_getClientIP_XForwardedFor_skip_loopback(self):
+        request = self.createRequestWithIPs()
+        request.headers.update({'x-forwarded-for': '3.3.3.3, 127.0.0.1'})
+        clientIP = server.getClientIP(request, useForwardedHeader=True, skipLoopback=True)
+        self.assertEqual(clientIP, '3.3.3.3')
+
+    def test_getClientIP_XForwardedFor_skip_loopback_multiple(self):
+        request = self.createRequestWithIPs()
+        request.headers.update({'x-forwarded-for': '3.3.3.3, 127.0.0.6, 127.0.0.1'})
+        clientIP = server.getClientIP(request, useForwardedHeader=True, skipLoopback=True)
+        self.assertEqual(clientIP, '3.3.3.3')
+
+    def test_getClientIP_XForwardedFor_no_skip_loopback(self):
+        request = self.createRequestWithIPs()
+        request.headers.update({'x-forwarded-for': '3.3.3.3, 127.0.0.1'})
+        clientIP = server.getClientIP(request, useForwardedHeader=True, skipLoopback=False)
+        self.assertEqual(clientIP, '127.0.0.1')
+
     def test_getClientIP_fromRequest(self):
         """getClientIP() should return the IP address from the request instance
         when ``useForwardedHeader=False``.
diff --git a/bridgedb/test/test_distributors_moat_server.py b/bridgedb/test/test_distributors_moat_server.py
index f29f224..33069ef 100644
--- a/bridgedb/test/test_distributors_moat_server.py
+++ b/bridgedb/test/test_distributors_moat_server.py
@@ -337,6 +337,7 @@ class CaptchaFetchResourceTests(unittest.TestCase):
         self.root.putChild(self.pagename, self.resource)
 
         self.make_captcha_directory()
+        server.setPreferredTransports(['obfs4', 'vanilla'])
 
     def make_captcha_directory(self):
         if not os.path.isdir(self.captchaDir):





More information about the tor-commits mailing list