Fixed CVE-2019-19118 -- Required edit permissions on parent model for editable inlines in admin.

carltongibson · carltongibson · commit 36f580a17f0b · 2019-12-02T08:58:14.000+01:00
Thank you to Shen Ying for reporting this issue.
diff --git a/django/contrib/admin/options.py b/django/contrib/admin/options.py
@@ -1471,13 +1471,20 @@ def render_delete_form(self, request, context):
         )
 
     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 @@ def _changeform_view(self, request, object_id, form_url, extra_context):
         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
@@ -12,7 +12,7 @@ <h2>{{ inline_admin_formset.opts.verbose_name_plural|capfirst }}</h2>
   <h3><b>{{ inline_admin_formset.opts.verbose_name|capfirst }}:</b>&nbsp;<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
@@ -17,7 +17,7 @@ <h2>{{ inline_admin_formset.opts.verbose_name_plural|capfirst }}</h2>
          </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 @@ <h2>{{ inline_admin_formset.opts.verbose_name_plural|capfirst }}</h2>
             {% 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
@@ -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
@@ -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
@@ -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
@@ -852,6 +854,98 @@ def test_inline_change_fk_all_perms(self):
         )
 
 
+@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):
 
@@ -955,6 +1049,24 @@ def test_add_inlines(self):
         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
@@ -1149,12 +1149,3 @@ def has_change_permission(self, request, obj=None):
 
 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
diff --git a/tests/admin_views/urls.py b/tests/admin_views/urls.py
diff --git a/tests/auth_tests/test_views.py b/tests/auth_tests/test_views.py
