[pbs-devel] [PATCH proxmox 04/12] auth-api: move to hmac signing for csrf tokens
Stefan Sterz
s.sterz at proxmox.com
Thu Feb 15 16:19:53 CET 2024
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 at 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 at 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 at 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
More information about the pbs-devel
mailing list