CVE-2023-22737: Improper Handling of Insufficient Permissions or Privileges  (CWE-280)
value: wire-server provides back end services for Wire, a team communication and collaboration platform. Prior to version 2022-12-09, every member of a Conversation can remove a Bot from a Conversation due to a missing permissions check. Only Conversation admins should be able to remove Bots. Regular Conversations are not allowed to do so. The issue is fixed in wire-server 2022-12-09 and is already deployed on all Wire managed services. On-premise instances of wire-server need to be updated to 2022-12-09/Chart 4.29.0, so that their backends are no longer affected. There are no known workarounds.
CWE description: The product does not handle or incorrectly handles when it has insufficient privileges to access resources or functionality as specified by their permissions. This may cause it to follow unexpected code paths that may leave the product in an invalid state.
CVSS v3 base score: 6.5
Published: 2023-01-28T00:15Z
Repository: https://github.com/wireapp/wire-server
Language: Other
Files changed: changelog.d/3-bug-fixes/pr-2870, services/brig/src/Brig/Provider/API.hs, services/brig/test/integration/API/Provider.hs, services/brig/test/integration/API/Team/Util.hs, services/galley/src/Galley/API/Update.hs
Commit date: 2022-12-09 11:45:52 +0000
Commit message: [SQSERVICES-1801] Prevent dead bots in database (#2870)

Diff: diff --git a/changelog.d/3-bug-fixes/pr-2870 b/changelog.d/3-bug-fixes/pr-2870
new file mode 100644
index 000000000..765f957fb
--- /dev/null
+++ b/changelog.d/3-bug-fixes/pr-2870
@@ -0,0 +1 @@
+Prevention of storing unnecessary data in the database if adding a bot to a conversation fails.
diff --git a/services/brig/src/Brig/Provider/API.hs b/services/brig/src/Brig/Provider/API.hs
index 26cbdc0ba..84ea17a47 100644
--- a/services/brig/src/Brig/Provider/API.hs
+++ b/services/brig/src/Brig/Provider/API.hs
@@ -896,10 +896,18 @@ addBot zuid zcon cid add = do
   zusr <- lift (wrapClient $ User.lookupUser NoPendingInvitations zuid) >>= maybeInvalidUser
   let pid = addBotProvider add
   let sid = addBotService add
   -- Get the conversation and check preconditions
   cnv <- lift (liftSem $ GalleyProvider.getConv zuid cid) >>= maybeConvNotFound
+  -- Check that the user is a conversation admin and therefore is allowed to add a bot to this conversation.
+  -- Note that this precondition is also checked in the internal galley API,
+  -- but by having this check here we prevent any (useless) data to be written to the database
+  -- as well as the unnecessary creation of the bot via the external service API call.
+  -- However, in case we refine the roles model in the future, this check might not be granular enough.
+  -- In that case we should rather do an internal call to galley to check for the correct permissions.
+  -- Also see `removeBot` for a similar check.
+  guardConvAdmin cnv
   let mems = cnvMembers cnv
   unless (cnvType cnv == RegularConv) $
     throwStd invalidConv
   maxSize <- fromIntegral . setMaxConvSize <$> view settings
   unless (length (cmOthers mems) < maxSize - 1) $
@@ -972,10 +980,16 @@ removeBotH (zusr ::: zcon ::: cid ::: bid) = do
 
 removeBot :: Members '[GalleyProvider] r => UserId -> ConnId -> ConvId -> BotId -> (Handler r) (Maybe Public.RemoveBotResponse)
 removeBot zusr zcon cid bid = do
   -- Get the conversation and check preconditions
   cnv <- lift (liftSem $ GalleyProvider.getConv zusr cid) >>= maybeConvNotFound
+  -- Check that the user is a conversation admin and therefore is allowed to remove a bot from the conversation.
+  -- Note that this precondition is also checked in the internal galley API.
+  -- However, in case we refine the roles model in the future, this check might not be granular enough.
+  -- In that case we should rather do an internal call to galley to check for the correct permissions.
+  -- Also see `addBot` for a similar check.
+  guardConvAdmin cnv
   let mems = cnvMembers cnv
   unless (cnvType cnv == RegularConv) $
     throwStd invalidConv
   -- Find the bot in the member list and delete it
   let busr = botUserId bid
@@ -983,10 +997,15 @@ removeBot zusr zcon cid bid = do
   case bot >>= omService of
     Nothing -> pure Nothing
     Just _ -> do
       lift $ Public.RemoveBotResponse <$$> wrapHttpClient (deleteBot zusr (Just zcon) bid cid)
 
+guardConvAdmin :: Conversation -> ExceptT Error (AppT r) ()
+guardConvAdmin conv = do
+  let selfMember = cmSelf . cnvMembers $ conv
+  unless (memConvRoleName selfMember == roleNameWireAdmin) $ throwStd accessDenied
+
 --------------------------------------------------------------------------------
 -- Bot API
 
 botGetSelfH :: Members '[GalleyProvider] r => BotId -> (Handler r) Response
 botGetSelfH bot = do
diff --git a/services/brig/test/integration/API/Provider.hs b/services/brig/test/integration/API/Provider.hs
index 1449ce29c..ebadad1e6 100644
--- a/services/brig/test/integration/API/Provider.hs
+++ b/services/brig/test/integration/API/Provider.hs
@@ -146,10 +146,11 @@ tests dom conf p db b c g = do
             test p "add bot forbidden" $ testAddBotForbidden conf db b g
           ],
         testGroup
           "bot-teams"
           [ test p "add-remove" $ testAddRemoveBotTeam conf db b g c,
+            test p "add-remove-access-denied-for-non-conv-admin" $ testNonConvAdminCannotAddRemoveBot conf db b g,
             test p "team-only" $ testBotTeamOnlyConv conf db b g c,
             test p "message" $ testMessageBotTeam conf db b g c,
             test p "delete conv" $ testDeleteConvBotTeam conf db b g c,
             test p "delete team" $ testDeleteTeamBotTeam conf db b g c
           ],
