Skip to content

Commit 092cd66

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 db0cc4a commit 092cd66

File tree

10 files changed

+216
-87
lines changed

10 files changed

+216
-87
lines changed

‎django/contrib/admin/options.py

+16-5
Original file line numberDiff line numberDiff line change
@@ -1464,13 +1464,20 @@ def render_delete_form(self, request, context):
14641464
)
14651465

14661466
def get_inline_formsets(self, request, formsets, inline_instances, obj=None):
1467+
# Edit permissions on parent model are required for editable inlines.
1468+
can_edit_parent = self.has_change_permission(request, obj) if obj else self.has_add_permission(request)
14671469
inline_admin_formsets = []
14681470
for inline, formset in zip(inline_instances, formsets):
14691471
fieldsets = list(inline.get_fieldsets(request, obj))
14701472
readonly = list(inline.get_readonly_fields(request, obj))
1471-
has_add_permission = inline.has_add_permission(request, obj)
1472-
has_change_permission = inline.has_change_permission(request, obj)
1473-
has_delete_permission = inline.has_delete_permission(request, obj)
1473+
if can_edit_parent:
1474+
has_add_permission = inline.has_add_permission(request, obj)
1475+
has_change_permission = inline.has_change_permission(request, obj)
1476+
has_delete_permission = inline.has_delete_permission(request, obj)
1477+
else:
1478+
# Disable all edit-permissions, and overide formset settings.
1479+
has_add_permission = has_change_permission = has_delete_permission = False
1480+
formset.extra = formset.max_num = 0
14741481
has_view_permission = inline.has_view_permission(request, obj)
14751482
prepopulated = dict(inline.get_prepopulated_fields(request, obj))
14761483
inline_admin_formset = helpers.InlineAdminFormSet(
@@ -1535,8 +1542,12 @@ def _changeform_view(self, request, object_id, form_url, extra_context):
15351542
else:
15361543
obj = self.get_object(request, unquote(object_id), to_field)
15371544

1538-
if not self.has_view_or_change_permission(request, obj):
1539-
raise PermissionDenied
1545+
if request.method == 'POST':
1546+
if not self.has_change_permission(request, obj):
1547+
raise PermissionDenied
1548+
else:
1549+
if not self.has_view_or_change_permission(request, obj):
1550+
raise PermissionDenied
15401551

15411552
if obj is None:
15421553
return self._get_obj_does_not_exist_redirect(request, opts, object_id)

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

+1-1
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

+2-2
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

+40-1
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
========

‎docs/releases/2.2.8.txt

+41-2
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,47 @@ Django 2.2.8 release notes
44

55
*Expected December 2, 2019*
66

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

1049
Bugfixes
1150
========

‎tests/admin_inlines/tests.py

+112
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
@@ -862,6 +864,98 @@ def test_inline_change_fk_all_perms(self):
862864
)
863865

864866

