[pbs-devel] [PATCH proxmox-backup 09/12] auth: move to hmac keys for csrf tokens

[Max Carrara]

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 <s.sterz at proxmox.com>
> ---
> 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<u8>,
> +    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<i64, Error> {
> -    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<u8> = file_get_contents(configdir!("/csrf.key")).unwrap();
> -    }
> -
> -    &SECRET
> +pub fn csrf_secret() -> &'static HMACKey {
> +    static SECRET: OnceLock<HMACKey> = 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<PKey<Public>, Error> {
> --
> 2.39.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
>