티스토리 수익 글 보기

티스토리 수익 글 보기

Fixed CVE-2019-19118 — Required edit permissions on parent model for… · django/django@103ebe2 · GitHub
Skip to content

Commit 103ebe2

Browse files
carltongibsoncarltongibson
committed
Fixed CVE-2019-19118 — Required edit permissions on parent model for editable inlines in admin.
Thank you to Shen Ying for reporting this issue.
1 parent f57f81a commit 103ebe2

File tree

10 files changed

+175
–86
lines changed

10 files changed

+175
–86
lines changed

‎django/contrib/admin/options.py‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1474,13 +1474,20 @@ def render_delete_form(self, request, context):
14741474
)
14751475

14761476
def get_inline_formsets(self, request, formsets, inline_instances, obj=None):
1477+
# Edit permissions on parent model are required for editable inlines.
1478+
can_edit_parent = self.has_change_permission(request, obj) if obj else self.has_add_permission(request)
14771479
inline_admin_formsets = []
14781480
for inline, formset in zip(inline_instances, formsets):
14791481
fieldsets = list(inline.get_fieldsets(request, obj))
14801482
readonly = list(inline.get_readonly_fields(request, obj))
1481-
has_add_permission = inline._has_add_permission(request, obj)
1482-
has_change_permission = inline.has_change_permission(request, obj)
1483-
has_delete_permission = inline.has_delete_permission(request, obj)
1483+
if can_edit_parent:
1484+
has_add_permission = inline._has_add_permission(request, obj)
1485+
has_change_permission = inline.has_change_permission(request, obj)
1486+
has_delete_permission = inline.has_delete_permission(request, obj)
1487+
else:
1488+
# Disable all edit-permissions, and overide formset settings.
1489+
has_add_permission = has_change_permission = has_delete_permission = False
1490+
formset.extra = formset.max_num = 0
14841491
has_view_permission = inline.has_view_permission(request, obj)
14851492
prepopulated = dict(inline.get_prepopulated_fields(request, obj))
14861493
inline_admin_formset = helpers.InlineAdminFormSet(
@@ -1545,8 +1552,12 @@ def _changeform_view(self, request, object_id, form_url, extra_context):
15451552
else:
15461553
obj = self.get_object(request, unquote(object_id), to_field)
15471554

1548-
if not self.has_view_or_change_permission(request, obj):
1549-
raise PermissionDenied
1555+
if request.method == 'POST':
1556+
if not self.has_change_permission(request, obj):
1557+
raise PermissionDenied
1558+
else:
1559+
if not self.has_view_or_change_permission(request, obj):
1560+
raise PermissionDenied
15501561

15511562
if obj is None:
15521563
return self._get_obj_does_not_exist_redirect(request, opts, object_id)

‎django/contrib/admin/templates/admin/edit_inline/stacked.html‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ <h2>{{ inline_admin_formset.opts.verbose_name_plural|capfirst }}</h2>
1212
<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 %}
1313
{% else %}#{{ forloop.counter }}{% endif %}</span>
1414
{% if inline_admin_form.show_url %}<a href="{{ inline_admin_form.absolute_url }}">{% trans "View on site" %}</a>{% endif %}
15-
{% 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 %}
15+
{% 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 %}
1616
</h3>
1717
{% if inline_admin_form.form.non_field_errors %}{{ inline_admin_form.form.non_field_errors }}{% endif %}
1818
{% for fieldset in inline_admin_form %}

‎django/contrib/admin/templates/admin/edit_inline/tabular.html‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ <h2>{{ inline_admin_formset.opts.verbose_name_plural|capfirst }}</h2>
1717
</th>
1818
{% endif %}
1919
{% endfor %}
20-
{% if inline_admin_formset.formset.can_delete %}<th>{% trans "Delete?" %}</th>{% endif %}
20+
{% if inline_admin_formset.formset.can_delete and inline_admin_formset.has_delete_permission %}<th>{% trans "Delete?" %}</th>{% endif %}
2121
</tr></thead>
2222

