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

[Stefan Sterz <s.sterz@proxmox.com>]

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@proxmox.com>
---
note that the fallbacks here and in `proxmox-auth-api` should be removed
with the next (major) version if possible. 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();
+
+    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
+            .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