From 9c50d8e8335ee27fc44b0a295baa1c460c7afc77 Mon Sep 17 00:00:00 2001 From: Ken Cochrane Date: Tue, 24 Feb 2015 18:16:08 -0500 Subject: [PATCH 1/5] added fixes for issue #32, hopefully this closes the security hole that @mmetince found --- README.md | 17 +++++++++---- defender/config.py | 2 -- defender/tests.py | 47 +++++++++++----------------------- defender/utils.py | 63 ++++++++++++++-------------------------------- setup.py | 2 +- 5 files changed, 46 insertions(+), 85 deletions(-) diff --git a/README.md b/README.md index bff8baa..ed713b3 100644 --- a/README.md +++ b/README.md @@ -19,8 +19,16 @@ Sites using Defender: ===================== - https://hub.docker.com -0.1 Features -============ + +Versions +======== +- 0.2 - security fix for XFF headers +- 0.1.1 - setup.py fix +- 0.1 - initial release + + +Features +======== - Log all login attempts to the database - support for reverse proxies with different headers for IP addresses @@ -255,9 +263,8 @@ These should be defined in your ``settings.py`` file. * ``DEFENDER_LOGIN_FAILURE_LIMIT``: Int: The number of login attempts allowed before a record is created for the failed logins. [Default: ``3``] -* ``DEFENDER_USE_USER_AGENT``: Boolean: If ``True``, lock out / log based on an IP address -AND a user agent. This means requests from different user agents but from -the same IP are treated differently. [Default: ``False``] +* ``DEFENDER_BEHIND_REVERSE_PROXY``: Boolean: Is defender behind a reverse proxy? +[Default: False] * ``DEFENDER_COOLOFF_TIME``: Int: If set, defines a period of inactivity after which old failed login attempts will be forgotten. An integer, will be interpreted as a number of seconds. If ``0``, the locks will not expire. [Default: ``300``] diff --git a/defender/config.py b/defender/config.py index e9a5979..b63ef65 100644 --- a/defender/config.py +++ b/defender/config.py @@ -15,8 +15,6 @@ MOCK_REDIS = get_setting('DEFENDER_MOCK_REDIS', False) # see if the user has overridden the failure limit FAILURE_LIMIT = get_setting('DEFENDER_LOGIN_FAILURE_LIMIT', 3) -USE_USER_AGENT = get_setting('DEFENDER_USE_USER_AGENT', False) - # use a specific username field to retrieve from login POST data USERNAME_FORM_FIELD = get_setting('DEFENDER_USERNAME_FORM_FIELD', 'username') diff --git a/defender/tests.py b/defender/tests.py index 5ec3927..e5b1310 100644 --- a/defender/tests.py +++ b/defender/tests.py @@ -140,7 +140,8 @@ class AccessAttemptTest(DefenderTestCase): def test_valid_login(self): """ Tests a valid login for a real username """ - response = self._login(username=VALID_USERNAME, password=VALID_PASSWORD) + response = self._login(username=VALID_USERNAME, + password=VALID_PASSWORD) self.assertNotContains(response, LOGIN_FORM_KEY, status_code=302) def test_reset_after_valid_login(self): @@ -206,6 +207,7 @@ class AccessAttemptTest(DefenderTestCase): self.assertNotContains(response, LOGIN_FORM_KEY, status_code=302) @patch('defender.config.BEHIND_REVERSE_PROXY', True) + @patch('defender.config.REVERSE_PROXY_HEADER', 'HTTP_X_FORWARDED_FOR') def test_get_ip_reverse_proxy(self): """ Tests if can handle a long user agent """ @@ -344,8 +346,7 @@ class AccessAttemptTest(DefenderTestCase): self.assertIsNotNone(str(AccessAttempt.objects.all()[0])) def test_is_valid_ip(self): - """ Test the is_valid_ip() method - """ + """ Test the is_valid_ip() method """ self.assertEquals(utils.is_valid_ip('192.168.0.1'), True) self.assertEquals(utils.is_valid_ip('130.80.100.24'), True) self.assertEquals(utils.is_valid_ip('8.8.8.8'), True) @@ -353,6 +354,8 @@ class AccessAttemptTest(DefenderTestCase): self.assertEquals(utils.is_valid_ip('fish'), False) self.assertEquals(utils.is_valid_ip(None), False) self.assertEquals(utils.is_valid_ip(''), False) + self.assertEquals(utils.is_valid_ip('0x41.0x41.0x41.0x41'), False) + self.assertEquals(utils.is_valid_ip('192.168.100.34.y'), False) def test_parse_redis_url(self): """ test the parse_redis_url method """ @@ -407,52 +410,28 @@ class AccessAttemptTest(DefenderTestCase): def test_get_ip_address_from_request(self): req = HttpRequest() - req.META['HTTP_X_FORWARDED_FOR'] = '1.2.3.4' + req.META['REMOTE_ADDR'] = '1.2.3.4' ip = utils.get_ip_address_from_request(req) self.assertEqual(ip, '1.2.3.4') req = HttpRequest() - req.META['HTTP_X_FORWARDED_FOR'] = ','.join( - ['192.168.100.23', '1.2.3.4'] - ) + req.META['REMOTE_ADDR'] = '1.2.3.4 ' ip = utils.get_ip_address_from_request(req) self.assertEqual(ip, '1.2.3.4') req = HttpRequest() - req.META['HTTP_X_FORWARDED_FOR'] = '192.168.100.34' + req.META['REMOTE_ADDR'] = '192.168.100.34.y' ip = utils.get_ip_address_from_request(req) self.assertEqual(ip, '127.0.0.1') req = HttpRequest() - req.META['HTTP_X_FORWARDED_FOR'] = '127.0.0.1' - req.META['HTTP_X_REAL_IP'] = '1.2.3.4' + req.META['REMOTE_ADDR'] = 'cat' ip = utils.get_ip_address_from_request(req) - self.assertEqual(ip, '1.2.3.4') + self.assertEqual(ip, '127.0.0.1') req = HttpRequest() - req.META['HTTP_X_FORWARDED_FOR'] = '1.2.3.4' - req.META['HTTP_X_REAL_IP'] = '5.6.7.8' ip = utils.get_ip_address_from_request(req) - self.assertEqual(ip, '1.2.3.4') - - req = HttpRequest() - req.META['HTTP_X_REAL_IP'] = '5.6.7.8' - ip = utils.get_ip_address_from_request(req) - self.assertEqual(ip, '5.6.7.8') - - req = HttpRequest() - req.META['REMOTE_ADDR'] = '1.2.3.4' - ip = utils.get_ip_address_from_request(req) - self.assertEqual(ip, '1.2.3.4') - - req = HttpRequest() - req.META['HTTP_X_FORWARDED_FOR'] = ','.join( - ['127.0.0.1', '192.168.132.98'] - ) - req.META['HTTP_X_REAL_IP'] = '10.0.0.34' - req.META['REMOTE_ADDR'] = '1.2.3.4' - ip = utils.get_ip_address_from_request(req) - self.assertEqual(ip, '1.2.3.4') + self.assertEqual(ip, '127.0.0.1') @patch('defender.config.BEHIND_REVERSE_PROXY', True) @patch('defender.config.REVERSE_PROXY_HEADER', 'HTTP_X_PROXIED') @@ -469,6 +448,8 @@ class AccessAttemptTest(DefenderTestCase): req.META['REMOTE_ADDR'] = '1.2.3.4' self.assertEqual(utils.get_ip(req), '1.2.3.4') + @patch('defender.config.BEHIND_REVERSE_PROXY', True) + @patch('defender.config.REVERSE_PROXY_HEADER', 'HTTP_X_REAL_IP') def test_get_user_attempts(self): ip_attempts = random.randint(3, 12) username_attempts = random.randint(3, 12) diff --git a/defender/utils.py b/defender/utils.py index f0d66df..9d7bc87 100644 --- a/defender/utils.py +++ b/defender/utils.py @@ -1,5 +1,6 @@ import logging import socket +import re from django.http import HttpResponse from django.http import HttpResponseRedirect @@ -17,63 +18,37 @@ log = logging.getLogger(__name__) def is_valid_ip(ip_address): """ Check Validity of an IP address """ - valid = True - try: - socket.inet_aton(ip_address.strip()) - except (socket.error, AttributeError): - valid = False - return valid + if not ip_address: + return False + ip_address = ip_address.strip() + ipv4_re = r'^(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$' + ipv6_re = r'\[[0-9a-f:\.]+\]' + ipv4_regex = re.compile(ipv4_re) + ipv6_regex = re.compile(ipv6_re) + + if ipv4_regex.match(ip_address) or ipv6_regex.match(ip_address): + return True + return False def get_ip_address_from_request(request): """ Makes the best attempt to get the client's real IP or return the loopback """ - PRIVATE_IPS_PREFIX = ('10.', '172.', '192.', '127.') - ip_address = '' - x_forwarded_for = request.META.get('HTTP_X_FORWARDED_FOR', '') - if x_forwarded_for and ',' not in x_forwarded_for: - if not x_forwarded_for.startswith(PRIVATE_IPS_PREFIX) and is_valid_ip( - x_forwarded_for): - ip_address = x_forwarded_for.strip() - else: - ips = [ip.strip() for ip in x_forwarded_for.split(',')] - for ip in ips: - if ip.startswith(PRIVATE_IPS_PREFIX): - continue - elif not is_valid_ip(ip): - continue - else: - ip_address = ip - break - if not ip_address: - x_real_ip = request.META.get('HTTP_X_REAL_IP', '') - if x_real_ip: - if not x_real_ip.startswith(PRIVATE_IPS_PREFIX) and is_valid_ip( - x_real_ip): - ip_address = x_real_ip.strip() - if not ip_address: - remote_addr = request.META.get('REMOTE_ADDR', '') - if remote_addr: - if not remote_addr.startswith(PRIVATE_IPS_PREFIX) and is_valid_ip( - remote_addr): - ip_address = remote_addr.strip() - if remote_addr.startswith(PRIVATE_IPS_PREFIX) and is_valid_ip( - remote_addr): - ip_address = remote_addr.strip() - if not ip_address: - ip_address = '127.0.0.1' - return ip_address + remote_addr = request.META.get('REMOTE_ADDR', '') + if remote_addr and is_valid_ip(remote_addr): + return remote_addr.strip() + return '127.0.0.1' def get_ip(request): """ get the ip address from the request """ - if not config.BEHIND_REVERSE_PROXY: - ip = get_ip_address_from_request(request) - else: + if config.BEHIND_REVERSE_PROXY: ip = request.META.get(config.REVERSE_PROXY_HEADER, '') ip = ip.split(",", 1)[0].strip() if ip == '': ip = request.META.get('REMOTE_ADDR', '') + else: + ip = get_ip_address_from_request(request) return ip diff --git a/setup.py b/setup.py index 9977833..ce0b052 100644 --- a/setup.py +++ b/setup.py @@ -6,7 +6,7 @@ except ImportError: from distutils.core import setup -version = '0.1.1' +version = '0.2' setup(name='django-defender', version=version, From 2f6afbdb6eee70d84fb7728e026258240c5ecaa8 Mon Sep 17 00:00:00 2001 From: Ken Cochrane Date: Tue, 24 Feb 2015 21:52:10 -0500 Subject: [PATCH 2/5] added ipv6 addresses to the test, and updated the ipv6 regex to something that worked better --- defender/tests.py | 6 ++++++ defender/utils.py | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/defender/tests.py b/defender/tests.py index e5b1310..8cfa1ed 100644 --- a/defender/tests.py +++ b/defender/tests.py @@ -356,6 +356,12 @@ class AccessAttemptTest(DefenderTestCase): self.assertEquals(utils.is_valid_ip(''), False) self.assertEquals(utils.is_valid_ip('0x41.0x41.0x41.0x41'), False) self.assertEquals(utils.is_valid_ip('192.168.100.34.y'), False) + self.assertEquals( + utils.is_valid_ip('2001:0db8:85a3:0000:0000:8a2e:0370:7334'), True) + self.assertEquals( + utils.is_valid_ip('2001:db8:85a3:0:0:8a2e:370:7334'), True) + self.assertEquals( + utils.is_valid_ip('2001:db8:85a3::8a2e:370:7334'), True) def test_parse_redis_url(self): """ test the parse_redis_url method """ diff --git a/defender/utils.py b/defender/utils.py index 9d7bc87..69ca614 100644 --- a/defender/utils.py +++ b/defender/utils.py @@ -22,7 +22,7 @@ def is_valid_ip(ip_address): return False ip_address = ip_address.strip() ipv4_re = r'^(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$' - ipv6_re = r'\[[0-9a-f:\.]+\]' + ipv6_re = r'^(?:(?:[0-9A-Fa-f]{1,4}:){6}(?:[0-9A-Fa-f]{1,4}:[0-9A-Fa-f]{1,4}|(?:(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\\.){3}(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5]))|::(?:[0-9A-Fa-f]{1,4}:){5}(?:[0-9A-Fa-f]{1,4}:[0-9A-Fa-f]{1,4}|(?:(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\\.){3}(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5]))|(?:[0-9A-Fa-f]{1,4})?::(?:[0-9A-Fa-f]{1,4}:){4}(?:[0-9A-Fa-f]{1,4}:[0-9A-Fa-f]{1,4}|(?:(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\\.){3}(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5]))|(?:[0-9A-Fa-f]{1,4}:[0-9A-Fa-f]{1,4})?::(?:[0-9A-Fa-f]{1,4}:){3}(?:[0-9A-Fa-f]{1,4}:[0-9A-Fa-f]{1,4}|(?:(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\\.){3}(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5]))|(?:(?:[0-9A-Fa-f]{1,4}:){,2}[0-9A-Fa-f]{1,4})?::(?:[0-9A-Fa-f]{1,4}:){2}(?:[0-9A-Fa-f]{1,4}:[0-9A-Fa-f]{1,4}|(?:(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\\.){3}(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5]))|(?:(?:[0-9A-Fa-f]{1,4}:){,3}[0-9A-Fa-f]{1,4})?::[0-9A-Fa-f]{1,4}:(?:[0-9A-Fa-f]{1,4}:[0-9A-Fa-f]{1,4}|(?:(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\\.){3}(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5]))|(?:(?:[0-9A-Fa-f]{1,4}:){,4}[0-9A-Fa-f]{1,4})?::(?:[0-9A-Fa-f]{1,4}:[0-9A-Fa-f]{1,4}|(?:(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\\.){3}(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5]))|(?:(?:[0-9A-Fa-f]{1,4}:){,5}[0-9A-Fa-f]{1,4})?::[0-9A-Fa-f]{1,4}|(?:(?:[0-9A-Fa-f]{1,4}:){,6}[0-9A-Fa-f]{1,4})?::)$' ipv4_regex = re.compile(ipv4_re) ipv6_regex = re.compile(ipv6_re) From fd4f58a20c39f82d701b47694b2c4d6524e83e8b Mon Sep 17 00:00:00 2001 From: Ken Cochrane Date: Tue, 24 Feb 2015 22:02:06 -0500 Subject: [PATCH 3/5] took marcus's advice and used the built in django validator --- defender/tests.py | 4 ++++ defender/utils.py | 15 ++++++--------- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/defender/tests.py b/defender/tests.py index 8cfa1ed..bc73e63 100644 --- a/defender/tests.py +++ b/defender/tests.py @@ -362,6 +362,10 @@ class AccessAttemptTest(DefenderTestCase): utils.is_valid_ip('2001:db8:85a3:0:0:8a2e:370:7334'), True) self.assertEquals( utils.is_valid_ip('2001:db8:85a3::8a2e:370:7334'), True) + self.assertEquals( + utils.is_valid_ip('::ffff:192.0.2.128'), True) + self.assertEquals( + utils.is_valid_ip('::ffff:8.8.8.8'), True) def test_parse_redis_url(self): """ test the parse_redis_url method """ diff --git a/defender/utils.py b/defender/utils.py index 69ca614..11c51d3 100644 --- a/defender/utils.py +++ b/defender/utils.py @@ -1,11 +1,11 @@ import logging -import socket -import re from django.http import HttpResponse from django.http import HttpResponseRedirect from django.shortcuts import render_to_response from django.template import RequestContext +from django.core.validators import validate_ipv46_address +from django.core.exceptions import ValidationError from .connection import get_redis_connection from . import config @@ -21,14 +21,11 @@ def is_valid_ip(ip_address): if not ip_address: return False ip_address = ip_address.strip() - ipv4_re = r'^(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$' - ipv6_re = r'^(?:(?:[0-9A-Fa-f]{1,4}:){6}(?:[0-9A-Fa-f]{1,4}:[0-9A-Fa-f]{1,4}|(?:(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\\.){3}(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5]))|::(?:[0-9A-Fa-f]{1,4}:){5}(?:[0-9A-Fa-f]{1,4}:[0-9A-Fa-f]{1,4}|(?:(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\\.){3}(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5]))|(?:[0-9A-Fa-f]{1,4})?::(?:[0-9A-Fa-f]{1,4}:){4}(?:[0-9A-Fa-f]{1,4}:[0-9A-Fa-f]{1,4}|(?:(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\\.){3}(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5]))|(?:[0-9A-Fa-f]{1,4}:[0-9A-Fa-f]{1,4})?::(?:[0-9A-Fa-f]{1,4}:){3}(?:[0-9A-Fa-f]{1,4}:[0-9A-Fa-f]{1,4}|(?:(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\\.){3}(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5]))|(?:(?:[0-9A-Fa-f]{1,4}:){,2}[0-9A-Fa-f]{1,4})?::(?:[0-9A-Fa-f]{1,4}:){2}(?:[0-9A-Fa-f]{1,4}:[0-9A-Fa-f]{1,4}|(?:(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\\.){3}(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5]))|(?:(?:[0-9A-Fa-f]{1,4}:){,3}[0-9A-Fa-f]{1,4})?::[0-9A-Fa-f]{1,4}:(?:[0-9A-Fa-f]{1,4}:[0-9A-Fa-f]{1,4}|(?:(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\\.){3}(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5]))|(?:(?:[0-9A-Fa-f]{1,4}:){,4}[0-9A-Fa-f]{1,4})?::(?:[0-9A-Fa-f]{1,4}:[0-9A-Fa-f]{1,4}|(?:(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\\.){3}(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5]))|(?:(?:[0-9A-Fa-f]{1,4}:){,5}[0-9A-Fa-f]{1,4})?::[0-9A-Fa-f]{1,4}|(?:(?:[0-9A-Fa-f]{1,4}:){,6}[0-9A-Fa-f]{1,4})?::)$' - ipv4_regex = re.compile(ipv4_re) - ipv6_regex = re.compile(ipv6_re) - - if ipv4_regex.match(ip_address) or ipv6_regex.match(ip_address): + try: + validate_ipv46_address(ip_address) return True - return False + except ValidationError: + return False def get_ip_address_from_request(request): From ea7a8cde06997d8245aaffbf95f656e9b49441ec Mon Sep 17 00:00:00 2001 From: Ken Cochrane Date: Wed, 25 Feb 2015 10:03:05 -0500 Subject: [PATCH 4/5] bumped the django versions on travis, and added a fix to get_ip() --- .travis.yml | 6 +++--- defender/utils.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.travis.yml b/.travis.yml index 1ca8a1e..c268f5e 100644 --- a/.travis.yml +++ b/.travis.yml @@ -8,8 +8,8 @@ python: - "pypy" env: - - DJANGO=Django==1.6.9 - - DJANGO=Django==1.7.2 + - DJANGO=Django==1.6.10 + - DJANGO=Django==1.7.5 services: - redis-server @@ -28,7 +28,7 @@ script: matrix: exclude: - python: "2.6" - env: DJANGO=Django==1.7.2 + env: DJANGO=Django==1.7.5 after_success: - coveralls --verbose diff --git a/defender/utils.py b/defender/utils.py index 11c51d3..fb21264 100644 --- a/defender/utils.py +++ b/defender/utils.py @@ -43,7 +43,7 @@ def get_ip(request): ip = request.META.get(config.REVERSE_PROXY_HEADER, '') ip = ip.split(",", 1)[0].strip() if ip == '': - ip = request.META.get('REMOTE_ADDR', '') + ip = get_ip_address_from_request(request) else: ip = get_ip_address_from_request(request) return ip From 1816365ed7870f77170bfa32a7fd2546a1dc3615 Mon Sep 17 00:00:00 2001 From: Ken Cochrane Date: Wed, 25 Feb 2015 11:22:19 -0500 Subject: [PATCH 5/5] fixed missing package_data --- setup.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/setup.py b/setup.py index ce0b052..93db448 100644 --- a/setup.py +++ b/setup.py @@ -37,6 +37,12 @@ setup(name='django-defender', author_email='kencochrane@gmail.com', license='Apache 2', packages=['defender'], + package_data={ + "defender": ["templates/*.html", + "migrations/*.py", + "south_migrations/*.py", + "exampleapp/*.*"], + }, install_requires=['Django>=1.6,<1.8', 'redis==2.10.3', 'hiredis==0.1.4'], tests_require=['mock', 'mockredispy', 'coverage', 'celery'], )