@@ -564,10 +565,34 @@ testAddBotBlocked config db brig galley = withTestService config db brig defServ
   enabled2ndFaForTeamInternal galley tid
   addBot brig u1 pid sid cid !!! do
     const 403 === statusCode
     const (Just "access-denied") === fmap Error.label . responseJsonMaybe
 
+testNonConvAdminCannotAddRemoveBot :: Config -> DB.ClientState -> Brig -> Galley -> Http ()
+testNonConvAdminCannotAddRemoveBot config db brig galley = withTestService config db brig defServiceApp $ \sref _buf -> do
+  let pid = sref ^. serviceRefProvider
+  let sid = sref ^. serviceRefId
+  (ownerId, tid) <- Team.createUserWithTeam brig
+  member <- Team.createTeamMember brig galley ownerId tid fullPermissions
+  let memberId = userId member
+  whitelistService brig ownerId tid pid sid
+  cid <- Team.createTeamConvWithRole roleNameWireMember galley tid ownerId [memberId] Nothing
+  addBot brig memberId pid sid cid !!! do
+    const 403 === statusCode
+    const (Just "access-denied") === fmap Error.label . responseJsonMaybe
+  rs <- responseJsonError =<< addBot brig ownerId pid sid cid <!! const 201 === statusCode
+  let bid = rsAddBotId rs
+      buid = botUserId bid
+  getUser brig ownerId buid !!! const 200 === statusCode
+  removeBot brig memberId cid bid !!! do
+    const 403 === statusCode
+    const (Just "access-denied") === fmap Error.label . responseJsonMaybe
+  -- also check the internal galley API
+  removeBotInternal galley memberId cid bid !!! do
+    const 403 === statusCode
+    const (Just "action-denied") === fmap Error.label . responseJsonMaybe
+
 testGetBotConvBlocked :: Config -> DB.ClientState -> Brig -> Galley -> Cannon -> Http ()
 testGetBotConvBlocked config db brig galley cannon = withTestService config db brig defServiceApp $ \sref buf -> do
   (user1, userId -> u2, _, tid, cid, pid, sid) <- prepareBotUsersTeam brig galley sref
   let Qualified u1 localDomain = userQualifiedId user1
   bid <- addBotConv localDomain brig cannon u1 u2 cid pid sid buf
