From 6336322d2f9bbccaacfc80cba83a3c62eefd5737 Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Fri, 29 Sep 2023 23:34:27 +0530 Subject: [PATCH] CVE-2023-47642: Invalid metadata access for formerly subscribed streams. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It was discovered by the Zulip development team that active users who had previously been subscribed to a stream incorrectly continued being able to use the Zulip API to access metadata for that stream. As a result, users who had been removed from a stream, but still had an account in the organization, could still view metadata for that stream (including the stream name, description, settings, and an email address used to send emails into the stream via the incoming email integration). This potentially allowed users to see changes to a stream’s metadata after they had lost access to the stream. This bug was present in all Zulip releases prior to today's Zulip Server 7.5. --- zerver/actions/streams.py | 15 +++------ zerver/lib/events.py | 50 ++++++++++++++++++++++++----- zerver/lib/subscription_info.py | 56 +++++++++++++++++++++++++++++++-- zerver/tests/test_subs.py | 39 +++++++++++++++++++++-- 4 files changed, 136 insertions(+), 24 deletions(-) diff --git a/zerver/actions/streams.py b/zerver/actions/streams.py index 9346202ab02ac..769d3a1f3afad 100644 --- a/zerver/actions/streams.py +++ b/zerver/actions/streams.py @@ -1028,21 +1028,14 @@ def do_change_stream_permission( if old_invite_only_value and not stream.invite_only: # We need to send stream creation event to users who can access the # stream now but were not able to do so previously. So, we can exclude - # subscribers, users who were previously subscribed to the stream and - # realm admins from the non-guest user list. - assert stream.recipient_id is not None - previously_subscribed_user_ids = Subscription.objects.filter( - recipient_id=stream.recipient_id, active=False, is_user_active=True - ).values_list("user_profile_id", flat=True) + # subscribers and realm admins from the non-guest user list. stream_subscriber_user_ids = get_active_subscriptions_for_stream_id( stream.id, include_deactivated_users=False ).values_list("user_profile_id", flat=True) - old_can_access_stream_user_ids = ( - set(stream_subscriber_user_ids) - | set(previously_subscribed_user_ids) - | {user.id for user in stream.realm.get_admin_users_and_bots()} - ) + old_can_access_stream_user_ids = set(stream_subscriber_user_ids) | { + user.id for user in stream.realm.get_admin_users_and_bots() + } non_guest_user_ids = set(active_non_guest_user_ids(stream.realm_id)) notify_stream_creation_ids = non_guest_user_ids - old_can_access_stream_user_ids send_stream_creation_event(stream, list(notify_stream_creation_ids)) diff --git a/zerver/lib/events.py b/zerver/lib/events.py index d2690ca9110d8..4b1acf3852e48 100644 --- a/zerver/lib/events.py +++ b/zerver/lib/events.py @@ -45,7 +45,11 @@ from zerver.lib.sounds import get_available_notification_sounds from zerver.lib.stream_subscription import handle_stream_notifications_compatibility from zerver.lib.streams import do_get_streams, get_web_public_streams -from zerver.lib.subscription_info import gather_subscriptions_helper, get_web_public_subs +from zerver.lib.subscription_info import ( + build_unsubscribed_sub_from_stream_dict, + gather_subscriptions_helper, + get_web_public_subs, +) from zerver.lib.timestamp import datetime_to_timestamp from zerver.lib.timezone import canonicalize_timezone from zerver.lib.topic import TOPIC_NAME @@ -61,7 +65,9 @@ Message, Realm, RealmUserDefault, + Recipient, Stream, + Subscription, UserMessage, UserProfile, UserStatus, @@ -1001,17 +1007,45 @@ def _draft_update_action(i: int) -> None: if include_subscribers: stream_data["subscribers"] = [] - # We know the stream has no traffic, and this - # field is not present in the event. - # - # TODO: Probably this should just be added to the event. - stream_data["stream_weekly_traffic"] = None + # Here we need to query the database to check whether the + # user was previously subscribed. If they were, we need to + # include the stream in the unsubscribed list after adding + # personal subscription metadata (such as configured stream + # color; most of the other personal setting have no effect + # when not subscribed). + unsubscribed_stream_sub = Subscription.objects.filter( + user_profile=user_profile, + recipient__type_id=stream["stream_id"], + recipient__type=Recipient.STREAM, + ).values( + *Subscription.API_FIELDS, + "recipient_id", + "active", + ) + + if len(unsubscribed_stream_sub) == 1: + unsubscribed_stream_dict = build_unsubscribed_sub_from_stream_dict( + user_profile, unsubscribed_stream_sub[0], stream_data + ) + if include_subscribers: + unsubscribed_stream_dict["subscribers"] = [] + + # The stream might have traffic, but we do not have the + # data to compute it in the event, so we just set to + # "None" here like we would do for newly created streams. + # + # TODO: Probably this should just be added to the event. + unsubscribed_stream_dict["stream_weekly_traffic"] = None + state["unsubscribed"].append(unsubscribed_stream_dict) + else: + assert len(unsubscribed_stream_sub) == 0 + stream_data["stream_weekly_traffic"] = None + state["never_subscribed"].append(stream_data) - # Add stream to never_subscribed (if not invite_only) - state["never_subscribed"].append(stream_data) if "streams" in state: state["streams"].append(stream) + state["unsubscribed"].sort(key=lambda elt: elt["name"]) state["never_subscribed"].sort(key=lambda elt: elt["name"]) if "streams" in state: state["streams"].sort(key=lambda elt: elt["name"]) diff --git a/zerver/lib/subscription_info.py b/zerver/lib/subscription_info.py index f0bafdc5ff3b3..4fe39d4ea779f 100644 --- a/zerver/lib/subscription_info.py +++ b/zerver/lib/subscription_info.py @@ -16,8 +16,9 @@ ) from zerver.lib.stream_traffic import get_average_weekly_stream_traffic, get_streams_traffic from zerver.lib.streams import get_web_public_streams_queryset, subscribed_to_stream -from zerver.lib.timestamp import datetime_to_timestamp +from zerver.lib.timestamp import datetime_to_timestamp, timestamp_to_datetime from zerver.lib.types import ( + APIStreamDict, NeverSubscribedStreamDict, RawStreamDict, RawSubscriptionDict, @@ -102,6 +103,34 @@ def get_next_color() -> str: ) +def build_unsubscribed_sub_from_stream_dict( + user: UserProfile, sub_dict: RawSubscriptionDict, stream_dict: APIStreamDict +) -> SubscriptionStreamDict: + # This function is only called from `apply_event` code. + raw_stream_dict = RawStreamDict( + can_remove_subscribers_group_id=stream_dict["can_remove_subscribers_group_id"], + date_created=timestamp_to_datetime(stream_dict["date_created"]), + description=stream_dict["description"], + first_message_id=stream_dict["first_message_id"], + history_public_to_subscribers=stream_dict["history_public_to_subscribers"], + invite_only=stream_dict["invite_only"], + is_web_public=stream_dict["is_web_public"], + message_retention_days=stream_dict["message_retention_days"], + name=stream_dict["name"], + rendered_description=stream_dict["rendered_description"], + id=stream_dict["stream_id"], + stream_post_policy=stream_dict["stream_post_policy"], + ) + + # We pass recent_traffic as an empty objecy and avoid extra database + # query since we would just set it to None later. + subscription_stream_dict = build_stream_dict_for_sub( + user, sub_dict, raw_stream_dict, recent_traffic={} + ) + + return subscription_stream_dict + + def build_stream_dict_for_sub( user: UserProfile, sub_dict: RawSubscriptionDict, @@ -378,6 +407,21 @@ def get_subscribers_query( return get_active_subscriptions_for_stream_id(stream.id, include_deactivated_users=False) +def has_metadata_access_to_previously_subscribed_stream( + user_profile: UserProfile, stream_dict: SubscriptionStreamDict +) -> bool: + if stream_dict["is_web_public"]: + return True + + if not user_profile.can_access_public_streams(): + return False + + if stream_dict["invite_only"]: + return user_profile.is_realm_admin + + return True + + # In general, it's better to avoid using .values() because it makes # the code pretty ugly, but in this case, it has significant # performance impact for loading / for users with large numbers of @@ -445,7 +489,15 @@ def get_stream_id(sub_dict: RawSubscriptionDict) -> int: if is_active: subscribed.append(stream_dict) else: - unsubscribed.append(stream_dict) + if has_metadata_access_to_previously_subscribed_stream(user_profile, stream_dict): + """ + User who are no longer subscribed to a stream that they don't have + metadata access to will not receive metadata related to this stream + and their clients will see it as an unkown stream if referenced + somewhere (e.g. a markdown stream link), just like they would see + a reference to a private stream they had never been subscribed to. + """ + unsubscribed.append(stream_dict) if user_profile.can_access_public_streams(): never_subscribed_stream_ids = set(all_streams_map) - sub_unsub_stream_ids diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index f1aed2104be3b..8130907236c14 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -6135,14 +6135,47 @@ def test_previously_subscribed_private_streams(self) -> None: sub_data = gather_subscriptions_helper(non_admin_user) self.verify_sub_fields(sub_data) unsubscribed_streams = sub_data.unsubscribed - self.assert_length(unsubscribed_streams, 1) - self.assertEqual(unsubscribed_streams[0]["subscribers"], []) + self.assert_length(unsubscribed_streams, 0) + sub_data = gather_subscriptions_helper(guest_user) + self.verify_sub_fields(sub_data) + unsubscribed_streams = sub_data.unsubscribed + self.assert_length(unsubscribed_streams, 0) + + def test_previously_subscribed_public_streams(self) -> None: + public_stream_name = "public_stream" + web_public_stream_name = "web_public_stream" + guest_user = self.example_user("polonius") + member_user = self.example_user("hamlet") + + self.make_stream(public_stream_name, realm=get_realm("zulip")) + self.make_stream(web_public_stream_name, realm=get_realm("zulip"), is_web_public=True) + + for stream_name in [public_stream_name, web_public_stream_name]: + self.subscribe(guest_user, stream_name) + self.subscribe(member_user, stream_name) + self.subscribe(self.example_user("othello"), stream_name) + + for stream_name in [public_stream_name, web_public_stream_name]: + self.unsubscribe(guest_user, stream_name) + self.unsubscribe(member_user, stream_name) + + # Test member user gets previously subscribed public stream and its subscribers. + sub_data = gather_subscriptions_helper(member_user) + self.verify_sub_fields(sub_data) + unsubscribed_streams = sub_data.unsubscribed + self.assert_length(unsubscribed_streams, 2) + self.assert_length(unsubscribed_streams[0]["subscribers"], 1) + self.assert_length(unsubscribed_streams[1]["subscribers"], 1) + + # Test guest users cannot get previously subscribed public stream but can get + # web-public stream and its subscribers. sub_data = gather_subscriptions_helper(guest_user) self.verify_sub_fields(sub_data) unsubscribed_streams = sub_data.unsubscribed self.assert_length(unsubscribed_streams, 1) - self.assertEqual(unsubscribed_streams[0]["subscribers"], []) + self.assertEqual(unsubscribed_streams[0]["is_web_public"], True) + self.assert_length(unsubscribed_streams[0]["subscribers"], 1) def test_gather_subscriptions_mit(self) -> None: """