[pbs-devel] [PATCH proxmox-backup v2 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>
---
 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