Browse Source

Harden the trusted device flow

Findings from a review of the earlier commits.

A key rotation dropped the device key pairs along with the wrapped user
keys, so the devices listed in `/devices/update-trust` could never be
trusted again and silently lost their trust on every rotation. Those key
pairs are wrapped with the device key, which a rotation does not touch, so
they now stay and the re-wrap works. The invalidation also moved ahead of
the new user key: failing after it had been written left devices handing
out a key that no longer opens the vault.

The three key blobs are now checked against the shape of an `EncString`
instead of only for emptiness, as upstream does, which also keeps
multi-megabyte values out of those columns.

`/auth-requests/admin-request` is rate limited and reuses the open request
of a device instead of adding one per attempt, which mailed every
administrator again each time. Only confirmed memberships are asked, and
answered: an invitation that was never accepted is not a membership yet, a
revoked one is not one anymore, and neither should learn the address, the
IP and the device of the asker. `UserDecryptionOptions` reports the same
condition, so no way out is announced that would be refused when taken.

The bulk endpoints skip what they cannot answer rather than failing the
whole call after having answered everything before it, matching upstream,
and are bounded. The purge does its work per type in the database instead
of reading the table and deleting row by row, with indexes to go with it.

Also: approving an auth request is limited to the newest one of a device,
`GET /auth-requests/<id>` refuses an expired request like the anonymous
lookup already did, the approval notification addresses the device that
asked rather than the administrator's, and accepting an invitation while
enrolling is tied to the trusted device flow it was meant for.
pull/7534/head
tom27052006 2 weeks ago
parent
commit
054a249004
  1. 10
      .env.template
  2. 2
      migrations/mysql/2026-08-01-120000_add_auth_request_indexes/down.sql
  3. 2
      migrations/mysql/2026-08-01-120000_add_auth_request_indexes/up.sql
  4. 2
      migrations/postgresql/2026-08-01-120000_add_auth_request_indexes/down.sql
  5. 2
      migrations/postgresql/2026-08-01-120000_add_auth_request_indexes/up.sql
  6. 2
      migrations/sqlite/2026-08-01-120000_add_auth_request_indexes/down.sql
  7. 2
      migrations/sqlite/2026-08-01-120000_add_auth_request_indexes/up.sql
  8. 153
      src/api/core/accounts.rs
  9. 118
      src/api/core/organizations.rs
  10. 15
      src/api/identity.rs
  11. 6
      src/config.rs
  12. 92
      src/db/models/auth_request.rs
  13. 65
      src/db/models/device.rs
  14. 4
      src/mail.rs
  15. 2
      src/static/templates/email/device_approval_requested.hbs
  16. 2
      src/static/templates/email/device_approval_requested.html.hbs
  17. 2
      src/static/templates/email/trusted_device_admin_approval.hbs
  18. 2
      src/static/templates/email/trusted_device_admin_approval.html.hbs
  19. 89
      src/util.rs

10
.env.template

@ -552,9 +552,13 @@
## Trusted device encryption ("passwordless SSO"), see https://bitwarden.com/help/login-with-sso-trusted-devices/ ## 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 ## 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 ## 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. ## is unlocked by approving it from an already trusted one, by asking an administrator of the
## WARNING: a user who never sets a master password and then loses every trusted device cannot ## organization (which needs the member enrolled into account recovery), or with the master password.
## recover their vault. This server does not implement the admin approval flow to fall back on. ## 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 # SSO_TRUSTED_DEVICE_ENCRYPTION=false
######################## ########################

2
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;

2
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);

2
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;

2
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);

2
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;

2
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);

153
src/api/core/accounts.rs

