From 8098211cecc826fbb0f556f56088976999c5cec0 Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Sat, 29 Aug 2026 17:29:45 +0200 Subject: [PATCH] Fix trusted device review findings --- src/api/core/accounts.rs | 336 ++++++++++++++++++++++++++++------ src/api/core/organizations.rs | 36 ++-- src/api/identity.rs | 294 ++++++++++++++++++++++++----- src/api/notifications.rs | 9 +- src/api/push.rs | 16 +- src/auth.rs | 35 ++++ src/db/models/auth_request.rs | 41 ++++- src/db/models/device.rs | 116 ++++++++---- src/db/models/organization.rs | 42 +++++ src/util.rs | 123 ++++++++++--- 10 files changed, 857 insertions(+), 191 deletions(-) diff --git a/src/api/core/accounts.rs b/src/api/core/accounts.rs index d1cdf384..fb3abf08 100644 --- a/src/api/core/accounts.rs +++ b/src/api/core/accounts.rs @@ -1,4 +1,4 @@ -use std::collections::{HashMap, HashSet}; +use std::collections::HashSet; use chrono::Utc; use rocket::{ @@ -22,8 +22,8 @@ use crate::{ models::{ AuthRequest, AuthRequestId, AuthRequestType, Cipher, CipherId, Device, DeviceId, DeviceType, DeviceWithAuthRequest, EmergencyAccess, EmergencyAccessId, EventType, Folder, FolderId, Invitation, - Membership, MembershipId, MembershipStatus, MembershipType, OrgPolicy, OrgPolicyType, Organization, - OrganizationId, Send, SendId, User, UserId, UserKdfType, + Membership, MembershipId, MembershipStatus, OrgPolicy, OrgPolicyType, Organization, OrganizationId, Send, + SendId, User, UserId, UserKdfType, }, }, mail, @@ -816,6 +816,20 @@ struct RotateAccountUnlockData { emergency_access_unlock_data: Vec, master_password_unlock_data: MasterPasswordUnlockData, organization_account_recovery_unlock_data: Vec, + /// The user key, re-wrapped for every device that unlocks the vault without a master password. + /// + /// Absent rather than empty tells the two generations of clients apart: one that sends this + /// rotates the trust of its devices right here, an older one does it afterwards through + /// `POST /devices/update-trust` and leaves this out entirely. See `post_rotatekey`. + device_key_unlock_data: Option>, +} + +#[derive(Deserialize)] +#[serde(rename_all = "camelCase")] +struct UpdateDeviceKeysData { + device_id: DeviceId, + encrypted_user_key: String, + encrypted_public_key: String, } #[derive(Deserialize)] @@ -845,6 +859,55 @@ struct RotateAccountData { sends: Vec, } +/// Works out what a key rotation has to write to the user's devices. +/// +/// Returns the devices that keep their trust, each with the user key freshly wrapped for it. +/// Whatever the user owns beyond that list ends up untrusted, so the caller can hand the result to +/// `Device::replace_trust` and be done in one transaction. +/// +/// Mirrors `DeviceRotationValidator` upstream, which refuses a rotation that would quietly drop the +/// trust of a device the user still relies on. Untrusting is the client's own separate step, and +/// the current ones take it before they get here. +/// https://github.com/bitwarden/server/blob/main/src/Api/KeyManagement/Validators/DeviceRotationValidator.cs +fn validate_device_keydata( + updates: &[UpdateDeviceKeysData], + existing_devices: &[Device], +) -> ApiResult> { + let mut listed: HashSet<&DeviceId> = HashSet::with_capacity(updates.len()); + let mut rotated = Vec::with_capacity(updates.len()); + + for update in updates { + if !listed.insert(&update.device_id) { + err!("A device was listed more than once in the rotation") + } + + let Some(device) = existing_devices.iter().find(|device| device.uuid == update.device_id) else { + err!(format!("Device {} does not belong to this user", update.device_id)) + }; + + validate_enc_strings(&[ + ("encryptedUserKey", &update.encrypted_user_key), + ("encryptedPublicKey", &update.encrypted_public_key), + ])?; + + // Without its own key pair a device has nothing these two keys could belong to, so it + // cannot be put back into a trust and is left to be untrusted instead. + if device.holds_private_key() { + rotated.push(( + update.device_id.clone(), + update.encrypted_user_key.clone(), + update.encrypted_public_key.clone(), + )); + } + } + + if existing_devices.iter().any(|device| device.is_trusted() && !listed.contains(&device.uuid)) { + err!("All existing trusted devices must be included in the rotation") + } + + Ok(rotated) +} + fn validate_keydata( data: &KeyData, existing_ciphers: &[Cipher], @@ -949,6 +1012,7 @@ async fn post_rotatekey(data: Json, headers: Headers, conn: DbConn, nt: // We only rotate the reset password key if it is set. existing_memberships.retain(|m| m.reset_password_key.is_some()); let mut existing_sends = Send::find_by_user(user_id, &conn).await; + let existing_devices = Device::find_by_user(user_id, &conn).await; validate_keydata( &data, @@ -960,6 +1024,11 @@ async fn post_rotatekey(data: Json, headers: Headers, conn: DbConn, nt: &headers.user, )?; + let rotated_devices = match data.account_unlock_data.device_key_unlock_data.as_deref() { + Some(updates) => Some(validate_device_keydata(updates, &existing_devices)?), + None => None, + }; + // Update folder data for folder_data in data.account_data.folders { // Skip `null` folder id entries. @@ -1023,12 +1092,19 @@ 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?; + // Settle that here rather than after the account itself: by this point the ciphers have already + // been rewritten under the new user key, so a device that holds the new one is the half that + // still works if what follows fails. The other order would leave a device counting itself + // trusted while handing its owner the key it just stopped needing. + match rotated_devices { + // The current clients send the re-wrapped user key for every trusted device along with the + // rotation, so their trust survives it. Anything they left out is untrusted here. + Some(rotated) => Device::replace_trust(&headers.user.uuid, rotated, &conn).await?, + // A client old enough to leave the field out does this afterwards through + // `POST /devices/update-trust`. Until it does, no device counts as trusted, so the worst it + // costs its owner is another login rather than an unlock that fails. + None => Device::invalidate_wrapped_user_keys(&headers.user.uuid, &conn).await?, + } // Update user data let mut user = headers.user; @@ -1707,6 +1783,10 @@ struct UpdateDevicesTrustData { /// /// Every trusted device that is not listed loses its trust: its stored copy of the user key is the /// old one and would no longer unlock anything. +/// +/// The current clients do this as part of the rotation itself and never come here; this is the +/// route the older ones take, and the only one that can rotate the trust of a single device without +/// rotating the account. See `post_rotatekey`. #[post("/devices/update-trust", data = "")] async fn post_devices_update_trust(data: Json, headers: Headers, conn: DbConn) -> EmptyResult { let data = data.into_inner(); @@ -1718,55 +1798,48 @@ async fn post_devices_update_trust(data: Json, headers: ("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") - } - 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") - } - } - let devices = Device::find_by_user(&headers.user.uuid, &conn).await; if !devices.iter().any(|device| device.uuid == headers.device.uuid) { err!("No device found") } - // Validate everything before writing anything: a rotation that stops halfway would leave the - // devices wrapping a mix of the old and the new user key. - if let Some(unknown) = updates.keys().find(|device_id| !devices.iter().any(|device| device.uuid == **device_id)) { - err!(format!("Device {unknown} does not belong to this user")) - } + // The current device is written whatever it holds now, as upstream does: it is the one the + // caller is speaking from and just proved it can unlock. + let mut updates = vec![( + headers.device.uuid.clone(), + data.current_device.encrypted_user_key, + data.current_device.encrypted_public_key, + )]; + let mut listed: HashSet = HashSet::from([headers.device.uuid.clone()]); - for mut device in devices { - 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 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; + // Validate everything before writing anything, so one bad entry cannot leave the devices + // wrapping a mix of the old and the new user key. + for other in data.other_devices { + if !listed.insert(other.device_id.clone()) { + if other.device_id == headers.device.uuid { + err!("The current device cannot also be part of the optional rotation") } - device.encrypted_user_key = Some(keys.encrypted_user_key); - device.encrypted_public_key = Some(keys.encrypted_public_key); - } else if device.holds_any_key() { - // Not listed, so whatever it still holds wraps the previous user key. - device.untrust(); - } else { - continue; + err!("A device was listed more than once in the rotation") } - device.save(true, &conn).await?; + let Some(device) = devices.iter().find(|device| device.uuid == other.device_id) else { + err!(format!("Device {} does not belong to this user", other.device_id)) + }; + + validate_enc_strings(&[ + ("encryptedUserKey", &other.keys.encrypted_user_key), + ("encryptedPublicKey", &other.keys.encrypted_public_key), + ])?; + + // 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, so the device is left to be untrusted instead. + if device.holds_private_key() { + updates.push((other.device_id, other.keys.encrypted_user_key, other.keys.encrypted_public_key)); + } } - Ok(()) + Device::replace_trust(&headers.user.uuid, updates, &conn).await } #[derive(Debug, Deserialize)] @@ -1779,22 +1852,16 @@ struct UntrustDevicesData { async fn post_devices_untrust(data: Json, headers: Headers, conn: DbConn) -> EmptyResult { let data = data.into_inner(); - let mut devices = Device::find_by_user(&headers.user.uuid, &conn).await; + let owned: HashSet = + Device::find_by_user(&headers.user.uuid, &conn).await.into_iter().map(|device| device.uuid).collect(); // Check that the user owns all of them first, so a single foreign id does not leave the request // half applied. - if let Some(unknown) = - data.devices.iter().find(|device_id| !devices.iter().any(|device| &device.uuid == *device_id)) - { + if let Some(unknown) = data.devices.iter().find(|device_id| !owned.contains(*device_id)) { err!(format!("Device {unknown} does not belong to this user")) } - for device in devices.iter_mut().filter(|device| data.devices.contains(&device.uuid)) { - device.untrust(); - device.save(true, &conn).await?; - } - - Ok(()) + Device::untrust_many(&headers.user.uuid, data.devices, &conn).await } /// Reported by a client that still holds a device key but did not get any keys back from us. @@ -1834,14 +1901,46 @@ struct AuthRequestRequest { atype: i32, } +/// Upstream puts `[StringLength(25)]` on the access code, so no client sends more than that. +/// https://github.com/bitwarden/server/blob/main/src/Core/Auth/Models/Api/Request/AuthRequest/AuthRequestCreateRequestModel.cs +const MAX_ACCESS_CODE_LENGTH: usize = 25; + +/// A base64 SPKI RSA-4096 public key is under a kilobyte; this leaves room for whatever comes next. +const MAX_REQUEST_PUBLIC_KEY_LENGTH: usize = 4096; + +impl AuthRequestRequest { + /// Both of these end up stored, and the admin approval route stores a copy per organization the + /// user belongs to, so neither may be unbounded. The public key is handed to the answering + /// client as base64 to wrap a key against; one that is not base64 at all would break the page + /// listing the requests rather than just this one. + fn validate(&self) -> EmptyResult { + if self.access_code.is_empty() || self.access_code.len() > MAX_ACCESS_CODE_LENGTH { + err!("Invalid access code") + } + + if self.public_key.is_empty() + || self.public_key.len() > MAX_REQUEST_PUBLIC_KEY_LENGTH + || data_encoding::BASE64.decode(self.public_key.as_bytes()).is_err() + { + err!("Invalid public key") + } + + Ok(()) + } +} + fn auth_request_json(auth_request: &AuthRequest) -> Value { json!({ "id": auth_request.uuid, "publicKey": auth_request.public_key, "type": auth_request.atype, "requestDeviceType": DeviceType::from_i32(auth_request.device_type).to_string(), + // The clients read the raw enum value as well, to pick an icon for the asking device. + "requestDeviceTypeValue": auth_request.device_type, "requestDeviceIdentifier": auth_request.request_device_identifier, "requestIpAddress": auth_request.request_ip, + // Not recorded here, but the clients read it, so it is answered rather than missing. + "requestCountryName": null, "key": auth_request.enc_key, "masterPasswordHash": auth_request.master_password_hash, "creationDate": format_date(&auth_request.creation_date), @@ -1867,6 +1966,8 @@ async fn post_auth_request( err!("You must be authenticated to create a request of that type") } + data.validate()?; + let Some(user) = User::find_by_mail(&data.email, &conn).await else { err!("AuthRequest doesn't exist", "User not found") }; @@ -1928,6 +2029,8 @@ async fn post_admin_auth_request(data: Json, headers: Header err!("AuthRequest doesn't exist", "Device verification failed") } + data.validate()?; + // 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. @@ -2009,10 +2112,12 @@ async fn notify_device_approval_requested(user: &User, org_id: &OrganizationId, return; }; + // The same set that may answer the request, see `ManageResetPasswordHeaders`. Mailing anyone + // else would tell them who is asking for something they cannot do anything about. let approvers = Membership::find_confirmed_by_org(org_id, conn) .await .into_iter() - .filter(|member| member.atype <= MembershipType::Admin as i32); + .filter(Membership::has_manage_reset_password_permission); for approver in approvers { let Some(admin) = User::find_by_uuid(&approver.user_uuid, conn).await else { @@ -2112,7 +2217,7 @@ async fn put_auth_request( auth_request.save(&conn).await?; 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; + nt.send_auth_response(&auth_request.user_uuid, &auth_request.uuid, Some(&headers.device), &conn).await; log_user_event( EventType::OrganizationUserApprovedAuthRequest as i32, @@ -2209,3 +2314,116 @@ pub async fn purge_auth_requests(pool: DbPool) { error!("Failed to get DB connection while purging auth requests"); } } + +#[cfg(test)] +mod tests { + use super::*; + + fn device(id: &str, trusted: bool) -> Device { + let mut device = Device::new(id.to_owned().into(), String::from("user").into(), String::new(), 9); + if trusted { + device.encrypted_user_key = Some(String::from("4.b2xkdXNlcmtleQ==")); + device.encrypted_public_key = Some(String::from("2.aXY=|Y2lwaGVy|bWFj")); + device.encrypted_private_key = Some(String::from("2.aXY=|Y2lwaGVy|bWFj")); + } + device + } + + fn update(device_id: &str) -> UpdateDeviceKeysData { + UpdateDeviceKeysData { + device_id: device_id.to_owned().into(), + encrypted_user_key: String::from("4.bmV3dXNlcmtleQ=="), + encrypted_public_key: String::from("2.aXY=|bmV3|bWFj"), + } + } + + /// The ids and keys the rotation would write, so a test can say what it expects in one line. + fn rotated(result: &[(DeviceId, String, String)]) -> Vec { + result.iter().map(|(device_id, user_key, _)| format!("{device_id}={user_key}")).collect() + } + + #[test] + fn a_trusted_device_that_is_listed_keeps_its_trust() { + let devices = [device("a", true), device("b", true)]; + let updates = [update("a"), update("b")]; + + let result = validate_device_keydata(&updates, &devices).unwrap(); + assert_eq!( + rotated(&result), + ["a=4.bmV3dXNlcmtleQ==", "b=4.bmV3dXNlcmtleQ=="], + "both are re-wrapped, neither keeps the previous user key" + ); + } + + #[test] + fn a_trusted_device_that_is_left_out_takes_the_rotation_down_with_it() { + // Silently dropping the trust of a device the user still relies on is not the server's call + // to make; the client untrusts it first if that is what it means. + let devices = [device("a", true), device("b", true)]; + + let err = validate_device_keydata(&[update("a")], &devices).unwrap_err(); + assert!(format!("{err}").contains("All existing trusted devices must be included")); + } + + #[test] + fn a_device_of_somebody_else_is_refused() { + let devices = [device("a", true)]; + + let err = validate_device_keydata(&[update("a"), update("stranger")], &devices).unwrap_err(); + assert!(format!("{err}").contains("does not belong to this user")); + } + + #[test] + fn the_same_device_may_not_be_listed_twice() { + // Two entries for one device means one of the two keys is dropped without anyone noticing + // which, so neither is taken. + let devices = [device("a", true)]; + + let err = validate_device_keydata(&[update("a"), update("a")], &devices).unwrap_err(); + assert!(format!("{err}").contains("listed more than once")); + } + + #[test] + fn a_key_that_is_not_an_encrypted_string_is_refused() { + let devices = [device("a", true)]; + + let mut broken = update("a"); + broken.encrypted_user_key = String::from("not an enc string"); + let err = validate_device_keydata(&[broken], &devices).unwrap_err(); + assert!(format!("{err}").contains("encryptedUserKey")); + + let mut broken = update("a"); + broken.encrypted_public_key = String::new(); + let err = validate_device_keydata(&[broken], &devices).unwrap_err(); + assert!(format!("{err}").contains("encryptedPublicKey")); + } + + #[test] + fn a_device_without_its_own_key_pair_is_not_given_a_user_key() { + // Half a trust is worth nothing to the client and would only fail at the next unlock, so + // the device is dropped from the rotation and ends up untrusted instead. + let devices = [device("a", true), device("b", false)]; + + let result = validate_device_keydata(&[update("a"), update("b")], &devices).unwrap(); + assert_eq!(rotated(&result), ["a=4.bmV3dXNlcmtleQ=="]); + } + + #[test] + fn a_user_who_trusts_no_device_rotates_nothing() { + let devices = [device("a", false)]; + + let result = validate_device_keydata(&[], &devices).unwrap(); + assert!(result.is_empty(), "and the leftovers of `a` are cleared by the write that follows"); + } + + #[test] + fn a_partially_trusted_device_does_not_have_to_be_listed() { + // It cannot unlock anything as it stands, so leaving it out is not the loss of a trust. + let mut half = device("b", true); + half.encrypted_user_key = None; + let devices = [device("a", true), half]; + + let result = validate_device_keydata(&[update("a")], &devices).unwrap(); + assert_eq!(rotated(&result), ["a=4.bmV3dXNlcmtleQ=="]); + } +} diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index b6a207c6..5d6b759f 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -12,13 +12,16 @@ use crate::{ AnonymousNotify, EmptyResult, JsonResult, Notify, PasswordOrOtpData, UpdateType, core::{CipherSyncData, CipherSyncType, accept_org_invite, log_event, two_factor}, }, - auth::{AdminHeaders, Headers, ManagerHeaders, ManagerHeadersLoose, OrgMemberHeaders, OwnerHeaders, decode_invite}, + auth::{ + AdminHeaders, Headers, ManageResetPasswordHeaders, ManagerHeaders, ManagerHeadersLoose, OrgMemberHeaders, + OwnerHeaders, decode_invite, + }, db::{ DbConn, models::{ AuthRequest, AuthRequestId, Cipher, CipherId, Collection, CollectionCipher, CollectionGroup, CollectionId, - CollectionUser, Device, DeviceType, EventType, Group, GroupId, GroupUser, Invitation, Membership, - MembershipId, MembershipStatus, MembershipType, OrgPolicy, OrgPolicyType, Organization, OrganizationApiKey, + CollectionUser, DeviceType, EventType, Group, GroupId, GroupUser, Invitation, Membership, MembershipId, + MembershipStatus, MembershipType, OrgPolicy, OrgPolicyType, Organization, OrganizationApiKey, OrganizationId, User, UserId, }, }, @@ -3214,7 +3217,11 @@ async fn put_reset_password_enrollment( /// The requests waiting for an answer in this organization. #[get("/organizations//auth-requests")] -async fn get_organization_auth_requests(org_id: OrganizationId, headers: AdminHeaders, conn: DbConn) -> JsonResult { +async fn get_organization_auth_requests( + org_id: OrganizationId, + headers: ManageResetPasswordHeaders, + conn: DbConn, +) -> JsonResult { if org_id != headers.org_id { err!("Organization not found", "Organization id's do not match"); } @@ -3290,7 +3297,7 @@ async fn update_organization_auth_request( org_id: OrganizationId, request_id: AuthRequestId, data: Json, - headers: AdminHeaders, + headers: ManageResetPasswordHeaders, conn: DbConn, ant: AnonymousNotify<'_>, nt: Notify<'_>, @@ -3314,7 +3321,7 @@ async fn update_organization_auth_request( async fn deny_organization_auth_requests( org_id: OrganizationId, data: Json, - headers: AdminHeaders, + headers: ManageResetPasswordHeaders, conn: DbConn, ant: AnonymousNotify<'_>, nt: Notify<'_>, @@ -3346,7 +3353,7 @@ async fn deny_organization_auth_requests( async fn update_many_organization_auth_requests( org_id: OrganizationId, data: Json>, - headers: AdminHeaders, + headers: ManageResetPasswordHeaders, conn: DbConn, ant: AnonymousNotify<'_>, nt: Notify<'_>, @@ -3381,7 +3388,7 @@ async fn answer_organization_auth_request( approved: bool, encrypted_user_key: Option, on_unanswerable: OnUnanswerable, - headers: &AdminHeaders, + headers: &ManageResetPasswordHeaders, conn: &DbConn, ant: &AnonymousNotify<'_>, nt: &Notify<'_>, @@ -3453,14 +3460,11 @@ async fn answer_organization_auth_request( ant.send_auth_response(&auth_request.user_uuid, &auth_request.uuid).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; - } + // No acting device: the answer did not come from one of this user's devices, so every one of + // them, the one that asked above all, should hear about it. Naming the administrator's device + // here would leave it out of a notification meant for somebody else's account and hand its + // identifiers to the push relay under a foreign user id. + nt.send_auth_response(&auth_request.user_uuid, &auth_request.uuid, None, 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 84d332b4..a56cc65c 100644 --- a/src/api/identity.rs +++ b/src/api/identity.rs @@ -31,8 +31,8 @@ use crate::{ DbConn, models::{ AuthRequest, AuthRequestId, Device, DeviceId, DeviceType, EventType, Invitation, Membership, - MembershipStatus, MembershipType, OIDCCodeResponseError, OrganizationApiKey, OrganizationId, SendId, - SsoAuth, SsoUser, TwoFactor, TwoFactorIncomplete, TwoFactorType, User, UserId, + MembershipStatus, OIDCCodeResponseError, OrgPolicy, OrgPolicyType, Organization, OrganizationApiKey, + OrganizationId, SendId, SsoAuth, SsoUser, TwoFactor, TwoFactorIncomplete, TwoFactorType, User, UserId, }, }, error::MapResult, @@ -481,16 +481,98 @@ async fn password_login( authenticated_response(&user, &mut device, auth_tokens, twofactor_token, false, conn, ip).await } -/// Whether offering the trusted device options can lead anywhere for this account. +/// Whether the account creation the clients run when nothing else is on offer can succeed here. /// -/// Creating an account this way ends with enrolling into account recovery, which the clients do -/// unconditionally and which needs an organization to enroll into. An account that has nothing yet -/// and belongs to nowhere would therefore be shown the screen for a new account and get stuck -/// halfway through it, with its keys already written and its device still untrusted. Withholding -/// the options sends it to setting a master password instead, which works and leaves the door to -/// trusted devices open for the next login. -fn trusted_device_flow_is_completable(has_account_keys: bool, in_organization: bool) -> bool { - has_account_keys || in_organization +/// A client that gets the trusted device options, but neither a master password nor an approval an +/// administrator could give, decides it is looking at a fresh account and walks it through +/// creation: generate the account keys and post them, enrol into the account recovery of the +/// organization behind the SSO login, then trust the device. Every one of those has to be able to +/// go through. If enrolment is refused, the keys are already written, and the next login walks into +/// the same screen and fails at posting them a second time, leaving an account that can never be +/// unlocked at all. +/// +/// So the same conditions the enrolment endpoint enforces are checked here, before the client has +/// written anything. Withholding the options instead sends it to setting a master password, which +/// works and leaves the door to trusted devices open for the next login. +async fn account_creation_can_succeed(user: &User, conn: &DbConn) -> bool { + // `POST /accounts/keys` refuses to replace the keys of an account that has them, and the + // clients post a freshly generated pair without looking. + if user.private_key.is_some() || user.public_key.is_some() { + return false; + } + + // The organization the client enrols into is the one `GET /organizations//auto-enroll-status` + // hands it, so ask the same question here. + let Some(membership) = Membership::find_main_user_org(&user.uuid, conn).await else { + return false; + }; + + // What `check_reset_password_applicable` demands of that organization. + if !CONFIG.mail_enabled() { + return false; + } + if !OrgPolicy::find_by_org_and_type(&membership.org_uuid, OrgPolicyType::ResetPassword, conn) + .await + .is_some_and(|policy| policy.enabled) + { + return false; + } + + // Enrolling wraps the user key for the organization, so it needs its public key. + Organization::find_by_uuid(&membership.org_uuid, conn) + .await + .is_some_and(|org| org.public_key.is_some_and(|key| !key.is_empty())) +} + +/// The ways an account could get through the trusted device flow, which is what decides whether +/// offering it leads anywhere. +#[expect( + clippy::struct_excessive_bools, + reason = "Four independent facts about one account, not a state that could be an enum" +)] +struct TrustedDeviceWaysIn { + /// This device already holds the keys, so it unlocks without asking anyone. + device_is_trusted: bool, + /// A master password to fall back on. + has_master_password: bool, + /// An administrator of an organization who could let a new device in, which they can only do + /// once the member enrolled into account recovery. + has_admin_approval: bool, + /// Nothing set up yet, but the account creation the clients run in that case would go through. + can_create_account: bool, +} + +impl TrustedDeviceWaysIn { + /// Whether the trusted device options belong in a login response, and in which of their two + /// roles. + /// + /// `Some(true)` means they are only there to walk a user without a master password off the + /// feature after it was switched off; `None` means they are withheld, because nothing the + /// client could do with them would work. + /// + /// The order mirrors how the clients read them: a trusted device unlocks straight away, + /// otherwise an administrator to ask or a master password to type is offered, and only when + /// there is neither does the client decide it is looking at a fresh account and try to create + /// one. + fn offer(&self, enabled: bool) -> Option { + // Once the feature is switched off again, a user without a master password would be locked + // out of their own vault. Keep telling their still trusted devices about it so their client + // can walk them through setting one while they can still unlock. + let offboarding = !enabled && self.offboarding_candidate(); + if !(enabled || offboarding) { + return None; + } + + let leads_somewhere = + self.device_is_trusted || self.has_admin_approval || self.has_master_password || self.can_create_account; + leads_somewhere.then_some(offboarding) + } + + /// A user who is still on a trusted device and has no master password to fall back on, and so + /// has to be told when the feature goes away. + fn offboarding_candidate(&self) -> bool { + self.device_is_trusted && !self.has_master_password + } } /// Trusted device encryption ("passwordless SSO"): instead of deriving the user key from a master @@ -504,20 +586,37 @@ fn trusted_device_flow_is_completable(has_account_keys: bool, in_organization: b async fn trusted_device_option(user: &User, device: &Device, conn: &DbConn) -> Option { let enabled = CONFIG.sso_trusted_device_encryption(); - // Once the feature is switched off again, a user without a master password would be locked out - // of their own vault. Keep telling their still trusted devices about it so their client can walk - // them through setting one while they can still unlock. - let offboarding = !enabled && device.is_trusted() && user.password_hash.is_empty(); - if !enabled && !offboarding { + let mut ways_in = TrustedDeviceWaysIn { + device_is_trusted: device.is_trusted(), + has_master_password: !user.password_hash.is_empty(), + has_admin_approval: false, + can_create_account: false, + }; + + // Answered ahead of everything else so a server that does not offer trusted devices, and has no + // user left on them, does no work for the feature at all. + if !(enabled || ways_in.offboarding_candidate()) { return None; } let memberships = Membership::find_by_user(&user.uuid, conn).await; - if !trusted_device_flow_is_completable(user.private_key.is_some(), !memberships.is_empty()) { - return None; + // 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. 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. + ways_in.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()) + }); + + // Only worth asking when nothing cheaper already lets the client in. + if !(ways_in.device_is_trusted || ways_in.has_admin_approval || ways_in.has_master_password) { + ways_in.can_create_account = account_creation_can_succeed(user, conn).await; } + let offboarding = ways_in.offer(enabled)?; + // Any other device of this user that could show an approval prompt. The user unlocks a new // device from one of these, or with the master password if they have one. let has_login_approving_device = Device::find_by_user(&user.uuid, conn) @@ -525,24 +624,14 @@ async fn trusted_device_option(user: &User, device: &Device, conn: &DbConn) -> O .iter() .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. 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. Matches what - // `AdminHeaders` actually lets through. - let has_manage_reset_password_permission = memberships.iter().any(|member| { - member.status == MembershipStatus::Confirmed as i32 && member.atype <= MembershipType::Admin as i32 - }); + // could approve others, but has no master password themselves, into setting one. Upstream reads + // a `ManageResetPassword` permission here, which in Vaultwarden's role model only the + // administrators of an organization have. + let has_manage_reset_password_permission = memberships.iter().any(Membership::has_manage_reset_password_permission); Some(json!({ - "HasAdminApproval": has_admin_approval, + "HasAdminApproval": ways_in.has_admin_approval, "HasLoginApprovingDevice": has_login_approving_device, "HasManageResetPasswordPermission": has_manage_reset_password_permission, "IsTdeOffboarding": offboarding, @@ -1407,19 +1496,132 @@ async fn authorize(data: AuthorizeData, cookies: &CookieJar<'_>, secure: Secure, mod tests { use super::*; + /// A `TrustedDeviceWaysIn` plus the server setting, so the cases below read as what they are. + #[expect(clippy::struct_excessive_bools, reason = "Mirrors the struct under test")] + struct Account { + enabled: bool, + device_is_trusted: bool, + has_master_password: bool, + has_admin_approval: bool, + can_create_account: bool, + } + + impl Account { + /// A user of a server that offers trusted devices, on a device it does not know yet, with + /// nothing set up: the shape everything below varies from. + fn new() -> Self { + Self { + enabled: true, + device_is_trusted: false, + has_master_password: false, + has_admin_approval: false, + can_create_account: false, + } + } + + fn offer(&self) -> Option { + TrustedDeviceWaysIn { + device_is_trusted: self.device_is_trusted, + has_master_password: self.has_master_password, + has_admin_approval: self.has_admin_approval, + can_create_account: self.can_create_account, + } + .offer(self.enabled) + } + } + + #[test] + fn a_server_that_does_not_offer_trusted_devices_says_nothing_about_them() { + for (device_is_trusted, has_master_password) in [(false, false), (false, true), (true, true)] { + let account = Account { + enabled: false, + device_is_trusted, + has_master_password, + ..Account::new() + }; + assert_eq!(account.offer(), None); + } + } + #[test] - fn an_account_with_nothing_and_nowhere_to_go_is_not_offered_trusted_devices() { - // The one combination the clients cannot finish: nothing set up yet and no organization - // to enroll into. - assert!(!trusted_device_flow_is_completable(false, false)); - - // A brand new account that was invited somewhere can enroll, so the flow completes. - assert!(trusted_device_flow_is_completable(false, true)); - - // An account that is already set up does not go through account creation at all, with or - // without an organization. This covers the master password first route as well as an - // account that already trusts a device. - assert!(trusted_device_flow_is_completable(true, false)); - assert!(trusted_device_flow_is_completable(true, true)); + fn a_user_left_on_a_trusted_device_is_walked_off_the_feature() { + // The feature is gone but this device still unlocks and its owner has no master password. + // They are told so, so their client can walk them through setting one while they still can. + let account = Account { + enabled: false, + device_is_trusted: true, + ..Account::new() + }; + assert_eq!(account.offer(), Some(true), "offboarding"); + + // With the feature on, the same device is simply trusted. + let account = Account { + device_is_trusted: true, + ..Account::new() + }; + assert_eq!(account.offer(), Some(false)); + } + + #[test] + fn an_account_with_no_way_through_the_flow_is_not_offered_it() { + // Nothing set up, nobody to ask, and account creation would fail at the enrolment: the one + // combination that would leave the account half built. The client is sent to setting a + // master password instead. + assert_eq!(Account::new().offer(), None); + } + + #[test] + fn every_way_through_the_flow_is_offered_it() { + // A device that can unlock right now. + assert_eq!( + Account { + device_is_trusted: true, + ..Account::new() + } + .offer(), + Some(false) + ); + + // An administrator to ask, which needs the member to be enrolled in account recovery. + assert_eq!( + Account { + has_admin_approval: true, + ..Account::new() + } + .offer(), + Some(false) + ); + + // A master password to fall back on. + assert_eq!( + Account { + has_master_password: true, + ..Account::new() + } + .offer(), + Some(false) + ); + + // A fresh account in an organization that can actually take the enrolment. + assert_eq!( + Account { + can_create_account: true, + ..Account::new() + } + .offer(), + Some(false) + ); + } + + #[test] + fn a_user_with_a_master_password_is_never_offboarded() { + // There is nothing to walk them off, they can unlock either way. + let account = Account { + enabled: false, + device_is_trusted: true, + has_master_password: true, + ..Account::new() + }; + assert_eq!(account.offer(), None); } } diff --git a/src/api/notifications.rs b/src/api/notifications.rs index 8bfcd518..2cd6ecf8 100644 --- a/src/api/notifications.rs +++ b/src/api/notifications.rs @@ -529,11 +529,14 @@ impl WebSocketUsers { } } + /// `acting_device` is the device of this user that answered the request, and is the one left out + /// of the notification. An answer that came from outside their devices, as an approval by an + /// administrator of their organization does, passes `None` so that all of them hear about it. pub async fn send_auth_response( &self, user_id: &UserId, auth_request_id: &AuthRequestId, - device: &Device, + acting_device: Option<&Device>, conn: &DbConn, ) { // Skip any processing if both WebSockets and Push are not active @@ -543,14 +546,14 @@ impl WebSocketUsers { let data = create_update( vec![("Id".into(), auth_request_id.to_string().into()), ("UserId".into(), user_id.to_string().into())], UpdateType::AuthRequestResponse, - Some(device.uuid.clone()), + acting_device.map(|device| device.uuid.clone()), ); if CONFIG.enable_websocket() { self.send_update(user_id, &data).await; } if CONFIG.push_enabled() { - push_auth_response(user_id, auth_request_id, device, conn).await; + push_auth_response(user_id, auth_request_id, acting_device, conn).await; } } } diff --git a/src/api/push.rs b/src/api/push.rs index e87a0985..87c52ede 100644 --- a/src/api/push.rs +++ b/src/api/push.rs @@ -317,13 +317,23 @@ pub async fn push_auth_request(user_id: &UserId, auth_request_id: &str, device: } } -pub async fn push_auth_response(user_id: &UserId, auth_request_id: &AuthRequestId, device: &Device, conn: &DbConn) { +/// `acting_device` is the device that answered, and is the one device left out of the notification, +/// since it already knows. An answer that did not come from a device of this user at all, as an +/// approval by an administrator of their organization does, leaves it out: naming a device of +/// somebody else here would both hand its identifiers to the push relay under a foreign user id and +/// tell the wrong device to ignore the answer. +pub async fn push_auth_response( + user_id: &UserId, + auth_request_id: &AuthRequestId, + acting_device: Option<&Device>, + conn: &DbConn, +) { if Device::check_user_has_push_device(user_id, conn).await { tokio::task::spawn(send_to_push_relay(json!({ "userId": user_id, "organizationId": null, - "deviceId": device.push_uuid, // Should be the records unique uuid of the acting device (unique uuid per user/device) - "identifier": device.uuid, // Should be the acting device id (aka uuid per device/app) + "deviceId": acting_device.and_then(|device| device.push_uuid.as_ref()), // Should be the records unique uuid of the acting device (unique uuid per user/device) + "identifier": acting_device.map(|device| &device.uuid), // Should be the acting device id (aka uuid per device/app) "type": UpdateType::AuthRequestResponse as i32, "payload": { "userId": user_id, diff --git a/src/auth.rs b/src/auth.rs index 762088e5..6431178f 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -843,6 +843,41 @@ impl<'r> FromRequest<'r> for AdminHeaders { } } +/// A member who may act on the account recovery of an organization, which is also what answering +/// its device approvals comes down to. +/// +/// Upstream guards those endpoints on a permission, `ManageResetPassword`, rather than on a role, +/// so this asks `Membership::has_manage_reset_password_permission` instead of naming roles here. +/// Today that permission belongs to the administrators of an organization and to nobody else, which +/// makes this the same set of callers as `AdminHeaders`; keeping it apart is what lets a custom role +/// hold the permission later without every endpoint having to be revisited. +/// https://github.com/bitwarden/server/blob/main/src/Api/AdminConsole/Controllers/OrganizationAuthRequestsController.cs +pub struct ManageResetPasswordHeaders { + pub device: Device, + pub user: User, + pub ip: ClientIp, + pub org_id: OrganizationId, +} + +#[rocket::async_trait] +impl<'r> FromRequest<'r> for ManageResetPasswordHeaders { + type Error = &'static str; + + async fn from_request(request: &'r Request<'_>) -> Outcome { + let headers = try_outcome!(OrgHeaders::from_request(request).await); + if headers.membership.has_manage_reset_password_permission() { + Outcome::Success(Self { + device: headers.device, + user: headers.user, + ip: headers.ip, + org_id: headers.membership.org_uuid, + }) + } else { + err_handler!("You need permission to manage account recovery 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. // First check the path, if this is not a valid uuid, try the query values. diff --git a/src/db/models/auth_request.rs b/src/db/models/auth_request.rs index bda821fe..d1a1a993 100644 --- a/src/db/models/auth_request.rs +++ b/src/db/models/auth_request.rs @@ -142,8 +142,13 @@ impl AuthRequest { }) } - /// What an administrator gets to see about a request. Deliberately without the access code: - /// that one is the requesting device's proof, not something the answering side needs. + /// What an administrator gets to see about a request that is waiting for them, which is the + /// public key of the asking device and enough about it to recognise it. Same shape as + /// `PendingOrganizationAuthRequestResponseModel` upstream. + /// + /// Deliberately no access code, which is the asking device's own proof, and no wrapped key: a + /// request that is still waiting has none, and handing one out here would be crypto material + /// the answering side has no use for. pub fn to_json_for_organization(&self, email: &str, member_id: &MembershipId) -> Value { json!({ "id": self.uuid, @@ -154,11 +159,10 @@ impl AuthRequest { "requestDeviceIdentifier": self.request_device_identifier, "requestDeviceType": DeviceType::from_i32(self.device_type).to_string(), "requestIpAddress": self.request_ip, - "key": self.enc_key, + // Not recorded here, but the clients read it, so it is answered rather than missing. + "requestCountryName": null, "creationDate": format_date(&self.creation_date), - "requestApproved": self.approved, - "responseDate": self.response_date.as_ref().map(format_date), - "object": "organizationAuthRequest", + "object": "pending-org-auth-request", }) } } @@ -252,12 +256,18 @@ impl AuthRequest { /// /// 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. + /// + /// A request past its window does not count: it is one nobody can answer any more, and reviving + /// it by moving its date forward would leave the user waiting on a request the administrators + /// were never told about. Asking again after it ran out is a new request, and is announced. pub async fn find_pending_admin_approval( user_uuid: &UserId, device_uuid: &DeviceId, org_uuid: &OrganizationId, conn: &DbConn, ) -> Option { + let oldest = Utc::now().naive_utc() - Self::admin_request_expiration(); + conn.run(move |conn| { auth_requests::table .filter(auth_requests::user_uuid.eq(user_uuid)) @@ -265,6 +275,7 @@ impl AuthRequest { .filter(auth_requests::organization_uuid.eq(org_uuid)) .filter(auth_requests::atype.eq(AuthRequestType::AdminApproval as i32)) .filter(auth_requests::approved.is_null()) + .filter(auth_requests::creation_date.gt(oldest)) .order_by(auth_requests::creation_date.desc()) .first::(conn) .ok() @@ -421,6 +432,24 @@ mod tests { assert!(request(AuthRequestType::AdminApproval, TimeDelta::try_days(8).unwrap()).is_expired()); } + #[test] + fn a_request_nobody_answered_in_time_is_not_still_pending() { + // `find_pending_admin_approval` decides whether asking again reuses the open request or + // starts a new one, and filters on the same window as this. A request past it must not come + // back: reviving it by moving its date forward would leave the user waiting on something + // the administrators were never told about, because only a new request mails them. + let mut auth_request = + request(AuthRequestType::AdminApproval, AuthRequest::admin_request_expiration() + TimeDelta::seconds(1)); + assert_eq!(auth_request.approved, None, "still unanswered"); + assert!(auth_request.is_expired()); + + // One minute short of the window is still the same request, and asking again updates it + // rather than mailing everyone a second time. + auth_request.creation_date = + Utc::now().naive_utc() - AuthRequest::admin_request_expiration() + TimeDelta::minutes(1); + assert!(!auth_request.is_expired()); + } + #[test] fn the_answer_of_an_administrator_starts_its_own_clock() { // Answered right at the end of the week, so the request itself is long past its window. diff --git a/src/db/models/device.rs b/src/db/models/device.rs index c0192c8a..2ecb14d2 100644 --- a/src/db/models/device.rs +++ b/src/db/models/device.rs @@ -116,19 +116,6 @@ impl Device { 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; - self.encrypted_private_key = None; - } - pub fn to_json(&self) -> Value { json!({ "id": self.uuid, @@ -264,12 +251,13 @@ 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. 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. + /// Called when the user key itself is replaced and the client did not say what to put in their + /// place, which leaves all of those copies pointing at a 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 { @@ -285,6 +273,82 @@ impl Device { .await } + /// Drops every stored key of the named devices, in one statement so it cannot half apply. + /// + /// The caller has already checked that each id belongs to this user. + pub async fn untrust_many(user_uuid: &UserId, device_ids: Vec, conn: &DbConn) -> EmptyResult { + if device_ids.is_empty() { + return Ok(()); + } + + conn.run(move |conn| { + diesel::update( + devices::table.filter(devices::user_uuid.eq(user_uuid)).filter(devices::uuid.eq_any(device_ids)), + ) + .set(( + devices::encrypted_user_key.eq::>(None), + devices::encrypted_public_key.eq::>(None), + devices::encrypted_private_key.eq::>(None), + )) + .execute(conn) + .map_res("Error untrusting the devices") + }) + .await + } + + /// Replaces the trust of every device of the user in one go: the listed ones are re-wrapped for + /// the current user key, everything else loses whatever it still holds. + /// + /// This is what both a key rotation and `POST /devices/update-trust` come down to. The caller + /// has already checked that every id belongs to this user, that none is listed twice, and that + /// no device is asked to keep a trust it cannot complete; this only writes. + /// + /// One transaction, so the devices cannot be left split between the old and the new user key, + /// which is a state no client can tell apart from a working one until an unlock fails. + pub async fn replace_trust( + user_uuid: &UserId, + updates: Vec<(DeviceId, String, String)>, + conn: &DbConn, + ) -> EmptyResult { + conn.run(move |conn| { + conn.transaction(|conn| -> EmptyResult { + let keep: Vec = updates.iter().map(|(device_id, ..)| device_id.clone()).collect(); + + // Whatever the untouched devices hold wraps the previous user key, or is one half of + // a trust that was never finished. Either way it unlocks nothing and must not stay. + let cleared = ( + devices::encrypted_user_key.eq::>(None), + devices::encrypted_public_key.eq::>(None), + devices::encrypted_private_key.eq::>(None), + ); + let outdated = devices::table.filter(devices::user_uuid.eq(&user_uuid)); + let _: () = if keep.is_empty() { + diesel::update(outdated).set(cleared).execute(conn) + } else { + diesel::update(outdated.filter(devices::uuid.ne_all(keep))).set(cleared).execute(conn) + } + .map_res("Error untrusting the devices left out of the rotation")?; + + // The device key pair is deliberately not touched here: it is wrapped with the + // device key, which the server never sees and a rotation never changes. + for (device_id, encrypted_user_key, encrypted_public_key) in updates { + let _: () = diesel::update( + devices::table.filter(devices::uuid.eq(device_id)).filter(devices::user_uuid.eq(&user_uuid)), + ) + .set(( + devices::encrypted_user_key.eq(Some(encrypted_user_key)), + devices::encrypted_public_key.eq(Some(encrypted_public_key)), + )) + .execute(conn) + .map_res("Error rotating the wrapped user key of a device")?; + } + + Ok(()) + }) + }) + .await + } + pub async fn find_by_uuid_and_user(uuid: &DeviceId, user_uuid: &UserId, conn: &DbConn) -> Option { conn.run(move |conn| { devices::table @@ -550,32 +614,18 @@ mod tests { 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); - } - #[test] fn only_interactive_clients_can_approve_a_login_request() { for atype in 0..=26 { diff --git a/src/db/models/organization.rs b/src/db/models/organization.rs index bdb69864..f9628a62 100644 --- a/src/db/models/organization.rs +++ b/src/db/models/organization.rs @@ -277,6 +277,20 @@ impl Membership { } } + /// Whether this membership may act on the account recovery of the organization's members: + /// reset their master password, and answer the device approvals they ask their organization for. + /// + /// Upstream is a permission of its own, `ManageResetPassword`, which an administrator has by + /// virtue of the role and a custom role can be granted separately. Vaultwarden folds the custom + /// role into `Manager` and drops the permissions that came with it, so only the administrators + /// are left holding it. Asking here rather than comparing roles at each call site keeps that one + /// decision in one place for when custom roles arrive. + /// https://github.com/bitwarden/server/blob/main/src/Core/Context/CurrentContext.cs + pub fn has_manage_reset_password_permission(&self) -> bool { + self.status == MembershipStatus::Confirmed as i32 + && MembershipType::from_i32(self.atype).is_some_and(|atype| atype >= MembershipType::Admin) + } + pub fn restore(&mut self) -> bool { if self.status < MembershipStatus::Invited as i32 { self.status += ACTIVATE_REVOKE_DIFF; @@ -1285,4 +1299,32 @@ mod tests { assert!(MembershipType::Manager > MembershipType::User); assert!(MembershipType::Manager == MembershipType::from_str("4").unwrap()); } + + #[test] + fn only_a_confirmed_administrator_manages_account_recovery() { + let mut membership = Membership::new(String::from("user").into(), String::from("org").into(), None); + + for (atype, expected) in [ + (MembershipType::Owner, true), + (MembershipType::Admin, true), + // The custom role is folded into the manager one, losing whatever permissions came + // with it, so it cannot be assumed to hold this one. + (MembershipType::Manager, false), + (MembershipType::User, false), + ] { + membership.atype = atype as i32; + + for status in [MembershipStatus::Revoked, MembershipStatus::Invited, MembershipStatus::Accepted] { + let status = status as i32; + membership.status = status; + assert!( + !membership.has_manage_reset_password_permission(), + "a membership that is not confirmed manages nothing, status {status}" + ); + } + + membership.status = MembershipStatus::Confirmed as i32; + assert_eq!(membership.has_manage_reset_password_permission(), expected, "type {}", atype as i32); + } + } } diff --git a/src/util.rs b/src/util.rs index b8b86bf6..75eaa0d7 100644 --- a/src/util.rs +++ b/src/util.rs @@ -543,40 +543,81 @@ 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. +/// The most an `EncString` we are willing to store may weigh. +/// +/// Upstream puts no length on the fields this guards; this is ours, so a client cannot park +/// megabytes in a column that is supposed to hold a wrapped key. The largest thing that legitimately +/// lands there is a device's RSA-2048 private key wrapped with AES-CBC plus a MAC, around 1.7 kB, so +/// this leaves room to spare. const MAX_ENC_STRING_LENGTH: usize = 4096; +/// The number of `|` separated parts an `EncString` of the given `EncryptionType` is made of. +/// +/// - `3` Rsa2048_OaepSha256_B64 and `4` Rsa2048_OaepSha1_B64 are the ciphertext by itself, and +/// `7` XChaCha20Poly1305_B64 is one blob of COSE bytes; +/// - `0` AesCbc256_B64 is `iv|ct`, while `5` Rsa2048_OaepSha256_HmacSha256_B64 and +/// `6` Rsa2048_OaepSha1_HmacSha256_B64 are `rsaCt|mac`; +/// - `1` AesCbc128_HmacSha256_B64 and `2` AesCbc256_HmacSha256_B64 are `iv|ct|mac`. +/// +/// https://github.com/bitwarden/server/blob/main/src/Core/Enums/EncryptionType.cs +fn enc_string_parts(enc_type: u8) -> Option { + match enc_type { + 3 | 4 | 7 => Some(1), + 0 | 5 | 6 => Some(2), + 1 | 2 => Some(3), + _ => None, + } +} + +/// Whether a single part is base64, as permissively as upstream reads it. +/// +/// Upstream accepts a final character whose unused padding bits are not zero, because such values +/// exist in the wild; a strict decoder rejects them. Everything else is the usual shape: a multiple +/// of four characters from the base64 alphabet, with at most two `=` closing it off. +/// https://github.com/bitwarden/server/blob/main/src/Core/Utilities/EncryptedStringAttribute.cs +fn is_valid_base64_permissive(value: &str) -> bool { + if value.is_empty() || !value.len().is_multiple_of(4) { + return false; + } + + // A group of four holds at least two characters of data, so at most two may be padded away. + let data = value.strip_suffix("==").or_else(|| value.strip_suffix('=')).unwrap_or(value); + + !data.is_empty() && data.bytes().all(|b| b.is_ascii_alphanumeric() || b == b'+' || b == b'/') +} + /// 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. +/// Mirrors `EncryptedStringAttribute` upstream, including its header-less legacy form, but not its +/// acceptance of a type spelled out by name (`AesCbc256_B64.…`), which no client has ever written. /// 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 (parts, data) = if let Some((enc_type, data)) = value.split_once('.') { + let Some(parts) = enc_type.parse::().ok().and_then(enc_string_parts) else { + return false; + }; + (parts, data) + } else { + // Without a header the type is guessed from the number of parts, the same two candidates + // upstream picks between: three means it carries a MAC, anything else is read as iv|ct. + let parts = if value.matches('|').count() == 2 { + 3 + } else { + 2 + }; + (parts, value) }; 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() { + if seen > parts || !is_valid_base64_permissive(part) { return false; } } @@ -591,18 +632,33 @@ mod enc_string_tests { #[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", + "0.aXY=|Y2lwaGVy", // AesCbc256_B64 + "1.aXY=|Y2lwaGVy|bWFj", // AesCbc128_HmacSha256_B64 + "2.aXY=|Y2lwaGVy|bWFj", // AesCbc256_HmacSha256_B64 + "3.Y2lwaGVy", // Rsa2048_OaepSha256_B64 + "4.Y2lwaGVy", // Rsa2048_OaepSha1_B64 + "5.Y2lwaGVy|bWFj", // Rsa2048_OaepSha256_HmacSha256_B64 + "6.Y2lwaGVy|bWFj", // Rsa2048_OaepSha1_HmacSha256_B64 + "7.Y29zZWJ5dGVz", // XChaCha20Poly1305_B64, one blob of COSE bytes + "07.Y29zZWJ5dGVz", // the header is read as a number, not matched as text + "aXY=|Y2lwaGVy", // header-less legacy form, read as iv|ct + "aXY=|Y2lwaGVy|bWFj", // and as iv|ct|mac when it has three parts + "3.Y2lwaGVyLysvdGV4dA==", // the whole base64 alphabet, padded + "3.Y2lwaGVyLysvdGV4dGE=", // and with a single pad character ] { assert!(is_valid_enc_string(value), "{value}"); } } + #[test] + fn a_non_canonical_final_character_is_accepted() { + // The unused padding bits of the last character are not zero. A strict decoder refuses + // these, upstream deliberately does not, and such values exist in the wild. + assert!(is_valid_enc_string("3.QR==")); + assert!(is_valid_enc_string("3.QUJDRR==")); + assert!(is_valid_enc_string("2.aXY=|QR==|bWFj")); + } + #[test] fn anything_that_is_not_one_is_refused() { for value in [ @@ -612,13 +668,24 @@ mod enc_string_tests { "2", "2.", ".aXY=|Y2lwaGVy|bWFj", - "7.Y2lwaGVy", // no such type + "8.Y2lwaGVy", // no such type + "255.Y2lwaGVy", // nor at the top of the byte the header is read as + "256.Y2lwaGVy", // nor past it "-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 + "7.Y29zZQ==|bWFj", // and neither does type 7 "2.aXY=||bWFj", // an empty part is not base64 "4.not base64!", + "4.Y2lwaGV", // a length that is not a multiple of four + "4.Y2lwaGVy=", // a stray pad character breaks that length + "4.====", // padding only + "4.Y2lw=GVy", // padding in the middle + "4.Y2lw-GVy", // url-safe base64 is a different alphabet + "4.Y2lw GVy", // whitespace is not part of it either + "aXY=", // header-less, but only one part + "aXY=|Y2lwaGVy|bWFj|Zm91cg==", // header-less with four ] { assert!(!is_valid_enc_string(value), "{value}"); } @@ -629,6 +696,12 @@ mod enc_string_tests { 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"); + + // A wrapped RSA-2048 device private key, the largest thing that legitimately arrives here, + // has to fit with room to spare. + let private_key = format!("2.{}|{}|{}", "A".repeat(24), "B".repeat(1652), "C".repeat(44)); + assert!(private_key.len() < 2048); + assert!(is_valid_enc_string(&private_key)); } }