From 8289e2dcff314853e5cc8c0fdac27edb9535284c Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Sun, 19 Jul 2026 20:20:15 +0200 Subject: [PATCH] Fix custom-role privilege escalation and align read guards (security review) Follow-up hardening on the custom-role work, found during a static review: - Admin panel type change (apply_membership_type_change): when converting a Custom member to the legacy Manager role, only preserve `access_all` if the member actually held the full "manage all collections" grant (all three collection flags). Previously an Edit-any-collection-only Custom member (whose access_all is just the Edit mirror) became a Manager with a broad access_all grant, silently escalating Edit-only into Create + Edit + Delete. Mirrors the collection-permissions down-migration. Unit test updated. - Bulk collection access (post_bulk_access_collections): drop the blanket has_full_access() requirement and rely on the existing per-collection is_manageable_by_user check (as the single-collection endpoint and the pre-existing behavior do). Pure manage_users / manage_groups / manage_policies Custom members hold no per-collection manage grant and are still rejected, while a Manager/Custom member who manages some collections regains the ability to bulk-edit exactly those collections. - Group details (GET /organizations//groups//details): align the guard with the list endpoint (/groups/details) to ManageUsersOrGroups, so a manage_users member is not denied the single-group view of the same data it can already read in bulk. --- src/api/admin.rs | 42 +++++++++++++++++++++++++++++------ src/api/core/organizations.rs | 20 +++++++++++------ 2 files changed, 48 insertions(+), 14 deletions(-) diff --git a/src/api/admin.rs b/src/api/admin.rs index 0c85d562..8c60e0c7 100644 --- a/src/api/admin.rs +++ b/src/api/admin.rs @@ -547,6 +547,16 @@ struct MembershipTypeData { fn apply_membership_type_change(membership: &mut Membership, new_type: MembershipType) { let was_custom = membership.atype == MembershipType::Custom; + // Leaving Custom for the legacy Manager role: `access_all` is the internal mirror of + // Edit any collection, but a Manager's `access_all` means the broad "manage all collections" + // grant (Create + Edit + Delete). Carrying an Edit-only mirror over would silently escalate the + // member to Create/Edit/Delete. Only preserve `access_all` when the member actually held the + // full manage-all grant, mirroring the collection-permissions down-migration. Must be evaluated + // before the flags are cleared below (and while the type is still Custom). + if was_custom && new_type == MembershipType::Manager { + membership.access_all = membership.has_manage_all_collections(); + } + // Entering Custom through the Vaultwarden admin panel is deliberately fail-closed because // that UI cannot select granular permissions; they can be granted later through the regular // organization member dialog. @@ -956,12 +966,30 @@ mod tests { apply_membership_type_change(&mut user, MembershipType::Admin); assert!(user.access_all); - let mut custom = membership(MembershipType::Custom); - custom.access_all = true; - custom.edit_any_collection = true; - apply_membership_type_change(&mut custom, MembershipType::Manager); - assert_eq!(custom.atype, MembershipType::Manager as i32); - assert!(custom.access_all); - assert!(!custom.edit_any_collection); + // REGRESSION (privilege escalation, PR #7397 / finding F2): a Custom member whose + // `access_all` is only the Edit-any-collection mirror must NOT be turned into a legacy + // Manager with the broad "manage all collections" `access_all` grant. That would escalate + // an Edit-only member into Create + Edit + Delete. Mirrors the collection down-migration. + let mut edit_only = membership(MembershipType::Custom); + edit_only.access_all = true; + edit_only.edit_any_collection = true; + apply_membership_type_change(&mut edit_only, MembershipType::Manager); + assert_eq!(edit_only.atype, MembershipType::Manager as i32); + assert!(!edit_only.access_all, "Edit-only Custom must not become an access_all Manager"); + assert!(!edit_only.edit_any_collection); + + // A Custom member who genuinely held the full manage-all grant (all three collection + // flags) keeps the equivalent legacy Manager `access_all`. + let mut manage_all = membership(MembershipType::Custom); + manage_all.access_all = true; + manage_all.create_new_collections = true; + manage_all.edit_any_collection = true; + manage_all.delete_any_collection = true; + apply_membership_type_change(&mut manage_all, MembershipType::Manager); + assert_eq!(manage_all.atype, MembershipType::Manager as i32); + assert!(manage_all.access_all, "full manage-all Custom keeps legacy Manager access_all"); + assert!(!manage_all.create_new_collections); + assert!(!manage_all.edit_any_collection); + assert!(!manage_all.delete_any_collection); } } diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index ca30766e..4945f3dc 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -629,12 +629,13 @@ async fn post_bulk_access_collections( err!("Can't find organization details") } - // Security: only callers who can actually manage collections (Admins/Owners, or users with - // full access) may change collection access in bulk. A custom user with only manage_users / - // manage_groups / manage_policies must not be able to modify collection assignments here. - if !headers.membership.has_full_access() { - err!("You don't have permission to modify collection access") - } + // Security: authorization is enforced per collection below via `is_manageable_by_user`, which + // requires a real manage grant on each requested collection (direct users_collections.manage, + // a manage group, membership/group access_all, or Admin/Owner). This matches the single + // collection edit endpoint and the pre-existing behavior of this endpoint. A custom user with + // only manage_users / manage_groups / manage_policies holds no such grant and is therefore + // rejected, while a Manager/Custom member who manages some collections keeps the ability to + // bulk-edit exactly those (a blanket full-access requirement would wrongly deny that here). // Security (audit H-3) and atomicity (audit M-2): validate the whole request against this // organization *before* mutating anything. Every collection must exist in the org and be @@ -3110,11 +3111,16 @@ async fn add_update_group( }))) } +// Reads a single group's details (accessAll, externalId, collection mappings). This is the same +// data the `/groups/details` list endpoint returns, so it uses the same guard: Manage Users OR +// Manage Groups (or Admin/Owner). Requiring Manage Groups here while the list only requires Manage +// Users-or-Groups would let a manage_users member read every group's details in bulk but be denied +// the single-group view of the same data. #[get("/organizations//groups//details")] async fn get_group_details( org_id: OrganizationId, group_id: GroupId, - headers: ManageGroupsHeaders, + headers: ManageUsersOrGroupsHeaders, conn: DbConn, ) -> JsonResult { if org_id != headers.org_id {