CVE-2021-30487: Prevent admins from moving topics to disallowed streams.

A bug in the implementation of the topic moving API resulted in
organization administrators being able to move messages to streams they
shouldn't be allowed to - private streams they weren't subscribed to and
streams in other organization hosted by the same Zulip installation.

In our current model realm admins can't send messages to private streams
they're not subscribed to - and being able move messages to a
stream effectively allows to send messages to that stream and thus the
two need to be consistent.
This commit is contained in:
Mateusz Mandera 2021-04-09 15:31:07 +02:00 committed by Alex Vandiver
parent 140655d69e
commit 3ba8348c51
2 changed files with 46 additions and 4 deletions

View File

@ -17,7 +17,7 @@ from zerver.lib.message import MessageDict, has_message_access, messages_for_ids
from zerver.lib.test_classes import ZulipTestCase from zerver.lib.test_classes import ZulipTestCase
from zerver.lib.test_helpers import cache_tries_captured, queries_captured from zerver.lib.test_helpers import cache_tries_captured, queries_captured
from zerver.lib.topic import LEGACY_PREV_TOPIC, TOPIC_NAME from zerver.lib.topic import LEGACY_PREV_TOPIC, TOPIC_NAME
from zerver.models import Message, Stream, UserMessage, UserProfile from zerver.models import Message, Stream, UserMessage, UserProfile, get_realm
class EditMessageTest(ZulipTestCase): class EditMessageTest(ZulipTestCase):
@ -1084,6 +1084,48 @@ class EditMessageTest(ZulipTestCase):
f"This topic was moved here from #**test move stream>test** by @_**Iago|{user_profile.id}**", f"This topic was moved here from #**test move stream>test** by @_**Iago|{user_profile.id}**",
) )
def test_move_message_realm_admin_cant_move_to_another_realm(self) -> None:
user_profile = self.example_user("iago")
self.assertEqual(user_profile.role, UserProfile.ROLE_REALM_ADMINISTRATOR)
self.login("iago")
lear_realm = get_realm("lear")
new_stream = self.make_stream("new", lear_realm)
msg_id = self.send_stream_message(user_profile, "Verona", topic_name="test123")
result = self.client_patch(
"/json/messages/" + str(msg_id),
{
"message_id": msg_id,
"stream_id": new_stream.id,
"propagate_mode": "change_all",
},
)
self.assert_json_error(result, "Invalid stream id")
def test_move_message_realm_admin_cant_move_to_private_stream_without_subscription(
self,
) -> None:
user_profile = self.example_user("iago")
self.assertEqual(user_profile.role, UserProfile.ROLE_REALM_ADMINISTRATOR)
self.login("iago")
new_stream = self.make_stream("new", invite_only=True)
msg_id = self.send_stream_message(user_profile, "Verona", topic_name="test123")
result = self.client_patch(
"/json/messages/" + str(msg_id),
{
"message_id": msg_id,
"stream_id": new_stream.id,
"propagate_mode": "change_all",
},
)
self.assert_json_error(result, "Invalid stream id")
def test_move_message_to_stream_change_later(self) -> None: def test_move_message_to_stream_change_later(self) -> None:
(user_profile, old_stream, new_stream, msg_id, msg_id_later) = self.prepare_move_topics( (user_profile, old_stream, new_stream, msg_id, msg_id_later) = self.prepare_move_topics(
"iago", "test move stream", "new stream", "test" "iago", "test move stream", "new stream", "test"
@ -1173,7 +1215,7 @@ class EditMessageTest(ZulipTestCase):
"topic": "new topic", "topic": "new topic",
}, },
) )
self.assertEqual(len(queries), 48) self.assertEqual(len(queries), 49)
self.assertEqual(len(cache_tries), 13) self.assertEqual(len(cache_tries), 13)
messages = get_topic_messages(user_profile, old_stream, "test") messages = get_topic_messages(user_profile, old_stream, "test")

View File

@ -20,7 +20,7 @@ from zerver.lib.markdown import MentionData
from zerver.lib.message import access_message, normalize_body from zerver.lib.message import access_message, normalize_body
from zerver.lib.queue import queue_json_publish from zerver.lib.queue import queue_json_publish
from zerver.lib.response import json_error, json_success from zerver.lib.response import json_error, json_success
from zerver.lib.streams import get_stream_by_id from zerver.lib.streams import access_stream_by_id
from zerver.lib.timestamp import datetime_to_timestamp from zerver.lib.timestamp import datetime_to_timestamp
from zerver.lib.topic import LEGACY_PREV_TOPIC, REQ_topic from zerver.lib.topic import LEGACY_PREV_TOPIC, REQ_topic
from zerver.lib.validator import check_bool, check_string_in, to_non_negative_int from zerver.lib.validator import check_bool, check_string_in, to_non_negative_int
@ -211,7 +211,7 @@ def update_message_backend(
if content is not None: if content is not None:
raise JsonableError(_("Cannot change message content while changing stream")) raise JsonableError(_("Cannot change message content while changing stream"))
new_stream = get_stream_by_id(stream_id) new_stream = access_stream_by_id(user_profile, stream_id, require_active=True)[0]
number_changed = do_update_message( number_changed = do_update_message(
user_profile, user_profile,