[1.6.x] Prevented data leakage in contrib.admin via query string mani…

…pulation.

This is a security fix. Disclosure following shortly.

django/contrib/admin/exceptions.py

@@ -4,3 +4,8 @@
 class DisallowedModelAdminLookup(SuspiciousOperation):
     """Invalid filter was passed to admin view via URL querystring"""
     pass
+
+
+class DisallowedModelAdminToField(SuspiciousOperation):
+    """Invalid to_field was passed to admin view via URL query string"""
+    pass

django/contrib/admin/options.py

@@ -327,6 +327,24 @@ def lookup_allowed(self, lookup, value):
         clean_lookup = LOOKUP_SEP.join(parts)
         return clean_lookup in self.list_filter or clean_lookup == self.date_hierarchy
 
+    def to_field_allowed(self, request, to_field):
+        opts = self.model._meta
+
+        try:
+            field = opts.get_field(to_field)
+        except FieldDoesNotExist:
+            return False
+
+        # Make sure at least one of the models registered for this site
+        # references this field.
+        registered_models = self.admin_site._registry
+        for related_object in opts.get_all_related_objects():
+            if (related_object.model in registered_models and
+                    field in related_object.field.foreign_related_fields):
+                return True
+
+        return False
+
     def has_add_permission(self, request):
         """
         Returns True if the given request has permission to add an object.

django/contrib/admin/views/main.py

@@ -14,7 +14,7 @@
 from django.utils.http import urlencode
 
 from django.contrib.admin import FieldListFilter
-from django.contrib.admin.exceptions import DisallowedModelAdminLookup
+from django.contrib.admin.exceptions import DisallowedModelAdminLookup, DisallowedModelAdminToField
 from django.contrib.admin.options import IncorrectLookupParameters, IS_POPUP_VAR
 from django.contrib.admin.util import (quote, get_fields_from_path,
     lookup_needs_distinct, prepare_lookup_value)
@@ -90,7 +90,10 @@ def __init__(self, request, model, list_display, list_display_links,
             self.page_num = 0
         self.show_all = ALL_VAR in request.GET
         self.is_popup = _is_changelist_popup(request)
-        self.to_field = request.GET.get(TO_FIELD_VAR)
+        to_field = request.GET.get(TO_FIELD_VAR)
+        if to_field and not model_admin.to_field_allowed(request, to_field):
+            raise DisallowedModelAdminToField("The field %s cannot be referenced." % to_field)
+        self.to_field = to_field
         self.params = dict(request.GET.items())
         if PAGE_VAR in self.params:
             del self.params[PAGE_VAR]

docs/ref/exceptions.txt

@@ -56,6 +56,7 @@ SuspiciousOperation
 
     * DisallowedHost
     * DisallowedModelAdminLookup
+    * DisallowedModelAdminToField
     * DisallowedRedirect
     * InvalidSessionKey
     * SuspiciousFileOperation

docs/releases/1.4.14.txt

@@ -47,3 +47,18 @@ and the ``RemoteUserBackend``, a change to the ``REMOTE_USER`` header between
 requests without an intervening logout could result in the prior user's session
 being co-opted by the subsequent user. The middleware now logs the user out on
 a failed login attempt.
+
+Data leakage via query string manipulation in ``contrib.admin``
+===============================================================
+
+In older versions of Django it was possible to reveal any field's data by
+modifying the "popup" and "to_field" parameters of the query string on an admin
+change form page. For example, requesting a URL like
+``/admin/auth/user/?pop=1&t=password`` and viewing the page's HTML allowed
+viewing the password hash of each user. While the admin requires users to have
+permissions to view the change form pages in the first place, this could leak
+data if you rely on users having access to view only certain fields on a model.
+
+To address the issue, an exception will now be raised if a ``to_field`` value
+that isn't a related field to a model that has been registered with the admin
+is specified.

docs/releases/1.5.9.txt

@@ -47,3 +47,18 @@ and the ``RemoteUserBackend``, a change to the ``REMOTE_USER`` header between
 requests without an intervening logout could result in the prior user's session
 being co-opted by the subsequent user. The middleware now logs the user out on
 a failed login attempt.
+
+Data leakage via query string manipulation in ``contrib.admin``
+===============================================================
+
+In older versions of Django it was possible to reveal any field's data by
+modifying the "popup" and "to_field" parameters of the query string on an admin
+change form page. For example, requesting a URL like
+``/admin/auth/user/?pop=1&t=password`` and viewing the page's HTML allowed
+viewing the password hash of each user. While the admin requires users to have
+permissions to view the change form pages in the first place, this could leak
+data if you rely on users having access to view only certain fields on a model.
+
+To address the issue, an exception will now be raised if a ``to_field`` value
+that isn't a related field to a model that has been registered with the admin
+is specified.

docs/releases/1.6.6.txt

@@ -48,6 +48,21 @@ requests without an intervening logout could result in the prior user's session
 being co-opted by the subsequent user. The middleware now logs the user out on
 a failed login attempt.
 
+Data leakage via query string manipulation in ``contrib.admin``
+===============================================================
+
+In older versions of Django it was possible to reveal any field's data by
+modifying the "popup" and "to_field" parameters of the query string on an admin
+change form page. For example, requesting a URL like
+``/admin/auth/user/?_popup=1&t=password`` and viewing the page's HTML allowed
+viewing the password hash of each user. While the admin requires users to have
+permissions to view the change form pages in the first place, this could leak
+data if you rely on users having access to view only certain fields on a model.
+
+To address the issue, an exception will now be raised if a ``to_field`` value
+that isn't a related field to a model that has been registered with the admin
+is specified.
+
 Bugfixes
 ========
 

tests/admin_views/tests.py

@@ -14,6 +14,7 @@
 from django.contrib import admin
 from django.contrib.auth import get_permission_codename
 from django.contrib.admin.helpers import ACTION_CHECKBOX_NAME
+from django.contrib.admin.views.main import TO_FIELD_VAR
 from django.contrib.admin.models import LogEntry, DELETION
 from django.contrib.admin.sites import LOGIN_FORM_KEY
 from django.contrib.admin.templatetags.admin_urls import add_preserved_filters
@@ -577,6 +578,23 @@ def test_disallowed_filtering(self):
         response = self.client.get("/test_admin/admin/admin_views/workhour/?employee__person_ptr__exact=%d" % e1.pk)
         self.assertEqual(response.status_code, 200)
 
+    def test_disallowed_to_field(self):
+        with patch_logger('django.security.DisallowedModelAdminToField', 'error') as calls:
+            response = self.client.get("/test_admin/admin/admin_views/section/", {TO_FIELD_VAR: 'missing_field'})
+            self.assertEqual(response.status_code, 400)
+            self.assertEqual(len(calls), 1)
+
+        # Specifying a field that is not refered by any other model registered
+        # to this admin site should raise an exception.
+        with patch_logger('django.security.DisallowedModelAdminToField', 'error') as calls:
+            response = self.client.get("/test_admin/admin/admin_views/section/", {TO_FIELD_VAR: 'name'})
+            self.assertEqual(response.status_code, 400)
+            self.assertEqual(len(calls), 1)
+
+        # Specifying a field referenced by another model should be allowed.
+        response = self.client.get("/test_admin/admin/admin_views/section/", {TO_FIELD_VAR: 'id'})
+        self.assertEqual(response.status_code, 200)
+
     def test_allowed_filtering_15103(self):
         """
         Regressions test for ticket 15103 - filtering on fields defined in a
@@ -2204,10 +2222,9 @@ def test_with_fk_to_field(self):
         """Ensure that the to_field GET parameter is preserved when a search
         is performed. Refs #10918.
         """
-        from django.contrib.admin.views.main import TO_FIELD_VAR
-        response = self.client.get('/test_admin/admin/auth/user/?q=joe&%s=username' % TO_FIELD_VAR)
+        response = self.client.get('/test_admin/admin/auth/user/?q=joe&%s=id' % TO_FIELD_VAR)
         self.assertContains(response, "\n1 user\n")
-        self.assertContains(response, '<input type="hidden" name="t" value="username"/>', html=True)
+        self.assertContains(response, '<input type="hidden" name="%s" value="id"/>' % TO_FIELD_VAR, html=True)
 
     def test_exact_matches(self):
         response = self.client.get('/test_admin/admin/admin_views/recommendation/?q=bar')