p31829507
/
django
mirror of https://github.com/django/django.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

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

Thank you to Shen Ying for reporting this issue.
This commit is contained in:
Carlton Gibson 2019-11-25 11:32:30 +01:00
parent f57f81a7fe
commit 103ebe2b5f
10 changed files with 175 additions and 86 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
django/contrib/admin/options.py
View File

@ -1474,13 +1474,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(
@ -1545,8 +1552,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)

2
django/contrib/admin/templates/admin/edit_inline/stacked.html
View File

@ -12,7 +12,7 @@
<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 %}

4
django/contrib/admin/templates/admin/edit_inline/tabular.html
View File

@ -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>

41
docs/releases/2.1.15.txt
View File

@ -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
========

1
tests/admin_inlines/models.py
View File

@ -155,6 +155,7 @@ class Poll(models.Model):
class Question(models.Model):
text = models.CharField(max_length=40)
poll = models.ForeignKey(Poll, models.CASCADE)

111
tests/admin_inlines/tests.py
View File

@ -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 @@ 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):
@ -940,6 +1033,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'))

9
tests/admin_views/admin.py
View File

@ -1143,12 +1143,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)

69
tests/admin_views/tests.py
View File

@ -1778,8 +1778,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)
@ -1790,7 +1789,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'))
@ -1847,8 +1846,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.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 @@ 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'
@ -3981,52 +3964,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

1
tests/admin_views/urls.py
View File

@ -17,7 +17,6 @@ urlpatterns = [
# 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),
]

2
tests/auth_tests/test_views.py
View File

@ -1251,7 +1251,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)