[pbs-devel] [PATCH proxmox 04/12] auth-api: move to hmac signing for csrf tokens

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

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 <s.sterz@proxmox.com>
---
 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<Option<TfaChallenge>, 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<u8> {
+    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,
+    )? {
+        // 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();
@@ -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<bool, Error>;
 
     /// 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<Vec<u8>, 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