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 25, 2019
1 parent f57f81a commit 103ebe2b5ff1b2614b85a52c239f471904d26244
@@ -1474,13 +1474,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(
@@ -1545,8 +1552,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
========
@@ -155,6 +155,7 @@ class Poll(models.Model):


class Question(models.Model):
text = models.CharField(max_length=40)
poll = models.ForeignKey(Poll, models.CASCADE)


@@ -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
@@ -837,6 +839,97 @@ 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):

@@ -940,6 +1033,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'))
@@ -1143,12 +1143,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)
@@ -1778,8 +1778,7 @@ def test_change_view(self):
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)
@@ -1790,7 +1789,7 @@ def test_change_view(self):
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'))

@@ -1847,8 +1846,7 @@ def test_change_view(self):
response = self.client.get(change_url_3)
self.assertEqual(response.status_code, 200)
response = self.client.post(change_url_3, {'name': 'changed'})
self.assertEqual(response.status_code, 302)
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)
@@ -1884,21 +1882,6 @@ def test_change_view_without_object_change_permission(self):
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'
@@ -3981,52 +3964,6 @@ def test_simple_inline(self):
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
@@ -17,7 +17,6 @@
# All admin views accept `extra_context` to allow adding it like this:
url(r'^test_admin/admin8/', (admin.site.get_urls(), 'admin', 'admin-extra-context'), {'extra_context': {}}),
url(r'^test_admin/admin9/', admin.site9.urls),
url(r'^test_admin/admin10/', admin.site10.urls),
url(r'^test_admin/has_permission_admin/', custom_has_permission_admin.site.urls),
url(r'^test_admin/autocomplete_admin/', autocomplete_site.urls),
]
@@ -1251,7 +1251,7 @@ def test_view_user_password_is_readonly(self):
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)

0 comments on commit 103ebe2

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