From 1da8b6f4e5747de0c352fe26b506a5404a696d4a Mon Sep 17 00:00:00 2001 From: Hartmnt Date: Wed, 17 Jul 2024 10:06:57 +0000 Subject: [PATCH 1/3] FIX(server): Make sure context is applied to traverse and write ACL Previously, both the traverse and write ACL would be evaluated without taking the "in this channel" and "in sub-channels" context options into account. This would lead to denying traverse for channels that were actually supposed to be traversable. This commit refactors the ACL calculation for readability and makes sure the write and traverse checks are actually taking the context into account. Fixes #3579 --- src/ACL.cpp | 70 +++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 54 insertions(+), 16 deletions(-) diff --git a/src/ACL.cpp b/src/ACL.cpp index 8d3180ce510..f745dcaa96b 100644 --- a/src/ACL.cpp +++ b/src/ACL.cpp @@ -133,38 +133,76 @@ QFlags< ChanACL::Perm > ChanACL::effectivePermissions(ServerUser *p, Channel *ch bool traverse = true; bool write = false; - ChanACL *acl; + // Iterate over all parent channels from root to the channel the user is in (inclusive) while (!chanstack.isEmpty()) { ch = chanstack.pop(); - if (!ch->bInheritACL) + if (!ch->bInheritACL) { granted = def; + } - foreach (acl, ch->qlACL) { + for (const ChanACL *acl : ch->qlACL) { bool matchUser = (acl->iUserId != -1) && (acl->iUserId == p->iId); bool matchGroup = Group::appliesToUser(*chan, *ch, acl->qsGroup, *p); + + bool applyFromSelf = (ch == chan && acl->bApplyHere); + bool applyInherited = (ch != chan && acl->bApplySubs); + + // Flag indicating whether the current ACL affects the target channel "chan" + bool apply = applyFromSelf || applyInherited; + + // "apply" will be true for ACLs set in the reference channel directly (applyHere), + // or from a parent channel which hands the ACLs down (applySubs). + // However, we have one ACL that needs to be evaluated differently - the Traverse ACL. + // Consider this channel layout: + // Root + // - A (Traverse denied for THIS channel, but not sub channels) + // - B + // - C + // If the user tries to enter C, we need to deny Traverse, because the user + // should already be blocked from traversing A. But "apply" will be false, + // as the "normal" ACL inheritence rules do not apply here. + // Therefore, we need applyDenyTraverse which will be true, if any channel + // from root to the reference channel denies Traverse without necessarily + // handing it down. + bool applyDenyTraverse = applyInherited || acl->bApplyHere; + if (matchUser || matchGroup) { - if (acl->pAllow & Traverse) + // The "traverse" and "write" booleans do not grant or deny anything here. + // We merely check, if we are missing traverse AND write in this + // channel and therefore abort without any permissions later on. + if (apply && (acl->pAllow & Traverse)) { traverse = true; - if (acl->pDeny & Traverse) + } + if (applyDenyTraverse && (acl->pDeny & Traverse)) { traverse = false; - if (acl->pAllow & Write) - write = true; - if (acl->pDeny & Write) - write = false; - if (ch->iId == 0 && chan == ch && acl->bApplyHere) { - if (acl->pAllow & Kick) + } + + write = apply && (acl->pAllow & Write) && !(acl->pDeny & Write); + + // These permissions are only grantable from the root channel + // as they affect the users globally. For example: You can not + // kick a client from a channel without kicking them from the server. + if (ch->iId == 0 && applyFromSelf) { + if (acl->pAllow & Kick) { granted |= Kick; - if (acl->pAllow & Ban) + } + if (acl->pAllow & Ban) { granted |= Ban; - if (acl->pAllow & ResetUserContent) + } + if (acl->pAllow & ResetUserContent) { granted |= ResetUserContent; - if (acl->pAllow & Register) + } + if (acl->pAllow & Register) { granted |= Register; - if (acl->pAllow & SelfRegister) + } + if (acl->pAllow & SelfRegister) { granted |= SelfRegister; + } } - if ((ch == chan && acl->bApplyHere) || (ch != chan && acl->bApplySubs)) { + + // Every other regular ACL is handled here + if (apply) { granted |= (acl->pAllow & ~(Kick | Ban | ResetUserContent | Register | SelfRegister | Cached)); granted &= ~acl->pDeny; } From 1e05f14f5b7d96e89efe11f8df7f3d2159edd81d Mon Sep 17 00:00:00 2001 From: Hartmnt Date: Wed, 17 Jul 2024 16:31:58 +0000 Subject: [PATCH 2/3] FIX(server, client): Remove "Write" ACL parent channel inheritance Since 2a9dcfde4e423c4414f975a4b0b77cb08d08e782 and 62b1536fe0e91c03bf803075dff031a1f4dba9f4 the Mumble server would overwrite the current channel Write ACL, if the user had Write ACL permission in the parent channel. Supposedly, this was done because otherwise malicious users could create temporary "ungovernable" channels by locking admins out denying Write ACL for them. However, this makes ACL management a lot less intuitive with regard to the Write permission. This commit reverts those commits and instead adds a check to see if the user has Write permission in the root channel instead. The reasoning being: If the server owner grants Write ACL on root, they probably want those users to be able to moderate every channel. If instead the server owner only grants Write on part of the channel tree, normal ACL rules apply and users may lock other users out for whatever reason. --- src/mumble/MainWindow.cpp | 16 +--------------- src/murmur/Messages.cpp | 11 +++++++++-- 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/src/mumble/MainWindow.cpp b/src/mumble/MainWindow.cpp index 4f511bb10ab..92c50dbf3d1 100644 --- a/src/mumble/MainWindow.cpp +++ b/src/mumble/MainWindow.cpp @@ -2561,20 +2561,6 @@ void MainWindow::updateMenuPermissions() { target.channel->uiPermissions = p; } - Channel *cparent = target.channel ? target.channel->cParent : nullptr; - ChanACL::Permissions pparent = - cparent ? static_cast< ChanACL::Permissions >(cparent->uiPermissions) : ChanACL::None; - - if (cparent && !pparent) { - Global::get().sh->requestChannelPermissions(cparent->iId); - if (cparent->iId == 0) - pparent = Global::get().pPermissions; - else - pparent = ChanACL::All; - - cparent->uiPermissions = pparent; - } - ClientUser *user = Global::get().uiSession ? ClientUser::get(Global::get().uiSession) : nullptr; Channel *homec = user ? user->cChannel : nullptr; ChanACL::Permissions homep = homec ? static_cast< ChanACL::Permissions >(homec->uiPermissions) : ChanACL::None; @@ -2610,7 +2596,7 @@ void MainWindow::updateMenuPermissions() { qaChannelAdd->setEnabled(p & (ChanACL::Write | ChanACL::MakeChannel | ChanACL::MakeTempChannel)); qaChannelRemove->setEnabled(p & ChanACL::Write); - qaChannelACL->setEnabled((p & ChanACL::Write) || (pparent & ChanACL::Write)); + qaChannelACL->setEnabled((p & ChanACL::Write) || (Global::get().pPermissions & ChanACL::Write)); qaChannelLink->setEnabled((p & (ChanACL::Write | ChanACL::LinkChannel)) && (homep & (ChanACL::Write | ChanACL::LinkChannel))); diff --git a/src/murmur/Messages.cpp b/src/murmur/Messages.cpp index c92e366b070..8b1e6a58334 100644 --- a/src/murmur/Messages.cpp +++ b/src/murmur/Messages.cpp @@ -1745,8 +1745,15 @@ void Server::msgACL(ServerUser *uSource, MumbleProto::ACL &msg) { if (!c) return; - if (!hasPermission(uSource, c, ChanACL::Write) - && !(c->cParent && hasPermission(uSource, c->cParent, ChanACL::Write))) { + // For changing channel properties (the 'Write') ACL we allow two things: + // 1) As per regular ACL propagating mechanism, we check if the user has been + // granted Write in the channel they try to edit + // 2) We allow all users who have been granted 'Write' on the root channel + // to be able to edit _all_ channels, independent of actual propagated ACLs + // This is done to prevent users who have permission to create (temporary) + // channels being able to "lock-out" admins by denying them 'Write' in their + // channel effectively becoming ungovernable. + if (!hasPermission(uSource, c, ChanACL::Write) && !hasPermission(uSource, qhChannels.value(0), ChanACL::Write)) { PERM_DENIED(uSource, c, ChanACL::Write); return; } From 55b4de02abb99cc388c6f721541744df0bdefea5 Mon Sep 17 00:00:00 2001 From: Hartmnt Date: Thu, 18 Jul 2024 10:48:04 +0000 Subject: [PATCH 3/3] FIX(client): Prevent unchecking both ACL context checkboxes Previously, it was possible to have both context checkboxes disabled in the ACLEditor, leaving the ACL entry in a dangleing inactive state. This commit makes sure, that at least one of the checkboxes is always enabled. --- src/mumble/ACLEditor.cpp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/mumble/ACLEditor.cpp b/src/mumble/ACLEditor.cpp index 1bacc42dbbe..9e4c4ad84aa 100644 --- a/src/mumble/ACLEditor.cpp +++ b/src/mumble/ACLEditor.cpp @@ -815,18 +815,26 @@ void ACLEditor::on_qcbACLInherit_clicked(bool) { void ACLEditor::on_qcbACLApplyHere_clicked(bool checked) { ChanACL *as = currentACL(); - if (!as || as->bInherited) + if (!as || as->bInherited) { return; + } as->bApplyHere = checked; + if (!checked && !qcbACLApplySubs->isChecked()) { + qcbACLApplySubs->setCheckState(Qt::Checked); + } } void ACLEditor::on_qcbACLApplySubs_clicked(bool checked) { ChanACL *as = currentACL(); - if (!as || as->bInherited) + if (!as || as->bInherited) { return; + } as->bApplySubs = checked; + if (!checked && !qcbACLApplyHere->isChecked()) { + qcbACLApplyHere->setCheckState(Qt::Checked); + } } void ACLEditor::qcbACLGroup_focusLost() {