diff --git a/.env.template b/.env.template index 375a99e5..dbae390a 100644 --- a/.env.template +++ b/.env.template @@ -552,9 +552,13 @@ ## Trusted device encryption ("passwordless SSO"), see https://bitwarden.com/help/login-with-sso-trusted-devices/ ## After an SSO login the client may keep a copy of the user key on the device, wrapped for a key ## pair that the device generated, so the vault unlocks without a master password. A second device -## is unlocked either by approving it from an already trusted one or with the master password. -## WARNING: a user who never sets a master password and then loses every trusted device cannot -## recover their vault. This server does not implement the admin approval flow to fall back on. +## is unlocked by approving it from an already trusted one, by asking an administrator of the +## organization (which needs the member enrolled into account recovery), or with the master password. +## WARNING: a user who never sets a master password and then loses every trusted device depends on +## an administrator to get back in, and cannot recover their vault at all without one. +## To turn this off again, clear this setting but leave `SSO_ENABLED` on: users without a master +## password keep receiving their keys while they still have a trusted device, so their client can +## walk them through setting one. Turning off `SSO_ENABLED` instead leaves them no way to log in. # SSO_TRUSTED_DEVICE_ENCRYPTION=false ######################## diff --git a/migrations/mysql/2026-08-01-120000_add_auth_request_indexes/down.sql b/migrations/mysql/2026-08-01-120000_add_auth_request_indexes/down.sql new file mode 100644 index 00000000..38258616 --- /dev/null +++ b/migrations/mysql/2026-08-01-120000_add_auth_request_indexes/down.sql @@ -0,0 +1,2 @@ +DROP INDEX auth_requests_organization_type ON auth_requests; +DROP INDEX auth_requests_creation_date ON auth_requests; diff --git a/migrations/mysql/2026-08-01-120000_add_auth_request_indexes/up.sql b/migrations/mysql/2026-08-01-120000_add_auth_request_indexes/up.sql new file mode 100644 index 00000000..a757bdef --- /dev/null +++ b/migrations/mysql/2026-08-01-120000_add_auth_request_indexes/up.sql @@ -0,0 +1,2 @@ +CREATE INDEX auth_requests_organization_type ON auth_requests (organization_uuid, atype, approved); +CREATE INDEX auth_requests_creation_date ON auth_requests (creation_date); diff --git a/migrations/postgresql/2026-08-01-120000_add_auth_request_indexes/down.sql b/migrations/postgresql/2026-08-01-120000_add_auth_request_indexes/down.sql new file mode 100644 index 00000000..9f880bae --- /dev/null +++ b/migrations/postgresql/2026-08-01-120000_add_auth_request_indexes/down.sql @@ -0,0 +1,2 @@ +DROP INDEX auth_requests_organization_type; +DROP INDEX auth_requests_creation_date; diff --git a/migrations/postgresql/2026-08-01-120000_add_auth_request_indexes/up.sql b/migrations/postgresql/2026-08-01-120000_add_auth_request_indexes/up.sql new file mode 100644 index 00000000..a757bdef --- /dev/null +++ b/migrations/postgresql/2026-08-01-120000_add_auth_request_indexes/up.sql @@ -0,0 +1,2 @@ +CREATE INDEX auth_requests_organization_type ON auth_requests (organization_uuid, atype, approved); +CREATE INDEX auth_requests_creation_date ON auth_requests (creation_date); diff --git a/migrations/sqlite/2026-08-01-120000_add_auth_request_indexes/down.sql b/migrations/sqlite/2026-08-01-120000_add_auth_request_indexes/down.sql new file mode 100644 index 00000000..9f880bae --- /dev/null +++ b/migrations/sqlite/2026-08-01-120000_add_auth_request_indexes/down.sql @@ -0,0 +1,2 @@ +DROP INDEX auth_requests_organization_type; +DROP INDEX auth_requests_creation_date; diff --git a/migrations/sqlite/2026-08-01-120000_add_auth_request_indexes/up.sql b/migrations/sqlite/2026-08-01-120000_add_auth_request_indexes/up.sql new file mode 100644 index 00000000..a757bdef --- /dev/null +++ b/migrations/sqlite/2026-08-01-120000_add_auth_request_indexes/up.sql @@ -0,0 +1,2 @@ +CREATE INDEX auth_requests_organization_type ON auth_requests (organization_uuid, atype, approved); +CREATE INDEX auth_requests_creation_date ON auth_requests (creation_date); diff --git a/src/api/core/accounts.rs b/src/api/core/accounts.rs index a831f0df..22282d28 100644 --- a/src/api/core/accounts.rs +++ b/src/api/core/accounts.rs @@ -22,8 +22,8 @@ use crate::{ models::{ AuthRequest, AuthRequestId, AuthRequestType, Cipher, CipherId, Device, DeviceId, DeviceType, DeviceWithAuthRequest, EmergencyAccess, EmergencyAccessId, EventType, Folder, FolderId, Invitation, - Membership, MembershipId, MembershipType, OrgPolicy, OrgPolicyType, Organization, OrganizationId, Send, - SendId, User, UserId, UserKdfType, + Membership, MembershipId, MembershipStatus, MembershipType, OrgPolicy, OrgPolicyType, Organization, + OrganizationId, Send, SendId, User, UserId, UserKdfType, }, }, mail, @@ -1022,6 +1022,14 @@ async fn post_rotatekey(data: Json, headers: Headers, conn: DbConn, nt: } } + // Every device holds the previous user key wrapped for itself, which unlocks nothing anymore. + // Drop those copies before the new key is written, never after: the other order leaves a window + // in which a device still counts as trusted and hands its owner a key that no longer opens the + // vault. This way a failure here means the rotation simply did not happen. + // The clients re-wrap the new user key for every device right after this via + // `POST /devices/update-trust`; whatever they leave out stays untrusted. + Device::invalidate_wrapped_user_keys(&headers.user.uuid, &conn).await?; + // Update user data let mut user = headers.user; @@ -1037,12 +1045,6 @@ async fn post_rotatekey(data: Json, headers: Headers, conn: DbConn, nt: let save_result = user.save(&conn).await; - // Every trusted device holds the previous user key wrapped for itself, which unlocks nothing - // anymore. The client of the rotating device re-wraps the new one right after this via - // `POST /devices/update-trust`, so that device keeps its private key; the rest is dropped. A - // client that skips that call simply ends up with no trusted device instead of a broken unlock. - Device::invalidate_wrapped_user_keys(&user.uuid, &headers.device.uuid, &conn).await?; - // Prevent logging out the client where the user requested this endpoint from. // If you do logout the user it will causes issues at the client side. // Adding the device uuid will prevent this. @@ -1608,6 +1610,20 @@ struct TrustedDeviceKeysData { encrypted_private_key: String, } +/// Refuses anything that does not even have the shape of an `EncString`. +/// +/// The server cannot tell whether a blob decrypts, but storing something that certainly does not +/// only leaves a device that calls itself trusted and fails its owner at the next unlock. Upstream +/// puts `[EncryptedString]` on the same fields. +fn validate_enc_strings(values: &[(&str, &str)]) -> EmptyResult { + for (name, value) in values { + if !crate::util::is_valid_enc_string(value) { + err!(format!("{name} is not a valid encrypted string")) + } + } + Ok(()) +} + /// Marks a device of the current user as trusted. /// /// Upstream keys this on the device identifier and does not require it to be the device the request @@ -1622,12 +1638,11 @@ async fn put_device_keys( ) -> JsonResult { let data = data.into_inner(); - if data.encrypted_user_key.is_empty() - || data.encrypted_public_key.is_empty() - || data.encrypted_private_key.is_empty() - { - err!("All three device keys are required to trust a device") - } + validate_enc_strings(&[ + ("encryptedUserKey", &data.encrypted_user_key), + ("encryptedPublicKey", &data.encrypted_public_key), + ("encryptedPrivateKey", &data.encrypted_private_key), + ])?; let Some(mut device) = Device::find_by_uuid_and_user(&device_id, &headers.user.uuid, &conn).await else { err!("No device found") @@ -1698,18 +1713,20 @@ async fn post_devices_update_trust(data: Json, headers: data.secret.validate(&headers.user, true, &conn).await?; - if data.current_device.encrypted_user_key.is_empty() || data.current_device.encrypted_public_key.is_empty() { - err!("The keys of the current device are required") - } + validate_enc_strings(&[ + ("encryptedUserKey", &data.current_device.encrypted_user_key), + ("encryptedPublicKey", &data.current_device.encrypted_public_key), + ])?; let mut updates: HashMap = HashMap::new(); for other in data.other_devices { if other.device_id == headers.device.uuid { err!("The current device cannot also be part of the optional rotation") } - if other.keys.encrypted_user_key.is_empty() || other.keys.encrypted_public_key.is_empty() { - err!("Both keys are required for every device in the rotation") - } + validate_enc_strings(&[ + ("encryptedUserKey", &other.keys.encrypted_user_key), + ("encryptedPublicKey", &other.keys.encrypted_public_key), + ])?; if updates.insert(other.device_id, other.keys).is_some() { err!("A device was listed more than once in the rotation") } @@ -1730,15 +1747,20 @@ async fn post_devices_update_trust(data: Json, headers: if device.uuid == headers.device.uuid { device.encrypted_user_key = Some(data.current_device.encrypted_user_key.clone()); device.encrypted_public_key = Some(data.current_device.encrypted_public_key.clone()); - } else if !device.is_trusted() { - // Nothing to rotate, and handing an untrusted device two of the three keys would not - // make it trusted anyway. - continue; } else if let Some(keys) = updates.remove(&device.uuid) { + // A rotation clears the wrapped user key of every device, so the listed ones are not + // trusted at this point; their key pair is what they are restored from. Without it + // there is nothing the two keys could belong to. + if !device.holds_private_key() { + continue; + } device.encrypted_user_key = Some(keys.encrypted_user_key); device.encrypted_public_key = Some(keys.encrypted_public_key); - } else { + } else if device.holds_any_key() { + // Not listed, so whatever it still holds wraps the previous user key. device.untrust(); + } else { + continue; } device.save(true, &conn).await?; @@ -1892,6 +1914,10 @@ async fn post_auth_request( /// https://github.com/bitwarden/server/blob/main/src/Api/Auth/Controllers/AuthRequestsController.cs #[post("/auth-requests/admin-request", data = "")] async fn post_admin_auth_request(data: Json, headers: Headers, conn: DbConn) -> JsonResult { + // Every call mails all administrators of every organization involved, so it is worth a limit of + // its own even though the caller is authenticated. + crate::ratelimit::check_limit_unauthenticated(&headers.ip.ip)?; + let data = data.into_inner(); if AuthRequestType::from_i32(data.atype) != Some(AuthRequestType::AdminApproval) { @@ -1902,9 +1928,16 @@ async fn post_admin_auth_request(data: Json, headers: Header err!("AuthRequest doesn't exist", "Device verification failed") } - let memberships = Membership::find_by_user(&headers.user.uuid, &conn).await; + // Only an organization the user really belongs to can answer for them. A pending invitation is + // not a membership yet, and a revoked one is not one anymore; sending either of them the email + // address, the address and the device of the asker is more than they are owed. + let memberships: Vec = Membership::find_by_user(&headers.user.uuid, &conn) + .await + .into_iter() + .filter(|membership| membership.status == MembershipStatus::Confirmed as i32) + .collect(); if memberships.is_empty() { - err!("User does not belong to any organization") + err!("User does not belong to any organization that could approve a device") } log_user_event( @@ -1918,19 +1951,42 @@ async fn post_admin_auth_request(data: Json, headers: Header let mut first_request = None; for membership in memberships { - let mut auth_request = AuthRequest::new( - headers.user.uuid.clone(), - Some(membership.org_uuid.clone()), - AuthRequestType::AdminApproval, - data.device_identifier.clone(), - headers.device.atype, - headers.ip.ip.to_string(), - data.access_code.clone(), - data.public_key.clone(), - ); + // Asking again from the same device replaces the open request instead of adding one, so a + // client that retries does not pile up rows and does not mail the administrators twice. + let existing = AuthRequest::find_pending_admin_approval( + &headers.user.uuid, + &data.device_identifier, + &membership.org_uuid, + &conn, + ) + .await; + let is_new = existing.is_none(); + + let mut auth_request = match existing { + Some(mut auth_request) => { + auth_request.access_code.clone_from(&data.access_code); + auth_request.public_key.clone_from(&data.public_key); + auth_request.device_type = headers.device.atype; + auth_request.request_ip = headers.ip.ip.to_string(); + auth_request.creation_date = Utc::now().naive_utc(); + auth_request + } + None => AuthRequest::new( + headers.user.uuid.clone(), + Some(membership.org_uuid.clone()), + AuthRequestType::AdminApproval, + data.device_identifier.clone(), + headers.device.atype, + headers.ip.ip.to_string(), + data.access_code.clone(), + data.public_key.clone(), + ), + }; auth_request.save(&conn).await?; - notify_device_approval_requested(&headers.user, &membership.org_uuid, &conn).await; + if is_new { + notify_device_approval_requested(&headers.user, &membership.org_uuid, &conn).await; + } if first_request.is_none() { first_request = Some(auth_request); @@ -1976,6 +2032,12 @@ async fn get_auth_request(auth_request_id: AuthRequestId, headers: Headers, conn err!("AuthRequest doesn't exist", "Record not found or user uuid does not match") }; + // The anonymous lookup refuses an expired request, and so does this one: the window an approval + // stays usable in should not depend on which of the two the client happens to poll. + if auth_request.is_expired() { + err!("AuthRequest doesn't exist", "Request has expired") + } + Ok(Json(auth_request_json(&auth_request))) } @@ -2022,6 +2084,21 @@ async fn put_auth_request( err!("AuthRequest doesn't exist", "Request has expired") } + // Only the newest request of a device may be approved. Anyone can create a request for a known + // device, so without this an older one could still be sitting there when the user approves what + // their screen shows, and the answer would go to whoever left it. Same check as upstream. + if data.request_approved + && AuthRequest::find_by_user_and_requested_device( + &headers.user.uuid, + &auth_request.request_device_identifier, + &conn, + ) + .await + .is_none_or(|newest| newest.uuid != auth_request.uuid) + { + err!("This request is no longer valid. Make sure to approve the most recent request.") + } + let response_date = Utc::now().naive_utc(); if data.request_approved { diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index 197480f3..b6a207c6 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -17,8 +17,8 @@ use crate::{ DbConn, models::{ AuthRequest, AuthRequestId, Cipher, CipherId, Collection, CollectionCipher, CollectionGroup, CollectionId, - CollectionUser, DeviceType, EventType, Group, GroupId, GroupUser, Invitation, Membership, MembershipId, - MembershipStatus, MembershipType, OrgPolicy, OrgPolicyType, Organization, OrganizationApiKey, + CollectionUser, Device, DeviceType, EventType, Group, GroupId, GroupUser, Invitation, Membership, + MembershipId, MembershipStatus, MembershipType, OrgPolicy, OrgPolicyType, Organization, OrganizationApiKey, OrganizationId, User, UserId, }, }, @@ -3181,7 +3181,13 @@ async fn put_reset_password_enrollment( // Enrolling is where a member who was invited into a trusted device organization turns into a // real one; upstream accepts the invitation at this point as well. Without it they would stay // invited forever and no admin could ever confirm them. - if enrolled && membership.status == MembershipStatus::Invited as i32 { + // + // Tied to the same condition as the exception above, so that turning the feature off leaves the + // invitation flow exactly as it was: an invitation is otherwise accepted only against the token + // that was mailed out, and that is the only thing proving the address belongs to the account. + if enrolled && trusted_device_enrollment && membership.status == MembershipStatus::Invited as i32 { + // Do not leave the open invitation behind, it would keep the address signup-eligible. + Invitation::take(&headers.user.email, &conn).await; accept_org_invite(&headers.user, membership, reset_password_key, &conn).await?; } else { membership.reset_password_key = reset_password_key; @@ -3219,14 +3225,18 @@ async fn get_organization_auth_requests(org_id: OrganizationId, headers: AdminHe continue; } - // A request whose asker is no longer a member of this organization is none of its business - // anymore, so it is quietly left out instead of being offered for approval. + // A request whose asker is not a confirmed member of this organization is none of its + // business, so it is quietly left out instead of being offered for approval. Same condition + // as when answering, so nothing is shown here that would be refused there. let (Some(member), Some(user)) = ( Membership::find_by_user_and_org(&auth_request.user_uuid, &org_id, &conn).await, User::find_by_uuid(&auth_request.user_uuid, &conn).await, ) else { continue; }; + if member.status != MembershipStatus::Confirmed as i32 { + continue; + } requests.push(auth_request.to_json_for_organization(&user.email, &member.uuid)); } @@ -3259,6 +3269,22 @@ struct OrganizationAuthRequestUpdateData { key: Option, } +/// How many requests one call may answer. A screen full of pending approvals is a handful. +const MAX_BULK_AUTH_REQUESTS: usize = 500; + +/// Whether one entry that cannot be answered takes the whole call down with it. +/// +/// A single request is addressed by its id, so a caller that names a request nobody can answer +/// deserves to hear about it. A batch is a list of what an administrator saw a moment ago, where an +/// entry may well have expired or been answered by a colleague since; upstream processes those as +/// far as it can and passes over the rest. Failing the batch instead would report an error while +/// having already answered everything before the bad entry. +#[derive(Clone, Copy, PartialEq, Eq)] +enum OnUnanswerable { + Fail, + Skip, +} + #[post("/organizations//auth-requests/", data = "", rank = 2)] async fn update_organization_auth_request( org_id: OrganizationId, @@ -3275,6 +3301,7 @@ async fn update_organization_auth_request( &request_id, data.request_approved, data.encrypted_user_key, + OnUnanswerable::Fail, &headers, &conn, &ant, @@ -3292,8 +3319,24 @@ async fn deny_organization_auth_requests( ant: AnonymousNotify<'_>, nt: Notify<'_>, ) -> EmptyResult { - for request_id in data.into_inner().ids { - answer_organization_auth_request(&org_id, &request_id, false, None, &headers, &conn, &ant, &nt).await?; + let ids = data.into_inner().ids; + if ids.len() > MAX_BULK_AUTH_REQUESTS { + err!(format!("At most {MAX_BULK_AUTH_REQUESTS} requests can be answered at once")) + } + + for request_id in ids { + answer_organization_auth_request( + &org_id, + &request_id, + false, + None, + OnUnanswerable::Skip, + &headers, + &conn, + &ant, + &nt, + ) + .await?; } Ok(()) @@ -3308,9 +3351,24 @@ async fn update_many_organization_auth_requests( ant: AnonymousNotify<'_>, nt: Notify<'_>, ) -> EmptyResult { - for update in data.into_inner() { - answer_organization_auth_request(&org_id, &update.id, update.approved, update.key, &headers, &conn, &ant, &nt) - .await?; + let updates = data.into_inner(); + if updates.len() > MAX_BULK_AUTH_REQUESTS { + err!(format!("At most {MAX_BULK_AUTH_REQUESTS} requests can be answered at once")) + } + + for update in updates { + answer_organization_auth_request( + &org_id, + &update.id, + update.approved, + update.key, + OnUnanswerable::Skip, + &headers, + &conn, + &ant, + &nt, + ) + .await?; } Ok(()) @@ -3322,6 +3380,7 @@ async fn answer_organization_auth_request( request_id: &AuthRequestId, approved: bool, encrypted_user_key: Option, + on_unanswerable: OnUnanswerable, headers: &AdminHeaders, conn: &DbConn, ant: &AnonymousNotify<'_>, @@ -3331,30 +3390,47 @@ async fn answer_organization_auth_request( err!("Organization not found", "Organization id's do not match"); } + // Everything below this point is a request that this administrator cannot answer, whether it + // never existed, was already dealt with, or ran out. In a batch that is expected and skipped. + macro_rules! unanswerable { + ($($err:tt)*) => {{ + if on_unanswerable == OnUnanswerable::Skip { + return Ok(()); + } + err!($($err)*) + }}; + } + // Only ever reachable through the organization it was addressed to, so an administrator cannot // answer for an organization they have no say in. let Some(mut auth_request) = AuthRequest::find_admin_approval_by_org_and_uuid(request_id, org_id, conn).await else { - err!("AuthRequest doesn't exist", "Record not found or not addressed to this organization") + unanswerable!("AuthRequest doesn't exist", "Record not found or not addressed to this organization") }; if auth_request.approved.is_some() { - err!("This request has already been answered") + unanswerable!("This request has already been answered"); } if auth_request.is_expired() { - err!("AuthRequest doesn't exist", "Request has expired") + unanswerable!("AuthRequest doesn't exist", "Request has expired"); } - let Some(member) = Membership::find_by_user_and_org(&auth_request.user_uuid, org_id, conn).await else { - err!("AuthRequest doesn't exist", "The requesting user is no longer a member of this organization") + // Answering means acting for a member of this organization, so it has to be one: an invitation + // that was never accepted is not a membership yet, and a revoked one is not one anymore. + let member = match Membership::find_by_user_and_org(&auth_request.user_uuid, org_id, conn).await { + Some(member) if member.status == MembershipStatus::Confirmed as i32 => member, + _ => unanswerable!("AuthRequest doesn't exist", "The requesting user is not a member of this organization"), }; if approved { // Without the wrapped user key the answer is worthless: it is the whole point of approving. let Some(key) = encrypted_user_key.filter(|key| !key.is_empty()) else { - err!("An approved request needs the encrypted user key") + unanswerable!("An approved request needs the encrypted user key") }; + if !crate::util::is_valid_enc_string(&key) { + unanswerable!("encryptedUserKey is not a valid encrypted string"); + } auth_request.enc_key = Some(key); } @@ -3376,7 +3452,15 @@ async fn answer_organization_auth_request( } ant.send_auth_response(&auth_request.user_uuid, &auth_request.uuid).await; - nt.send_auth_response(&auth_request.user_uuid, &auth_request.uuid, &headers.device, conn).await; + + // The device that asked, not the one the administrator happens to be answering from: that one + // belongs to somebody else, and naming it here would both address the notification at a device + // of the wrong account and hand its identifiers to the push relay under a foreign user id. + if let Some(device) = + Device::find_by_uuid_and_user(&auth_request.request_device_identifier, &auth_request.user_uuid, conn).await + { + nt.send_auth_response(&auth_request.user_uuid, &auth_request.uuid, &device, conn).await; + } if CONFIG.mail_enabled() && let Some(user) = User::find_by_uuid(&auth_request.user_uuid, conn).await diff --git a/src/api/identity.rs b/src/api/identity.rs index a29945a4..aad29d1a 100644 --- a/src/api/identity.rs +++ b/src/api/identity.rs @@ -526,14 +526,19 @@ async fn trusted_device_option(user: &User, device: &Device, conn: &DbConn) -> O .any(|other| other.uuid != device.uuid && DeviceType::from_i32(other.atype).can_approve_login_requests()); // An admin can only take over the approval once the member handed them a key to work with, - // which is what enrolling into account recovery does. - let has_admin_approval = - memberships.iter().any(|member| member.reset_password_key.as_ref().is_some_and(|key| !key.is_empty())); + // which is what enrolling into account recovery does. Only a confirmed membership counts, the + // same condition the request itself is created and answered under, so this does not announce a + // way out that would be refused the moment it is taken. + let has_admin_approval = memberships.iter().any(|member| { + member.status == MembershipStatus::Confirmed as i32 + && member.reset_password_key.as_ref().is_some_and(|key| !key.is_empty()) + }); // Whether the user is on the answering side of that. The clients use it to push someone who - // could approve others, but has no master password themselves, into setting one. + // could approve others, but has no master password themselves, into setting one. Matches what + // `AdminHeaders` actually lets through. let has_manage_reset_password_permission = memberships.iter().any(|member| { - member.status != MembershipStatus::Revoked as i32 && member.atype <= MembershipType::Admin as i32 + member.status == MembershipStatus::Confirmed as i32 && member.atype <= MembershipType::Admin as i32 }); Some(json!({ diff --git a/src/config.rs b/src/config.rs index aee67802..f581697f 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1110,7 +1110,11 @@ fn validate_config(cfg: &ConfigItems, on_update: bool) -> Result<(), Error> { validate_internal_sso_redirect_url(&cfg.sso_callback_path)?; validate_sso_master_password_policy(cfg.sso_master_password_policy.as_ref())?; } else if cfg.sso_trusted_device_encryption { - err!("`SSO_TRUSTED_DEVICE_ENCRYPTION` requires `SSO_ENABLED` to be set, it only applies to SSO logins") + err!( + "`SSO_TRUSTED_DEVICE_ENCRYPTION` requires `SSO_ENABLED` to be set, it only applies to SSO logins. \ + To stop offering trusted devices, clear `SSO_TRUSTED_DEVICE_ENCRYPTION` and leave `SSO_ENABLED` on \ + until every user without a master password has set one, otherwise they can no longer log in at all" + ) } if cfg._enable_yubico { diff --git a/src/db/models/auth_request.rs b/src/db/models/auth_request.rs index 4a1a1124..0472f62f 100644 --- a/src/db/models/auth_request.rs +++ b/src/db/models/auth_request.rs @@ -237,6 +237,30 @@ impl AuthRequest { .await } + /// The open request a device already has waiting at this organization, if any. + /// + /// Asking again from the same device updates that one instead of adding another, so a client + /// that retries cannot fill the table or mail the administrators over and over. + pub async fn find_pending_admin_approval( + user_uuid: &UserId, + device_uuid: &DeviceId, + org_uuid: &OrganizationId, + conn: &DbConn, + ) -> Option { + conn.run(move |conn| { + auth_requests::table + .filter(auth_requests::user_uuid.eq(user_uuid)) + .filter(auth_requests::request_device_identifier.eq(device_uuid)) + .filter(auth_requests::organization_uuid.eq(org_uuid)) + .filter(auth_requests::atype.eq(AuthRequestType::AdminApproval as i32)) + .filter(auth_requests::approved.is_null()) + .order_by(auth_requests::creation_date.desc()) + .first::(conn) + .ok() + }) + .await + } + /// Everything an administrator of this organization still has to answer. pub async fn find_pending_admin_approval_by_org(org_uuid: &OrganizationId, conn: &DbConn) -> Vec { conn.run(move |conn| { @@ -269,16 +293,6 @@ impl AuthRequest { .await } - pub async fn find_created_before(dt: &NaiveDateTime, conn: &DbConn) -> Vec { - conn.run(move |conn| { - auth_requests::table - .filter(auth_requests::creation_date.lt(dt)) - .load::(conn) - .expect("Error loading auth_requests") - }) - .await - } - pub async fn delete(&self, conn: &DbConn) -> EmptyResult { conn.run(move |conn| { diesel::delete(auth_requests::table.filter(auth_requests::uuid.eq(&self.uuid))) @@ -292,16 +306,56 @@ impl AuthRequest { ct_eq(&self.access_code, access_code) } + /// Drops everything past its window, which is a different one per type. + /// + /// https://github.com/bitwarden/server/blob/f8ee2270409f7a13125cd414c450740af605a175/src/Sql/dbo/Auth/Stored%20Procedures/AuthRequest_DeleteIfExpired.sql + /// One statement per case rather than reading the table and deleting row by row, so the work + /// stays in the database however many requests have piled up. pub async fn purge_expired_auth_requests(conn: &DbConn) { - // https://github.com/bitwarden/server/blob/f8ee2270409f7a13125cd414c450740af605a175/src/Sql/dbo/Auth/Stored%20Procedures/AuthRequest_DeleteIfExpired.sql - // Nothing can be expired before the shortest window has passed, so that is the cheapest - // way to narrow the table down; which of them really are is decided per type afterwards, - // because a request waiting for an administrator lives a week rather than 15 minutes. - let candidates = Utc::now().naive_utc() - Self::user_request_expiration(); - for auth_request in Self::find_created_before(&candidates, conn).await { - if auth_request.is_expired() { - auth_request.delete(conn).await.ok(); - } + let now = Utc::now().naive_utc(); + let admin = AuthRequestType::AdminApproval as i32; + + let between_devices = now - Self::user_request_expiration(); + let for_an_admin = now - Self::admin_request_expiration(); + let after_approval = now - Self::after_admin_approval_expiration(); + + let result = conn + .run(move |conn| -> EmptyResult { + // Between the user's own devices: 15 minutes from the moment it was asked. + let _: () = diesel::delete( + auth_requests::table + .filter(auth_requests::atype.ne(admin)) + .filter(auth_requests::creation_date.lt(between_devices)), + ) + .execute(conn) + .map_res("Error purging the expired auth requests")?; + + // Approved by an administrator: half a day from the answer, so the user has time to + // come back and use it. + let _: () = diesel::delete( + auth_requests::table + .filter(auth_requests::atype.eq(admin)) + .filter(auth_requests::approved.eq(true)) + .filter(auth_requests::response_date.lt(after_approval)), + ) + .execute(conn) + .map_res("Error purging the approved auth requests")?; + + // Waiting for an administrator, or refused by one: a week from the moment it was + // asked either way, a refusal does not extend anything. + diesel::delete( + auth_requests::table + .filter(auth_requests::atype.eq(admin)) + .filter(auth_requests::approved.is_null().or(auth_requests::approved.eq(false))) + .filter(auth_requests::creation_date.lt(for_an_admin)), + ) + .execute(conn) + .map_res("Error purging the unanswered auth requests") + }) + .await; + + if let Err(e) = result { + error!("Error purging the expired auth requests: {e:#?}"); } } } diff --git a/src/db/models/device.rs b/src/db/models/device.rs index 3d7391ec..c0192c8a 100644 --- a/src/db/models/device.rs +++ b/src/db/models/device.rs @@ -107,6 +107,22 @@ impl Device { self.is_trusted().then_some(self.encrypted_private_key.as_ref()).flatten() } + /// Whether the device still holds the private key of its own key pair. + /// + /// That key is wrapped with the device key, which a rotation of the user key does not touch, so + /// it outlives one. It is what decides whether a device can be handed a freshly wrapped user + /// key and be trusted again, or whether it has to be set up from scratch. + pub fn holds_private_key(&self) -> bool { + Self::present(self.encrypted_private_key.as_ref()).is_some() + } + + /// Whether any part of a trust is stored, complete or not. + pub fn holds_any_key(&self) -> bool { + Self::present(self.encrypted_user_key.as_ref()).is_some() + || Self::present(self.encrypted_public_key.as_ref()).is_some() + || self.holds_private_key() + } + pub fn untrust(&mut self) { self.encrypted_user_key = None; self.encrypted_public_key = None; @@ -249,23 +265,15 @@ impl Device { /// Invalidates every copy of the user key that is wrapped for one of the user's devices. /// /// Called when the user key itself is replaced, which leaves all of those copies pointing at a - /// key that no longer unlocks anything. `keep_private_key_for` (the device that performed the - /// rotation) keeps its own private key, so its client can immediately re-wrap the new user key - /// via `POST /devices/update-trust`; every other device is untrusted outright. Until that - /// happens no device counts as trusted, so the worst case is an extra login, not a broken vault. - pub async fn invalidate_wrapped_user_keys( - user_uuid: &UserId, - keep_private_key_for: &DeviceId, - conn: &DbConn, - ) -> EmptyResult { + /// key that no longer unlocks anything. No device counts as trusted afterwards, so a client + /// that stops here ends up with an extra login rather than a broken unlock. The device key + /// pairs are deliberately left alone: they are wrapped with the device key, which a rotation + /// does not touch, so `POST /devices/update-trust` can hand every device the new user key and + /// restore its trust. Whatever it does not list is dropped there. + /// + /// One statement, so there is no half applied state to reason about. + pub async fn invalidate_wrapped_user_keys(user_uuid: &UserId, conn: &DbConn) -> EmptyResult { conn.run(move |conn| { - let _: () = diesel::update( - devices::table.filter(devices::user_uuid.eq(user_uuid)).filter(devices::uuid.ne(keep_private_key_for)), - ) - .set(devices::encrypted_private_key.eq::>(None)) - .execute(conn) - .map_res("Error untrusting the devices")?; - diesel::update(devices::table.filter(devices::user_uuid.eq(user_uuid))) .set(( devices::encrypted_user_key.eq::>(None), @@ -532,12 +540,37 @@ mod tests { assert_eq!(device.trusted_private_key(), None); } + #[test] + fn a_rotation_leaves_the_device_key_pair_in_place() { + // What `invalidate_wrapped_user_keys` does: the wrapped user key and the public key go, + // the private key stays, because the device key that wraps it is untouched by a rotation. + let mut device = trusted_device(); + device.encrypted_user_key = None; + device.encrypted_public_key = None; + + assert!(!device.is_trusted(), "nothing may unlock until the client re-wraps"); + assert!(device.holds_private_key(), "but the device can still be handed a new user key"); + assert!(device.holds_any_key()); + } + + #[test] + fn a_device_that_never_had_a_trust_holds_nothing() { + let device = Device::new(String::from("device").into(), String::from("user").into(), String::new(), 9); + assert!(!device.holds_private_key()); + assert!(!device.holds_any_key()); + + let mut device = trusted_device(); + device.encrypted_private_key = Some(String::new()); + assert!(!device.holds_private_key(), "an empty key is as good as a missing one"); + } + #[test] fn untrusting_clears_every_key() { let mut device = trusted_device(); device.untrust(); assert!(!device.is_trusted()); + assert!(!device.holds_any_key()); assert_eq!(device.encrypted_user_key, None); assert_eq!(device.encrypted_public_key, None); assert_eq!(device.encrypted_private_key, None); diff --git a/src/mail.rs b/src/mail.rs index fe5f217a..ac4336e4 100644 --- a/src/mail.rs +++ b/src/mail.rs @@ -543,7 +543,9 @@ pub async fn send_device_approval_requested( let (subject, body_html, body_text) = get_text( "email/device_approval_requested", json!({ - "url": CONFIG.domain(), + // The page that can actually answer these, which is in the admin panel rather than in + // the web vault: the one upstream uses is not part of any open source build. + "url": format!("{}/admin/device-approvals", CONFIG.domain()), "img_src": CONFIG._smtp_img_src(), "org_name": org_name, "user_email": user_email, diff --git a/src/static/templates/email/device_approval_requested.hbs b/src/static/templates/email/device_approval_requested.hbs index e436c01c..7c786775 100644 --- a/src/static/templates/email/device_approval_requested.hbs +++ b/src/static/templates/email/device_approval_requested.hbs @@ -2,5 +2,5 @@ Device Approval Requested {{user_name}} ({{user_email}}) is asking to have a new device approved in your {{org_name}} organization. Until an administrator approves it, they cannot get into their vault on that device. -Review the request in the organization administration of {{{url}}}. +Review the request at {{{url}}}. {{> email/email_footer_text }} diff --git a/src/static/templates/email/device_approval_requested.html.hbs b/src/static/templates/email/device_approval_requested.html.hbs index ee66c98d..580b89cb 100644 --- a/src/static/templates/email/device_approval_requested.html.hbs +++ b/src/static/templates/email/device_approval_requested.html.hbs @@ -9,7 +9,7 @@ Device Approval Requested - Review the request in the organization administration of {{{url}}}. + Review the request at {{{url}}}. diff --git a/src/static/templates/email/trusted_device_admin_approval.hbs b/src/static/templates/email/trusted_device_admin_approval.hbs index af5fbcf2..27dbd74a 100644 --- a/src/static/templates/email/trusted_device_admin_approval.hbs +++ b/src/static/templates/email/trusted_device_admin_approval.hbs @@ -5,5 +5,5 @@ An administrator of your {{org_name}} organization approved a device for your ac Device: {{device}} IP address: {{ip}} -If this was not you, change your password and contact your administrator. +If this was not you, contact your administrator and remove the device from your account. {{> email/email_footer_text }} diff --git a/src/static/templates/email/trusted_device_admin_approval.html.hbs b/src/static/templates/email/trusted_device_admin_approval.html.hbs index 535cd41a..94a79717 100644 --- a/src/static/templates/email/trusted_device_admin_approval.html.hbs +++ b/src/static/templates/email/trusted_device_admin_approval.html.hbs @@ -15,7 +15,7 @@ Device Approved - If this was not you, change your password and contact your administrator. + If this was not you, contact your administrator and remove the device from your account. diff --git a/src/util.rs b/src/util.rs index 91f075d1..43bce822 100644 --- a/src/util.rs +++ b/src/util.rs @@ -505,6 +505,95 @@ pub fn is_valid_email(email: &str) -> bool { true } +/// The most an `EncString` we are willing to store may weigh. The largest legitimate one is an +/// RSA-4096 envelope with a MAC, which stays an order of magnitude below this. +const MAX_ENC_STRING_LENGTH: usize = 4096; + +/// Whether a value has the shape of a Bitwarden `EncString`: `.|...`. +/// +/// The server cannot tell whether a blob decrypts, but it can refuse everything that is not even +/// of the right form, which keeps unbounded junk out of the columns that hold key material. +/// Mirrors `EncryptedStringAttribute` upstream. +/// https://github.com/bitwarden/server/blob/main/src/Core/Utilities/EncryptedStringAttribute.cs +pub fn is_valid_enc_string(value: &str) -> bool { + if value.is_empty() || value.len() > MAX_ENC_STRING_LENGTH { + return false; + } + + let Some((enc_type, data)) = value.split_once('.') else { + return false; + }; + + // The number of `|` separated parts each type is made of. + let parts = match enc_type { + // An RSA envelope is the ciphertext by itself. + "3" | "4" => 1, + // An AES value carries its IV, an RSA one from type 5 on carries a MAC. + "0" | "5" | "6" => 2, + // And an AES value from type 1 on carries both. + "1" | "2" => 3, + _ => return false, + }; + + let mut seen = 0; + for part in data.split('|') { + seen += 1; + if seen > parts || part.is_empty() || data_encoding::BASE64.decode(part.as_bytes()).is_err() { + return false; + } + } + + seen == parts +} + +#[cfg(test)] +mod enc_string_tests { + use super::is_valid_enc_string; + + #[test] + fn a_well_formed_enc_string_of_every_type_is_accepted() { + for value in [ + "0.aXY=|Y2lwaGVy", + "1.aXY=|Y2lwaGVy|bWFj", + "2.aXY=|Y2lwaGVy|bWFj", + "3.Y2lwaGVy", + "4.Y2lwaGVy", + "5.Y2lwaGVy|bWFj", + "6.Y2lwaGVy|bWFj", + ] { + assert!(is_valid_enc_string(value), "{value}"); + } + } + + #[test] + fn anything_that_is_not_one_is_refused() { + for value in [ + "", + " ", + "not-an-enc-string", + "2", + "2.", + ".aXY=|Y2lwaGVy|bWFj", + "7.Y2lwaGVy", // no such type + "-1.Y2lwaGVy", // and none below zero either + "2.aXY=|Y2lwaGVy", // type 2 without its mac + "2.aXY=|Y2lwaGVy|bWFj|x", // or with one part too many + "4.Y2lwaGVy|bWFj", // type 4 carries no mac + "2.aXY=||bWFj", // an empty part is not base64 + "4.not base64!", + ] { + assert!(!is_valid_enc_string(value), "{value}"); + } + } + + #[test] + fn an_oversized_value_is_refused() { + let payload = "A".repeat(4096); + assert!(is_valid_enc_string(&format!("4.{}", &payload[..4000]))); + assert!(!is_valid_enc_string(&format!("4.{payload}")), "must not grow without bound"); + } +} + // // Deployment environment methods //