From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id D44D7934DF for ; Mon, 19 Feb 2024 19:55:52 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id AC61934A15 for ; Mon, 19 Feb 2024 19:55:22 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Mon, 19 Feb 2024 19:55:21 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 9A74D439A6 for ; Mon, 19 Feb 2024 19:55:21 +0100 (CET) Message-ID: Date: Mon, 19 Feb 2024 19:55:20 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: pbs-devel@lists.proxmox.com References: <20240215152001.269490-1-s.sterz@proxmox.com> <20240215152001.269490-10-s.sterz@proxmox.com> From: Max Carrara In-Reply-To: <20240215152001.269490-10-s.sterz@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.056 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment PROLO_LEO1 0.1 Meta Catches all Leo drug variations so far SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pbs-devel] [PATCH proxmox-backup 09/12] auth: move to hmac keys for csrf tokens X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 19 Feb 2024 18:55:52 -0000 On 2/15/24 16:19, Stefan Sterz wrote: > previously we used a self-rolled implementation for csrf tokens. while > it's unlikely to cause issues in reality, as csrf tokens are only > valid for a given tickets lifetime, there are still theoretical > attacks on our implementation. so move all of this code into the > proxmox-auth-api crate and use hmac instead. > > this change should not impact existing installations for now, as this > falls back to the old implementation if a key is already present. hmac > keys will only be used for new installations and if users manually > remove the old key and > > Signed-off-by: Stefan Sterz > --- > note that the fallbacks here and in `proxmox-auth-api` should be removed > with the next (major) version if possible. [...] As mentioned in my reply to patch 04, we should somehow ensure that this removed with some kind of compile time check or similar, so we *really* don't miss it. > [...] simply removing the csrf key > before restarting the services should be enough here. it is possible > that this will break some sessions, possibly forcing a user to log back > in. so only do this when we can assume that an admin would have a > reasonable expectation that something like this will happen (i.e.: > possibly document this in the upgrade notes etc.). > > src/auth.rs | 10 ++--- > src/auth_helpers.rs | 97 ++++++++++----------------------------------- > 2 files changed, 26 insertions(+), 81 deletions(-) > > diff --git a/src/auth.rs b/src/auth.rs > index ec0bc41f..c89314f5 100644 > --- a/src/auth.rs > +++ b/src/auth.rs > @@ -15,7 +15,7 @@ use serde_json::json; > use proxmox_auth_api::api::{Authenticator, LockedTfaConfig}; > use proxmox_auth_api::ticket::{Empty, Ticket}; > use proxmox_auth_api::types::Authid; > -use proxmox_auth_api::Keyring; > +use proxmox_auth_api::{HMACKey, Keyring}; > use proxmox_ldap::{Config, Connection, ConnectionMode}; > use proxmox_tfa::api::{OpenUserChallengeData, TfaConfig}; > > @@ -267,7 +267,7 @@ pub fn setup_auth_context(use_private_key: bool) { > AUTH_CONTEXT > .set(PbsAuthContext { > keyring, > - csrf_secret: crate::auth_helpers::csrf_secret().to_vec(), > + csrf_secret: crate::auth_helpers::csrf_secret(), > }) > .map_err(drop) > .expect("auth context setup twice"); > @@ -285,7 +285,7 @@ pub(crate) fn public_auth_keyring() -> &'static Keyring { > > struct PbsAuthContext { > keyring: &'static Keyring, > - csrf_secret: Vec, > + csrf_secret: &'static HMACKey, > } > > impl proxmox_auth_api::api::AuthContext for PbsAuthContext { > @@ -327,8 +327,8 @@ impl proxmox_auth_api::api::AuthContext for PbsAuthContext { > } > > /// CSRF prevention token secret data. > - fn csrf_secret(&self) -> &[u8] { > - &self.csrf_secret > + fn csrf_secret(&self) -> &'static HMACKey { > + self.csrf_secret > } > > /// Verify a token secret. > diff --git a/src/auth_helpers.rs b/src/auth_helpers.rs > index c2eaaef1..1a483d84 100644 > --- a/src/auth_helpers.rs > +++ b/src/auth_helpers.rs > @@ -1,81 +1,20 @@ > use std::path::PathBuf; > +use std::sync::OnceLock; > > -use anyhow::{bail, format_err, Error}; > +use anyhow::Error; > use lazy_static::lazy_static; > use openssl::pkey::{PKey, Private, Public}; > use openssl::rsa::Rsa; > -use openssl::sha; > > use pbs_config::BackupLockGuard; > -use proxmox_lang::try_block; > +use proxmox_auth_api::HMACKey; > use proxmox_sys::fs::{file_get_contents, replace_file, CreateOptions}; > > -use pbs_api_types::Userid; > use pbs_buildcfg::configdir; > use serde_json::json; > > pub use crate::auth::setup_auth_context; > - > -fn compute_csrf_secret_digest(timestamp: i64, secret: &[u8], userid: &Userid) -> String { > - let mut hasher = sha::Sha256::new(); > - let data = format!("{:08X}:{}:", timestamp, userid); > - hasher.update(data.as_bytes()); > - hasher.update(secret); > - > - base64::encode_config(hasher.finish(), base64::STANDARD_NO_PAD) > -} > - > -pub fn assemble_csrf_prevention_token(secret: &[u8], userid: &Userid) -> String { > - let epoch = proxmox_time::epoch_i64(); > - > - let digest = compute_csrf_secret_digest(epoch, secret, userid); > - > - format!("{:08X}:{}", epoch, digest) > -} > - > -pub fn verify_csrf_prevention_token( > - secret: &[u8], > - userid: &Userid, > - token: &str, > - min_age: i64, > - max_age: i64, > -) -> Result { > - use std::collections::VecDeque; > - > - let mut parts: VecDeque<&str> = token.split(':').collect(); > - > - try_block!({ > - if parts.len() != 2 { > - bail!("format error - wrong number of parts."); > - } > - > - let timestamp = parts.pop_front().unwrap(); > - let sig = parts.pop_front().unwrap(); > - > - let ttime = i64::from_str_radix(timestamp, 16) > - .map_err(|err| format_err!("timestamp format error - {}", err))?; > - > - let digest = compute_csrf_secret_digest(ttime, secret, userid); > - > - if digest != sig { > - bail!("invalid signature."); > - } > - > - let now = proxmox_time::epoch_i64(); > - > - let age = now - ttime; > - if age < min_age { > - bail!("timestamp newer than expected."); > - } > - > - if age > max_age { > - bail!("timestamp too old."); > - } > - > - Ok(age) > - }) > - .map_err(|err| format_err!("invalid csrf token - {}", err)) > -} > +pub use proxmox_auth_api::api::assemble_csrf_prevention_token; > > pub fn generate_csrf_key() -> Result<(), Error> { > let path = PathBuf::from(configdir!("/csrf.key")); > @@ -84,17 +23,14 @@ pub fn generate_csrf_key() -> Result<(), Error> { > return Ok(()); > } > > - let rsa = Rsa::generate(2048).unwrap(); > - > - let pem = rsa.private_key_to_pem()?; > + let key = HMACKey::generate()?.to_base64()?; > > use nix::sys::stat::Mode; > - > let backup_user = pbs_config::backup_user()?; > > replace_file( > &path, > - &pem, > + key.as_bytes(), > CreateOptions::new() > .perm(Mode::from_bits_truncate(0o0640)) > .owner(nix::unistd::ROOT) > @@ -145,12 +81,21 @@ pub fn generate_auth_key() -> Result<(), Error> { > Ok(()) > } > > -pub fn csrf_secret() -> &'static [u8] { > - lazy_static! { > - static ref SECRET: Vec = file_get_contents(configdir!("/csrf.key")).unwrap(); > - } > - > - &SECRET > +pub fn csrf_secret() -> &'static HMACKey { > + static SECRET: OnceLock = OnceLock::new(); Nice that we can use a `OnceLock` here! > + > + SECRET.get_or_init(|| { > + let bytes = file_get_contents(configdir!("/csrf.key")).unwrap(); > + std::str::from_utf8(&bytes) > + .map_err(anyhow::Error::new) > + .and_then(HMACKey::from_base64) > + // legacy fall back to load legacy csrf secrets > + // TODO: remove once we move away from legacy token verification See my comment above for the TODO up there. > + .unwrap_or_else(|_| { > + let key_as_b64 = base64::encode_config(bytes, base64::STANDARD_NO_PAD); > + HMACKey::from_base64(&key_as_b64).unwrap() > + }) > + }) > } > > fn load_public_auth_key() -> Result, Error> { > -- > 2.39.2 > > > > _______________________________________________ > pbs-devel mailing list > pbs-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel > >