SECURITY ALERT: Corrected a problem with the Admin media handler that could lead to the exposure of system files. Thanks to Gary Wilson for the patch.

This is a security-related update. A full announcement, as well as backports for 1.0.X and 0.96.X will be forthcoming.

git-svn-id: http://code.djangoproject.com/svn/django/trunk@11351 bcc190cf-cafb-0310-a4f2-bffc1f526a37

5 files changed, 94 insertions, 7 deletions

diff --git a/django/core/management/commands/runserver.py b/django/core/management/commands/runserver.py
index 16d75fd88b..fc2c694ab6 100644
--- a/django/core/management/commands/runserver.py
+++ b/django/core/management/commands/runserver.py
@@ -56,8 +56,7 @@ class Command(BaseCommand):
             translation.activate(settings.LANGUAGE_CODE)
 
             try:
-                path = admin_media_path or django.__path__[0] + '/contrib/admin/media'
-                handler = AdminMediaHandler(WSGIHandler(), path)
+                handler = AdminMediaHandler(WSGIHandler(), admin_media_path)
                 run(addr, int(port), handler)
             except WSGIServerException, e:
                 # Use helpful error messages instead of ugly tracebacks.
diff --git a/django/core/servers/basehttp.py b/django/core/servers/basehttp.py
index 6fed0acc59..f7c0a77f7c 100644
--- a/django/core/servers/basehttp.py
+++ b/django/core/servers/basehttp.py
@@ -16,6 +16,7 @@ import sys
 import urllib
 
 from django.utils.http import http_date
+from django.utils._os import safe_join
 
 __version__ = "0.1"
 __all__ = ['WSGIServer','WSGIRequestHandler']
@@ -621,11 +622,25 @@ class AdminMediaHandler(object):
         self.application = application
         if not media_dir:
             import django
-            self.media_dir = django.__path__[0] + '/contrib/admin/media'
+            self.media_dir = \
+                os.path.join(django.__path__[0], 'contrib', 'admin', 'media')
         else:
             self.media_dir = media_dir
         self.media_url = settings.ADMIN_MEDIA_PREFIX
 
+    def file_path(self, url):
+        """
+        Returns the path to the media file on disk for the given URL.
+
+        The passed URL is assumed to begin with ADMIN_MEDIA_PREFIX.  If the
+        resultant file path is outside the media directory, then a ValueError
+        is raised.
+        """
+        # Remove ADMIN_MEDIA_PREFIX.
+        relative_url = url[len(self.media_url):]
+        relative_path = urllib.url2pathname(relative_url)
+        return safe_join(self.media_dir, relative_path)
+
     def __call__(self, environ, start_response):
         import os.path
 
@@ -636,19 +651,25 @@ class AdminMediaHandler(object):
             return self.application(environ, start_response)
 
         # Find the admin file and serve it up, if it exists and is readable.
-        relative_url = environ['PATH_INFO'][len(self.media_url):]
-        file_path = os.path.join(self.media_dir, relative_url)
+        try:
+            file_path = self.file_path(environ['PATH_INFO'])
+        except ValueError: # Resulting file path was not valid.
+            status = '404 NOT FOUND'
+            headers = {'Content-type': 'text/plain'}
+            output = ['Page not found: %s' % environ['PATH_INFO']]
+            start_response(status, headers.items())
+            return output
         if not os.path.exists(file_path):
             status = '404 NOT FOUND'
             headers = {'Content-type': 'text/plain'}
-            output = ['Page not found: %s' % file_path]
+            output = ['Page not found: %s' % environ['PATH_INFO']]
         else:
             try:
                 fp = open(file_path, 'rb')
             except IOError:
                 status = '401 UNAUTHORIZED'
                 headers = {'Content-type': 'text/plain'}
-                output = ['Permission denied: %s' % file_path]
+                output = ['Permission denied: %s' % environ['PATH_INFO']]
             else:
                 # This is a very simple implementation of conditional GET with
                 # the Last-Modified header. It makes media files a bit speedier
diff --git a/tests/regressiontests/servers/__init__.py b/tests/regressiontests/servers/__init__.py
new file mode 100644
index 0000000000..e69de29bb2
--- /dev/null
+++ b/tests/regressiontests/servers/__init__.py
diff --git a/tests/regressiontests/servers/models.py b/tests/regressiontests/servers/models.py
new file mode 100644
index 0000000000..e69de29bb2
--- /dev/null
+++ b/tests/regressiontests/servers/models.py
diff --git a/tests/regressiontests/servers/tests.py b/tests/regressiontests/servers/tests.py
new file mode 100644
index 0000000000..7621439032
--- /dev/null
+++ b/tests/regressiontests/servers/tests.py
@@ -0,0 +1,67 @@
+"""
+Tests for django.core.servers.
+"""
+
+import os
+
+import django
+from django.test import TestCase
+from django.core.handlers.wsgi import WSGIHandler
+from django.core.servers.basehttp import AdminMediaHandler
+
+
+class AdminMediaHandlerTests(TestCase):
+
+    def setUp(self):
+        self.admin_media_file_path = \
+            os.path.join(django.__path__[0], 'contrib', 'admin', 'media')
+        self.handler = AdminMediaHandler(WSGIHandler())
+
+    def test_media_urls(self):
+        """
+        Tests that URLs that look like absolute file paths after the
+        settings.ADMIN_MEDIA_PREFIX don't turn into absolute file paths.
+        """
+        # Cases that should work on all platforms.
+        data = (
+            ('/media/css/base.css', ('css', 'base.css')),
+        )
+        # Cases that should raise an exception.
+        bad_data = ()
+
+        # Add platform-specific cases.
+        if os.sep == '/':
+            data += (
+                # URL, tuple of relative path parts.
+                ('/media/\\css/base.css', ('\\css', 'base.css')),
+            )
+            bad_data += (
+                '/media//css/base.css',
+                '/media////css/base.css',
+                '/media/../css/base.css',
+            )
+        elif os.sep == '\\':
+            bad_data += (
+                '/media/C:\css/base.css',
+                '/media//\\css/base.css',
+                '/media/\\css/base.css',
+                '/media/\\\\css/base.css'
+            )
+        for url, path_tuple in data:
+            try:
+                output = self.handler.file_path(url)
+            except ValueError:
+                self.fail("Got a ValueError exception, but wasn't expecting"
+                          " one. URL was: %s" % url)
+            rel_path = os.path.join(*path_tuple)
+            desired = os.path.normcase(
+                os.path.join(self.admin_media_file_path, rel_path))
+            self.assertEqual(output, desired,
+                "Got: %s, Expected: %s, URL was: %s" % (output, desired, url))
+        for url in bad_data:
+            try:
+                output = self.handler.file_path(url)
+            except ValueError:
+                continue
+            self.fail('URL: %s should have caused a ValueError exception.'
+                      % url)