From a992695c0075d0b0c91d5c71bf49d8a9f8b8a9d6 Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Thu, 16 Jul 2026 21:51:29 +0200 Subject: [PATCH] Fix cross-tenant and revoke authorization bypasses from security audit Addresses the confirmed High/Medium findings of the granular-collection- permissions security audit: - H-1: bind direct (non-sync) cipher access to a confirmed membership in the cipher's organization, and harden the user/group collection access-flag queries to require org consistency plus confirmed status. Revoked/invited members can no longer reach ciphers via stale assignment rows. - H-2: validate group ids against the org before mutating group membership in edit_member, and reject cross-org group<->membership links in GroupUser::save and Group::is_in_full_access_group. - H-3: fully pre-validate collections/groups/users in bulk-access and collection create before any mutation, and reject cross-org collection<->group links in CollectionGroup::save. - M-1: require Manage Users for the full member list and Manage Users/Groups for group details (new ManageUsersOrGroupsHeaders guard). - M-2: validate the whole bulk-access request before the destructive delete/replace so an invalid element can't leave partial state behind. - M-3: bounds-validate import collection relationships before writing and propagate cipher-save errors instead of discarding them. --- src/api/core/organizations.rs | 126 +++++++++++++++++++++++++++------- src/auth.rs | 14 ++++ src/db/models/cipher.rs | 59 ++++++++++++++-- src/db/models/group.rs | 35 +++++++++- 4 files changed, 202 insertions(+), 32 deletions(-) diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index bf78e0d8..4b2e7187 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -13,8 +13,8 @@ use crate::{ }, auth::{ AdminHeaders, CollectionDeleteHeaders, CollectionReadHeaders, Headers, ManageGroupsHeaders, - ManagePoliciesHeaders, ManageUsersHeaders, ManagerHeaders, ManagerHeadersLoose, OrgMemberHeaders, OwnerHeaders, - decode_invite, + ManagePoliciesHeaders, ManageUsersHeaders, ManageUsersOrGroupsHeaders, ManagerHeaders, ManagerHeadersLoose, + OrgMemberHeaders, OwnerHeaders, decode_invite, }, db::{ DbConn, @@ -548,6 +548,20 @@ async fn post_organization_collections( let data: FullCollectionData = data.into_inner(); data.validate(&org_id, &conn).await?; + // Security (audit H-3): validate every referenced group and user against this organization + // *before* creating the collection or any assignment, so a foreign-tenant group can't be + // attached to the new collection and no partial state is left behind on rejection. + for group in &data.groups { + if Group::find_by_uuid_and_org(&group.id, &org_id, &conn).await.is_none() { + err!("Group not found in this organization") + } + } + for user in &data.users { + if Membership::find_by_uuid_and_org(&user.id, &org_id, &conn).await.is_none() { + err!("User is not part of organization") + } + } + let collection = Collection::new(org_id.clone(), data.name, data.external_id); collection.save(&conn).await?; @@ -622,8 +636,25 @@ async fn post_bulk_access_collections( err!("You don't have permission to modify collection access") } - for col_id in data.collection_ids { - let Some(collection) = Collection::find_by_uuid_and_org(&col_id, &org_id, &conn).await else { + // 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 + // manageable by the caller, and every referenced group and user must belong to the org. Only + // once the entire request is known-valid do we begin the destructive delete/replace of + // assignments, so a foreign-tenant group can never be linked and a later invalid element can no + // longer leave earlier collections with their assignments already wiped. + for group in &data.groups { + if Group::find_by_uuid_and_org(&group.id, &org_id, &conn).await.is_none() { + err!("Group not found in this organization") + } + } + for user in &data.users { + if Membership::find_by_uuid_and_org(&user.id, &org_id, &conn).await.is_none() { + err!("User is not part of organization") + } + } + let mut collections = Vec::with_capacity(data.collection_ids.len()); + for col_id in &data.collection_ids { + let Some(collection) = Collection::find_by_uuid_and_org(col_id, &org_id, &conn).await else { err!("Collection not found") }; @@ -631,6 +662,12 @@ async fn post_bulk_access_collections( err!("Collection not found", "The current user isn't a manager for this collection") } + collections.push(collection); + } + + for collection in collections { + let col_id = &collection.uuid; + // update collection modification date collection.save(&conn).await?; @@ -645,14 +682,14 @@ async fn post_bulk_access_collections( ) .await; - CollectionGroup::delete_all_by_collection(&col_id, &org_id, &conn).await?; + CollectionGroup::delete_all_by_collection(col_id, &org_id, &conn).await?; for group in &data.groups { CollectionGroup::new(col_id.clone(), group.id.clone(), group.read_only, group.hide_passwords, group.manage) .save(&org_id, &conn) .await?; } - CollectionUser::delete_all_by_collection(&col_id, &conn).await?; + CollectionUser::delete_all_by_collection(col_id, &conn).await?; for user in &data.users { let Some(member) = Membership::find_by_uuid_and_org(&user.id, &org_id, &conn).await else { err!("User is not part of organization") @@ -662,7 +699,7 @@ async fn post_bulk_access_collections( continue; } - CollectionUser::save(&member.user_uuid, &col_id, user.read_only, user.hide_passwords, user.manage, &conn) + CollectionUser::save(&member.user_uuid, col_id, user.read_only, user.hide_passwords, user.manage, &conn) .await?; } } @@ -1024,10 +1061,14 @@ struct GetOrgUserData { async fn get_members( data: GetOrgUserData, org_id: OrganizationId, - headers: ManagerHeadersLoose, + // Security (audit M-1): the full member list exposes each member's PII, 2FA/enrollment status, + // permission flags and (optionally) collection/group assignments. Reading it requires the + // 'Manage Users' permission (or Admin/Owner), matching Bitwarden. Members who only need to + // reference other users (e.g. the collection dialog) use the member-readable mini-details. + headers: ManageUsersHeaders, conn: DbConn, ) -> JsonResult { - if org_id != headers.membership.org_uuid { + if org_id != headers.org_id { err!("Organization not found", "Organization id's do not match"); } let mut users_json = Vec::new(); @@ -1812,6 +1853,16 @@ async fn edit_member( }; if caller_can_manage_groups { + // Security (audit H-2): validate that every requested group belongs to this organization + // *before* mutating any group membership. Otherwise a caller could link the member to a + // group of a foreign tenant (e.g. an access-all group), which the direct cipher-access + // checks would then honor. Fail closed on the whole request if any group is foreign. + for group_id in data.groups.iter().flatten() { + if Group::find_by_uuid_and_org(group_id, &org_id, &conn).await.is_none() { + err!("Group not found in this organization") + } + } + if caller_can_manage_collections { // Caller may grant/revoke collection access via groups: full replace. GroupUser::delete_all_by_member(&member_to_edit.uuid, &conn).await?; @@ -2046,6 +2097,22 @@ async fn post_org_import( // TODO: See if we can optimize the whole cipher adding/importing and prevent duplicate code and checks. Cipher::validate_cipher_data(&data.ciphers)?; + // Robustness/DoS (audit M-3): validate every collection<->cipher relationship index against the + // import payload *before* creating any collection or cipher. `key` indexes into `ciphers` and + // `value` into `collections`; an out-of-range index would otherwise cause an out-of-bounds panic + // when the relations are applied below — a 500 (or a process abort under panic="abort") that + // happens after rows have already been written, leaving partial state behind. + let import_cipher_count = data.ciphers.len(); + let import_collection_count = data.collections.len(); + for relation in &data.collection_relationships { + if relation.key >= import_cipher_count || relation.value >= import_collection_count { + err!( + "Invalid collection relationship", + "A collection relationship references a non-existent cipher or collection" + ) + } + } + let existing_collections: HashSet> = Collection::find_by_organization(&org_id, &conn).await.into_iter().map(|c| Some(c.uuid)).collect(); let mut collections: Vec = Vec::with_capacity(data.collections.len()); @@ -2095,6 +2162,9 @@ async fn post_org_import( // Always clear folder_id's via an organization import cipher_data.folder_id = None; let mut cipher = Cipher::new(cipher_data.r#type, cipher_data.name.clone()); + // Propagate cipher-save failures instead of silently discarding them (audit M-3): a + // discarded error would still push the cipher id and let a relationship reference a cipher + // that was never persisted. This matches Bitwarden's all-or-nothing import semantics. update_cipher_from_data( &mut cipher, cipher_data, @@ -2104,15 +2174,16 @@ async fn post_org_import( &nt, UpdateType::None, ) - .await - .ok(); + .await?; ciphers.push(cipher.uuid); } - // Assign the collections + // Assign the collections. Indices were bounds-validated above, but use `.get()` here as well so + // any future drift fails closed with an error instead of panicking. for (cipher_index, col_index) in relations { - let cipher_id = &ciphers[cipher_index]; - let col_id = &collections[col_index]; + let (Some(cipher_id), Some(col_id)) = (ciphers.get(cipher_index), collections.get(col_index)) else { + err!("Invalid collection relationship", "A collection relationship references a non-existent cipher or collection") + }; CollectionCipher::save(cipher_id, col_id, &conn).await?; } @@ -2696,15 +2767,7 @@ async fn restore_member_impl( Ok(()) } -async fn get_groups_data( - details: bool, - org_id: OrganizationId, - headers: ManagerHeadersLoose, - conn: DbConn, -) -> JsonResult { - if org_id != headers.membership.org_uuid { - err!("Organization not found", "Organization id's do not match"); - } +async fn get_groups_data(details: bool, org_id: OrganizationId, conn: DbConn) -> JsonResult { let groups: Vec = if CONFIG.org_groups_enabled() { let groups = Group::find_by_organization(&org_id, &conn).await; let mut groups_json = Vec::with_capacity(groups.len()); @@ -2732,14 +2795,25 @@ async fn get_groups_data( }))) } +// The plain group list (id, name, externalId) stays member-readable: the web vault needs it to +// render group names, and it exposes no access mappings. #[get("/organizations//groups")] async fn get_groups(org_id: OrganizationId, headers: ManagerHeadersLoose, conn: DbConn) -> JsonResult { - get_groups_data(false, org_id, headers, conn).await + if org_id != headers.membership.org_uuid { + err!("Organization not found", "Organization id's do not match"); + } + get_groups_data(false, org_id, conn).await } +// Security (audit M-1): group *details* expose accessAll, external IDs and collection mappings, so +// reading them requires the 'Manage Users' or 'Manage Groups' permission (or Admin/Owner), matching +// Bitwarden's ReadAll/ReadAllWithAccess authorization. #[get("/organizations//groups/details", rank = 1)] -async fn get_groups_details(org_id: OrganizationId, headers: ManagerHeadersLoose, conn: DbConn) -> JsonResult { - get_groups_data(true, org_id, headers, conn).await +async fn get_groups_details(org_id: OrganizationId, headers: ManageUsersOrGroupsHeaders, conn: DbConn) -> JsonResult { + if org_id != headers.org_id { + err!("Organization not found", "Organization id's do not match"); + } + get_groups_data(true, org_id, conn).await } #[derive(Deserialize)] diff --git a/src/auth.rs b/src/auth.rs index bdaf2b6e..c50b3d4c 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -746,6 +746,15 @@ impl OrgHeaders { fn can_manage_policies(&self) -> bool { self.is_confirmed() && (self.membership_type >= MembershipType::Admin || self.membership.has_manage_policies()) } + // Reading the full member/group *details* (PII, 2FA status, permission flags, access mappings) + // requires the ability to manage users or groups, matching Bitwarden's `ReadAll`/`ReadAllWithAccess` + // authorization. Basic member mini-details and the plain group list remain member-readable. + fn can_manage_users_or_groups(&self) -> bool { + self.is_confirmed() + && (self.membership_type >= MembershipType::Admin + || self.membership.has_manage_users() + || self.membership.has_manage_groups()) + } } // org_id is usually the second path param ("/organizations/"), @@ -923,6 +932,11 @@ generate_manage_headers!( can_manage_policies, "You need the 'Manage Policies' permission, or to be an Admin or Owner, to call this endpoint" ); +generate_manage_headers!( + ManageUsersOrGroupsHeaders, + can_manage_users_or_groups, + "You need the 'Manage Users' or 'Manage Groups' permission, or to be an Admin or Owner, to call this endpoint" +); // col_id is usually the fourth path param ("/organizations//collections/"), // but there could be cases where it is a query value. diff --git a/src/db/models/cipher.rs b/src/db/models/cipher.rs index 8357c9eb..40b27d5a 100644 --- a/src/db/models/cipher.rs +++ b/src/db/models/cipher.rs @@ -604,6 +604,24 @@ impl Cipher { cipher_sync_data: Option<&CipherSyncData>, conn: &DbConn, ) -> Option<(bool, bool, bool)> { + // Security: central fail-closed check binding cipher -> organization -> confirmed membership. + // + // In the direct (non-sync) authorization path an organization cipher is only accessible to a + // user who has a *confirmed* membership in that same organization. This denies access to + // members whose collection/group assignment rows still exist after they were revoked (or are + // still only invited/accepted), and to cross-organization collection/group assignments that + // another code path might have persisted. Without it, the queries below would keep granting + // access from those stale or cross-tenant rows (security audit findings H-1, H-2, H-3). + // + // The sync path (cipher_sync_data is Some) is intentionally left to the caller: it is built + // only from confirmed memberships and evaluated below against that cached data. + if cipher_sync_data.is_none() + && let Some(ref org_uuid) = self.organization_uuid + && Membership::find_confirmed_by_user_and_org(user_uuid, org_uuid, conn).await.is_none() + { + return None; + } + // Check whether this cipher is directly owned by the user, or is in // a collection that the user has full access to. If so, there are no // access restrictions. @@ -669,16 +687,34 @@ impl Cipher { } async fn get_user_collections_access_flags(&self, user_uuid: &UserId, conn: &DbConn) -> Vec<(bool, bool, bool)> { + let cipher_uuid = self.uuid.clone(); + let user_uuid = user_uuid.clone(); conn.run(move |conn| { // Check whether this cipher is in any collections accessible to the // user. If so, retrieve the access flags for each collection. + // + // Security: bind the assignment to a *confirmed* membership in the same organization as + // both the cipher and the collection. Without this, a `users_collections` row left behind + // after a revoke, or an assignment pointing at a collection in a different organization, + // would keep granting access (defense in depth for audit findings H-1 and H-3). ciphers::table - .filter(ciphers::uuid.eq(&self.uuid)) + .filter(ciphers::uuid.eq(cipher_uuid)) .inner_join(ciphers_collections::table.on(ciphers::uuid.eq(ciphers_collections::cipher_uuid))) + .inner_join( + collections::table.on(collections::uuid + .eq(ciphers_collections::collection_uuid) + .and(collections::org_uuid.nullable().eq(ciphers::organization_uuid))), + ) .inner_join( users_collections::table.on(ciphers_collections::collection_uuid .eq(users_collections::collection_uuid) - .and(users_collections::user_uuid.eq(user_uuid))), + .and(users_collections::user_uuid.eq(user_uuid.clone()))), + ) + .inner_join( + users_organizations::table.on(users_organizations::user_uuid + .eq(user_uuid) + .and(users_organizations::org_uuid.eq(collections::org_uuid)) + .and(users_organizations::status.eq(MembershipStatus::Confirmed as i32))), ) .select((users_collections::read_only, users_collections::hide_passwords, users_collections::manage)) .load::<(bool, bool, bool)>(conn) @@ -691,9 +727,16 @@ impl Cipher { if !CONFIG.org_groups_enabled() { return Vec::new(); } + let cipher_uuid = self.uuid.clone(); + let user_uuid = user_uuid.clone(); conn.run(move |conn| { + // Security: bind the group assignment to a *confirmed* membership and require that the + // cipher, the collection, the group and the membership all belong to the same + // organization. The `collections` join in particular prevents a cross-organization + // collection<->group assignment from granting access to a foreign organization's ciphers + // (defense in depth for audit findings H-1, H-2 and H-3). ciphers::table - .filter(ciphers::uuid.eq(&self.uuid)) + .filter(ciphers::uuid.eq(cipher_uuid)) .inner_join(ciphers_collections::table.on(ciphers::uuid.eq(ciphers_collections::cipher_uuid))) .inner_join( collections_groups::table @@ -701,13 +744,21 @@ impl Cipher { ) .inner_join(groups_users::table.on(groups_users::groups_uuid.eq(collections_groups::groups_uuid))) .inner_join( - users_organizations::table.on(users_organizations::uuid.eq(groups_users::users_organizations_uuid)), + users_organizations::table.on(users_organizations::uuid + .eq(groups_users::users_organizations_uuid) + .and(users_organizations::status.eq(MembershipStatus::Confirmed as i32))), ) .inner_join( groups::table.on(groups::uuid .eq(collections_groups::groups_uuid) .and(groups::organizations_uuid.eq(users_organizations::org_uuid))), ) + .inner_join( + collections::table.on(collections::uuid + .eq(ciphers_collections::collection_uuid) + .and(collections::org_uuid.eq(groups::organizations_uuid)) + .and(collections::org_uuid.nullable().eq(ciphers::organization_uuid))), + ) .filter(users_organizations::user_uuid.eq(user_uuid)) .select((collections_groups::read_only, collections_groups::hide_passwords, collections_groups::manage)) .load::<(bool, bool, bool)>(conn) diff --git a/src/db/models/group.rs b/src/db/models/group.rs index 820d3700..2f5348d7 100644 --- a/src/db/models/group.rs +++ b/src/db/models/group.rs @@ -13,7 +13,7 @@ use crate::{ }; use macros::UuidFromParam; -use super::{CollectionId, Membership, MembershipId, OrganizationId, User, UserId}; +use super::{Collection, CollectionId, Membership, MembershipId, MembershipStatus, OrganizationId, User, UserId}; #[derive(Identifiable, Queryable, Insertable, AsChangeset)] #[diesel(table_name = groups)] @@ -257,6 +257,7 @@ impl Group { .and(groups::organizations_uuid.eq(users_organizations::org_uuid))), ) .filter(users_organizations::user_uuid.eq(user_uuid)) + .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) .filter(groups::access_all.eq(true)) .select(groups::organizations_uuid) .distinct() @@ -268,12 +269,19 @@ impl Group { pub async fn is_in_full_access_group(user_uuid: &UserId, org_uuid: &OrganizationId, conn: &DbConn) -> bool { conn.run(move |conn| { + // Security: the membership linked through `groups_users` must itself belong to the same + // organization as the group and must be confirmed. Otherwise a cross-organization + // `groups_users` row (a member of org A linked to an access-all group of org B) would let + // that member pass as having full access to org B (audit finding H-2). groups::table .inner_join(groups_users::table.on(groups_users::groups_uuid.eq(groups::uuid))) .inner_join( - users_organizations::table.on(users_organizations::uuid.eq(groups_users::users_organizations_uuid)), + users_organizations::table.on(users_organizations::uuid + .eq(groups_users::users_organizations_uuid) + .and(users_organizations::org_uuid.eq(groups::organizations_uuid))), ) .filter(users_organizations::user_uuid.eq(user_uuid)) + .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) .filter(groups::organizations_uuid.eq(org_uuid)) .filter(groups::access_all.eq(true)) .select(groups::access_all) @@ -319,6 +327,17 @@ impl Group { impl CollectionGroup { pub async fn save(&mut self, org_uuid: &OrganizationId, conn: &DbConn) -> EmptyResult { + // Security (audit H-3): never persist a cross-organization link between a collection and a + // group. Both must belong to the organization this assignment is scoped to; otherwise a + // caller could attach a foreign-tenant group to this organization's collection and thereby + // grant that group's members access to it. This is a defense-in-depth guard so no route can + // create such a link even if it fails to validate its inputs. + if Collection::find_by_uuid_and_org(&self.collections_uuid, org_uuid, conn).await.is_none() + || Group::find_by_uuid_and_org(&self.groups_uuid, org_uuid, conn).await.is_none() + { + err!("Collection and group must belong to the same organization") + } + let group_users = GroupUser::find_by_group(&self.groups_uuid, org_uuid, conn).await; for group_user in group_users { group_user.update_user_revision(conn).await; @@ -493,6 +512,18 @@ impl CollectionGroup { impl GroupUser { pub async fn save(&mut self, conn: &DbConn) -> EmptyResult { + // Security (audit H-2): never persist a cross-organization link between a group and a + // membership. The group must belong to the same organization as the membership; otherwise a + // caller could grant a member of one organization full access to another organization's + // collections through an access-all group. This is a defense-in-depth guard so no route can + // create such a link even if it fails to validate its inputs. + let Some(member) = Membership::find_by_uuid(&self.users_organizations_uuid, conn).await else { + err!("Member not found while assigning to group") + }; + if Group::find_by_uuid_and_org(&self.groups_uuid, &member.org_uuid, conn).await.is_none() { + err!("Group and member must belong to the same organization") + } + self.update_user_revision(conn).await; db_run! { conn: