From 87c15444673ea1ba4579bc7fbed1903669a76e07 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 (cherry picked from commit 1da8b6f4e5747de0c352fe26b506a5404a696d4a) --- src/ACL.cpp | 70 +++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 54 insertions(+), 16 deletions(-) diff --git a/src/ACL.cpp b/src/ACL.cpp index ec780715c41..ecf33422821 100644 --- a/src/ACL.cpp +++ b/src/ACL.cpp @@ -139,38 +139,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 37eb6958e16cd4bd3c9b6c273abe6243d22ef274 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. (cherry picked from commit 1e05f14f5b7d96e89efe11f8df7f3d2159edd81d) --- 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 d98691bb6d2..4544f50f979 100644 --- a/src/mumble/MainWindow.cpp +++ b/src/mumble/MainWindow.cpp @@ -2556,20 +2556,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; @@ -2605,7 +2591,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 7cde61ab9ae..718bbeb3dba 100644 --- a/src/murmur/Messages.cpp +++ b/src/murmur/Messages.cpp @@ -1756,8 +1756,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 dcd739dbe60cb05dbf7c39773c320547e304ceb0 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. (cherry picked from commit 55b4de02abb99cc388c6f721541744df0bdefea5) --- 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 7d990e047cc..9107f994db6 100644 --- a/src/mumble/ACLEditor.cpp +++ b/src/mumble/ACLEditor.cpp @@ -827,18 +827,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() {