CVE-2020-5240: Incorrect Authorization (CWE-863)
value: In wagtail-2fa before 1.4.1, any user with access to the CMS can view and delete other users 2FA devices by going to the correct path. The user does not require special permissions in order to do so. By deleting the other users device they can disable the target users 2FA devices and potentially compromise the account if they figure out their password. The problem has been patched in version 1.4.1.
CWE description: The product performs an authorization check when an actor attempts to access a resource or perform an action, but it does not correctly perform the check. This allows attackers to bypass intended access restrictions.
CVSS v3 base score: 8.5
Published: 2020-03-13T22:15Z
Repository: https://github.com/labd/wagtail-2fa
Language: Python
Files changed: src/wagtail_2fa/views.py, tests/test_views.py
Commit date: 2020-02-10 16:29:56 +0100
Commit message: Prevent unauthorized users managing others' device

This adds a permissions check on the device list and delete views. When
a user has the 'change_user' permission, they are allowed to view
and delete other users' 2FA devices. If they don't have the
permission, they are only allowed to manage their own devices.

Diff: diff --git a/src/wagtail_2fa/views.py b/src/wagtail_2fa/views.py
index 9ec004f..463b2bb 100644
--- a/src/wagtail_2fa/views.py
+++ b/src/wagtail_2fa/views.py
@@ -1,5 +1,6 @@
+from django.core.exceptions import PermissionDenied
 import qrcode
 import qrcode.image.svg
 from django.conf import settings
 from django.contrib.auth import REDIRECT_FIELD_NAME
 from django.contrib.auth.views import SuccessURLAllowedHostsMixin
@@ -73,10 +74,19 @@ class DeviceListView(OtpRequiredMixin, ListView):
     def get_context_data(self, **kwargs):
         context = super().get_context_data(**kwargs)
         context['user_id'] = int(self.kwargs['user_id'])
         return context
 
+    def dispatch(self, request, *args, **kwargs):
+        if (int(self.kwargs["user_id"]) == request.user.pk or
+                request.user.has_perm("user.change_user")):
+            if not self.user_allowed(request.user):
+                return self.handle_no_permission(request)
+
+            return super(OtpRequiredMixin, self).dispatch(request, *args, **kwargs)
+        raise PermissionDenied
+
 
 class DeviceCreateView(OtpRequiredMixin, FormView):
     form_class = forms.DeviceForm
     template_name = "wagtail_2fa/device_form.html"
 
@@ -132,10 +142,21 @@ class DeviceDeleteView(OtpRequiredMixin, DeleteView):
         return TOTPDevice.objects.devices_for_user(device.user, confirmed=True)
 
     def get_success_url(self):
         return reverse('wagtail_2fa_device_list', kwargs={'user_id': self.request.POST.get('user_id')})
 
+    def dispatch(self, request, *args, **kwargs):
+        device = TOTPDevice.objects.get(**self.kwargs)
+
+        if device.user.pk == request.user.pk or request.user.has_perm("user.change_user"):
+            if not self.user_allowed(request.user):
+                return self.handle_no_permission(request)
+
+            return super(OtpRequiredMixin, self).dispatch(request, *args, **kwargs)
+
+        raise PermissionDenied
+
 
 class DeviceQRCodeView(OtpRequiredMixin, View):
     # require OTP if configured
     if_configured = True
 
diff --git a/tests/test_views.py b/tests/test_views.py
index 3d8c921..af13b39 100644
--- a/tests/test_views.py
+++ b/tests/test_views.py
@@ -1,14 +1,18 @@
+import pytest
+from django.core.exceptions import PermissionDenied
+from django.contrib.auth.models import Permission
+from django.contrib.contenttypes.models import ContentType
 from django.test import override_settings
 from unittest.mock import patch
+from django.http.response import Http404
 
 from django.contrib.auth import get_user_model
 from django.urls import reverse
 from django_otp import DEVICE_ID_SESSION_KEY
 from django_otp.plugins.otp_totp.models import TOTPDevice
-from wagtail_2fa.views import DeviceListView
-
+from wagtail_2fa.views import DeviceListView, DeviceDeleteView, DeviceUpdateView
 
 def test_device_list_view(admin_client, admin_user, django_assert_num_queries):
     with override_settings(WAGTAIL_2FA_REQUIRED=True):
         admin_device = TOTPDevice.objects.create(name='Initial', user=admin_user, confirmed=True)
 
@@ -127,5 +131,72 @@ def test_delete_user_device_unauthorized(client, user, monkeypatch):
         response = client.post(endpoint, {
             'user_id': user.id
         })
         assert response.status_code == 302
         assert TOTPDevice.objects.all().count() == 1
