Attachment 926650 Details for Bug 1129959 – 1.6 patch

[patch] 1.6 patch

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

(hide)

Description:

Filename:

MIME Type:

Creator: Murray McAllister

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

Size: 10.21 KB

patch

obsolete

>commit e3f77f2c2d6cdae64d296568b62f3e7ed792b70f
>Author: Simon Charette <charette.s@gmail.com>
>Date:   Thu Aug 7 00:18:10 2014 -0400
>
>    [1.6.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
>index 2e094c6..f619bc2 100644
>--- a/django/contrib/admin/exceptions.py
>+++ b/django/contrib/admin/exceptions.py
>@@ -4,3 +4,8 @@ from django.core.exceptions import SuspiciousOperation
> 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
>diff --git a/django/contrib/admin/options.py b/django/contrib/admin/options.py
>index 8a7483c..2ffb924 100644
>--- a/django/contrib/admin/options.py
>+++ b/django/contrib/admin/options.py
>@@ -327,6 +327,24 @@ class BaseModelAdmin(six.with_metaclass(RenameBaseModelAdminMethods)):
>         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.
>diff --git a/django/contrib/admin/views/main.py b/django/contrib/admin/views/main.py
>index e7b2391..0a0dc60 100644
>--- a/django/contrib/admin/views/main.py
>+++ b/django/contrib/admin/views/main.py
>@@ -14,7 +14,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 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 @@ class ChangeList(six.with_metaclass(RenameChangeListMethods)):
>             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]
>diff --git a/docs/ref/exceptions.txt b/docs/ref/exceptions.txt
>index 956ee5a..13cbb34 100644
>--- a/docs/ref/exceptions.txt
>+++ b/docs/ref/exceptions.txt
>@@ -56,6 +56,7 @@ SuspiciousOperation
> 
>     * DisallowedHost
>     * DisallowedModelAdminLookup
>+    * DisallowedModelAdminToField
>     * DisallowedRedirect
>     * InvalidSessionKey
>     * SuspiciousFileOperation
>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/docs/releases/1.6.6.txt b/docs/releases/1.6.6.txt
>index 0485f95..00e3ee1 100644
>--- a/docs/releases/1.6.6.txt
>+++ b/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
> ========
> 
>diff --git a/tests/admin_views/tests.py b/tests/admin_views/tests.py
>index 3fb80f1..189b0c8 100644
>--- a/tests/admin_views/tests.py
>+++ b/tests/admin_views/tests.py
>@@ -14,6 +14,7 @@ from django.core.urlresolvers import get_script_prefix, reverse, set_script_pref
> 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 @@ class AdminViewBasicTest(AdminViewBasicTestCase):
>         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 @@ 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