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

@@ -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)

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', {})

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

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)

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'