Skip to content
Permalink
Browse files

streams: Fix autosubscribe security bug (CVE-2017-0881).

A bug in Zulip's implementation of the "stream exists" endpoint meant
that any user of a Zulip server could subscribe to an invite-only
stream without needing to be invited by using the "autosubscribe"
argument.

Thanks to Rafid Aslam for discovering this issue.
  • Loading branch information...
timabbott committed Jan 23, 2017
1 parent 7e0ce22 commit 7ecda1ac8e26d8fb3725e954b2dc4723dda2255f
Showing with 24 additions and 1 deletion.
  1. +23 −0 zerver/tests/test_subs.py
  2. +1 −1 zerver/views/streams.py
@@ -1936,6 +1936,29 @@ def test_existing_subscriptions_autosubscription(self):
self.assertIn("exists", json)
self.assertTrue(json["exists"])

def test_existing_subscriptions_autosubscription_private_stream(self):
# type: () -> None
"""Call /json/subscriptions/exist on an existing private stream with
autosubscribe should fail.
"""
stream_name = "Saxony"
result = self.common_subscribe_to_streams("cordelia@zulip.com", [stream_name],
invite_only=True)
stream = get_stream(stream_name, self.realm)

result = self.client_post("/json/subscriptions/exists",
{"stream": stream_name, "autosubscribe": True})
self.assert_json_success(result)
json = ujson.loads(result.content)
self.assertIn("exists", json)
self.assertTrue(json["exists"])
self.assertIn("subscribed", json)
# Importantly, we are not now subscribed
self.assertFalse(json["subscribed"])
self.assertEqual(Subscription.objects.filter(
recipient__type=Recipient.STREAM,
recipient__type_id=stream.id).count(), 1)

def get_subscription(self, user_profile, stream_name):
# type: (UserProfile, Text) -> Subscription
stream = get_stream(stream_name, self.realm)
@@ -481,7 +481,7 @@ def stream_exists_backend(request, user_profile, stream_id, autosubscribe):
result = {"exists": bool(stream)}
if stream is not None:
recipient = get_recipient(Recipient.STREAM, stream.id)
if autosubscribe:
if not stream.invite_only and autosubscribe:

This comment has been minimized.

Copy link
@lfaraone

lfaraone Jan 30, 2017

Member

Wouldn't it be better to centralise this check? Either in bulk_add_subscriptions() or in some can_add_to_stream(stream, user, actor) method.

This comment has been minimized.

Copy link
@timabbott

timabbott Jan 30, 2017

Author Member

I'm doing that overhaul across the codebase for Zulip 1.5; it's a pretty big refactoring (400 lines of code changes including extensive new tests, etc.) and thus not suitable for a focused security bug fix release (a lot of the other code changed in that refactoring is different enough on master from 1.4.x that it'd be an expensive rebase).

bulk_add_subscriptions([stream], [user_profile])
result["subscribed"] = is_active_subscriber(
user_profile=user_profile,

0 comments on commit 7ecda1a

Please sign in to comment.
You can’t perform that action at this time.