Attachment 926649 Details for Bug 1129959 – 1.5 patch

[patch] 1.5 patch

admin-data-leak-1.5.diff (text/plain), 8.46 KB, created by Murray McAllister on 2014-08-14 05:10:30 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Murray McAllister

Created: 2014-08-14 05:10:30 UTC

Size: 8.46 KB

patch

obsolete

>commit df7cc7b1996c3be0296995b2ce0ace8c0200e4ec
>Author: Simon Charette <charette.s@gmail.com>
>Date:   Mon Aug 11 15:36:16 2014 -0400
>
>    [1.5.x] Prevented data leakage in contrib.admin via query string manipulation.
>    
>    This is a security fix. Disclosure following shortly.
>
>diff --git a/django/contrib/admin/exceptions.py b/django/contrib/admin/exceptions.py
>new file mode 100644
>index 0000000..d9de47e
>--- /dev/null
>+++ b/django/contrib/admin/exceptions.py
>@@ -0,0 +1,6 @@
>+from django.core.exceptions import SuspiciousOperation
>+
>+
>+class DisallowedModelAdminToField(SuspiciousOperation):
>+    """Invalid to_field was passed to admin view via URL query string"""
>+    pass
>diff --git a/django/contrib/admin/options.py b/django/contrib/admin/options.py
>index 6204e0b..e7cb85f 100644
>--- a/django/contrib/admin/options.py
>+++ b/django/contrib/admin/options.py
>@@ -275,6 +275,24 @@ class BaseModelAdmin(six.with_metaclass(forms.MediaDefiningClass)):
>         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 == related_object.field.rel.get_related_field()):
>+                return True
>+
>+        return False
>+
>     def has_add_permission(self, request):
>         """
>         Returns True if the given request has permission to add an object.
>diff --git a/django/contrib/admin/views/main.py b/django/contrib/admin/views/main.py
>index be7067f..cf53d6a 100644
>--- a/django/contrib/admin/views/main.py
>+++ b/django/contrib/admin/views/main.py
>@@ -12,6 +12,7 @@ from django.utils.translation import ugettext, ugettext_lazy
> from django.utils.http import urlencode
> 
> from django.contrib.admin import FieldListFilter
>+from django.contrib.admin.exceptions import DisallowedModelAdminToField
> from django.contrib.admin.options import IncorrectLookupParameters
> from django.contrib.admin.util import (quote, get_fields_from_path,
>     lookup_needs_distinct, prepare_lookup_value)
>@@ -58,7 +59,10 @@ class ChangeList(object):
>             self.page_num = 0
>         self.show_all = ALL_VAR in request.GET
>         self.is_popup = IS_POPUP_VAR in request.GET
>-        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]
>diff --git a/docs/releases/1.4.14.txt b/docs/releases/1.4.14.txt
>index 811c3f6..98de8b0 100644
>--- a/docs/releases/1.4.14.txt
>+++ b/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.
>diff --git a/docs/releases/1.5.9.txt b/docs/releases/1.5.9.txt
>index 5070f0e..5b91de0 100644
>--- a/docs/releases/1.5.9.txt
>+++ b/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.
>diff --git a/tests/regressiontests/admin_views/tests.py b/tests/regressiontests/admin_views/tests.py
>index 72c1829..e7efca2 100644
>--- a/tests/regressiontests/admin_views/tests.py
>+++ b/tests/regressiontests/admin_views/tests.py
>@@ -16,11 +16,12 @@ from django.core.files import temp as tempfile
> from django.core.urlresolvers import reverse
> # Register auth models with the admin.
> from django.contrib import admin
>+from django.contrib.admin.exceptions import DisallowedModelAdminToField
> from django.contrib.admin.helpers import ACTION_CHECKBOX_NAME
> from django.contrib.admin.models import LogEntry, DELETION
> from django.contrib.admin.sites import LOGIN_FORM_KEY
> from django.contrib.admin.util import quote
>-from django.contrib.admin.views.main import IS_POPUP_VAR
>+from django.contrib.admin.views.main import IS_POPUP_VAR, TO_FIELD_VAR
> from django.contrib.admin.tests import AdminSeleniumWebDriverTestCase
> from django.contrib.auth import REDIRECT_FIELD_NAME
> from django.contrib.auth.models import Group, User, Permission, UNUSABLE_PASSWORD
>@@ -557,6 +558,19 @@ class AdminViewBasicTest(TestCase):
>         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 self.assertRaises(DisallowedModelAdminToField):
>+            response = self.client.get("/test_admin/admin/admin_views/section/", {TO_FIELD_VAR: 'missing_field'})
>+
>+        # Specifying a field that is not refered by any other model registered
>+        # to this admin site should raise an exception.
>+        with self.assertRaises(DisallowedModelAdminToField):
>+            response = self.client.get("/test_admin/admin/admin_views/section/", {TO_FIELD_VAR: 'name'})
>+
>+        # 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
>@@ -2138,10 +2152,9 @@ class AdminSearchTest(TestCase):
>         """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')

Actions: View | Diff

Attachments on bug 1129959: 926648 | 926649 | 926650