From 08fb368d5f8786be198d758dd5fe1df44402809a Mon Sep 17 00:00:00 2001 From: BlackDex Date: Fri, 24 Jan 2025 10:35:47 +0100 Subject: [PATCH] Security fixes for admin and sendmail Because the Vaultwarden Admin Backend endpoints did not validated the Content-Type during a request, it was possible to update settings via CSRF. But, this was only possible if there was no `ADMIN_TOKEN` set at all. To make sure these environments are also safe I added the needed content-type checks at the functions. This could cause some users who have scripts which uses cURL for example to adjust there commands to provide the correct headers. By using a crafted favicon and having access to the Admin Backend an attacker could run custom commands on the host/container where Vaultwarden is running on. The main issue here is that we allowed the sendmail binary name/path to be changed. To mitigate this we removed this configuration item and only then `sendmail` binary as a name can be used. This could cause some issues where the `sendmail` binary is not in the `$PATH` and thus not able to be started. In these cases the admins should make sure `$PATH` is set correctly or create a custom shell script or symlink at a location which is in the `$PATH`. Added an extra security header and adjusted the CSP to be more strict by setting `default-src` to `none` and added the needed missing specific policies. Also created a general email validation function which does some more checking to catch invalid email address not found by the email_address crate. Signed-off-by: BlackDex --- .env.template | 2 -- src/api/admin.rs | 32 +++++++++++------------ src/config.rs | 34 +++++++++++-------------- src/db/models/organization.rs | 5 ++-- src/db/models/user.rs | 8 +++--- src/mail.rs | 15 ++++------- src/static/scripts/admin_diagnostics.js | 5 +++- src/util.rs | 23 ++++++++++++++++- 8 files changed, 69 insertions(+), 55 deletions(-) diff --git a/.env.template b/.env.template index 80eb4756..74b78547 100644 --- a/.env.template +++ b/.env.template @@ -534,8 +534,6 @@ # Whether to send mail via the `sendmail` command # USE_SENDMAIL=false -# Which sendmail command to use. The one found in the $PATH is used if not specified. -# SENDMAIL_COMMAND="/path/to/sendmail" ## Defaults for SSL is "Plain" and "Login" and nothing for Non-SSL connections. ## Possible values: ["Plain", "Login", "Xoauth2"]. diff --git a/src/api/admin.rs b/src/api/admin.rs index 31159816..c2e747e2 100644 --- a/src/api/admin.rs +++ b/src/api/admin.rs @@ -171,7 +171,7 @@ struct LoginForm { redirect: Option, } -#[post("/", data = "")] +#[post("/", format = "application/x-www-form-urlencoded", data = "")] fn post_admin_login( data: Form, cookies: &CookieJar<'_>, @@ -289,7 +289,7 @@ async fn get_user_or_404(user_id: &UserId, conn: &mut DbConn) -> ApiResult } } -#[post("/invite", data = "")] +#[post("/invite", format = "application/json", data = "")] async fn invite_user(data: Json, _token: AdminToken, mut conn: DbConn) -> JsonResult { let data: InviteData = data.into_inner(); if User::find_by_mail(&data.email, &mut conn).await.is_some() { @@ -315,7 +315,7 @@ async fn invite_user(data: Json, _token: AdminToken, mut conn: DbCon Ok(Json(user.to_json(&mut conn).await)) } -#[post("/test/smtp", data = "")] +#[post("/test/smtp", format = "application/json", data = "")] async fn test_smtp(data: Json, _token: AdminToken) -> EmptyResult { let data: InviteData = data.into_inner(); @@ -393,7 +393,7 @@ async fn get_user_json(user_id: UserId, _token: AdminToken, mut conn: DbConn) -> Ok(Json(usr)) } -#[post("/users//delete")] +#[post("/users//delete", format = "application/json")] async fn delete_user(user_id: UserId, token: AdminToken, mut conn: DbConn) -> EmptyResult { let user = get_user_or_404(&user_id, &mut conn).await?; @@ -417,7 +417,7 @@ async fn delete_user(user_id: UserId, token: AdminToken, mut conn: DbConn) -> Em res } -#[post("/users//deauth")] +#[post("/users//deauth", format = "application/json")] async fn deauth_user(user_id: UserId, _token: AdminToken, mut conn: DbConn, nt: Notify<'_>) -> EmptyResult { let mut user = get_user_or_404(&user_id, &mut conn).await?; @@ -438,7 +438,7 @@ async fn deauth_user(user_id: UserId, _token: AdminToken, mut conn: DbConn, nt: user.save(&mut conn).await } -#[post("/users//disable")] +#[post("/users//disable", format = "application/json")] async fn disable_user(user_id: UserId, _token: AdminToken, mut conn: DbConn, nt: Notify<'_>) -> EmptyResult { let mut user = get_user_or_404(&user_id, &mut conn).await?; Device::delete_all_by_user(&user.uuid, &mut conn).await?; @@ -452,7 +452,7 @@ async fn disable_user(user_id: UserId, _token: AdminToken, mut conn: DbConn, nt: save_result } -#[post("/users//enable")] +#[post("/users//enable", format = "application/json")] async fn enable_user(user_id: UserId, _token: AdminToken, mut conn: DbConn) -> EmptyResult { let mut user = get_user_or_404(&user_id, &mut conn).await?; user.enabled = true; @@ -460,7 +460,7 @@ async fn enable_user(user_id: UserId, _token: AdminToken, mut conn: DbConn) -> E user.save(&mut conn).await } -#[post("/users//remove-2fa")] +#[post("/users//remove-2fa", format = "application/json")] async fn remove_2fa(user_id: UserId, token: AdminToken, mut conn: DbConn) -> EmptyResult { let mut user = get_user_or_404(&user_id, &mut conn).await?; TwoFactor::delete_all_by_user(&user.uuid, &mut conn).await?; @@ -469,7 +469,7 @@ async fn remove_2fa(user_id: UserId, token: AdminToken, mut conn: DbConn) -> Emp user.save(&mut conn).await } -#[post("/users//invite/resend")] +#[post("/users//invite/resend", format = "application/json")] async fn resend_user_invite(user_id: UserId, _token: AdminToken, mut conn: DbConn) -> EmptyResult { if let Some(user) = User::find_by_uuid(&user_id, &mut conn).await { //TODO: replace this with user.status check when it will be available (PR#3397) @@ -496,7 +496,7 @@ struct MembershipTypeData { org_uuid: OrganizationId, } -#[post("/users/org_type", data = "")] +#[post("/users/org_type", format = "application/json", data = "")] async fn update_membership_type(data: Json, token: AdminToken, mut conn: DbConn) -> EmptyResult { let data: MembershipTypeData = data.into_inner(); @@ -550,7 +550,7 @@ async fn update_membership_type(data: Json, token: AdminToke member_to_edit.save(&mut conn).await } -#[post("/users/update_revision")] +#[post("/users/update_revision", format = "application/json")] async fn update_revision_users(_token: AdminToken, mut conn: DbConn) -> EmptyResult { User::update_all_revisions(&mut conn).await } @@ -575,7 +575,7 @@ async fn organizations_overview(_token: AdminToken, mut conn: DbConn) -> ApiResu Ok(Html(text)) } -#[post("/organizations//delete")] +#[post("/organizations//delete", format = "application/json")] async fn delete_organization(org_id: OrganizationId, _token: AdminToken, mut conn: DbConn) -> EmptyResult { let org = Organization::find_by_uuid(&org_id, &mut conn).await.map_res("Organization doesn't exist")?; org.delete(&mut conn).await @@ -733,7 +733,7 @@ async fn diagnostics(_token: AdminToken, ip_header: IpHeader, mut conn: DbConn) Ok(Html(text)) } -#[get("/diagnostics/config")] +#[get("/diagnostics/config", format = "application/json")] fn get_diagnostics_config(_token: AdminToken) -> Json { let support_json = CONFIG.get_support_json(); Json(support_json) @@ -744,7 +744,7 @@ fn get_diagnostics_http(code: u16, _token: AdminToken) -> EmptyResult { err_code!(format!("Testing error {code} response"), code); } -#[post("/config", data = "")] +#[post("/config", format = "application/json", data = "")] fn post_config(data: Json, _token: AdminToken) -> EmptyResult { let data: ConfigBuilder = data.into_inner(); if let Err(e) = CONFIG.update_config(data) { @@ -753,7 +753,7 @@ fn post_config(data: Json, _token: AdminToken) -> EmptyResult { Ok(()) } -#[post("/config/delete")] +#[post("/config/delete", format = "application/json")] fn delete_config(_token: AdminToken) -> EmptyResult { if let Err(e) = CONFIG.delete_user_config() { err!(format!("Unable to delete config: {e:?}")) @@ -761,7 +761,7 @@ fn delete_config(_token: AdminToken) -> EmptyResult { Ok(()) } -#[post("/config/backup_db")] +#[post("/config/backup_db", format = "application/json")] async fn backup_db(_token: AdminToken, mut conn: DbConn) -> ApiResult { if *CAN_BACKUP { match backup_database(&mut conn).await { diff --git a/src/config.rs b/src/config.rs index e8536209..0b93c216 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,8 +1,10 @@ -use std::env::consts::EXE_SUFFIX; -use std::process::exit; -use std::sync::{ - atomic::{AtomicBool, Ordering}, - RwLock, +use std::{ + env::consts::EXE_SUFFIX, + process::exit, + sync::{ + atomic::{AtomicBool, Ordering}, + RwLock, + }, }; use job_scheduler_ng::Schedule; @@ -12,7 +14,7 @@ use reqwest::Url; use crate::{ db::DbConnType, error::Error, - util::{get_env, get_env_bool, get_web_vault_version, parse_experimental_client_feature_flags}, + util::{get_env, get_env_bool, get_web_vault_version, is_valid_email, parse_experimental_client_feature_flags}, }; static CONFIG_FILE: Lazy = Lazy::new(|| { @@ -676,8 +678,6 @@ make_config! { _enable_smtp: bool, true, def, true; /// Use Sendmail |> Whether to send mail via the `sendmail` command use_sendmail: bool, true, def, false; - /// Sendmail Command |> Which sendmail command to use. The one found in the $PATH is used if not specified. - sendmail_command: String, true, option; /// Host smtp_host: String, true, option; /// DEPRECATED smtp_ssl |> DEPRECATED - Please use SMTP_SECURITY @@ -890,16 +890,12 @@ fn validate_config(cfg: &ConfigItems) -> Result<(), Error> { } if cfg.use_sendmail { - let command = cfg.sendmail_command.clone().unwrap_or_else(|| format!("sendmail{EXE_SUFFIX}")); + let command = format!("sendmail{EXE_SUFFIX}"); - let mut path = std::path::PathBuf::from(&command); - - if !path.is_absolute() { - match which::which(&command) { - Ok(result) => path = result, - Err(_) => err!(format!("sendmail command {command:?} not found in $PATH")), - } - } + // Check if we can find the sendmail command to execute + let Ok(path) = which::which(&command) else { + err!(format!("sendmail command {command} not found in $PATH")) + }; match path.metadata() { Err(err) if err.kind() == std::io::ErrorKind::NotFound => { @@ -932,8 +928,8 @@ fn validate_config(cfg: &ConfigItems) -> Result<(), Error> { } } - if (cfg.smtp_host.is_some() || cfg.use_sendmail) && !cfg.smtp_from.contains('@') { - err!("SMTP_FROM does not contain a mandatory @ sign") + if !is_valid_email(&cfg.smtp_from) { + err!(format!("SMTP_FROM '{}' is not a valid email address", cfg.smtp_from)) } if cfg._enable_email_2fa && cfg.email_token_size < 6 { diff --git a/src/db/models/organization.rs b/src/db/models/organization.rs index 6e6ee77a..1aa5fb40 100644 --- a/src/db/models/organization.rs +++ b/src/db/models/organization.rs @@ -152,6 +152,7 @@ impl PartialOrd for i32 { /// Local methods impl Organization { pub fn new(name: String, billing_email: String, private_key: Option, public_key: Option) -> Self { + let billing_email = billing_email.to_lowercase(); Self { uuid: OrganizationId(crate::util::get_uuid()), name, @@ -307,8 +308,8 @@ use crate::error::MapResult; /// Database methods impl Organization { pub async fn save(&self, conn: &mut DbConn) -> EmptyResult { - if !email_address::EmailAddress::is_valid(self.billing_email.trim()) { - err!(format!("BillingEmail {} is not a valid email address", self.billing_email.trim())) + if !crate::util::is_valid_email(&self.billing_email) { + err!(format!("BillingEmail {} is not a valid email address", self.billing_email)) } for member in Membership::find_by_org(&self.uuid, conn).await.iter() { diff --git a/src/db/models/user.rs b/src/db/models/user.rs index 24c86a43..8978fc5a 100644 --- a/src/db/models/user.rs +++ b/src/db/models/user.rs @@ -267,8 +267,8 @@ impl User { } pub async fn save(&mut self, conn: &mut DbConn) -> EmptyResult { - if self.email.trim().is_empty() { - err!("User email can't be empty") + if !crate::util::is_valid_email(&self.email) { + err!(format!("User email {} is not a valid email address", self.email)) } self.updated_at = Utc::now().naive_utc(); @@ -408,8 +408,8 @@ impl Invitation { } pub async fn save(&self, conn: &mut DbConn) -> EmptyResult { - if self.email.trim().is_empty() { - err!("Invitation email can't be empty") + if !crate::util::is_valid_email(&self.email) { + err!(format!("Invitation email {} is not a valid email address", self.email)) } db_run! {conn: diff --git a/src/mail.rs b/src/mail.rs index 410bac4b..2a8e182e 100644 --- a/src/mail.rs +++ b/src/mail.rs @@ -1,7 +1,6 @@ -use std::str::FromStr; - use chrono::NaiveDateTime; use percent_encoding::{percent_encode, NON_ALPHANUMERIC}; +use std::{env::consts::EXE_SUFFIX, str::FromStr}; use lettre::{ message::{Attachment, Body, Mailbox, Message, MultiPart, SinglePart}, @@ -23,11 +22,7 @@ use crate::{ }; fn sendmail_transport() -> AsyncSendmailTransport { - if let Some(command) = CONFIG.sendmail_command() { - AsyncSendmailTransport::new_with_command(command) - } else { - AsyncSendmailTransport::new() - } + AsyncSendmailTransport::new_with_command(format!("sendmail{EXE_SUFFIX}")) } fn smtp_transport() -> AsyncSmtpTransport { @@ -595,13 +590,13 @@ async fn send_with_selected_transport(email: Message) -> EmptyResult { // Match some common errors and make them more user friendly Err(e) => { if e.is_client() { - debug!("Sendmail client error: {:#?}", e); + debug!("Sendmail client error: {:?}", e); err!(format!("Sendmail client error: {e}")); } else if e.is_response() { - debug!("Sendmail response error: {:#?}", e); + debug!("Sendmail response error: {:?}", e); err!(format!("Sendmail response error: {e}")); } else { - debug!("Sendmail error: {:#?}", e); + debug!("Sendmail error: {:?}", e); err!(format!("Sendmail error: {e}")); } } diff --git a/src/static/scripts/admin_diagnostics.js b/src/static/scripts/admin_diagnostics.js index 258df5e1..566e6a56 100644 --- a/src/static/scripts/admin_diagnostics.js +++ b/src/static/scripts/admin_diagnostics.js @@ -236,8 +236,11 @@ function checkSecurityHeaders(headers, omit) { "referrer-policy": ["same-origin"], "x-xss-protection": ["0"], "x-robots-tag": ["noindex", "nofollow"], + "cross-origin-resource-policy": ["same-origin"], "content-security-policy": [ - "default-src 'self'", + "default-src 'none'", + "font-src 'self'", + "manifest-src 'self'", "base-uri 'self'", "form-action 'self'", "object-src 'self' blob:", diff --git a/src/util.rs b/src/util.rs index 4a8af5e9..76de40d1 100644 --- a/src/util.rs +++ b/src/util.rs @@ -55,6 +55,8 @@ impl Fairing for AppHeaders { res.set_raw_header("Referrer-Policy", "same-origin"); res.set_raw_header("X-Content-Type-Options", "nosniff"); res.set_raw_header("X-Robots-Tag", "noindex, nofollow"); + res.set_raw_header("Cross-Origin-Resource-Policy", "same-origin"); + // Obsolete in modern browsers, unsafe (XS-Leak), and largely replaced by CSP res.set_raw_header("X-XSS-Protection", "0"); @@ -74,7 +76,9 @@ impl Fairing for AppHeaders { // # Mail Relay: https://bitwarden.com/blog/add-privacy-and-security-using-email-aliases-with-bitwarden/ // app.simplelogin.io, app.addy.io, api.fastmail.com, quack.duckduckgo.com let csp = format!( - "default-src 'self'; \ + "default-src 'none'; \ + font-src 'self'; \ + manifest-src 'self'; \ base-uri 'self'; \ form-action 'self'; \ object-src 'self' blob:; \ @@ -461,6 +465,23 @@ pub fn parse_date(date: &str) -> NaiveDateTime { DateTime::parse_from_rfc3339(date).unwrap().naive_utc() } +/// Returns true or false if an email address is valid or not +/// +/// Some extra checks instead of only using email_address +/// This prevents from weird email formats still excepted but in the end invalid +pub fn is_valid_email(email: &str) -> bool { + let Ok(email) = email_address::EmailAddress::from_str(email) else { + return false; + }; + let Ok(email_url) = url::Url::parse(&format!("https://{}", email.domain())) else { + return false; + }; + if email_url.path().ne("/") || email_url.domain().is_none() || email_url.query().is_some() { + return false; + } + true +} + // // Deployment environment methods //