2323
<tbody>
@@ -63,7 +63,7 @@ <h2>{{ inline_admin_formset.opts.verbose_name_plural|capfirst }}</h2>
6363
{% endfor %}
6464
{% endfor %}
6565
{% endfor %}
66-
{% if inline_admin_formset.formset.can_delete %}
66+
{% if inline_admin_formset.formset.can_delete and inline_admin_formset.has_delete_permission %}
6767
<td class="delete">{% if inline_admin_form.original %}{{ inline_admin_form.deletion_field.field }}{% endif %}</td>
6868
{% endif %}
6969
</tr>

‎docs/releases/2.1.15.txt‎

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,46 @@ Django 2.1.15 release notes
44

55
*Expected December 2, 2019*
66

7-
Django 2.1.15 fixes a data loss bug in 2.1.14.
7+
Django 2.1.15 fixes a security issue and a data loss bug in 2.1.14.
8+
9+
CVE-2019-19118: Privilege escalation in the Django admin.
10+
=========================================================
11+
12+
Since Django 2.1, a Django model admin displaying a parent model with related
13+
model inlines, where the user has view-only permissions to a parent model but
14+
edit permissions to the inline model, would display a read-only view of the
15+
parent model but editable forms for the inline.
16+
17+
Submitting these forms would not allow direct edits to the parent model, but
18+
would trigger the parent model's ``save()`` method, and cause pre and post-save
19+
signal handlers to be invoked. This is a privilege escalation as a user who
20+
lacks permission to edit a model should not be able to trigger its save-related
21+
signals.
22+
23+
To resolve this issue, the permission handling code of the Django admin
24+
interface has been changed. Now, if a user has only the "view" permission for a
25+
parent model, the entire displayed form will not be editable, even if the user
26+
has permission to edit models included in inlines.
27+
28+
This is a backwards-incompatible change, and the Django security team is aware
29+
that some users of Django were depending on the ability to allow editing of
30+
inlines in the admin form of an otherwise view-only parent model.
31+
32+
Given the complexity of the Django admin, and in-particular the permissions
33+
related checks, it is the view of the Django security team that this change was
34+
necessary: that it is not currently feasible to maintain the existing behavior
35+
whilst escaping the potential privilege escalation in a way that would avoid a
36+
recurrence of similar issues in the future, and that would be compatible with
37+
Django's *safe by default* philosophy.
38+
39+
For the time being, developers whose applications are affected by this change
40+
should replace the use of inlines in read-only parents with custom forms and
41+
views that explicitly implement the desired functionality. In the longer term,
42+
adding a documented, supported, and properly-tested mechanism for
43+
partially-editable multi-model forms to the admin interface may occur in Django
44+
itself.
45+
46+
Thank you to Shen Ying for reporting this issue.
847

948
Bugfixes
1049
========

‎tests/admin_inlines/models.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,7 @@ class Poll(models.Model):
155155

156156

157157
class Question(models.Model):
158+
text = models.CharField(max_length=40)
158159
poll = models.ForeignKey(Poll, models.CASCADE)
159160

160161

‎tests/admin_inlines/tests.py‎

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
from selenium.common.exceptions import NoSuchElementException
2+
13
from django.contrib.admin import ModelAdmin, TabularInline
24
from django.contrib.admin.helpers import InlineAdminForm
35
from django.contrib.admin.tests import AdminSeleniumTestCase
@@ -837,6 +839,97 @@ def test_inline_change_fk_all_perms(self):
837839
)
838840

839841

