From 53ff0969822ac2248a89ccb6fef1088212dc800d Mon Sep 17 00:00:00 2001 From: Simon Charette Date: Thu, 7 Aug 2014 00:18:10 -0400 Subject: [PATCH] Prevented data leakage in contrib.admin via query string manipulation. This is a security fix. Disclosure following shortly. --- django/contrib/admin/exceptions.py | 5 +++++ django/contrib/admin/options.py | 26 +++++++++++++++++++-- django/contrib/admin/views/main.py | 9 ++++++-- docs/ref/exceptions.txt | 1 + docs/releases/1.4.14.txt | 15 +++++++++++++ docs/releases/1.5.9.txt | 15 +++++++++++++ docs/releases/1.6.6.txt | 15 +++++++++++++ tests/admin_views/tests.py | 36 +++++++++++++++++++++++++++--- 8 files changed, 115 insertions(+), 7 deletions(-) diff --git a/django/contrib/admin/exceptions.py b/django/contrib/admin/exceptions.py index 2e094c6da1..f619bc2252 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 3b429fbaed..ce89cf4dbd 100644 --- a/django/contrib/admin/options.py +++ b/django/contrib/admin/options.py @@ -11,6 +11,7 @@ from django.contrib.admin import widgets, helpers from django.contrib.admin import validation from django.contrib.admin.checks import (BaseModelAdminChecks, ModelAdminChecks, InlineModelAdminChecks) +from django.contrib.admin.exceptions import DisallowedModelAdminToField from django.contrib.admin.utils import (quote, unquote, flatten_fieldsets, get_deleted_objects, model_format_dict, NestedObjects, lookup_needs_distinct) @@ -434,6 +435,24 @@ class BaseModelAdmin(six.with_metaclass(forms.MediaDefiningClass)): valid_lookups.append(filter_item) return clean_lookup in valid_lookups + 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. @@ -1337,6 +1356,10 @@ class ModelAdmin(BaseModelAdmin): @transaction.atomic def changeform_view(self, request, object_id=None, form_url='', extra_context=None): + to_field = request.POST.get(TO_FIELD_VAR, request.GET.get(TO_FIELD_VAR)) + if to_field and not self.to_field_allowed(request, to_field): + raise DisallowedModelAdminToField("The field %s cannot be referenced." % to_field) + model = self.model opts = model._meta add = object_id is None @@ -1409,8 +1432,7 @@ class ModelAdmin(BaseModelAdmin): original=obj, is_popup=(IS_POPUP_VAR in request.POST or IS_POPUP_VAR in request.GET), - to_field=request.POST.get(TO_FIELD_VAR, - request.GET.get(TO_FIELD_VAR)), + to_field=to_field, media=media, inline_admin_formsets=inline_formsets, errors=helpers.AdminErrorList(form, formsets), diff --git a/django/contrib/admin/views/main.py b/django/contrib/admin/views/main.py index d1b6d82a3e..1f42310f8e 100644 --- a/django/contrib/admin/views/main.py +++ b/django/contrib/admin/views/main.py @@ -12,7 +12,9 @@ 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, TO_FIELD_VAR from django.contrib.admin.utils import (quote, get_fields_from_path, lookup_needs_distinct, prepare_lookup_value) @@ -58,7 +60,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/ref/exceptions.txt b/docs/ref/exceptions.txt index 3b8b644c76..e74e0991e1 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 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/docs/releases/1.5.9.txt b/docs/releases/1.5.9.txt index 5070f0eca1..5b91de0eb2 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 e52ea6b23f..63a1537c97 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 adc49a5db0..60ad3d2b7b 100644 --- a/tests/admin_views/tests.py +++ b/tests/admin_views/tests.py @@ -18,6 +18,7 @@ from django.contrib.auth import get_permission_codename from django.contrib.admin import ModelAdmin from django.contrib.admin.helpers import ACTION_CHECKBOX_NAME from django.contrib.admin.models import LogEntry, DELETION +from django.contrib.admin.options import TO_FIELD_VAR from django.contrib.admin.templatetags.admin_static import static from django.contrib.admin.templatetags.admin_urls import add_preserved_filters from django.contrib.admin.tests import AdminSeleniumWebDriverTestCase @@ -598,6 +599,36 @@ 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) + + # We also want to prevent the add and change view from leaking a + # disallowed field value. + with patch_logger('django.security.DisallowedModelAdminToField', 'error') as calls: + response = self.client.post("/test_admin/admin/admin_views/section/add/", {TO_FIELD_VAR: 'name'}) + self.assertEqual(response.status_code, 400) + self.assertEqual(len(calls), 1) + + section = Section.objects.create() + with patch_logger('django.security.DisallowedModelAdminToField', 'error') as calls: + response = self.client.post("/test_admin/admin/admin_views/section/%d/" % section.pk, {TO_FIELD_VAR: 'name'}) + self.assertEqual(response.status_code, 400) + self.assertEqual(len(calls), 1) + def test_allowed_filtering_15103(self): """ Regressions test for ticket 15103 - filtering on fields defined in a @@ -2378,10 +2409,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')