From f351b05031dfeda252518ea66f6e007884aa433d Mon Sep 17 00:00:00 2001 From: Julia Hansbrough Date: Tue, 3 Dec 2013 19:53:48 +0000 Subject: [PATCH] Fixing email link injection bug Several templates used a variable set by the user (the request host header). This led to a vulnerability where an attacker could inject their domain name into these templates (i.e., activation emails). This patch fixes this vulnerability. LMS-532 --- common/djangoapps/edxmako/middleware.py | 3 +- common/djangoapps/student/tests/test_email.py | 27 +++++++++++++ common/djangoapps/util/request.py | 17 ++++++++ common/djangoapps/util/tests/test_request.py | 39 +++++++++++++++++++ lms/envs/common.py | 1 + 5 files changed, 86 insertions(+), 1 deletion(-) create mode 100644 common/djangoapps/util/request.py create mode 100644 common/djangoapps/util/tests/test_request.py diff --git a/common/djangoapps/edxmako/middleware.py b/common/djangoapps/edxmako/middleware.py index 778322c122..27b129f571 100644 --- a/common/djangoapps/edxmako/middleware.py +++ b/common/djangoapps/edxmako/middleware.py @@ -13,6 +13,7 @@ # limitations under the License. from django.template import RequestContext +from util.request import safe_get_host requestcontext = None @@ -22,4 +23,4 @@ class MakoMiddleware(object): global requestcontext requestcontext = RequestContext(request) requestcontext['is_secure'] = request.is_secure() - requestcontext['site'] = request.get_host() + requestcontext['site'] = safe_get_host(request) diff --git a/common/djangoapps/student/tests/test_email.py b/common/djangoapps/student/tests/test_email.py index c3e4fd0484..d81169f241 100644 --- a/common/djangoapps/student/tests/test_email.py +++ b/common/djangoapps/student/tests/test_email.py @@ -11,6 +11,8 @@ from mock import Mock, patch from django.http import Http404, HttpResponse from django.conf import settings from nose.plugins.skip import SkipTest +from edxmako.shortcuts import render_to_string +from util.request import safe_get_host class TestException(Exception): @@ -50,6 +52,11 @@ class EmailTestMixin(object): settings.DEFAULT_FROM_EMAIL ) + def append_allowed_hosts(self, hostname): + """ Append hostname to settings.ALLOWED_HOSTS """ + settings.ALLOWED_HOSTS.append(hostname) + self.addCleanup(settings.ALLOWED_HOSTS.pop) + @patch('student.views.render_to_string', Mock(side_effect=mock_render_to_string, autospec=True)) @patch('django.contrib.auth.models.User.email_user') @@ -83,6 +90,16 @@ class ReactivationEmailTests(EmailTestMixin, TestCase): context ) + # Thorough tests for safe_get_host are elsewhere; here we just want a quick URL sanity check + request = RequestFactory().post('unused_url') + request.META['HTTP_HOST'] = "aGenericValidHostName" + self.append_allowed_hosts("aGenericValidHostName") + + body = render_to_string('emails/activation_email.txt', context) + host = safe_get_host(request) + + self.assertIn(host, body) + def test_reactivation_email_failure(self, email_user): self.user.email_user.side_effect = Exception response_data = self.reactivation_email(self.user) @@ -227,6 +244,16 @@ class EmailChangeConfirmationTests(EmailTestMixin, TransactionTestCase): context ) + # Thorough tests for safe_get_host are elsewhere; here we just want a quick URL sanity check + request = RequestFactory().post('unused_url') + request.META['HTTP_HOST'] = "aGenericValidHostName" + self.append_allowed_hosts("aGenericValidHostName") + + body = render_to_string('emails/confirm_email_change.txt', context) + url = safe_get_host(request) + + self.assertIn(url, body) + def test_not_pending(self, email_user): self.key = 'not_a_key' self.check_confirm_email_change('invalid_email_key.html', {}) diff --git a/common/djangoapps/util/request.py b/common/djangoapps/util/request.py new file mode 100644 index 0000000000..0950fa3b42 --- /dev/null +++ b/common/djangoapps/util/request.py @@ -0,0 +1,17 @@ +""" Utility functions related to HTTP requests """ +from django.conf import settings + + +def safe_get_host(request): + """ + Get the host name for this request, as safely as possible. + + If ALLOWED_HOSTS is properly set, this calls request.get_host; + otherwise, this returns whatever settings.SITE_NAME is set to. + + This ensures we will never accept an untrusted value of get_host() + """ + if isinstance(settings.ALLOWED_HOSTS, (list, tuple)) and '*' not in settings.ALLOWED_HOSTS: + return request.get_host() + else: + return settings.SITE_NAME diff --git a/common/djangoapps/util/tests/test_request.py b/common/djangoapps/util/tests/test_request.py new file mode 100644 index 0000000000..76bd9b527f --- /dev/null +++ b/common/djangoapps/util/tests/test_request.py @@ -0,0 +1,39 @@ +from django.test.client import RequestFactory +from django.conf import settings +from util.request import safe_get_host +from django.core.exceptions import SuspiciousOperation +import unittest + + +class ResponseTestCase(unittest.TestCase): + """ Tests for response-related utility functions """ + def setUp(self): + self.old_site_name = settings.SITE_NAME + self.old_allowed_hosts = settings.ALLOWED_HOSTS + + def tearDown(self): + settings.SITE_NAME = self.old_site_name + settings.ALLOWED_HOSTS = self.old_allowed_hosts + + def test_safe_get_host(self): + """ Tests that the safe_get_host function returns the desired host """ + settings.SITE_NAME = 'siteName.com' + factory = RequestFactory() + request = factory.request() + request.META['HTTP_HOST'] = 'www.userProvidedHost.com' + # If ALLOWED_HOSTS is not set properly, safe_get_host should return SITE_NAME + settings.ALLOWED_HOSTS = None + self.assertEqual(safe_get_host(request), "siteName.com") + settings.ALLOWED_HOSTS = ["*"] + self.assertEqual(safe_get_host(request), "siteName.com") + settings.ALLOWED_HOSTS = ["foo.com", "*"] + self.assertEqual(safe_get_host(request), "siteName.com") + + # If ALLOWED_HOSTS is set properly, and the host is valid, we just return the user-provided host + settings.ALLOWED_HOSTS = [request.META['HTTP_HOST']] + self.assertEqual(safe_get_host(request), request.META['HTTP_HOST']) + + # If ALLOWED_HOSTS is set properly but the host is invalid, we should get a SuspiciousOperation + settings.ALLOWED_HOSTS = ["the_valid_website.com"] + with self.assertRaises(SuspiciousOperation): + safe_get_host(request) diff --git a/lms/envs/common.py b/lms/envs/common.py index 3e7841833a..b88f48a003 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -446,6 +446,7 @@ SITE_NAME = "edx.org" HTTPS = 'on' ROOT_URLCONF = 'lms.urls' IGNORABLE_404_ENDS = ('favicon.ico') +# NOTE: Please set ALLOWED_HOSTS to some sane value, as we do not allow the default '*' # Platform Email EMAIL_BACKEND = 'django.core.mail.backends.console.EmailBackend'