@ -22,8 +22,8 @@ use crate::{
models::{ models::{
AuthRequest, AuthRequestId, AuthRequestType, Cipher, CipherId, Device, DeviceId, DeviceType, AuthRequest, AuthRequestId, AuthRequestType, Cipher, CipherId, Device, DeviceId, DeviceType,
DeviceWithAuthRequest, EmergencyAccess, EmergencyAccessId, EventType, Folder, FolderId, Invitation, DeviceWithAuthRequest, EmergencyAccess, EmergencyAccessId, EventType, Folder, FolderId, Invitation,
Membership, MembershipId, MembershipType, OrgPolicy, OrgPolicyType, Organization, OrganizationId, Send, Membership, MembershipId, MembershipStatus, MembershipType, OrgPolicy, OrgPolicyType, Organization,
SendId, User, UserId, UserKdfType, OrganizationId, Send, SendId, User, UserId, UserKdfType,
}, },
}, },
mail, mail,
@ -1022,6 +1022,14 @@ async fn post_rotatekey(data: Json<KeyData>, 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 // Update user data
let mut user = headers.user; let mut user = headers.user;
@ -1037,12 +1045,6 @@ async fn post_rotatekey(data: Json<KeyData>, headers: Headers, conn: DbConn, nt:
let save_result = user.save(&conn).await; 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. // 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. // If you do logout the user it will causes issues at the client side.
// Adding the device uuid will prevent this. // Adding the device uuid will prevent this.
@ -1608,6 +1610,20 @@ struct TrustedDeviceKeysData {
encrypted_private_key: String, 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. /// 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 /// 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 { ) -> JsonResult {
let data = data.into_inner(); let data = data.into_inner();
if data.encrypted_user_key.is_empty() validate_enc_strings(&[
|| data.encrypted_public_key.is_empty() ("encryptedUserKey", &data.encrypted_user_key),
|| data.encrypted_private_key.is_empty() ("encryptedPublicKey", &data.encrypted_public_key),
{ ("encryptedPrivateKey", &data.encrypted_private_key),
err!("All three device keys are required to trust a device") ])?;
}
let Some(mut device) = Device::find_by_uuid_and_user(&device_id, &headers.user.uuid, &conn).await else { let Some(mut device) = Device::find_by_uuid_and_user(&device_id, &headers.user.uuid, &conn).await else {
err!("No device found") err!("No device found")
@ -1698,18 +1713,20 @@ async fn post_devices_update_trust(data: Json<UpdateDevicesTrustData>, headers:
data.secret.validate(&headers.user, true, &conn).await?; 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() { validate_enc_strings(&[
err!("The keys of the current device are required") ("encryptedUserKey", &data.current_device.encrypted_user_key),
} ("encryptedPublicKey", &data.current_device.encrypted_public_key),
])?;
let mut updates: HashMap<DeviceId, DeviceTrustUpdateData> = HashMap::new(); let mut updates: HashMap<DeviceId, DeviceTrustUpdateData> = HashMap::new();
for other in data.other_devices { for other in data.other_devices {
if other.device_id == headers.device.uuid { if other.device_id == headers.device.uuid {
err!("The current device cannot also be part of the optional rotation") 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() { validate_enc_strings(&[
err!("Both keys are required for every device in the rotation") ("encryptedUserKey", &other.keys.encrypted_user_key),
} ("encryptedPublicKey", &other.keys.encrypted_public_key),
])?;
if updates.insert(other.device_id, other.keys).is_some() { if updates.insert(other.device_id, other.keys).is_some() {
err!("A device was listed more than once in the rotation") err!("A device was listed more than once in the rotation")
} }
@ -1730,15 +1747,20 @@ async fn post_devices_update_trust(data: Json<UpdateDevicesTrustData>, headers:
if device.uuid == headers.device.uuid { if device.uuid == headers.device.uuid {
device.encrypted_user_key = Some(data.current_device.encrypted_user_key.clone()); device.encrypted_user_key = Some(data.current_device.encrypted_user_key.clone());
device.encrypted_public_key = Some(data.current_device.encrypted_public_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) { } 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_user_key = Some(keys.encrypted_user_key);
device.encrypted_public_key = Some(keys.encrypted_public_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(); device.untrust();
} else {
continue;
} }
device.save(true, &conn).await?; 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 /// https://github.com/bitwarden/server/blob/main/src/Api/Auth/Controllers/AuthRequestsController.cs
#[post("/auth-requests/admin-request", data = "<data>")] #[post("/auth-requests/admin-request", data = "<data>")]
async fn post_admin_auth_request(data: Json<AuthRequestRequest>, headers: Headers, conn: DbConn) -> JsonResult { async fn post_admin_auth_request(data: Json<AuthRequestRequest>, 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(); let data = data.into_inner();
if AuthRequestType::from_i32(data.atype) != Some(AuthRequestType::AdminApproval) { if AuthRequestType::from_i32(data.atype) != Some(AuthRequestType::AdminApproval) {
@ -1902,9 +1928,16 @@ async fn post_admin_auth_request(data: Json<AuthRequestRequest>, headers: Header
err!("AuthRequest doesn't exist", "Device verification failed") 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> = Membership::find_by_user(&headers.user.uuid, &conn)
.await
.into_iter()
.filter(|membership| membership.status == MembershipStatus::Confirmed as i32)
.collect();
if memberships.is_empty() { 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( log_user_event(
@ -1918,19 +1951,42 @@ async fn post_admin_auth_request(data: Json<AuthRequestRequest>, headers: Header
let mut first_request = None; let mut first_request = None;
for membership in memberships { for membership in memberships {
let mut auth_request = AuthRequest::new( // Asking again from the same device replaces the open request instead of adding one, so a
headers.user.uuid.clone(), // client that retries does not pile up rows and does not mail the administrators twice.
Some(membership.org_uuid.clone()), let existing = AuthRequest::find_pending_admin_approval(
AuthRequestType::AdminApproval, &headers.user.uuid,
data.device_identifier.clone(), &data.device_identifier,
headers.device.atype, &membership.org_uuid,
headers.ip.ip.to_string(), &conn,
data.access_code.clone(), )
data.public_key.clone(), .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?; 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() { if first_request.is_none() {
first_request = Some(auth_request); 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") 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))) Ok(Json(auth_request_json(&auth_request)))
} }
@ -2022,6 +2084,21 @@ async fn put_auth_request(
err!("AuthRequest doesn't exist", "Request has expired") 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(); let response_date = Utc::now().naive_utc();
if data.request_approved { if data.request_approved {

118
src/api/core/organizations.rs

@ -17,8 +17,8 @@ use crate::{
DbConn, DbConn,
models::{ models::{
AuthRequest, AuthRequestId, Cipher, CipherId, Collection, CollectionCipher, CollectionGroup, CollectionId, AuthRequest, AuthRequestId, Cipher, CipherId, Collection, CollectionCipher, CollectionGroup, CollectionId,
CollectionUser, DeviceType, EventType, Group, GroupId, GroupUser, Invitation, Membership, MembershipId, CollectionUser, Device, DeviceType, EventType, Group, GroupId, GroupUser, Invitation, Membership,
MembershipStatus, MembershipType, OrgPolicy, OrgPolicyType, Organization, OrganizationApiKey, MembershipId, MembershipStatus, MembershipType, OrgPolicy, OrgPolicyType, Organization, OrganizationApiKey,
OrganizationId, User, UserId, 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 // 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 // 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. // 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?; accept_org_invite(&headers.user, membership, reset_password_key, &conn).await?;
} else { } else {
membership.reset_password_key = reset_password_key; membership.reset_password_key = reset_password_key;
@ -3219,14 +3225,18 @@ async fn get_organization_auth_requests(org_id: OrganizationId, headers: AdminHe
continue; continue;
} }
// A request whose asker is no longer a member of this organization is none of its business // A request whose asker is not a confirmed member of this organization is none of its
// anymore, so it is quietly left out instead of being offered for approval. // 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)) = ( let (Some(member), Some(user)) = (
Membership::find_by_user_and_org(&auth_request.user_uuid, &org_id, &conn).await, Membership::find_by_user_and_org(&auth_request.user_uuid, &org_id, &conn).await,
User::find_by_uuid(&auth_request.user_uuid, &conn).await, User::find_by_uuid(&auth_request.user_uuid, &conn).await,
) else { ) else {
continue; continue;
}; };
if member.status != MembershipStatus::Confirmed as i32 {
continue;
}
requests.push(auth_request.to_json_for_organization(&user.email, &member.uuid)); requests.push(auth_request.to_json_for_organization(&user.email, &member.uuid));
} }
@ -3259,6 +3269,22 @@ struct OrganizationAuthRequestUpdateData {
key: Option<String>, key: Option<String>,
} }
/// 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/<org_id>/auth-requests/<request_id>", data = "<data>", rank = 2)] #[post("/organizations/<org_id>/auth-requests/<request_id>", data = "<data>", rank = 2)]
async fn update_organization_auth_request( async fn update_organization_auth_request(
org_id: OrganizationId, org_id: OrganizationId,
@ -3275,6 +3301,7 @@ async fn update_organization_auth_request(
&request_id, &request_id,
data.request_approved, data.request_approved,
data.encrypted_user_key, data.encrypted_user_key,
OnUnanswerable::Fail,
&headers, &headers,
&conn, &conn,
&ant, &ant,
@ -3292,8 +3319,24 @@ async fn deny_organization_auth_requests(
ant: AnonymousNotify<'_>, ant: AnonymousNotify<'_>,
nt: Notify<'_>, nt: Notify<'_>,
) -> EmptyResult { ) -> EmptyResult {
for request_id in data.into_inner().ids { let ids = data.into_inner().ids;
answer_organization_auth_request(&org_id, &request_id, false, None, &headers, &conn, &ant, &nt).await?; 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(()) Ok(())
@ -3308,9 +3351,24 @@ async fn update_many_organization_auth_requests(
ant: AnonymousNotify<'_>, ant: AnonymousNotify<'_>,
nt: Notify<'_>, nt: Notify<'_>,
) -> EmptyResult { ) -> EmptyResult {
for update in data.into_inner() { let updates = data.into_inner();
answer_organization_auth_request(&org_id, &update.id, update.approved, update.key, &headers, &conn, &ant, &nt) if updates.len() > MAX_BULK_AUTH_REQUESTS {
.await?; 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(()) Ok(())
@ -3322,6 +3380,7 @@ async fn answer_organization_auth_request(
request_id: &AuthRequestId, request_id: &AuthRequestId,
approved: bool, approved: bool,
encrypted_user_key: Option<String>, encrypted_user_key: Option<String>,
on_unanswerable: OnUnanswerable,
headers: &AdminHeaders, headers: &AdminHeaders,
conn: &DbConn, conn: &DbConn,
ant: &AnonymousNotify<'_>, ant: &AnonymousNotify<'_>,
@ -3331,30 +3390,47 @@ async fn answer_organization_auth_request(
err!("Organization not found", "Organization id's do not match"); 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 // Only ever reachable through the organization it was addressed to, so an administrator cannot
// answer for an organization they have no say in. // 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 let Some(mut auth_request) = AuthRequest::find_admin_approval_by_org_and_uuid(request_id, org_id, conn).await
else { 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() { 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() { 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 { // Answering means acting for a member of this organization, so it has to be one: an invitation
err!("AuthRequest doesn't exist", "The requesting user is no longer a member of this organization") // 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 { if approved {
// Without the wrapped user key the answer is worthless: it is the whole point of approving. // 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 { 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); 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; 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() if CONFIG.mail_enabled()
&& let Some(user) = User::find_by_uuid(&auth_request.user_uuid, conn).await && let Some(user) = User::find_by_uuid(&auth_request.user_uuid, conn).await

15
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()); .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, // 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. // which is what enrolling into account recovery does. Only a confirmed membership counts, the
let has_admin_approval = // same condition the request itself is created and answered under, so this does not announce a
memberships.iter().any(|member| member.reset_password_key.as_ref().is_some_and(|key| !key.is_empty())); // 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 // 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| { 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!({ Some(json!({

6
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_internal_sso_redirect_url(&cfg.sso_callback_path)?;
validate_sso_master_password_policy(cfg.sso_master_password_policy.as_ref())?; validate_sso_master_password_policy(cfg.sso_master_password_policy.as_ref())?;
} else if cfg.sso_trusted_device_encryption { } 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 { if cfg._enable_yubico {

92
src/db/models/auth_request.rs

@ -237,6 +237,30 @@ impl AuthRequest {
.await .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<Self> {
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::<Self>(conn)
.ok()
})
.await
}
/// Everything an administrator of this organization still has to answer. /// 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<Self> { pub async fn find_pending_admin_approval_by_org(org_uuid: &OrganizationId, conn: &DbConn) -> Vec<Self> {
conn.run(move |conn| { conn.run(move |conn| {
@ -269,16 +293,6 @@ impl AuthRequest {
.await .await
} }
pub async fn find_created_before(dt: &NaiveDateTime, conn: &DbConn) -> Vec<Self> {
conn.run(move |conn| {
auth_requests::table
.filter(auth_requests::creation_date.lt(dt))
.load::<Self>(conn)
.expect("Error loading auth_requests")
})
.await
}
pub async fn delete(&self, conn: &DbConn) -> EmptyResult { pub async fn delete(&self, conn: &DbConn) -> EmptyResult {
conn.run(move |conn| { conn.run(move |conn| {
diesel::delete(auth_requests::table.filter(auth_requests::uuid.eq(&self.uuid))) 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) 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) { 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 let now = Utc::now().naive_utc();
// Nothing can be expired before the shortest window has passed, so that is the cheapest let admin = AuthRequestType::AdminApproval as i32;
// 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 between_devices = now - Self::user_request_expiration();
let candidates = Utc::now().naive_utc() - Self::user_request_expiration(); let for_an_admin = now - Self::admin_request_expiration();
for auth_request in Self::find_created_before(&candidates, conn).await { let after_approval = now - Self::after_admin_approval_expiration();
if auth_request.is_expired() {
auth_request.delete(conn).await.ok(); 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:#?}");
} }
} }
} }

65
src/db/models/device.rs

@ -107,6 +107,22 @@ impl Device {
self.is_trusted().then_some(self.encrypted_private_key.as_ref()).flatten() 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) { pub fn untrust(&mut self) {
self.encrypted_user_key = None; self.encrypted_user_key = None;
self.encrypted_public_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. /// 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 /// 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 /// key that no longer unlocks anything. No device counts as trusted afterwards, so a client
/// rotation) keeps its own private key, so its client can immediately re-wrap the new user key /// that stops here ends up with an extra login rather than a broken unlock. The device key
/// via `POST /devices/update-trust`; every other device is untrusted outright. Until that /// pairs are deliberately left alone: they are wrapped with the device key, which a rotation
/// happens no device counts as trusted, so the worst case is an extra login, not a broken vault. /// does not touch, so `POST /devices/update-trust` can hand every device the new user key and
pub async fn invalidate_wrapped_user_keys( /// restore its trust. Whatever it does not list is dropped there.
user_uuid: &UserId, ///
keep_private_key_for: &DeviceId, /// One statement, so there is no half applied state to reason about.
conn: &DbConn, pub async fn invalidate_wrapped_user_keys(user_uuid: &UserId, conn: &DbConn) -> EmptyResult {
) -> EmptyResult {
conn.run(move |conn| { 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::<Option<String>>(None))
.execute(conn)
.map_res("Error untrusting the devices")?;
diesel::update(devices::table.filter(devices::user_uuid.eq(user_uuid))) diesel::update(devices::table.filter(devices::user_uuid.eq(user_uuid)))
.set(( .set((
devices::encrypted_user_key.eq::<Option<String>>(None), devices::encrypted_user_key.eq::<Option<String>>(None),
@ -532,12 +540,37 @@ mod tests {
assert_eq!(device.trusted_private_key(), None); 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] #[test]
fn untrusting_clears_every_key() { fn untrusting_clears_every_key() {
let mut device = trusted_device(); let mut device = trusted_device();
device.untrust(); device.untrust();
assert!(!device.is_trusted()); assert!(!device.is_trusted());
assert!(!device.holds_any_key());
assert_eq!(device.encrypted_user_key, None); assert_eq!(device.encrypted_user_key, None);
assert_eq!(device.encrypted_public_key, None); assert_eq!(device.encrypted_public_key, None);
assert_eq!(device.encrypted_private_key, None); assert_eq!(device.encrypted_private_key, None);

4
src/mail.rs

@ -543,7 +543,9 @@ pub async fn send_device_approval_requested(
let (subject, body_html, body_text) = get_text( let (subject, body_html, body_text) = get_text(
"email/device_approval_requested", "email/device_approval_requested",
json!({ 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(), "img_src": CONFIG._smtp_img_src(),
"org_name": org_name, "org_name": org_name,
"user_email": user_email, "user_email": user_email,

2
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. {{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 }} {{> email/email_footer_text }}

2
src/static/templates/email/device_approval_requested.html.hbs

@ -9,7 +9,7 @@ Device Approval Requested
</tr> </tr>
<tr style="margin: 0; font-family: 'Helvetica Neue', Helvetica, Arial, sans-serif; box-sizing: border-box; font-size: 16px; color: #333; line-height: 25px; -webkit-font-smoothing: antialiased; -webkit-text-size-adjust: none;"> <tr style="margin: 0; font-family: 'Helvetica Neue', Helvetica, Arial, sans-serif; box-sizing: border-box; font-size: 16px; color: #333; line-height: 25px; -webkit-font-smoothing: antialiased; -webkit-text-size-adjust: none;">
<td class="content-block" style="font-family: 'Helvetica Neue', Helvetica, Arial, sans-serif; box-sizing: border-box; font-size: 16px; color: #333; line-height: 25px; margin: 0; -webkit-font-smoothing: antialiased; padding: 0 0 10px; -webkit-text-size-adjust: none;" valign="top"> <td class="content-block" style="font-family: 'Helvetica Neue', Helvetica, Arial, sans-serif; box-sizing: border-box; font-size: 16px; color: #333; line-height: 25px; margin: 0; -webkit-font-smoothing: antialiased; padding: 0 0 10px; -webkit-text-size-adjust: none;" valign="top">
Review the request in the organization administration of <a href="{{{url}}}" style="margin: 0; font-family: 'Helvetica Neue', Helvetica, Arial, sans-serif; box-sizing: border-box; font-size: 16px; color: #175DDC; line-height: 25px; -webkit-font-smoothing: antialiased; text-decoration: underline; -webkit-text-size-adjust: none;">{{{url}}}</a>. Review the request at <a href="{{{url}}}" style="margin: 0; font-family: 'Helvetica Neue', Helvetica, Arial, sans-serif; box-sizing: border-box; font-size: 16px; color: #175DDC; line-height: 25px; -webkit-font-smoothing: antialiased; text-decoration: underline; -webkit-text-size-adjust: none;">{{{url}}}</a>.
</td> </td>
</tr> </tr>
</table> </table>

2
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}} Device: {{device}}
IP address: {{ip}} 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 }} {{> email/email_footer_text }}

2
src/static/templates/email/trusted_device_admin_approval.html.hbs

@ -15,7 +15,7 @@ Device Approved
</tr> </tr>
<tr style="margin: 0; font-family: 'Helvetica Neue', Helvetica, Arial, sans-serif; box-sizing: border-box; font-size: 16px; color: #333; line-height: 25px; -webkit-font-smoothing: antialiased; -webkit-text-size-adjust: none;"> <tr style="margin: 0; font-family: 'Helvetica Neue', Helvetica, Arial, sans-serif; box-sizing: border-box; font-size: 16px; color: #333; line-height: 25px; -webkit-font-smoothing: antialiased; -webkit-text-size-adjust: none;">
<td class="content-block" style="font-family: 'Helvetica Neue', Helvetica, Arial, sans-serif; box-sizing: border-box; font-size: 16px; color: #333; line-height: 25px; margin: 0; -webkit-font-smoothing: antialiased; padding: 0 0 10px; -webkit-text-size-adjust: none;" valign="top"> <td class="content-block" style="font-family: 'Helvetica Neue', Helvetica, Arial, sans-serif; box-sizing: border-box; font-size: 16px; color: #333; line-height: 25px; margin: 0; -webkit-font-smoothing: antialiased; padding: 0 0 10px; -webkit-text-size-adjust: none;" valign="top">
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.
</td> </td>
</tr> </tr>
</table> </table>

89
src/util.rs

@ -505,6 +505,95 @@ pub fn is_valid_email(email: &str) -> bool {
true 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`: `<type>.<part>|<part>...`.
///
/// 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 // Deployment environment methods
// //

Loading…
Cancel
Save