CVE-2023-47642: Invalid metadata access for formerly subscribed streams.

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

@@ -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))

zerver/lib/events.py

@@ -45,7 +45,11 @@ from zerver.lib.soft_deactivation import reactivate_user_if_soft_deactivated
 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 @@ from zerver.models import (
     Message,
     Realm,
     RealmUserDefault,
+    Recipient,
     Stream,
+    Subscription,
     UserMessage,
     UserProfile,
     UserStatus,
@@ -1001,17 +1007,45 @@ def apply_event(
                 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"])

zerver/lib/subscription_info.py

@@ -16,8 +16,9 @@ from zerver.lib.stream_subscription import (
 )
 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_web_public_subs(realm: Realm) -> SubscriptionInfo:
     )
 
 
+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 gather_subscriptions_helper(
         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

zerver/tests/test_subs.py

@@ -6135,14 +6135,47 @@ class GetSubscribersTest(ZulipTestCase):
         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:
         """