From c2a324e5da24bf2b59b5ccb745335783b4ea633f Mon Sep 17 00:00:00 2001 From: Jeremy Lin Date: Thu, 9 Apr 2020 01:42:27 -0700 Subject: [PATCH 1/3] Clean up domain whitelist logic * Make `SIGNUPS_DOMAINS_WHITELIST` override the `SIGNUPS_ALLOWED` setting. Otherwise, a common pitfall is to set `SIGNUPS_DOMAINS_WHITELIST` without realizing that `SIGNUPS_ALLOWED=false` must also be set. * Whitespace is now accepted in `SIGNUPS_DOMAINS_WHITELIST`. That is, `foo.com, bar.com` is now equivalent to `foo.com,bar.com`. * Add validation on `SIGNUPS_DOMAINS_WHITELIST`. For example, `foo.com,` is rejected as containing an empty token. --- src/api/core/accounts.rs | 11 +++++++---- src/config.rs | 36 +++++++++++++++++++++++++++--------- 2 files changed, 34 insertions(+), 13 deletions(-) diff --git a/src/api/core/accounts.rs b/src/api/core/accounts.rs index 39b4ae86..13f3e810 100644 --- a/src/api/core/accounts.rs +++ b/src/api/core/accounts.rs @@ -68,7 +68,7 @@ fn register(data: JsonUpcase, conn: DbConn) -> EmptyResult { let mut user = match User::find_by_mail(&data.Email, &conn) { Some(user) => { if !user.password_hash.is_empty() { - if CONFIG.signups_allowed() { + if CONFIG.is_signup_allowed(&data.Email) { err!("User already exists") } else { err!("Registration not allowed or user already exists") @@ -89,14 +89,17 @@ fn register(data: JsonUpcase, conn: DbConn) -> EmptyResult { } user - } else if CONFIG.signups_allowed() { + } else if CONFIG.is_signup_allowed(&data.Email) { err!("Account with this email already exists") } else { err!("Registration not allowed or user already exists") } } None => { - if CONFIG.signups_allowed() || Invitation::take(&data.Email, &conn) || CONFIG.can_signup_user(&data.Email) { + // Order is important here; the invitation check must come first + // because the bitwarden_rs admin can invite anyone, regardless + // of other signup restrictions. + if Invitation::take(&data.Email, &conn) || CONFIG.is_signup_allowed(&data.Email) { User::new(data.Email.clone()) } else { err!("Registration not allowed or user already exists") @@ -371,7 +374,7 @@ fn post_email_token(data: JsonUpcase, headers: Headers, conn: Db err!("Email already in use"); } - if !CONFIG.signups_allowed() && !CONFIG.can_signup_user(&data.NewEmail) { + if !CONFIG.is_signup_allowed(&data.NewEmail) { err!("Email cannot be changed to this address"); } diff --git a/src/config.rs b/src/config.rs index 0d79b215..d72ade30 100644 --- a/src/config.rs +++ b/src/config.rs @@ -112,6 +112,8 @@ macro_rules! make_config { )+)+ config.domain_set = _domain_set; + config.signups_domains_whitelist = config.signups_domains_whitelist.trim().to_lowercase(); + config } } @@ -263,7 +265,7 @@ make_config! { /// $ICON_CACHE_FOLDER, but it won't produce any external network request. Needs to set $ICON_CACHE_TTL to 0, /// otherwise it will delete them and they won't be downloaded again. disable_icon_download: bool, true, def, false; - /// Allow new signups |> Controls if new users can register. Note that while this is disabled, users could still be invited + /// Allow new signups |> Controls whether new users can register. Users can be invited by the bitwarden_rs admin even if this is disabled signups_allowed: bool, true, def, true; /// Require email verification on signups. This will prevent logins from succeeding until the address has been verified signups_verify: bool, true, def, false; @@ -271,9 +273,9 @@ make_config! { signups_verify_resend_time: u64, true, def, 3_600; /// If signups require email verification, limit how many emails are automatically sent when login is attempted (0 means no limit) signups_verify_resend_limit: u32, true, def, 6; - /// Allow signups only from this list of comma-separated domains + /// Email domain whitelist |> Allow signups only from this list of comma-separated domains, even when signups are otherwise disabled signups_domains_whitelist: String, true, def, "".to_string(); - /// Allow invitations |> Controls whether users can be invited by organization admins, even when signups are disabled + /// Allow invitations |> Controls whether users can be invited by organization admins, even when signups are otherwise disabled invitations_allowed: bool, true, def, true; /// Password iterations |> Number of server-side passwords hashing iterations. /// The changes only apply when a user changes their password. Not recommended to lower the value @@ -428,6 +430,11 @@ fn validate_config(cfg: &ConfigItems) -> Result<(), Error> { err!("DOMAIN variable needs to contain the protocol (http, https). Use 'http[s]://bw.example.com' instead of 'bw.example.com'"); } + let whitelist = &cfg.signups_domains_whitelist; + if !whitelist.is_empty() && whitelist.split(',').any(|d| d.trim().is_empty()) { + err!("`SIGNUPS_DOMAINS_WHITELIST` contains empty tokens"); + } + if let Some(ref token) = cfg.admin_token { if token.trim().is_empty() && !cfg.disable_admin_token { err!("`ADMIN_TOKEN` is enabled but has an empty value. To enable the admin page without token, use `DISABLE_ADMIN_TOKEN`") @@ -551,18 +558,29 @@ impl Config { self.update_config(builder) } - pub fn can_signup_user(&self, email: &str) -> bool { + /// Tests whether an email's domain is in signups_domains_whitelist. + /// Returns false if no whitelist is set. + pub fn is_email_domain_whitelisted(&self, email: &str) -> bool { let e: Vec<&str> = email.rsplitn(2, '@').collect(); if e.len() != 2 || e[0].is_empty() || e[1].is_empty() { warn!("Failed to parse email address '{}'", email); return false; } + let email_domain = e[0]; + let whitelist = self.signups_domains_whitelist(); - // Allow signups if the whitelist is empty/not configured - // (it doesn't contain any domains), or if it matches at least - // one domain. - let whitelist_str = self.signups_domains_whitelist(); - ( whitelist_str.is_empty() && CONFIG.signups_allowed() )|| whitelist_str.split(',').filter(|s| !s.is_empty()).any(|d| d == e[0]) + !whitelist.is_empty() && whitelist.split(',').any(|d| d.trim() == email_domain) + } + + /// Tests whether signup is allowed for an email address, taking into + /// account the signups_allowed and signups_domains_whitelist settings. + pub fn is_signup_allowed(&self, email: &str) -> bool { + if !self.signups_domains_whitelist().is_empty() { + // The whitelist setting overrides the signups_allowed setting. + self.is_email_domain_whitelisted(email) + } else { + self.signups_allowed() + } } pub fn delete_user_config(&self) -> Result<(), Error> { From e4d08836e2ccc8bd4f1b926f306aa881f26a33d8 Mon Sep 17 00:00:00 2001 From: Jeremy Lin Date: Thu, 9 Apr 2020 01:51:05 -0700 Subject: [PATCH 2/3] Make org owner invitations respect the email domain whitelist This closes a loophole where org owners can invite new users from any domain. --- src/api/core/organizations.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index 5c11b265..cdbaebd0 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -485,7 +485,11 @@ fn send_invite(org_id: String, data: JsonUpcase, headers: AdminHeade let user = match User::find_by_mail(&email, &conn) { None => { if !CONFIG.invitations_allowed() { - err!(format!("User email does not exist: {}", email)) + err!(format!("User does not exist: {}", email)) + } + + if !CONFIG.signups_domains_whitelist().is_empty() && !CONFIG.is_email_domain_whitelisted(&email) { + err!("Email domain not eligible for invitations") } if !CONFIG.mail_enabled() { @@ -978,4 +982,4 @@ fn put_policy(org_id: String, pol_type: i32, data: Json, _headers: A policy.save(&conn)?; Ok(Json(policy.to_json())) -} \ No newline at end of file +} From 86685c1cd2f8d1d8771bcd97d5dd5aa3c3efd4b9 Mon Sep 17 00:00:00 2001 From: Jeremy Lin Date: Sat, 11 Apr 2020 14:51:36 -0700 Subject: [PATCH 3/3] Ensure email domain comparison is case-insensitive --- src/config.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config.rs b/src/config.rs index d72ade30..bfb91828 100644 --- a/src/config.rs +++ b/src/config.rs @@ -566,7 +566,7 @@ impl Config { warn!("Failed to parse email address '{}'", email); return false; } - let email_domain = e[0]; + let email_domain = e[0].to_lowercase(); let whitelist = self.signups_domains_whitelist(); !whitelist.is_empty() && whitelist.split(',').any(|d| d.trim() == email_domain)