@@ -1303,10 +1328,35 @@ removeBot brig uid cid bid =
       . paths ["conversations", toByteString' cid, "bots", toByteString' bid]
       . header "Z-Type" "access"
       . header "Z-User" (toByteString' uid)
       . header "Z-Connection" "conn"
 
+data RemoveBot = RemoveBot
+  { _rmBotConv :: !ConvId,
+    _rmBotId :: !BotId
+  }
+
+instance ToJSON RemoveBot where
+  toJSON a =
+    object
+      [ "conversation" .= _rmBotConv a,
+        "bot" .= _rmBotId a
+      ]
+
+removeBotInternal ::
+  Galley ->
+  UserId ->
+  ConvId ->
+  BotId ->
+  Http ResponseLBS
+removeBotInternal galley uid cid bid =
+  delete $
+    galley
+      . paths ["i", "bots"]
+      . header "Z-User" (toByteString' uid)
+      . Bilge.json (RemoveBot cid bid)
+
 createConv ::
   Galley ->
   UserId ->
   [UserId] ->
   Http ResponseLBS
diff --git a/services/brig/test/integration/API/Team/Util.hs b/services/brig/test/integration/API/Team/Util.hs
index 6fa61788a..8bf86c2aa 100644
--- a/services/brig/test/integration/API/Team/Util.hs
+++ b/services/brig/test/integration/API/Team/Util.hs
@@ -212,14 +212,28 @@ updatePermissions from tid (to, perm) galley =
       === statusCode
   where
     changeMember = Member.mkNewTeamMember to perm Nothing
 
 createTeamConv :: HasCallStack => Galley -> TeamId -> UserId -> [UserId] -> Maybe Milliseconds -> Http ConvId
-createTeamConv g tid u us mtimer = do
+createTeamConv = createTeamConvWithRole roleNameWireAdmin
+
+createTeamConvWithRole :: HasCallStack => RoleName -> Galley -> TeamId -> UserId -> [UserId] -> Maybe Milliseconds -> Http ConvId
+createTeamConvWithRole role g tid u us mtimer = do
   let tinfo = Just $ ConvTeamInfo tid
   let conv =
-        NewConv us [] Nothing (Set.fromList []) Nothing tinfo mtimer Nothing roleNameWireAdmin ProtocolProteusTag Nothing
+        NewConv
+          us
+          []
+          Nothing
+          (Set.fromList [])
+          Nothing
+          tinfo
+          mtimer
+          Nothing
+          role
+          ProtocolProteusTag
+          Nothing
   r <-
     post
       ( g
           . path "/conversations"
           . zUser u
diff --git a/services/galley/src/Galley/API/Update.hs b/services/galley/src/Galley/API/Update.hs
index 0ee228151..5349bb128 100644
--- a/services/galley/src/Galley/API/Update.hs
+++ b/services/galley/src/Galley/API/Update.hs
@@ -1569,10 +1569,12 @@ addBot lusr zcon b = do
     regularConvChecks c = do
       let (bots, users) = localBotsAndUsers (Data.convLocalMembers c)
       unless (tUnqualified lusr `isMember` users) $ throwS @'ConvNotFound
       ensureGroupConversation c
       self <- getSelfMemberFromLocals (tUnqualified lusr) users
+      -- Note that in brig from where this internal handler is called, we additionally check for conversation admin role.
+      -- Remember to change this if we ever want to allow non admins to add bots.
       ensureActionAllowed SAddConversationMember self
       unless (any ((== b ^. addBotId) . botMemId) bots) $ do
         let botId = qualifyAs lusr (botUserId (b ^. addBotId))
         ensureMemberLimit (toList $ Data.convLocalMembers c) [qUntagged botId]
       pure (bots, users)
@@ -1585,11 +1587,12 @@ rmBotH ::
        ExternalAccess,
        GundeckAccess,
        Input (Local ()),
        Input UTCTime,
        MemberStore,
-       WaiRoutes
+       WaiRoutes,
+       ErrorS ('ActionDenied 'RemoveConversationMember)
      ]
     r =>
   UserId ::: Maybe ConnId ::: JsonRequest RemoveBot ->
   Sem r Response
 rmBotH (zusr ::: zcon ::: req) = do
@@ -1603,24 +1606,32 @@ rmBot ::
        ConversationStore,
        ErrorS 'ConvNotFound,
        ExternalAccess,
        GundeckAccess,
        Input UTCTime,
-       MemberStore
+       MemberStore,
+       ErrorS ('ActionDenied 'RemoveConversationMember)
      ]
     r =>
   Local UserId ->
   Maybe ConnId ->
   RemoveBot ->
   Sem r (UpdateResult Event)
 rmBot lusr zcon b = do
   c <-
     E.getConversation (b ^. rmBotConv) >>= noteS @'ConvNotFound
-  let lcnv = qualifyAs lusr (Data.convId c)
+  let (bots, users) = localBotsAndUsers (Data.convLocalMembers c)
   unless (tUnqualified lusr `isMember` Data.convLocalMembers c) $
     throwS @'ConvNotFound
-  let (bots, users) = localBotsAndUsers (Data.convLocalMembers c)
+  -- A bot can remove itself (which will internally be triggered when a service is deleted),
+  -- otherwise we have to check for the correct permissions
+  unless (botUserId (b ^. rmBotId) == tUnqualified lusr) $ do
+    -- Note that in brig from where this internal handler is called, we additionally check for conversation admin role.
+    -- Remember to change this if we ever want to allow non admins to remove bots.
+    self <- getSelfMemberFromLocals (tUnqualified lusr) users
+    ensureActionAllowed SRemoveConversationMember self
+  let lcnv = qualifyAs lusr (Data.convId c)
   if not (any ((== b ^. rmBotId) . botMemId) bots)
     then pure Unchanged
     else do
       t <- input
       do

Vulnerable code: createTeamConv g tid u us mtimer = do
        NewConv us [] Nothing (Set.fromList []) Nothing tinfo mtimer Nothing roleNameWireAdmin ProtocolProteusTag Nothing
       WaiRoutes
       MemberStore
  let lcnv = qualifyAs lusr (Data.convId c)
  let (bots, users) = localBotsAndUsers (Data.convLocalMembers c)
Fixed code: Prevention of storing unnecessary data in the database if adding a bot to a conversation fails.
  -- Check that the user is a conversation admin and therefore is allowed to add a bot to this conversation.
  -- Note that this precondition is also checked in the internal galley API,
  -- but by having this check here we prevent any (useless) data to be written to the database
  -- as well as the unnecessary creation of the bot via the external service API call.
  -- However, in case we refine the roles model in the future, this check might not be granular enough.
  -- In that case we should rather do an internal call to galley to check for the correct permissions.
  -- Also see `removeBot` for a similar check.
  guardConvAdmin cnv
  -- Check that the user is a conversation admin and therefore is allowed to remove a bot from the conversation.
  -- Note that this precondition is also checked in the internal galley API.
  -- However, in case we refine the roles model in the future, this check might not be granular enough.
  -- In that case we should rather do an internal call to galley to check for the correct permissions.
  -- Also see `addBot` for a similar check.
  guardConvAdmin cnv
guardConvAdmin :: Conversation -> ExceptT Error (AppT r) ()
guardConvAdmin conv = do
  let selfMember = cmSelf . cnvMembers $ conv
  unless (memConvRoleName selfMember == roleNameWireAdmin) $ throwStd accessDenied

            test p "add-remove-access-denied-for-non-conv-admin" $ testNonConvAdminCannotAddRemoveBot conf db b g,
testNonConvAdminCannotAddRemoveBot :: Config -> DB.ClientState -> Brig -> Galley -> Http ()
testNonConvAdminCannotAddRemoveBot config db brig galley = withTestService config db brig defServiceApp $ \sref _buf -> do
  let pid = sref ^. serviceRefProvider
  let sid = sref ^. serviceRefId
  (ownerId, tid) <- Team.createUserWithTeam brig
  member <- Team.createTeamMember brig galley ownerId tid fullPermissions
  let memberId = userId member
  whitelistService brig ownerId tid pid sid
  cid <- Team.createTeamConvWithRole roleNameWireMember galley tid ownerId [memberId] Nothing
  addBot brig memberId pid sid cid !!! do
    const 403 === statusCode
    const (Just "access-denied") === fmap Error.label . responseJsonMaybe
  rs <- responseJsonError =<< addBot brig ownerId pid sid cid <!! const 201 === statusCode
  let bid = rsAddBotId rs
      buid = botUserId bid
  getUser brig ownerId buid !!! const 200 === statusCode
  removeBot brig memberId cid bid !!! do
    const 403 === statusCode
    const (Just "access-denied") === fmap Error.label . responseJsonMaybe
  -- also check the internal galley API
  removeBotInternal galley memberId cid bid !!! do
    const 403 === statusCode
    const (Just "action-denied") === fmap Error.label . responseJsonMaybe

data RemoveBot = RemoveBot
  { _rmBotConv :: !ConvId,
    _rmBotId :: !BotId
  }

instance ToJSON RemoveBot where
  toJSON a =
    object
      [ "conversation" .= _rmBotConv a,
        "bot" .= _rmBotId a
      ]

removeBotInternal ::
  Galley ->
  UserId ->
  ConvId ->
  BotId ->
  Http ResponseLBS
removeBotInternal galley uid cid bid =
  delete $
    galley
      . paths ["i", "bots"]
      . header "Z-User" (toByteString' uid)
      . Bilge.json (RemoveBot cid bid)

createTeamConv = createTeamConvWithRole roleNameWireAdmin

createTeamConvWithRole :: HasCallStack => RoleName -> Galley -> TeamId -> UserId -> [UserId] -> Maybe Milliseconds -> Http ConvId
createTeamConvWithRole role g tid u us mtimer = do
        NewConv
          us
          []
          Nothing
          (Set.fromList [])
          Nothing
          tinfo
          mtimer
          Nothing
          role
          ProtocolProteusTag
          Nothing
      -- Note that in brig from where this internal handler is called, we additionally check for conversation admin role.
      -- Remember to change this if we ever want to allow non admins to add bots.
       WaiRoutes,
       ErrorS ('ActionDenied 'RemoveConversationMember)
       MemberStore,
       ErrorS ('ActionDenied 'RemoveConversationMember)
  let (bots, users) = localBotsAndUsers (Data.convLocalMembers c)
  -- A bot can remove itself (which will internally be triggered when a service is deleted),
  -- otherwise we have to check for the correct permissions
  unless (botUserId (b ^. rmBotId) == tUnqualified lusr) $ do
    -- Note that in brig from where this internal handler is called, we additionally check for conversation admin role.
    -- Remember to change this if we ever want to allow non admins to remove bots.
    self <- getSelfMemberFromLocals (tUnqualified lusr) users
    ensureActionAllowed SRemoveConversationMember self
  let lcnv = qualifyAs lusr (Data.convId c)