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 07C9B933AB for ; Mon, 19 Feb 2024 17:02:12 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id DA8A532B6D for ; Mon, 19 Feb 2024 17:02:11 +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 17:02:10 +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 7900A438B5 for ; Mon, 19 Feb 2024 17:02:10 +0100 (CET) Message-ID: <0e2e995a-ac9b-4b4a-b7ba-eeb154dfaab5@proxmox.com> Date: Mon, 19 Feb 2024 17:02:08 +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-5-s.sterz@proxmox.com> From: Max Carrara In-Reply-To: <20240215152001.269490-5-s.sterz@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.006 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: Re: [pbs-devel] [PATCH proxmox 04/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: Mon, 19 Feb 2024 16:02:12 -0000 On 2/15/24 16:19, Stefan Sterz wrote: > 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 I like the overall approach of this series quite a lot so far! However, I'm not entirely sure if introducing a breaking change here is what we actually want, though I'm curious what others are thinking. There are some more comments inline. > --- > proxmox-auth-api/src/api/access.rs | 88 ++++++++++++++++++++++++------ > proxmox-auth-api/src/api/mod.rs | 6 +- > proxmox-auth-api/src/auth_key.rs | 10 ++++ > 3 files changed, 84 insertions(+), 20 deletions(-) > > diff --git a/proxmox-auth-api/src/api/access.rs b/proxmox-auth-api/src/api/access.rs > index 428d22a..5ddf1c4 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, > @@ -287,14 +286,28 @@ fn verify_csrf_prevention_token_do( > > let timestamp = parts.pop_front().unwrap(); > 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); > - > - if digest != sig { > - bail!("invalid signature."); > + if !secret.verify( > + MessageDigest::sha3_256(), > + &csrf_token_data(ttime, userid), > + &sig, > + )? { The check above bothers me somewhat in particular, since we just fall back to the original verification code below. As you mentioned in your commit message: > 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. [...] > 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. So, technically, it would still be possible for an attacker to forge a signature during the transition period, because the condition above (most, most likely) fails anyway. (Also, you made a quick comment on the side about this off-list, but I fail to recall it at the moment, so I apologize if you've already mentioned this!) I feel like it would be more practical to separate the HMAC implementation out as a separate API and mark the current one as #[deprecated] (or similar) and provide an upgrade path for implementors of this crate. > + // legacy token verification code > + // TODO: remove once all dependent products had a major version release (PBS) Somewhat off-topic, but I feel that we should have guards in place for things that need to be removed in future versions so that we don't miss them, even if it ends up being a bit of a chore in the end. There are two ideas I had in mind: 1. Marker comments in a certain format that are scanned by a tool; tool emits warnings / messages for those comments --> not sure how "convenient" or adaptable to peoples' workflow this might be though 2. Automatic compile time checks for cargo env vars (etc.), for example: > macro_rules! crate_version { > () => { > env!("CARGO_PKG_VERSION_MAJOR") > .parse::() > .expect("Failed to parse crate major version") > }; > } > > #[test] > fn check_major() { > let v = crate_version!(); > > if v > 3 { > panic!("Encountered major version bump [...]") > } > } Putting this into a separate test in PBS would cause PBS to fail when running `make build` on a newer major version than 3 (the current one in this case). We could then refer to the things that still need to be changed for a major version bump. A combination with 1. could perhaps also work. Though, I realize that this could be quite annoying for some when working on things unrelated to the checks for the next PBS major release. > + 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."); > + } This check should IMO be split into two for some finer-grained error handling - meaning, one `bail!()` for different `.len()`s and one if `old_digest` and `sig` are equal. Speaking of, should `old_digest` and `sig` be equal here..? Unless I'm mistaken, the digest and signature must be of equal length *and* be equal in order to be correct, right? Or am I misunderstanding? (Do we want to fail if an old hash is being used?) It's great though that `openssl::memcmp::eq()` is used here like in patch 03, but these checks could perhaps go into a separate patch specifically for the old `compute_csrf_secret_digest()` function first, so that it also benefits from the usage of constant time comparison. This patch could then also be applied separately, of course. > } > > let now = crate::time::epoch_i64(); > @@ -310,3 +323,44 @@ 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 {