842+
@override_settings(ROOT_URLCONF='admin_inlines.urls')
843+
class TestReadOnlyChangeViewInlinePermissions(TestCase):
844+
845+
@classmethod
846+
def setUpTestData(cls):
847+
cls.user = User.objects.create_user('testing', password='password', is_staff=True)
848+
cls.user.user_permissions.add(
849+
Permission.objects.get(codename='view_poll', content_type=ContentType.objects.get_for_model(Poll))
850+
)
851+
cls.user.user_permissions.add(
852+
*Permission.objects.filter(
853+
codename__endswith="question", content_type=ContentType.objects.get_for_model(Question)
854+
).values_list('pk', flat=True)
855+
)
856+
cls.poll = Poll.objects.create(name="Survey")
857+
cls.add_url = reverse('admin:admin_inlines_poll_add')
858+
cls.change_url = reverse('admin:admin_inlines_poll_change', args=(cls.poll.id,))
859+
860+
def setUp(self):
861+
self.client.force_login(self.user)
862+
863+
def test_add_url_not_allowed(self):
864+
response = self.client.get(self.add_url)
865+
self.assertEqual(response.status_code, 403)
866+
867+
response = self.client.post(self.add_url, {})
868+
self.assertEqual(response.status_code, 403)
869+
870+
def test_post_to_change_url_not_allowed(self):
871+
response = self.client.post(self.change_url, {})
872+
self.assertEqual(response.status_code, 403)
873+
874+
def test_get_to_change_url_is_allowed(self):
875+
response = self.client.get(self.change_url)
876+
self.assertEqual(response.status_code, 200)
877+
878+
def test_main_model_is_rendered_as_read_only(self):
879+
response = self.client.get(self.change_url)
880+
self.assertContains(
881+
response,
882+
'<div class="readonly">%s</div>' % self.poll.name,
883+
html=True
884+
)
885+
input = '<input type="text" name="name" value="%s" class="vTextField" maxlength="40" required id="id_name">'
886+
self.assertNotContains(
887+
response,
888+
input % self.poll.name,
889+
html=True
890+
)
891+
892+
def test_inlines_are_rendered_as_read_only(self):
893+
question = Question.objects.create(text="How will this be rendered?", poll=self.poll)
894+
response = self.client.get(self.change_url)
895+
self.assertContains(
896+
response,
897+
'<td class="field-text"><p>%s</p></td>' % question.text,
898+
html=True
899+
)
900+
self.assertNotContains(response, 'id="id_question_set-0-text"')
901+
self.assertNotContains(response, 'id="id_related_objs-0-DELETE"')
902+
903+
def test_submit_line_shows_only_close_button(self):
904+
response = self.client.get(self.change_url)
905+
self.assertContains(
906+
response,
907+
'<a href="/admin/admin_inlines/poll/" class="closelink">Close</a>',
908+
html=True
909+
)
910+
delete_link = '<p class="deletelink-box"><a href="/admin/admin_inlines/poll/%s/delete/" class="deletelink">Delete</a></p>' # noqa
911+
self.assertNotContains(
912+
response,
913+
delete_link % self.poll.id,
914+
html=True
915+
)
916+
self.assertNotContains(response, '<input type="submit" value="Save and add another" name="_addanother">')
917+
self.assertNotContains(response, '<input type="submit" value="Save and continue editing" name="_continue">')
918+
919+
def test_inline_delete_buttons_are_not_shown(self):
920+
Question.objects.create(text="How will this be rendered?", poll=self.poll)
921+
response = self.client.get(self.change_url)
922+
self.assertNotContains(
923+
response,
924+
'<input type="checkbox" name="question_set-0-DELETE" id="id_question_set-0-DELETE">',
925+
html=True
926+
)
927+
928+
def test_extra_inlines_are_not_shown(self):
929+
response = self.client.get(self.change_url)
930+
self.assertNotContains(response, 'id="id_question_set-0-text"')
931+
932+
840933
@override_settings(ROOT_URLCONF='admin_inlines.urls')
841934
class SeleniumTests(AdminSeleniumTestCase):
842935

@@ -940,6 +1033,24 @@ def test_add_inlines(self):
9401033
self.assertEqual(ProfileCollection.objects.all().count(), 1)
9411034
self.assertEqual(Profile.objects.all().count(), 3)
9421035

1036+
def test_add_inline_link_absent_for_view_only_parent_model(self):
1037+
user = User.objects.create_user('testing', password='password', is_staff=True)
1038+
user.user_permissions.add(
1039+
Permission.objects.get(codename='view_poll', content_type=ContentType.objects.get_for_model(Poll))
1040+
)
1041+
user.user_permissions.add(
1042+
*Permission.objects.filter(
1043+
codename__endswith="question", content_type=ContentType.objects.get_for_model(Question)
1044+
).values_list('pk', flat=True)
1045+
)
1046+
self.admin_login(username='testing', password='password')
1047+
poll = Poll.objects.create(name="Survey")
1048+
change_url = reverse('admin:admin_inlines_poll_change', args=(poll.id,))
1049+
self.selenium.get(self.live_server_url + change_url)
1050+
with self.disable_implicit_wait():
1051+
with self.assertRaises(NoSuchElementException):
1052+
self.selenium.find_element_by_link_text('Add another Question')
1053+
9431054
def test_delete_inlines(self):
9441055
self.admin_login(username='super', password='secret')
9451056
self.selenium.get(self.live_server_url + reverse('admin:admin_inlines_profilecollection_add'))