+
+
+class TestViewsChangeUserPermission:
+    """Test suite which ensures that:
+        - users without the change_user permission cannot manage other users' 2FA devices
+        - users can manage their own devices
+    """
+
+    def test_verified_user_has_no_change_user_perm(self, verified_user):
+        """Sanity check."""
+        assert not verified_user.has_perm("user.change_user")
+
+    def test_device_list_view_for_own_user_returns_200(self, verified_user, rf):
+        with override_settings(WAGTAIL_2FA_REQUIRED=True):
+            request = rf.get('foo')
+            request.user = verified_user
+
+            response = DeviceListView.as_view()(request, user_id=verified_user.id)
+            assert response.status_code == 200
+
+    def test_device_list_view_for_other_user_raises_error(self, user, verified_user, rf):
+        with override_settings(WAGTAIL_2FA_REQUIRED=True):
+            request = rf.get('foo')
+            request.user = verified_user
+
+            with pytest.raises(PermissionDenied):
+                response = DeviceListView.as_view()(request, user_id=user.id)
+
+    def test_device_delete_view_for_own_user_returns_200(self, verified_user, rf):
+        with override_settings(WAGTAIL_2FA_REQUIRED=True):
+            device = TOTPDevice.objects.devices_for_user(verified_user, confirmed=True).first()
+            request = rf.get('foo')
+            request.user = verified_user
+
+            response = DeviceDeleteView.as_view()(request, pk=device.id)
+            assert response.status_code == 200
+
+    def test_device_delete_view_for_other_user_raises_error(self, user, verified_user, rf):
+        with override_settings(WAGTAIL_2FA_REQUIRED=True):
+            other_device = TOTPDevice.objects.create(name='Initial', user=user, confirmed=True)
+
+            device = TOTPDevice.objects.devices_for_user(verified_user, confirmed=True).first()
+            request = rf.get('foo')
+            request.user = verified_user
+
+            with pytest.raises(PermissionDenied):
+                response = DeviceDeleteView.as_view()(request, pk=other_device.id)
+
+    def test_device_update_view_for_own_user_returns_200(self, verified_user, rf):
+        with override_settings(WAGTAIL_2FA_REQUIRED=True):
+            device = TOTPDevice.objects.devices_for_user(verified_user, confirmed=True).first()
+            request = rf.get('foo')
+            request.user = verified_user
+
+            response = DeviceUpdateView.as_view()(request, pk=device.id)
+            assert response.status_code == 200
+
+    def test_device_update_view_for_other_user_raises_error(self, user, verified_user, rf):
+        with override_settings(WAGTAIL_2FA_REQUIRED=True):
+            other_device = TOTPDevice.objects.create(name='Initial', user=user, confirmed=True)
+
+            device = TOTPDevice.objects.devices_for_user(verified_user, confirmed=True).first()
+            request = rf.get('foo')
+            request.user = verified_user
+
+            with pytest.raises(Http404):
+                response = DeviceUpdateView.as_view()(request, pk=other_device.id)

Vulnerable code: from wagtail_2fa.views import DeviceListView

Fixed code: from django.core.exceptions import PermissionDenied
    def dispatch(self, request, *args, **kwargs):
        if (int(self.kwargs["user_id"]) == request.user.pk or
                request.user.has_perm("user.change_user")):
            if not self.user_allowed(request.user):
                return self.handle_no_permission(request)

            return super(OtpRequiredMixin, self).dispatch(request, *args, **kwargs)
        raise PermissionDenied

    def dispatch(self, request, *args, **kwargs):
        device = TOTPDevice.objects.get(**self.kwargs)

        if device.user.pk == request.user.pk or request.user.has_perm("user.change_user"):
            if not self.user_allowed(request.user):
                return self.handle_no_permission(request)

            return super(OtpRequiredMixin, self).dispatch(request, *args, **kwargs)

        raise PermissionDenied

import pytest
from django.core.exceptions import PermissionDenied
from django.contrib.auth.models import Permission
from django.contrib.contenttypes.models import ContentType
from django.http.response import Http404
from wagtail_2fa.views import DeviceListView, DeviceDeleteView, DeviceUpdateView


class TestViewsChangeUserPermission:
    """Test suite which ensures that:
        - users without the change_user permission cannot manage other users' 2FA devices
        - users can manage their own devices
    """

    def test_verified_user_has_no_change_user_perm(self, verified_user):
        """Sanity check."""
        assert not verified_user.has_perm("user.change_user")

    def test_device_list_view_for_own_user_returns_200(self, verified_user, rf):
        with override_settings(WAGTAIL_2FA_REQUIRED=True):
            request = rf.get('foo')
            request.user = verified_user

            response = DeviceListView.as_view()(request, user_id=verified_user.id)
            assert response.status_code == 200

    def test_device_list_view_for_other_user_raises_error(self, user, verified_user, rf):
        with override_settings(WAGTAIL_2FA_REQUIRED=True):
            request = rf.get('foo')
            request.user = verified_user

            with pytest.raises(PermissionDenied):
                response = DeviceListView.as_view()(request, user_id=user.id)

    def test_device_delete_view_for_own_user_returns_200(self, verified_user, rf):
        with override_settings(WAGTAIL_2FA_REQUIRED=True):
            device = TOTPDevice.objects.devices_for_user(verified_user, confirmed=True).first()
            request = rf.get('foo')
            request.user = verified_user

            response = DeviceDeleteView.as_view()(request, pk=device.id)
            assert response.status_code == 200

    def test_device_delete_view_for_other_user_raises_error(self, user, verified_user, rf):
        with override_settings(WAGTAIL_2FA_REQUIRED=True):
            other_device = TOTPDevice.objects.create(name='Initial', user=user, confirmed=True)

            device = TOTPDevice.objects.devices_for_user(verified_user, confirmed=True).first()
            request = rf.get('foo')
            request.user = verified_user

            with pytest.raises(PermissionDenied):
                response = DeviceDeleteView.as_view()(request, pk=other_device.id)

    def test_device_update_view_for_own_user_returns_200(self, verified_user, rf):
        with override_settings(WAGTAIL_2FA_REQUIRED=True):
            device = TOTPDevice.objects.devices_for_user(verified_user, confirmed=True).first()
            request = rf.get('foo')
            request.user = verified_user

            response = DeviceUpdateView.as_view()(request, pk=device.id)
            assert response.status_code == 200

    def test_device_update_view_for_other_user_raises_error(self, user, verified_user, rf):
        with override_settings(WAGTAIL_2FA_REQUIRED=True):
            other_device = TOTPDevice.objects.create(name='Initial', user=user, confirmed=True)

            device = TOTPDevice.objects.devices_for_user(verified_user, confirmed=True).first()
            request = rf.get('foo')
            request.user = verified_user

            with pytest.raises(Http404):
                response = DeviceUpdateView.as_view()(request, pk=other_device.id)