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 039F297DB4 for ; Wed, 6 Mar 2024 13:36:53 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0686D16EFE for ; Wed, 6 Mar 2024 13:36: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 ; Wed, 6 Mar 2024 13:36:19 +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 EBD0248855 for ; Wed, 6 Mar 2024 13:36:18 +0100 (CET) From: Stefan Sterz To: pbs-devel@lists.proxmox.com Date: Wed, 6 Mar 2024 13:36:02 +0100 Message-Id: <20240306123609.164021-6-s.sterz@proxmox.com> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20240306123609.164021-1-s.sterz@proxmox.com> References: <20240306123609.164021-1-s.sterz@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.074 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 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: [pbs-devel] [PATCH proxmox v2 05/12] auth-api: move to hmac signing 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: Wed, 06 Mar 2024 12:36:53 -0000 previously we used our own hmac-like implementation for csrf token signing that simply appended the key to the message (csrf token). however, this is possibly insecure as an attacker that finds a collision in the hash function can easily forge a signature. after all, two messages would then produce the same start conditions before hashing the key. while this is probably a theoretic attack on our csrf implementation, it does not hurt to move to the safer standard hmac implementation that avoids such pitfalls. this commit re-uses the hmac key wrapper used for the keyring. it also keeps the old construction around so we can use it for a transition period between old and new csrf token implementations. this is a breaking change as it changes the signature of the `csrf_secret` method of the `AuthContext` trait to return an hmac key. also exposes `assemble_csrf_prevention_toke` so we can re-use this code here instead of duplicating it in e.g. proxmox-backup's auth_helpers. Signed-off-by: Stefan Sterz --- proxmox-auth-api/src/api/access.rs | 94 ++++++++++++++++++++++++------ proxmox-auth-api/src/api/mod.rs | 6 +- proxmox-auth-api/src/auth_key.rs | 10 ++++ 3 files changed, 88 insertions(+), 22 deletions(-) diff --git a/proxmox-auth-api/src/api/access.rs b/proxmox-auth-api/src/api/access.rs index e22eea2..5c75094 100644 --- a/proxmox-auth-api/src/api/access.rs +++ b/proxmox-auth-api/src/api/access.rs @@ -1,6 +1,7 @@ //! Provides the "/access/ticket" API call. use anyhow::{bail, format_err, Error}; +use openssl::hash::MessageDigest; use serde_json::{json, Value}; use proxmox_rest_server::RestEnvironment; @@ -8,8 +9,8 @@ use proxmox_router::{http_err, Permission, RpcEnvironment}; use proxmox_schema::{api, api_types::PASSWORD_SCHEMA}; use proxmox_tfa::api::TfaChallenge; -use super::auth_context; use super::ApiTicket; +use super::{auth_context, HMACKey}; use crate::ticket::Ticket; use crate::types::{Authid, Userid}; @@ -242,25 +243,23 @@ fn login_challenge(userid: &Userid) -> Result, Error> { tfa_config.authentication_challenge(locked_config, userid.as_str(), None) } -fn assemble_csrf_prevention_token(secret: &[u8], userid: &Userid) -> String { +pub fn assemble_csrf_prevention_token(secret: &HMACKey, userid: &Userid) -> String { let epoch = crate::time::epoch_i64(); - let digest = compute_csrf_secret_digest(epoch, secret, userid); - + let data = csrf_token_data(epoch, userid); + let digest = base64::encode_config( + secret.sign(MessageDigest::sha3_256(), &data).unwrap(), + base64::STANDARD_NO_PAD, + ); format!("{:08X}:{}", epoch, digest) } -fn compute_csrf_secret_digest(timestamp: i64, secret: &[u8], userid: &Userid) -> String { - let mut hasher = openssl::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) +fn csrf_token_data(timestamp: i64, userid: &Userid) -> Vec { + format!("{:08X}:{}:", timestamp, userid).as_bytes().to_vec() } pub(crate) fn verify_csrf_prevention_token( - secret: &[u8], + secret: &HMACKey, userid: &Userid, token: &str, min_age: i64, @@ -271,7 +270,7 @@ pub(crate) fn verify_csrf_prevention_token( } fn verify_csrf_prevention_token_do( - secret: &[u8], + secret: &HMACKey, userid: &Userid, token: &str, min_age: i64, @@ -286,16 +285,31 @@ fn verify_csrf_prevention_token_do( } let timestamp = parts.pop_front().unwrap(); - let sig = parts.pop_front().unwrap().as_bytes(); + let sig = parts.pop_front().unwrap(); + let sig = base64::decode_config(sig, base64::STANDARD_NO_PAD) + .map_err(|e| format_err!("could not base64 decode csrf signature - {e}"))?; 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); - let digest = digest.as_bytes(); - - if digest.len() != sig.len() || !openssl::memcmp::eq(digest, sig) { - bail!("invalid signature."); + let verify = secret.verify( + MessageDigest::sha3_256(), + &sig, + &csrf_token_data(ttime, userid), + ); + + if verify.is_err() || !verify? { + // legacy token verification code + // TODO: remove once all dependent products had a major version release (PBS) + let mut hasher = openssl::sha::Sha256::new(); + let data = format!("{:08X}:{}:", ttime, userid); + hasher.update(data.as_bytes()); + hasher.update(&secret.as_bytes()?); + let old_digest = hasher.finish(); + + if old_digest.len() != sig.len() || !openssl::memcmp::eq(&old_digest, &sig) { + bail!("invalid signature."); + } } let now = crate::time::epoch_i64(); @@ -311,3 +325,45 @@ fn verify_csrf_prevention_token_do( Ok(age) } + +#[test] +fn test_assemble_and_verify_csrf_token() { + let secret = HMACKey::generate().expect("failed to generate HMAC key for testing"); + + let userid: Userid = "name@realm" + .parse() + .expect("could not parse user id for HMAC testing"); + let token = assemble_csrf_prevention_token(&secret, &userid); + + verify_csrf_prevention_token(&secret, &userid, &token, -300, 300) + .expect("could not verify csrf for testing"); +} + +#[test] +fn test_verify_legacy_csrf_tokens() { + use openssl::rsa::Rsa; + + // assemble legacy key and token + let key = Rsa::generate(2048) + .expect("could not generate RSA key for testing") + .private_key_to_pem() + .expect("could not create private PEM for testing"); + let userid: Userid = "name@realm" + .parse() + .expect("could not parse the user id for legacy csrf testing"); + let epoch = crate::time::epoch_i64(); + + let mut hasher = openssl::sha::Sha256::new(); + let data = format!("{:08X}:{}:", epoch, userid); + hasher.update(data.as_bytes()); + hasher.update(&key); + let old_digest = base64::encode_config(hasher.finish(), base64::STANDARD_NO_PAD); + + let token = format!("{:08X}:{}", epoch, old_digest); + + // load key into new hmackey wrapper and verify + let string = base64::encode_config(key.clone(), base64::STANDARD_NO_PAD); + let secret = + HMACKey::from_base64(&string).expect("could not create HMAC key from base64 for testing"); + verify_csrf_prevention_token(&secret, &userid, &token, -300, 300).unwrap(); +} diff --git a/proxmox-auth-api/src/api/mod.rs b/proxmox-auth-api/src/api/mod.rs index 129462f..c4e507c 100644 --- a/proxmox-auth-api/src/api/mod.rs +++ b/proxmox-auth-api/src/api/mod.rs @@ -9,7 +9,7 @@ use percent_encoding::percent_decode_str; use proxmox_rest_server::{extract_cookie, AuthError}; use proxmox_tfa::api::{OpenUserChallengeData, TfaConfig}; -use crate::auth_key::Keyring; +use crate::auth_key::{HMACKey, Keyring}; use crate::types::{Authid, RealmRef, Userid, UsernameRef}; mod access; @@ -18,7 +18,7 @@ mod ticket; use crate::ticket::Ticket; use access::verify_csrf_prevention_token; -pub use access::{create_ticket, API_METHOD_CREATE_TICKET}; +pub use access::{assemble_csrf_prevention_token, create_ticket, API_METHOD_CREATE_TICKET}; pub use ticket::{ApiTicket, PartialTicket}; /// Authentication realms are used to manage users: authenticate, change password or remove. @@ -67,7 +67,7 @@ pub trait AuthContext: Send + Sync { fn auth_id_is_active(&self, auth_id: &Authid) -> Result; /// CSRF prevention token secret data. - fn csrf_secret(&self) -> &[u8]; + fn csrf_secret(&self) -> &'static HMACKey; /// Verify a token secret. fn verify_token_secret(&self, token_id: &Authid, token_secret: &str) -> Result<(), Error>; diff --git a/proxmox-auth-api/src/auth_key.rs b/proxmox-auth-api/src/auth_key.rs index b0847a1..f42ed71 100644 --- a/proxmox-auth-api/src/auth_key.rs +++ b/proxmox-auth-api/src/auth_key.rs @@ -204,6 +204,16 @@ impl HMACKey { Ok(base64::encode_config(bytes, base64::STANDARD_NO_PAD)) } + + // This is needed for legacy CSRF token verifyication. + // + // TODO: remove once all dependent products had a major version release (PBS) + pub(crate) fn as_bytes(&self) -> Result, Error> { + // workaround to get access to the the bytes behind the key. + self.key + .raw_private_key() + .map_err(|e| format_err!("could not get raw bytes of HMAC key - {e}")) + } } enum SigningKey { -- 2.39.2