commit bf7ec9eeb61908bd3bc39efc68281638504cd3cf Author: Isis Lovecruft <isis@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):