From c9f6830ba6d30f01606d9876d239c77b702b8c38 Mon Sep 17 00:00:00 2001
From: Anders Kaseorg <anders@zulip.com>
Date: Mon, 18 Jul 2022 19:00:51 -0700
Subject: [PATCH] CVE-2022-31168: Fix authorization check for changing bot
 roles.

Due to an incorrect authorization check in Zulip Server 5.4 and
earlier, a member of an organization could craft an API call that
grants organization administrator privileges to one of their bots.

Signed-off-by: Anders Kaseorg <anders@zulip.com>
---
 zerver/tests/test_bots.py | 39 ++++++++++++++++++++++++++++++++++++++-
 zerver/views/users.py     |  7 ++++---
 2 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/zerver/tests/test_bots.py b/zerver/tests/test_bots.py
index b8d53b998d..b4b2bf92ff 100644
--- a/zerver/tests/test_bots.py
+++ b/zerver/tests/test_bots.py
@@ -8,9 +8,10 @@ from django.core import mail
 from django.test import override_settings
 from zulip_bots.custom_exceptions import ConfigValidationError
 
+from zerver.actions.bots import do_change_bot_owner
 from zerver.actions.realm_settings import do_set_realm_property
 from zerver.actions.streams import do_change_stream_permission
-from zerver.actions.users import do_change_can_create_users, do_deactivate_user
+from zerver.actions.users import do_change_can_create_users, do_change_user_role, do_deactivate_user
 from zerver.lib.bot_config import ConfigError, get_bot_config
 from zerver.lib.bot_lib import get_bot_handler
 from zerver.lib.integrations import EMBEDDED_BOTS, WebhookIntegration
@@ -1169,6 +1170,42 @@ class BotTest(ZulipTestCase, UploadSerializeMixin):
         bot = self.get_bot()
         self.assertEqual(None, bot["default_sending_stream"])
 
+    def test_patch_bot_role(self) -> None:
+        self.login("desdemona")
+
+        email = "default-bot@zulip.com"
+        user_profile = self.get_bot_user(email)
+
+        do_change_user_role(user_profile, UserProfile.ROLE_MEMBER, acting_user=user_profile)
+
+        req = dict(role=UserProfile.ROLE_GUEST)
+
+        result = self.client_patch(f"/json/users/{self.get_bot_user(email).id}", req)
+        self.assert_json_success(result)
+
+        user_profile = self.get_bot_user(email)
+        self.assertEqual(user_profile.role, UserProfile.ROLE_GUEST)
+
+        # Test for not allowing a non-owner user to make assign a bot an owner role
+        desdemona = self.example_user("desdemona")
+        do_change_user_role(desdemona, UserProfile.ROLE_REALM_ADMINISTRATOR, acting_user=None)
+
+        req = dict(role=UserProfile.ROLE_REALM_OWNER)
+
+        result = self.client_patch(f"/json/users/{user_profile.id}", req)
+        self.assert_json_error(result, "Must be an organization owner")
+
+        # Test for not allowing a non-administrator user to assign a bot an administrator role
+        shiva = self.example_user("shiva")
+        self.assertEqual(shiva.role, UserProfile.ROLE_MODERATOR)
+        self.login_user(shiva)
+        do_change_bot_owner(user_profile, shiva, acting_user=None)
+
+        req = dict(role=UserProfile.ROLE_REALM_ADMINISTRATOR)
+
+        result = self.client_patch(f"/json/users/{user_profile.id}", req)
+        self.assert_json_error(result, "Must be an organization administrator")
+
     def test_patch_bot_to_stream_private_allowed(self) -> None:
         self.login("hamlet")
         user_profile = self.example_user("hamlet")
diff --git a/zerver/views/users.py b/zerver/views/users.py
index 852a45c9d6..b83f39a44f 100644
--- a/zerver/views/users.py
+++ b/zerver/views/users.py
@@ -39,6 +39,7 @@ from zerver.lib.exceptions import (
     CannotDeactivateLastUserError,
     JsonableError,
     MissingAuthenticationError,
+    OrganizationAdministratorRequired,
     OrganizationOwnerRequired,
 )
 from zerver.lib.integrations import EMBEDDED_BOTS
@@ -188,11 +189,11 @@ def update_user_backend(
 
     if role is not None and target.role != role:
         # Require that the current user has permissions to
-        # grant/remove the role in question.  access_user_by_id has
-        # already verified we're an administrator; here we enforce
-        # that only owners can toggle the is_realm_owner flag.
+        # grant/remove the role in question.
         if UserProfile.ROLE_REALM_OWNER in [role, target.role] and not user_profile.is_realm_owner:
             raise OrganizationOwnerRequired()
+        elif not user_profile.is_realm_admin:
+            raise OrganizationAdministratorRequired()
 
         if target.role == UserProfile.ROLE_REALM_OWNER and check_last_owner(user_profile):
             raise JsonableError(