Skip to content
Permalink
Browse files

Fixed CVE-2019-19118 -- Required edit permissions on parent model for…

… editable inlines in admin.

Thank you to Shen Ying for reporting this issue.
  • Loading branch information
carltongibson committed Nov 14, 2019
1 parent 39e39d0 commit 11c5e0609bcc0db93809de2a08e0dc3d70b393e4
@@ -1464,13 +1464,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(
@@ -1535,8 +1542,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)
@@ -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 %}
@@ -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>
@@ -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
========
@@ -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
========
@@ -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
@@ -862,6 +864,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):

@@ -1057,6 +1151,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'))
@@ -1178,12 +1178,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)

0 comments on commit 11c5e06

Please sign in to comment.
You can’t perform that action at this time.