Browse Source

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/<org>/groups/<id>/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.
pull/7397/head
tom27052006 3 days ago
parent
commit
8289e2dcff
  1. 42
      src/api/admin.rs
  2. 20
      src/api/core/organizations.rs

42
src/api/admin.rs

@ -547,6 +547,16 @@ struct MembershipTypeData {
fn apply_membership_type_change(membership: &mut Membership, new_type: MembershipType) { fn apply_membership_type_change(membership: &mut Membership, new_type: MembershipType) {
let was_custom = membership.atype == MembershipType::Custom; 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 // 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 // that UI cannot select granular permissions; they can be granted later through the regular
// organization member dialog. // organization member dialog.
@ -956,12 +966,30 @@ mod tests {
apply_membership_type_change(&mut user, MembershipType::Admin); apply_membership_type_change(&mut user, MembershipType::Admin);
assert!(user.access_all); assert!(user.access_all);
let mut custom = membership(MembershipType::Custom); // REGRESSION (privilege escalation, PR #7397 / finding F2): a Custom member whose
custom.access_all = true; // `access_all` is only the Edit-any-collection mirror must NOT be turned into a legacy
custom.edit_any_collection = true; // Manager with the broad "manage all collections" `access_all` grant. That would escalate
apply_membership_type_change(&mut custom, MembershipType::Manager); // an Edit-only member into Create + Edit + Delete. Mirrors the collection down-migration.
assert_eq!(custom.atype, MembershipType::Manager as i32); let mut edit_only = membership(MembershipType::Custom);
assert!(custom.access_all); edit_only.access_all = true;
assert!(!custom.edit_any_collection); 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);
} }
} }

20
src/api/core/organizations.rs

@ -629,12 +629,13 @@ async fn post_bulk_access_collections(
err!("Can't find organization details") err!("Can't find organization details")
} }
// Security: only callers who can actually manage collections (Admins/Owners, or users with // Security: authorization is enforced per collection below via `is_manageable_by_user`, which
// full access) may change collection access in bulk. A custom user with only manage_users / // requires a real manage grant on each requested collection (direct users_collections.manage,
// manage_groups / manage_policies must not be able to modify collection assignments here. // a manage group, membership/group access_all, or Admin/Owner). This matches the single
if !headers.membership.has_full_access() { // collection edit endpoint and the pre-existing behavior of this endpoint. A custom user with
err!("You don't have permission to modify collection access") // 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 // 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 // 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/<org_id>/groups/<group_id>/details")] #[get("/organizations/<org_id>/groups/<group_id>/details")]
async fn get_group_details( async fn get_group_details(
org_id: OrganizationId, org_id: OrganizationId,
group_id: GroupId, group_id: GroupId,
headers: ManageGroupsHeaders, headers: ManageUsersOrGroupsHeaders,
conn: DbConn, conn: DbConn,
) -> JsonResult { ) -> JsonResult {
if org_id != headers.org_id { if org_id != headers.org_id {

Loading…
Cancel
Save