From 027bd348642007617518379f8b02546abacaa6e0 Mon Sep 17 00:00:00 2001 From: Simon Charette Date: Mon, 11 Aug 2014 15:36:16 -0400 Subject: [PATCH] [1.4.x] Prevented data leakage in contrib.admin via query string manipulation. This is a security fix. Disclosure following shortly. --- django/contrib/admin/exceptions.py | 6 ++++++ django/contrib/admin/options.py | 18 ++++++++++++++++++ django/contrib/admin/views/main.py | 6 +++++- docs/releases/1.4.14.txt | 15 +++++++++++++++ tests/regressiontests/admin_views/tests.py | 21 +++++++++++++++++---- 5 files changed, 61 insertions(+), 5 deletions(-) create mode 100644 django/contrib/admin/exceptions.py diff --git a/django/contrib/admin/exceptions.py b/django/contrib/admin/exceptions.py new file mode 100644 index 0000000000..d9de47eefd --- /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 78a08cd120..cf3497b93c 100644 --- a/django/contrib/admin/options.py +++ b/django/contrib/admin/options.py @@ -269,6 +269,24 @@ class BaseModelAdmin(object): 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 9d5c30434d..7f4bb2f7e5 100644 --- a/django/contrib/admin/views/main.py +++ b/django/contrib/admin/views/main.py @@ -10,6 +10,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) @@ -56,7 +57,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 811c3f67ea..98de8b018e 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/tests/regressiontests/admin_views/tests.py b/tests/regressiontests/admin_views/tests.py index b695453e17..c40f009177 100644 --- a/tests/regressiontests/admin_views/tests.py +++ b/tests/regressiontests/admin_views/tests.py @@ -13,11 +13,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 @@ -572,6 +573,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 @@ -2061,10 +2075,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, '', html=True) + self.assertContains(response, '' % TO_FIELD_VAR, html=True) def test_exact_matches(self): response = self.client.get('/test_admin/admin/admin_views/recommendation/?q=bar')