diff options
author | Carlton Gibson <carlton.gibson@noumenal.es> | 2019-11-25 12:01:49 +0100 |
---|---|---|
committer | Carlton Gibson <carlton.gibson@noumenal.es> | 2019-12-02 08:58:14 +0100 |
commit | 36f580a17f0b3cb087deadf3b65eea024f479c21 (patch) | |
tree | cda21439c213c5246b5ac2736399cf5d2894c0cf | |
parent | 70311e1d00ef5b6bbbc8961eac81b5c814396a43 (diff) | |
download | django-36f580a17f0b3cb087deadf3b65eea024f479c21.tar.gz |
Fixed CVE-2019-19118 -- Required edit permissions on parent model for editable inlines in admin.
Thank you to Shen Ying for reporting this issue.
-rw-r--r-- | django/contrib/admin/options.py | 21 | ||||
-rw-r--r-- | django/contrib/admin/templates/admin/edit_inline/stacked.html | 2 | ||||
-rw-r--r-- | django/contrib/admin/templates/admin/edit_inline/tabular.html | 4 | ||||
-rw-r--r-- | docs/releases/2.1.15.txt | 41 | ||||
-rw-r--r-- | docs/releases/2.2.8.txt | 43 | ||||
-rw-r--r-- | tests/admin_inlines/tests.py | 112 | ||||
-rw-r--r-- | tests/admin_views/admin.py | 9 | ||||
-rw-r--r-- | tests/admin_views/tests.py | 68 | ||||
-rw-r--r-- | tests/admin_views/urls.py | 1 | ||||
-rw-r--r-- | tests/auth_tests/test_views.py | 2 |
10 files changed, 216 insertions, 87 deletions
diff --git a/django/contrib/admin/options.py b/django/contrib/admin/options.py index 520e3d023e..57c3d4f4d6 100644 --- a/django/contrib/admin/options.py +++ b/django/contrib/admin/options.py @@ -1471,13 +1471,20 @@ class ModelAdmin(BaseModelAdmin): ) def get_inline_formsets(self, request, formsets, inline_instances, obj=None): + # Edit permissions on parent model are required for editable inlines. + can_edit_parent = self.has_change_permission(request, obj) if obj else self.has_add_permission(request) inline_admin_formsets = [] for inline, formset in zip(inline_instances, formsets): fieldsets = list(inline.get_fieldsets(request, obj)) readonly = list(inline.get_readonly_fields(request, obj)) - has_add_permission = inline._has_add_permission(request, obj) - has_change_permission = inline.has_change_permission(request, obj) - has_delete_permission = inline.has_delete_permission(request, obj) + if can_edit_parent: + has_add_permission = inline._has_add_permission(request, obj) + has_change_permission = inline.has_change_permission(request, obj) + has_delete_permission = inline.has_delete_permission(request, obj) + else: + # Disable all edit-permissions, and overide formset settings. + has_add_permission = has_change_permission = has_delete_permission = False + formset.extra = formset.max_num = 0 has_view_permission = inline.has_view_permission(request, obj) prepopulated = dict(inline.get_prepopulated_fields(request, obj)) inline_admin_formset = helpers.InlineAdminFormSet( @@ -1542,8 +1549,12 @@ class ModelAdmin(BaseModelAdmin): else: obj = self.get_object(request, unquote(object_id), to_field) - if not self.has_view_or_change_permission(request, obj): - raise PermissionDenied + if request.method == 'POST': + if not self.has_change_permission(request, obj): + raise PermissionDenied + else: + if not self.has_view_or_change_permission(request, obj): + raise PermissionDenied if obj is None: return self._get_obj_does_not_exist_redirect(request, opts, object_id) diff --git a/django/contrib/admin/templates/admin/edit_inline/stacked.html b/django/contrib/admin/templates/admin/edit_inline/stacked.html index 8af4d54791..d9c27b1578 100644 --- a/django/contrib/admin/templates/admin/edit_inline/stacked.html +++ b/django/contrib/admin/templates/admin/edit_inline/stacked.html @@ -12,7 +12,7 @@ <h3><b>{{ inline_admin_formset.opts.verbose_name|capfirst }}:</b> <span class="inline_label">{% if inline_admin_form.original %}{{ inline_admin_form.original }}{% if inline_admin_form.model_admin.show_change_link and inline_admin_form.model_admin.has_registered_model %} <a href="{% url inline_admin_form.model_admin.opts|admin_urlname:'change' inline_admin_form.original.pk|admin_urlquote %}" class="{% if inline_admin_formset.has_change_permission %}inlinechangelink{% else %}inlineviewlink{% endif %}">{% if inline_admin_formset.has_change_permission %}{% trans "Change" %}{% else %}{% trans "View" %}{% endif %}</a>{% endif %} {% else %}#{{ forloop.counter }}{% endif %}</span> {% if inline_admin_form.show_url %}<a href="{{ inline_admin_form.absolute_url }}">{% trans "View on site" %}</a>{% endif %} - {% if inline_admin_formset.formset.can_delete and inline_admin_form.original %}<span class="delete">{{ inline_admin_form.deletion_field.field }} {{ inline_admin_form.deletion_field.label_tag }}</span>{% endif %} + {% if inline_admin_formset.formset.can_delete and inline_admin_formset.has_delete_permission and inline_admin_form.original %}<span class="delete">{{ inline_admin_form.deletion_field.field }} {{ inline_admin_form.deletion_field.label_tag }}</span>{% endif %} </h3> {% if inline_admin_form.form.non_field_errors %}{{ inline_admin_form.form.non_field_errors }}{% endif %} {% for fieldset in inline_admin_form %} diff --git a/django/contrib/admin/templates/admin/edit_inline/tabular.html b/django/contrib/admin/templates/admin/edit_inline/tabular.html index 531d7b6a21..d643955dcf 100644 --- a/django/contrib/admin/templates/admin/edit_inline/tabular.html +++ b/django/contrib/admin/templates/admin/edit_inline/tabular.html @@ -17,7 +17,7 @@ </th> {% endif %} {% endfor %} - {% if inline_admin_formset.formset.can_delete %}<th>{% trans "Delete?" %}</th>{% endif %} + {% if inline_admin_formset.formset.can_delete and inline_admin_formset.has_delete_permission %}<th>{% trans "Delete?" %}</th>{% endif %} </tr></thead> <tbody> @@ -63,7 +63,7 @@ {% endfor %} {% endfor %} {% endfor %} - {% if inline_admin_formset.formset.can_delete %} + {% if inline_admin_formset.formset.can_delete and inline_admin_formset.has_delete_permission %} <td class="delete">{% if inline_admin_form.original %}{{ inline_admin_form.deletion_field.field }}{% endif %}</td> {% endif %} </tr> diff --git a/docs/releases/2.1.15.txt b/docs/releases/2.1.15.txt index 8d13bb281f..29764f986e 100644 --- a/docs/releases/2.1.15.txt +++ b/docs/releases/2.1.15.txt @@ -4,7 +4,46 @@ Django 2.1.15 release notes *Expected December 2, 2019* -Django 2.1.15 fixes a data loss bug in 2.1.14. +Django 2.1.15 fixes a security issue and a data loss bug in 2.1.14. + +CVE-2019-19118: Privilege escalation in the Django admin. +========================================================= + +Since Django 2.1, a Django model admin displaying a parent model with related +model inlines, where the user has view-only permissions to a parent model but +edit permissions to the inline model, would display a read-only view of the +parent model but editable forms for the inline. + +Submitting these forms would not allow direct edits to the parent model, but +would trigger the parent model's ``save()`` method, and cause pre and post-save +signal handlers to be invoked. This is a privilege escalation as a user who +lacks permission to edit a model should not be able to trigger its save-related +signals. + +To resolve this issue, the permission handling code of the Django admin +interface has been changed. Now, if a user has only the "view" permission for a +parent model, the entire displayed form will not be editable, even if the user +has permission to edit models included in inlines. + +This is a backwards-incompatible change, and the Django security team is aware +that some users of Django were depending on the ability to allow editing of +inlines in the admin form of an otherwise view-only parent model. + +Given the complexity of the Django admin, and in-particular the permissions +related checks, it is the view of the Django security team that this change was +necessary: that it is not currently feasible to maintain the existing behavior +whilst escaping the potential privilege escalation in a way that would avoid a +recurrence of similar issues in the future, and that would be compatible with +Django's *safe by default* philosophy. + +For the time being, developers whose applications are affected by this change +should replace the use of inlines in read-only parents with custom forms and +views that explicitly implement the desired functionality. In the longer term, +adding a documented, supported, and properly-tested mechanism for +partially-editable multi-model forms to the admin interface may occur in Django +itself. + +Thank you to Shen Ying for reporting this issue. Bugfixes ======== diff --git a/docs/releases/2.2.8.txt b/docs/releases/2.2.8.txt index 3c5eb5c754..2c4a6f9ec1 100644 --- a/docs/releases/2.2.8.txt +++ b/docs/releases/2.2.8.txt @@ -4,8 +4,47 @@ Django 2.2.8 release notes *Expected December 2, 2019* -Django 2.2.8 fixes several bugs in 2.2.7 and adds compatibility with Python -3.8. +Django 2.2.8 fixes a security issue, several bugs in 2.2.7, and adds +compatibility with Python 3.8. + +CVE-2019-19118: Privilege escalation in the Django admin. +========================================================= + +Since Django 2.1, a Django model admin displaying a parent model with related +model inlines, where the user has view-only permissions to a parent model but +edit permissions to the inline model, would display a read-only view of the +parent model but editable forms for the inline. + +Submitting these forms would not allow direct edits to the parent model, but +would trigger the parent model's ``save()`` method, and cause pre and post-save +signal handlers to be invoked. This is a privilege escalation as a user who +lacks permission to edit a model should not be able to trigger its save-related +signals. + +To resolve this issue, the permission handling code of the Django admin +interface has been changed. Now, if a user has only the "view" permission for a +parent model, the entire displayed form will not be editable, even if the user +has permission to edit models included in inlines. + +This is a backwards-incompatible change, and the Django security team is aware +that some users of Django were depending on the ability to allow editing of +inlines in the admin form of an otherwise view-only parent model. + +Given the complexity of the Django admin, and in-particular the permissions +related checks, it is the view of the Django security team that this change was +necessary: that it is not currently feasible to maintain the existing behavior +whilst escaping the potential privilege escalation in a way that would avoid a +recurrence of similar issues in the future, and that would be compatible with +Django's *safe by default* philosophy. + +For the time being, developers whose applications are affected by this change +should replace the use of inlines in read-only parents with custom forms and +views that explicitly implement the desired functionality. In the longer term, +adding a documented, supported, and properly-tested mechanism for +partially-editable multi-model forms to the admin interface may occur in Django +itself. + +Thank you to Shen Ying for reporting this issue. Bugfixes ======== diff --git a/tests/admin_inlines/tests.py b/tests/admin_inlines/tests.py index 09fcd0f300..dd40ff675c 100644 --- a/tests/admin_inlines/tests.py +++ b/tests/admin_inlines/tests.py @@ -1,3 +1,5 @@ +from selenium.common.exceptions import NoSuchElementException + from django.contrib.admin import ModelAdmin, TabularInline from django.contrib.admin.helpers import InlineAdminForm from django.contrib.admin.tests import AdminSeleniumTestCase @@ -853,6 +855,98 @@ class TestInlinePermissions(TestCase): @override_settings(ROOT_URLCONF='admin_inlines.urls') +class TestReadOnlyChangeViewInlinePermissions(TestCase): + + @classmethod + def setUpTestData(cls): + cls.user = User.objects.create_user('testing', password='password', is_staff=True) + cls.user.user_permissions.add( + Permission.objects.get(codename='view_poll', content_type=ContentType.objects.get_for_model(Poll)) + ) + cls.user.user_permissions.add( + *Permission.objects.filter( + codename__endswith="question", content_type=ContentType.objects.get_for_model(Question) + ).values_list('pk', flat=True) + ) + + cls.poll = Poll.objects.create(name="Survey") + cls.add_url = reverse('admin:admin_inlines_poll_add') + cls.change_url = reverse('admin:admin_inlines_poll_change', args=(cls.poll.id,)) + + def setUp(self): + self.client.force_login(self.user) + + def test_add_url_not_allowed(self): + response = self.client.get(self.add_url) + self.assertEqual(response.status_code, 403) + + response = self.client.post(self.add_url, {}) + self.assertEqual(response.status_code, 403) + + def test_post_to_change_url_not_allowed(self): + response = self.client.post(self.change_url, {}) + self.assertEqual(response.status_code, 403) + + def test_get_to_change_url_is_allowed(self): + response = self.client.get(self.change_url) + self.assertEqual(response.status_code, 200) + + def test_main_model_is_rendered_as_read_only(self): + response = self.client.get(self.change_url) + self.assertContains( + response, + '<div class="readonly">%s</div>' % self.poll.name, + html=True + ) + input = '<input type="text" name="name" value="%s" class="vTextField" maxlength="40" required id="id_name">' + self.assertNotContains( + response, + input % self.poll.name, + html=True + ) + + def test_inlines_are_rendered_as_read_only(self): + question = Question.objects.create(text="How will this be rendered?", poll=self.poll) + response = self.client.get(self.change_url) + self.assertContains( + response, + '<td class="field-text"><p>%s</p></td>' % question.text, + html=True + ) + self.assertNotContains(response, 'id="id_question_set-0-text"') + self.assertNotContains(response, 'id="id_related_objs-0-DELETE"') + + def test_submit_line_shows_only_close_button(self): + response = self.client.get(self.change_url) + self.assertContains( + response, + '<a href="/admin/admin_inlines/poll/" class="closelink">Close</a>', + html=True + ) + delete_link = '<p class="deletelink-box"><a href="/admin/admin_inlines/poll/%s/delete/" class="deletelink">Delete</a></p>' # noqa + self.assertNotContains( + response, + delete_link % self.poll.id, + html=True + ) + self.assertNotContains(response, '<input type="submit" value="Save and add another" name="_addanother">') + self.assertNotContains(response, '<input type="submit" value="Save and continue editing" name="_continue">') + + def test_inline_delete_buttons_are_not_shown(self): + Question.objects.create(text="How will this be rendered?", poll=self.poll) + response = self.client.get(self.change_url) + self.assertNotContains( + response, + '<input type="checkbox" name="question_set-0-DELETE" id="id_question_set-0-DELETE">', + html=True + ) + + def test_extra_inlines_are_not_shown(self): + response = self.client.get(self.change_url) + self.assertNotContains(response, 'id="id_question_set-0-text"') + + +@override_settings(ROOT_URLCONF='admin_inlines.urls') class SeleniumTests(AdminSeleniumTestCase): available_apps = ['admin_inlines'] + AdminSeleniumTestCase.available_apps @@ -955,6 +1049,24 @@ class SeleniumTests(AdminSeleniumTestCase): self.assertEqual(ProfileCollection.objects.all().count(), 1) self.assertEqual(Profile.objects.all().count(), 3) + def test_add_inline_link_absent_for_view_only_parent_model(self): + user = User.objects.create_user('testing', password='password', is_staff=True) + user.user_permissions.add( + Permission.objects.get(codename='view_poll', content_type=ContentType.objects.get_for_model(Poll)) + ) + user.user_permissions.add( + *Permission.objects.filter( + codename__endswith="question", content_type=ContentType.objects.get_for_model(Question) + ).values_list('pk', flat=True) + ) + self.admin_login(username='testing', password='password') + poll = Poll.objects.create(name="Survey") + change_url = reverse('admin:admin_inlines_poll_change', args=(poll.id,)) + self.selenium.get(self.live_server_url + change_url) + with self.disable_implicit_wait(): + with self.assertRaises(NoSuchElementException): + self.selenium.find_element_by_link_text('Add another Question') + def test_delete_inlines(self): self.admin_login(username='super', password='secret') self.selenium.get(self.live_server_url + reverse('admin:admin_inlines_profilecollection_add')) diff --git a/tests/admin_views/admin.py b/tests/admin_views/admin.py index 0b0ad41e2e..a18fb363aa 100644 --- a/tests/admin_views/admin.py +++ b/tests/admin_views/admin.py @@ -1149,12 +1149,3 @@ class ArticleAdmin9(admin.ModelAdmin): site9 = admin.AdminSite(name='admin9') site9.register(Article, ArticleAdmin9) - - -class ArticleAdmin10(admin.ModelAdmin): - def has_change_permission(self, request, obj=None): - return False - - -site10 = admin.AdminSite(name='admin10') -site10.register(Article, ArticleAdmin10) diff --git a/tests/admin_views/tests.py b/tests/admin_views/tests.py index 0cc16509ff..9fdca85c0d 100644 --- a/tests/admin_views/tests.py +++ b/tests/admin_views/tests.py @@ -1775,8 +1775,7 @@ class AdminViewPermissionsTest(TestCase): self.assertEqual(post.status_code, 403) self.client.get(reverse('admin:logout')) - # view user should be able to view the article but not change any of them - # (the POST can be sent, but no modification occurs) + # view user can view articles but not make changes. self.client.force_login(self.viewuser) response = self.client.get(article_changelist_url) self.assertEqual(response.status_code, 200) @@ -1787,7 +1786,7 @@ class AdminViewPermissionsTest(TestCase): self.assertContains(response, '<label>Extra form field:</label>') self.assertContains(response, '<a href="/test_admin/admin/admin_views/article/" class="closelink">Close</a>') post = self.client.post(article_change_url, change_dict) - self.assertEqual(post.status_code, 302) + self.assertEqual(post.status_code, 403) self.assertEqual(Article.objects.get(pk=self.a1.pk).content, '<p>Middle content</p>') self.client.get(reverse('admin:logout')) @@ -1845,7 +1844,7 @@ class AdminViewPermissionsTest(TestCase): response = self.client.get(change_url_3) self.assertEqual(response.status_code, 200) response = self.client.post(change_url_3, {'name': 'changed'}) - self.assertRedirects(response, self.index_url) + self.assertEqual(response.status_code, 403) self.assertEqual(RowLevelChangePermissionModel.objects.get(id=3).name, 'odd id mult 3') response = self.client.get(change_url_6) self.assertEqual(response.status_code, 200) @@ -1882,21 +1881,6 @@ class AdminViewPermissionsTest(TestCase): self.assertEqual(response.context['title'], 'View article') self.assertContains(response, '<a href="/test_admin/admin9/admin_views/article/" class="closelink">Close</a>') - def test_change_view_post_without_object_change_permission(self): - """A POST redirects to changelist without modifications.""" - change_dict = { - 'title': 'Ikke fordømt', - 'content': '<p>edited article</p>', - 'date_0': '2008-03-18', 'date_1': '10:54:39', - 'section': self.s1.pk, - } - change_url = reverse('admin10:admin_views_article_change', args=(self.a1.pk,)) - changelist_url = reverse('admin10:admin_views_article_changelist') - self.client.force_login(self.viewuser) - response = self.client.post(change_url, change_dict) - self.assertRedirects(response, changelist_url) - self.assertEqual(Article.objects.get(pk=self.a1.pk).content, '<p>Middle content</p>') - def test_change_view_save_as_new(self): """ 'Save as new' should raise PermissionDenied for users without the 'add' @@ -4053,52 +4037,6 @@ class AdminInlineTests(TestCase): self.assertEqual(Widget.objects.count(), 1) self.assertEqual(Widget.objects.all()[0].name, "Widget 1 Updated") - def test_simple_inline_permissions(self): - """ - Changes aren't allowed without change permissions for the inline object. - """ - # User who can view Articles - permissionuser = User.objects.create_user( - username='permissionuser', password='secret', - email='vuser@example.com', is_staff=True, - ) - permissionuser.user_permissions.add(get_perm(Collector, get_permission_codename('view', Collector._meta))) - permissionuser.user_permissions.add(get_perm(Widget, get_permission_codename('view', Widget._meta))) - self.client.force_login(permissionuser) - # Without add permission, a new inline can't be added. - self.post_data['widget_set-0-name'] = 'Widget 1' - collector_url = reverse('admin:admin_views_collector_change', args=(self.collector.pk,)) - response = self.client.post(collector_url, self.post_data) - self.assertEqual(response.status_code, 302) - self.assertEqual(Widget.objects.count(), 0) - # But after adding the permisson it can. - permissionuser.user_permissions.add(get_perm(Widget, get_permission_codename('add', Widget._meta))) - self.post_data['widget_set-0-name'] = "Widget 1" - collector_url = reverse('admin:admin_views_collector_change', args=(self.collector.pk,)) - response = self.client.post(collector_url, self.post_data) - self.assertEqual(response.status_code, 302) - self.assertEqual(Widget.objects.count(), 1) - self.assertEqual(Widget.objects.first().name, 'Widget 1') - widget_id = Widget.objects.first().id - # Without the change permission, a POST doesn't change the object. - self.post_data['widget_set-INITIAL_FORMS'] = '1' - self.post_data['widget_set-0-id'] = str(widget_id) - self.post_data['widget_set-0-name'] = 'Widget 1 Updated' - response = self.client.post(collector_url, self.post_data) - self.assertEqual(response.status_code, 302) - self.assertEqual(Widget.objects.count(), 1) - self.assertEqual(Widget.objects.first().name, 'Widget 1') - # Now adding the change permission and editing works. - permissionuser.user_permissions.remove(get_perm(Widget, get_permission_codename('add', Widget._meta))) - permissionuser.user_permissions.add(get_perm(Widget, get_permission_codename('change', Widget._meta))) - self.post_data['widget_set-INITIAL_FORMS'] = '1' - self.post_data['widget_set-0-id'] = str(widget_id) - self.post_data['widget_set-0-name'] = 'Widget 1 Updated' - response = self.client.post(collector_url, self.post_data) - self.assertEqual(response.status_code, 302) - self.assertEqual(Widget.objects.count(), 1) - self.assertEqual(Widget.objects.first().name, 'Widget 1 Updated') - def test_explicit_autofield_inline(self): "A model with an explicit autofield primary key can be saved as inlines. Regression for #8093" # First add a new inline diff --git a/tests/admin_views/urls.py b/tests/admin_views/urls.py index fdb61d759d..ca684b2f2e 100644 --- a/tests/admin_views/urls.py +++ b/tests/admin_views/urls.py @@ -17,7 +17,6 @@ urlpatterns = [ # All admin views accept `extra_context` to allow adding it like this: path('test_admin/admin8/', (admin.site.get_urls(), 'admin', 'admin-extra-context'), {'extra_context': {}}), path('test_admin/admin9/', admin.site9.urls), - path('test_admin/admin10/', admin.site10.urls), path('test_admin/has_permission_admin/', custom_has_permission_admin.site.urls), path('test_admin/autocomplete_admin/', autocomplete_site.urls), ] diff --git a/tests/auth_tests/test_views.py b/tests/auth_tests/test_views.py index e7ad1cbcdf..67d4840446 100644 --- a/tests/auth_tests/test_views.py +++ b/tests/auth_tests/test_views.py @@ -1246,7 +1246,7 @@ class ChangelistTests(AuthViewsTestCase): data['password'] = 'shouldnotchange' change_url = reverse('auth_test_admin:auth_user_change', args=(u.pk,)) response = self.client.post(change_url, data) - self.assertRedirects(response, reverse('auth_test_admin:auth_user_changelist')) + self.assertEqual(response.status_code, 403) u.refresh_from_db() self.assertEqual(u.password, original_password) |