867+
@override_settings(ROOT_URLCONF='admin_inlines.urls')
868+
class TestReadOnlyChangeViewInlinePermissions(TestCase):
869+
870+
@classmethod
871+
def setUpTestData(cls):
872+
cls.user = User.objects.create_user('testing', password='password', is_staff=True)
873+
cls.user.user_permissions.add(
874+
Permission.objects.get(codename='view_poll', content_type=ContentType.objects.get_for_model(Poll))
875+
)
876+
cls.user.user_permissions.add(
877+
*Permission.objects.filter(
878+
codename__endswith="question", content_type=ContentType.objects.get_for_model(Question)
879+
).values_list('pk', flat=True)
880+
)
881+
882+
cls.poll = Poll.objects.create(name="Survey")
883+
cls.add_url = reverse('admin:admin_inlines_poll_add')
884+
cls.change_url = reverse('admin:admin_inlines_poll_change', args=(cls.poll.id,))
885+
886+
def setUp(self):
887+
self.client.force_login(self.user)
888+
889+
def test_add_url_not_allowed(self):
890+
response = self.client.get(self.add_url)
891+
self.assertEqual(response.status_code, 403)
892+
893+
response = self.client.post(self.add_url, {})
894+
self.assertEqual(response.status_code, 403)
895+
896+
def test_post_to_change_url_not_allowed(self):
897+
response = self.client.post(self.change_url, {})
898+
self.assertEqual(response.status_code, 403)
899+
900+
def test_get_to_change_url_is_allowed(self):
901+
response = self.client.get(self.change_url)
902+
self.assertEqual(response.status_code, 200)
903+
904+
def test_main_model_is_rendered_as_read_only(self):
905+
response = self.client.get(self.change_url)
906+
self.assertContains(
907+
response,
908+
'<div class="readonly">%s</div>' % self.poll.name,
909+
html=True
910+
)
911+
input = '<input type="text" name="name" value="%s" class="vTextField" maxlength="40" required id="id_name">'
912+
self.assertNotContains(
913+
response,
914+
input % self.poll.name,
915+
html=True
916+
)
917+
918+
def test_inlines_are_rendered_as_read_only(self):
919+
question = Question.objects.create(text="How will this be rendered?", poll=self.poll)
920+
response = self.client.get(self.change_url)
921+
self.assertContains(
922+
response,
923+
'<td class="field-text"><p>%s</p></td>' % question.text,
924+
html=True
925+
)
926+
self.assertNotContains(response, 'id="id_question_set-0-text"')
927+
self.assertNotContains(response, 'id="id_related_objs-0-DELETE"')
928+
929+
def test_submit_line_shows_only_close_button(self):
930+
response = self.client.get(self.change_url)
931+
self.assertContains(
932+
response,
933+
'<a href="/admin/admin_inlines/poll/" class="closelink">Close</a>',
934+
html=True
935+
)
936+
delete_link = '<p class="deletelink-box"><a href="/admin/admin_inlines/poll/%s/delete/" class="deletelink">Delete</a></p>' # noqa
937+
self.assertNotContains(
938+
response,
939+
delete_link % self.poll.id,
940+
html=True
941+
)
942+
self.assertNotContains(response, '<input type="submit" value="Save and add another" name="_addanother">')
943+
self.assertNotContains(response, '<input type="submit" value="Save and continue editing" name="_continue">')
944+
945+
def test_inline_delete_buttons_are_not_shown(self):
946+
Question.objects.create(text="How will this be rendered?", poll=self.poll)
947+
response = self.client.get(self.change_url)
948+
self.assertNotContains(
949+
response,
950+
'<input type="checkbox" name="question_set-0-DELETE" id="id_question_set-0-DELETE">',
951+
html=True
952+
)
953+
954+
def test_extra_inlines_are_not_shown(self):
955+
response = self.client.get(self.change_url)
956+
self.assertNotContains(response, 'id="id_question_set-0-text"')
957+
958+
865959
@override_settings(ROOT_URLCONF='admin_inlines.urls')
866960
class SeleniumTests(AdminSeleniumTestCase):
867961

@@ -965,6 +1059,24 @@ def test_add_inlines(self):
9651059
self.assertEqual(ProfileCollection.objects.all().count(), 1)
9661060
self.assertEqual(Profile.objects.all().count(), 3)
9671061

1062+
def test_add_inline_link_absent_for_view_only_parent_model(self):
1063+
user = User.objects.create_user('testing', password='password', is_staff=True)
1064+
user.user_permissions.add(
1065+
Permission.objects.get(codename='view_poll', content_type=ContentType.objects.get_for_model(Poll))
1066+
)
1067+
user.user_permissions.add(
1068+
*Permission.objects.filter(
1069+
codename__endswith="question", content_type=ContentType.objects.get_for_model(Question)
1070+
).values_list('pk', flat=True)
1071+
)
1072+
self.admin_login(username='testing', password='password')
1073+
poll = Poll.objects.create(name="Survey")
1074+
change_url = reverse('admin:admin_inlines_poll_change', args=(poll.id,))
1075+
self.selenium.get(self.live_server_url + change_url)
1076+
with self.disable_implicit_wait():
1077+
with self.assertRaises(NoSuchElementException):
1078+
self.selenium.find_element_by_link_text('Add another Question')
1079+
9681080
def test_delete_inlines(self):
9691081
self.admin_login(username='super', password='secret')
9701082
self.selenium.get(self.live_server_url + reverse('admin:admin_inlines_profilecollection_add'))

‎tests/admin_views/admin.py

-9
Original file line numberDiff line numberDiff line change
@@ -1168,12 +1168,3 @@ def has_change_permission(self, request, obj=None):
11681168

11691169
site9 = admin.AdminSite(name='admin9')
11701170
site9.register(Article, ArticleAdmin9)
1171-
1172-
1173-
class ArticleAdmin10(admin.ModelAdmin):
1174-
def has_change_permission(self, request, obj=None):
1175-
return False
1176-
1177-
1178-
site10 = admin.AdminSite(name='admin10')
1179-
site10.register(Article, ArticleAdmin10)

0 commit comments

Comments
 (0)