CVE-2021-30479: Prevent guest user access to all_public_streams API.

A bug in the implementation of the all_public_streams API feature
resulted in guest users being able to receive message traffic to public
streams that should have been only accessible to members of the
organization.

zerver/tests/test_event_system.py

@@ -146,6 +146,35 @@ class EventsEndpointTest(ZulipTestCase):
         self.assertEqual(result_dict['realm_emoji'], [])
         self.assertEqual(result_dict['queue_id'], '15:13')
 
+    def test_events_register_endpoint_all_public_streams_access(self) -> None:
+        guest_user = self.example_user("polonius")
+        normal_user = self.example_user("hamlet")
+        self.assertEqual(guest_user.role, UserProfile.ROLE_GUEST)
+        self.assertEqual(normal_user.role, UserProfile.ROLE_MEMBER)
+
+        with mock.patch("zerver.views.events_register.do_events_register", return_value={}):
+            result = self.api_post(normal_user, "/json/register", dict(all_public_streams="true"))
+        self.assert_json_success(result)
+
+        with mock.patch("zerver.views.events_register.do_events_register", return_value={}):
+            result = self.api_post(guest_user, "/json/register", dict(all_public_streams="true"))
+        self.assert_json_error(result, "User not authorized for this query")
+
+    def test_events_get_events_endpoint_guest_cant_use_all_public_streams_param(self) -> None:
+        """
+        This test is meant to execute the very beginning of the codepath
+        to ensure guest users are immediately disallowed to use the
+        all_public_streams param. Deeper testing is hard (and not necessary for this case)
+        due to the codepath expecting AsyncDjangoHandler to be attached to the request,
+        which doesn't happen in our test setup.
+        """
+
+        guest_user = self.example_user("polonius")
+        self.assertEqual(guest_user.role, UserProfile.ROLE_GUEST)
+
+        result = self.api_get(guest_user, "/api/v1/events", dict(all_public_streams="true"))
+        self.assert_json_error(result, "User not authorized for this query")
+
     def test_tornado_endpoint(self) -> None:
 
         # This test is mostly intended to get minimal coverage on

zerver/tornado/views.py

@@ -79,6 +79,9 @@ def get_events_backend(request: HttpRequest, user_profile: UserProfile,
                        bulk_message_deletion: bool=REQ(default=False, validator=check_bool,
                                                        intentionally_undocumented=True)
                        ) -> HttpResponse:
+    if all_public_streams and not user_profile.can_access_public_streams():
+        return json_error(_("User not authorized for this query"))
+
     # Extract the Tornado handler from the request
     handler: AsyncDjangoHandler = request._tornado_handler
 

zerver/views/events_register.py

@@ -1,10 +1,11 @@
 from typing import Dict, Iterable, Optional, Sequence
 
 from django.http import HttpRequest, HttpResponse
+from django.utils.translation import ugettext as _
 
 from zerver.lib.events import do_events_register
 from zerver.lib.request import REQ, has_request_variables
-from zerver.lib.response import json_success
+from zerver.lib.response import json_error, json_success
 from zerver.lib.validator import check_bool, check_dict, check_list, check_string
 from zerver.models import Stream, UserProfile
 
@@ -47,6 +48,9 @@ def events_register_backend(
         narrow: NarrowT=REQ(validator=check_list(check_list(check_string, length=2)), default=[]),
         queue_lifespan_secs: int=REQ(converter=int, default=0, documentation_pending=True),
 ) -> HttpResponse:
+    if all_public_streams and not user_profile.can_access_public_streams():
+        return json_error(_("User not authorized for this query"))
+
     all_public_streams = _default_all_public_streams(user_profile, all_public_streams)
     narrow = _default_narrow(user_profile, narrow)