CVE-2023-22737: Missing Authorization (CWE-862)
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 perform an authorization check when an actor attempts to access a resource or perform an action.
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)