‎tests/admin_views/admin.py‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1143,12 +1143,3 @@ def has_change_permission(self, request, obj=None):
11431143

11441144
site9 = admin.AdminSite(name='admin9')
11451145
site9.register(Article, ArticleAdmin9)
1146-
1147-
1148-
class ArticleAdmin10(admin.ModelAdmin):
1149-
def has_change_permission(self, request, obj=None):
1150-
return False
1151-
1152-
1153-
site10 = admin.AdminSite(name='admin10')
1154-
site10.register(Article, ArticleAdmin10)

‎tests/admin_views/tests.py‎

Lines changed: 3 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -1778,8 +1778,7 @@ def test_change_view(self):
17781778
self.assertEqual(post.status_code, 403)
17791779
self.client.get(reverse('admin:logout'))
17801780

1781-
# view user should be able to view the article but not change any of them
1782-
# (the POST can be sent, but no modification occurs)
1781+
# view user can view articles but not make changes.
17831782
self.client.force_login(self.viewuser)
17841783
response = self.client.get(article_changelist_url)
17851784
self.assertEqual(response.status_code, 200)
@@ -1790,7 +1789,7 @@ def test_change_view(self):
17901789
self.assertContains(response, '<label>Extra form field:</label>')
17911790
self.assertContains(response, '<a href="/test_admin/admin/admin_views/article/" class="closelink">Close</a>')
17921791
post = self.client.post(article_change_url, change_dict)
1793-
self.assertEqual(post.status_code, 302)
1792+
self.assertEqual(post.status_code, 403)
17941793
self.assertEqual(Article.objects.get(pk=self.a1.pk).content, '<p>Middle content</p>')
17951794
self.client.get(reverse('admin:logout'))
17961795

@@ -1847,8 +1846,7 @@ def test_change_view(self):
18471846
response = self.client.get(change_url_3)
18481847
self.assertEqual(response.status_code, 200)
18491848
response = self.client.post(change_url_3, {'name': 'changed'})
1850-
self.assertEqual(response.status_code, 302)
1851-
self.assertRedirects(response, self.index_url)
1849+
self.assertEqual(response.status_code, 403)
18521850
self.assertEqual(RowLevelChangePermissionModel.objects.get(id=3).name, 'odd id mult 3')
18531851
response = self.client.get(change_url_6)
18541852
self.assertEqual(response.status_code, 200)
@@ -1884,21 +1882,6 @@ def test_change_view_without_object_change_permission(self):
18841882
self.assertEqual(response.context['title'], 'View article')
18851883
self.assertContains(response, '<a href="/test_admin/admin9/admin_views/article/" class="closelink">Close</a>')
18861884

