Merge pull request #392 from matrix-org/kegan/space-leak

bugfix: ensure metadata about space children doesn't leak to active connections

sync3/caches/user.go

@@ -26,6 +26,10 @@ type TransactionIDFetcher interface {
 	TransactionIDForEvents(userID, deviceID string, eventIDs []string) (eventIDToTxnID map[string]string)
 }
 
+type JoinChecker interface {
+	IsUserJoined(userID, roomID string) bool
+}
+
 // UserRoomData describes a single room from the perspective of particular user.
 // It is primarily used in two places:
 //   - in the caches.UserCache, to hold the latest version of user-specific data; and
@@ -194,11 +198,12 @@ type UserCache struct {
 	store                     UserCacheStore
 	globalCache               *GlobalCache
 	txnIDs                    TransactionIDFetcher
+	joinChecker               JoinChecker
 	ignoredUsers              map[string]struct{}
 	ignoredUsersMu            *sync.RWMutex
 }
 
-func NewUserCache(userID string, globalCache *GlobalCache, store UserCacheStore, txnIDs TransactionIDFetcher) *UserCache {
+func NewUserCache(userID string, globalCache *GlobalCache, store UserCacheStore, txnIDs TransactionIDFetcher, joinChecker JoinChecker) *UserCache {
 	// see SyncLiveHandler.userCache for the initialisation proper, which works by
 	// firing off a bunch of OnBlahBlah callbacks.
 	uc := &UserCache{
@@ -210,6 +215,7 @@ func NewUserCache(userID string, globalCache *GlobalCache, store UserCacheStore,
 		store:          store,
 		globalCache:    globalCache,
 		txnIDs:         txnIDs,
+		joinChecker:    joinChecker,
 		ignoredUsers:   make(map[string]struct{}),
 		ignoredUsersMu: &sync.RWMutex{},
 	}
@@ -572,12 +578,14 @@ func (c *UserCache) OnSpaceUpdate(ctx context.Context, parentRoomID, childRoomID
 	c.roomToDataMu.Unlock()
 
 	// now we need to notify connections for the _child_
-	roomUpdate := &RoomEventUpdate{
-		RoomUpdate: c.newRoomUpdate(ctx, childRoomID),
-		EventData:  eventData,
+	// but only if they are allowed to see the child event (i.e they are joined to the child room)
+	if c.joinChecker.IsUserJoined(c.UserID, childRoomID) {
+		roomUpdate := &RoomEventUpdate{
+			RoomUpdate: c.newRoomUpdate(ctx, childRoomID),
+			EventData:  eventData,
+		}
+		c.emitOnRoomUpdate(ctx, roomUpdate)
 	}
-
-	c.emitOnRoomUpdate(ctx, roomUpdate)
 }
 
 func (c *UserCache) OnNewEvent(ctx context.Context, eventData *EventData) {

sync3/caches/user_test.go

@@ -10,6 +10,12 @@ import (
 	"github.com/matrix-org/sliding-sync/sync3/caches"
 )
 
+type joinChecker struct{}
+
+func (c *joinChecker) IsUserJoined(userID, roomID string) bool {
+	return true
+}
+
 type txnIDFetcher struct {
 	data map[string]string
 }
@@ -82,7 +88,7 @@ func TestAnnotateWithTransactionIDs(t *testing.T) {
 		fetcher := &txnIDFetcher{
 			data: tc.eventIDToTxnIDs,
 		}
-		uc := caches.NewUserCache(userID, nil, nil, fetcher)
+		uc := caches.NewUserCache(userID, nil, nil, fetcher, &joinChecker{})
 		got := uc.AnnotateWithTransactionIDs(context.Background(), userID, "DEVICE", convertIDToEventStub(userID, tc.roomIDToEvents))
 		want := convertIDTxnToEventStub(userID, tc.wantRoomIDToEvents)
 		if !reflect.DeepEqual(got, want) {

sync3/handler/connstate_test.go

@@ -18,6 +18,12 @@ import (
 	"github.com/matrix-org/sliding-sync/testutils"
 )
 
+type joinChecker struct{}
+
+func (c *joinChecker) IsUserJoined(userID, roomID string) bool {
+	return true
+}
+
 type NopExtensionHandler struct{}
 
 func (h *NopExtensionHandler) Handle(ctx context.Context, req extensions.Request, extCtx extensions.Context) (res extensions.Response) {
@@ -106,7 +112,7 @@ func TestConnStateInitial(t *testing.T) {
 				roomC.RoomID: {NID: 780, Timestamp: 789},
 			}, nil, nil
 	}
-	userCache := caches.NewUserCache(userID, globalCache, &NopUserCacheStore{}, &NopTransactionFetcher{})
+	userCache := caches.NewUserCache(userID, globalCache, &NopUserCacheStore{}, &NopTransactionFetcher{}, &joinChecker{})
 	dispatcher.Register(context.Background(), userCache.UserID, userCache)
 	dispatcher.Register(context.Background(), sync3.DispatcherAllUsers, globalCache)
 	userCache.LazyLoadTimelinesOverride = func(loadPos int64, roomIDs []string, maxTimelineEvents int) map[string]state.LatestEvents {
@@ -279,7 +285,7 @@ func TestConnStateMultipleRanges(t *testing.T) {
 		}
 		return 1, roomMetadata, joinTimings, nil, nil
 	}
-	userCache := caches.NewUserCache(userID, globalCache, &NopUserCacheStore{}, &NopTransactionFetcher{})
+	userCache := caches.NewUserCache(userID, globalCache, &NopUserCacheStore{}, &NopTransactionFetcher{}, &joinChecker{})
 	userCache.LazyLoadTimelinesOverride = mockLazyRoomOverride
 	dispatcher.Register(context.Background(), userCache.UserID, userCache)
 	dispatcher.Register(context.Background(), sync3.DispatcherAllUsers, globalCache)
@@ -458,7 +464,7 @@ func TestBumpToOutsideRange(t *testing.T) {
 			}, nil, nil
 
 	}
-	userCache := caches.NewUserCache(userID, globalCache, &NopUserCacheStore{}, &NopTransactionFetcher{})
+	userCache := caches.NewUserCache(userID, globalCache, &NopUserCacheStore{}, &NopTransactionFetcher{}, &joinChecker{})
 	userCache.LazyLoadTimelinesOverride = mockLazyRoomOverride
 	dispatcher.Register(context.Background(), userCache.UserID, userCache)
 	dispatcher.Register(context.Background(), sync3.DispatcherAllUsers, globalCache)
@@ -561,7 +567,7 @@ func TestConnStateRoomSubscriptions(t *testing.T) {
 				roomD.RoomID: {NID: 4, Timestamp: 4},
 			}, nil, nil
 	}
-	userCache := caches.NewUserCache(userID, globalCache, &NopUserCacheStore{}, &NopTransactionFetcher{})
+	userCache := caches.NewUserCache(userID, globalCache, &NopUserCacheStore{}, &NopTransactionFetcher{}, &joinChecker{})
 	userCache.LazyLoadTimelinesOverride = func(loadPos int64, roomIDs []string, maxTimelineEvents int) map[string]state.LatestEvents {
 		result := make(map[string]state.LatestEvents)
 		for _, roomID := range roomIDs {

sync3/handler/handler.go

@@ -528,7 +528,7 @@ func (h *SyncLiveHandler) userCache(userID string) (*caches.UserCache, error) {
 	if ok {
 		return c.(*caches.UserCache), nil
 	}
-	uc := caches.NewUserCache(userID, h.GlobalCache, h.Storage, h)
+	uc := caches.NewUserCache(userID, h.GlobalCache, h.Storage, h, h.Dispatcher)
 	// select all non-zero highlight or notif counts and set them, as this is less costly than looping every room/user pair
 	err := h.Storage.UnreadTable.SelectAllNonZeroCountsForUser(userID, func(roomID string, highlightCount, notificationCount int) {
 		uc.OnUnreadCounts(context.Background(), roomID, &highlightCount, &notificationCount)

tests-e2e/security_test.go

@@ -324,3 +324,88 @@ func TestSecuritySpaceMetadataLeak(t *testing.T) {
 	})
 	m.MatchResponse(t, res, m.MatchNoV3Ops(), m.MatchRoomSubscriptionsStrict(nil))
 }
+
+// Test that adding a child room to a space does not leak global room metadata about that
+// child room to users in the parent space. This information isn't strictly confidential as
+// the /rooms/{roomId}/hierarchy endpoint will include such metadata (room name, avatar, join count, etc)
+// because the user is part of the parent space. There isn't an attack vector here, but repro steps:
+//   - Alice and Bob are in a parent space.
+//   - Bob has a poller on SS running.
+//   - Alice is live streaming from SS.
+//   - Bob creates a child room in that space, and sends both the m.space.parent in the child room AND
+//     the m.space.child in the parent space.
+//   - Ensure that no information about the child room comes down Alice's connection.
+func TestSecuritySpaceChildMetadataLeakFromParent(t *testing.T) {
+	alice := registerNewUser(t)
+	bob := registerNewUser(t)
+	parentName := "The Parent Room Name"
+	childName := "The Child Room Name"
+
+	// Alice and Bob are in a parent space.
+	parentSpace := bob.MustCreateRoom(t, map[string]interface{}{
+		"preset": "public_chat",
+		"name":   parentName,
+		"creation_content": map[string]string{
+			"type": "m.space",
+		},
+	})
+	alice.MustJoinRoom(t, parentSpace, []string{"hs1"})
+
+	// Bob has a poller on SS running.
+	bobRes := bob.SlidingSync(t, sync3.Request{})
+
+	// Alice is live streaming from SS.
+	aliceRes := alice.SlidingSync(t, sync3.Request{
+		Lists: map[string]sync3.RequestList{
+			"a": {
+				Ranges: [][2]int64{{0, 20}},
+			},
+		},
+	})
+	m.MatchResponse(t, aliceRes, m.MatchRoomSubscriptionsStrict(map[string][]m.RoomMatcher{
+		parentSpace: {
+			m.MatchJoinCount(2),
+			m.MatchRoomName(parentName),
+		},
+	}))
+
+	// Bob creates a child room in that space, and sends both the m.space.parent in the child room AND
+	// the m.space.child in the parent space.
+	childRoom := bob.MustCreateRoom(t, map[string]interface{}{
+		"preset": "public_chat",
+		"name":   childName,
+		"initial_state": []map[string]interface{}{
+			{
+				"type":      "m.space.parent",
+				"state_key": parentSpace,
+				"content": map[string]interface{}{
+					"canonical": true,
+					"via":       []string{"hs1"},
+				},
+			},
+		},
+	})
+	chlidEventID := bob.SendEventSynced(t, parentSpace, b.Event{
+		Type:     "m.space.child",
+		StateKey: ptr(childRoom),
+		Content: map[string]interface{}{
+			"via": []string{"hs1"},
+		},
+	})
+	// wait for SS to process it
+	bob.SlidingSyncUntilEventID(t, bobRes.Pos, parentSpace, chlidEventID)
+
+	// Ensure that no information about the child room comes down Alice's connection.
+	aliceRes = alice.SlidingSync(t, sync3.Request{}, WithPos(aliceRes.Pos))
+	m.MatchResponse(t, aliceRes, m.MatchRoomSubscriptionsStrict(map[string][]m.RoomMatcher{
+		parentSpace: {
+			MatchRoomTimeline([]Event{{
+				Type:     "m.space.child",
+				StateKey: ptr(childRoom),
+				Content: map[string]interface{}{
+					"via": []interface{}{"hs1"},
+				},
+			}}),
+		},
+	}))
+}