From 4c6d504022195684ba13cf178366d5e13d8e5744 Mon Sep 17 00:00:00 2001 From: Kegan Dougal <7190048+kegsay@users.noreply.github.com> Date: Fri, 12 Jan 2024 12:15:38 +0000 Subject: [PATCH] bugfix: ensure metadata about space children doesn't leak to active connections If Alice and Bob are in the same space, and Bob creates a child in that space, Alice would incorrectly receive global metadata about that child room if Alice was live syncing at that time. This leak did not expose confidential information as Alice could receive all that metadata via the /rooms/{roomId}/hierarchy endpoint already. However, it would cause clients to put the space child room into the room list which would be very confusing, as it would have no timeline and no other data. --- sync3/caches/user.go | 20 +++++--- sync3/caches/user_test.go | 8 +++- sync3/handler/connstate_test.go | 14 ++++-- sync3/handler/handler.go | 2 +- tests-e2e/security_test.go | 85 +++++++++++++++++++++++++++++++++ 5 files changed, 117 insertions(+), 12 deletions(-) diff --git a/sync3/caches/user.go b/sync3/caches/user.go index 5cdd0f15..d05fdb73 100644 --- a/sync3/caches/user.go +++ b/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) { diff --git a/sync3/caches/user_test.go b/sync3/caches/user_test.go index 5809317b..04b572cc 100644 --- a/sync3/caches/user_test.go +++ b/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) { diff --git a/sync3/handler/connstate_test.go b/sync3/handler/connstate_test.go index c5b4f11b..0515431a 100644 --- a/sync3/handler/connstate_test.go +++ b/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 { diff --git a/sync3/handler/handler.go b/sync3/handler/handler.go index 127deddc..01e36b9c 100644 --- a/sync3/handler/handler.go +++ b/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, ¬ificationCount) diff --git a/tests-e2e/security_test.go b/tests-e2e/security_test.go index d762c780..4e846171 100644 --- a/tests-e2e/security_test.go +++ b/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"}, + }, + }}), + }, + })) +}