1887-
def test_change_view_post_without_object_change_permission(self):
1888-
"""A POST redirectS to changelist without modifications."""
1889-
change_dict = {
1890-
'title': 'Ikke fordømt',
1891-
'content': '<p>edited article</p>',
1892-
'date_0': '2008-03-18', 'date_1': '10:54:39',
1893-
'section': self.s1.pk,
1894-
}
1895-
change_url = reverse('admin10:admin_views_article_change', args=(self.a1.pk,))
1896-
changelist_url = reverse('admin10:admin_views_article_changelist')
1897-
self.client.force_login(self.viewuser)
1898-
response = self.client.post(change_url, change_dict)
1899-
self.assertRedirects(response, changelist_url)
1900-
self.assertEqual(Article.objects.get(pk=self.a1.pk).content, '<p>Middle content</p>')
1901-
19021885
def test_change_view_save_as_new(self):
19031886
"""
19041887
'Save as new' should raise PermissionDenied for users without the 'add'
@@ -3981,52 +3964,6 @@ def test_simple_inline(self):
39813964
self.assertEqual(Widget.objects.count(), 1)
39823965
self.assertEqual(Widget.objects.all()[0].name, "Widget 1 Updated")
39833966

3984-
def test_simple_inline_permissions(self):
3985-
"""
3986-
Changes aren't allowed without change permissions for the inline object.
3987-
"""
3988-
# User who can view Articles
3989-
permissionuser = User.objects.create_user(
3990-
username='permissionuser', password='secret',
3991-
email='vuser@example.com', is_staff=True,
3992-
)
3993-
permissionuser.user_permissions.add(get_perm(Collector, get_permission_codename('view', Collector._meta)))
3994-
permissionuser.user_permissions.add(get_perm(Widget, get_permission_codename('view', Widget._meta)))
3995-
self.client.force_login(permissionuser)
3996-
# Without add permission, a new inline can't be added.
3997-
self.post_data['widget_set-0-name'] = 'Widget 1'
3998-
collector_url = reverse('admin:admin_views_collector_change', args=(self.collector.pk,))
3999-
response = self.client.post(collector_url, self.post_data)
4000-
self.assertEqual(response.status_code, 302)
4001-
self.assertEqual(Widget.objects.count(), 0)
4002-
# But after adding the permisson it can.
4003-
permissionuser.user_permissions.add(get_perm(Widget, get_permission_codename('add', Widget._meta)))
4004-
self.post_data['widget_set-0-name'] = "Widget 1"
4005-
collector_url = reverse('admin:admin_views_collector_change', args=(self.collector.pk,))
4006-
response = self.client.post(collector_url, self.post_data)
4007-
self.assertEqual(response.status_code, 302)
4008-
self.assertEqual(Widget.objects.count(), 1)
4009-
self.assertEqual(Widget.objects.first().name, 'Widget 1')
4010-
widget_id = Widget.objects.first().id
4011-
# Without the change permission, a POST doesn't change the object.
4012-
self.post_data['widget_set-INITIAL_FORMS'] = '1'
4013-
self.post_data['widget_set-0-id'] = str(widget_id)
4014-
self.post_data['widget_set-0-name'] = 'Widget 1 Updated'
4015-
response = self.client.post(collector_url, self.post_data)
4016-
self.assertEqual(response.status_code, 302)
4017-
self.assertEqual(Widget.objects.count(), 1)
4018-
self.assertEqual(Widget.objects.first().name, 'Widget 1')
4019-
# Now adding the change permission and editing works.
4020-
permissionuser.user_permissions.remove(get_perm(Widget, get_permission_codename('add', Widget._meta)))
4021-
permissionuser.user_permissions.add(get_perm(Widget, get_permission_codename('change', Widget._meta)))
4022-
self.post_data['widget_set-INITIAL_FORMS'] = '1'
4023-
self.post_data['widget_set-0-id'] = str(widget_id)
4024-
self.post_data['widget_set-0-name'] = 'Widget 1 Updated'
4025-
response = self.client.post(collector_url, self.post_data)
4026-
self.assertEqual(response.status_code, 302)
4027-
self.assertEqual(Widget.objects.count(), 1)
4028-
self.assertEqual(Widget.objects.first().name, 'Widget 1 Updated')
4029-
40303967
def test_explicit_autofield_inline(self):
40313968
"A model with an explicit autofield primary key can be saved as inlines. Regression for #8093"
40323969
# First add a new inline

‎tests/admin_views/urls.py‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
# All admin views accept `extra_context` to allow adding it like this:
1818
url(r'^test_admin/admin8/', (admin.site.get_urls(), 'admin', 'admin-extra-context'), {'extra_context': {}}),
1919
url(r'^test_admin/admin9/', admin.site9.urls),
20-
url(r'^test_admin/admin10/', admin.site10.urls),
2120
url(r'^test_admin/has_permission_admin/', custom_has_permission_admin.site.urls),
2221
url(r'^test_admin/autocomplete_admin/', autocomplete_site.urls),
2322
]

‎tests/auth_tests/test_views.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1251,7 +1251,7 @@ def test_view_user_password_is_readonly(self):
12511251
data['password'] = 'shouldnotchange'
12521252
change_url = reverse('auth_test_admin:auth_user_change', args=(u.pk,))
12531253
response = self.client.post(change_url, data)
1254-
self.assertRedirects(response, reverse('auth_test_admin:auth_user_changelist'))
1254+
self.assertEqual(response.status_code, 403)
12551255
u.refresh_from_db()
12561256
self.assertEqual(u.password, original_password)
12571257

0 commit comments

Comments
 (0)