auth-api: move to hmac signing for csrf tokens

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>
This commit is contained in:
Stefan Sterz 2024-03-06 13:36:02 +01:00 committed by Wolfgang Bumiller
parent 8609fb58ef
commit 4d6922e2c4
3 changed files with 87 additions and 21 deletions

View File

@ -1,6 +1,7 @@
//! Provides the "/access/ticket" API call. //! Provides the "/access/ticket" API call.
use anyhow::{bail, format_err, Error}; use anyhow::{bail, format_err, Error};
use openssl::hash::MessageDigest;
use serde_json::{json, Value}; use serde_json::{json, Value};
use proxmox_rest_server::RestEnvironment; 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_schema::{api, api_types::PASSWORD_SCHEMA};
use proxmox_tfa::api::TfaChallenge; use proxmox_tfa::api::TfaChallenge;
use super::auth_context;
use super::ApiTicket; use super::ApiTicket;
use super::{auth_context, HMACKey};
use crate::ticket::Ticket; use crate::ticket::Ticket;
use crate::types::{Authid, Userid}; 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) 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 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) format!("{:08X}:{}", epoch, digest)
} }
fn compute_csrf_secret_digest(timestamp: i64, secret: &[u8], userid: &Userid) -> String { fn csrf_token_data(timestamp: i64, userid: &Userid) -> Vec<u8> {
let mut hasher = openssl::sha::Sha256::new(); format!("{:08X}:{}:", timestamp, userid).as_bytes().to_vec()
let data = format!("{:08X}:{}:", timestamp, userid);
hasher.update(data.as_bytes());
hasher.update(secret);
base64::encode_config(hasher.finish(), base64::STANDARD_NO_PAD)
} }
pub(crate) fn verify_csrf_prevention_token( pub(crate) fn verify_csrf_prevention_token(
secret: &[u8], secret: &HMACKey,
userid: &Userid, userid: &Userid,
token: &str, token: &str,
min_age: i64, min_age: i64,
@ -271,7 +270,7 @@ pub(crate) fn verify_csrf_prevention_token(
} }
fn verify_csrf_prevention_token_do( fn verify_csrf_prevention_token_do(
secret: &[u8], secret: &HMACKey,
userid: &Userid, userid: &Userid,
token: &str, token: &str,
min_age: i64, min_age: i64,
@ -286,16 +285,31 @@ fn verify_csrf_prevention_token_do(
} }
let timestamp = parts.pop_front().unwrap(); let timestamp = parts.pop_front().unwrap();
let sig = parts.pop_front().unwrap().as_bytes(); 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) let ttime = i64::from_str_radix(timestamp, 16)
.map_err(|err| format_err!("timestamp format error - {}", err))?; .map_err(|err| format_err!("timestamp format error - {}", err))?;
let digest = compute_csrf_secret_digest(ttime, secret, userid); let verify = secret.verify(
let digest = digest.as_bytes(); MessageDigest::sha3_256(),
&sig,
&csrf_token_data(ttime, userid),
);
if digest.len() != sig.len() || !openssl::memcmp::eq(digest, sig) { if verify.is_err() || !verify? {
bail!("invalid signature."); // 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(); let now = crate::time::epoch_i64();
@ -311,3 +325,45 @@ fn verify_csrf_prevention_token_do(
Ok(age) 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();
}

View File

@ -9,7 +9,7 @@ use percent_encoding::percent_decode_str;
use proxmox_rest_server::{extract_cookie, AuthError}; use proxmox_rest_server::{extract_cookie, AuthError};
use proxmox_tfa::api::{OpenUserChallengeData, TfaConfig}; use proxmox_tfa::api::{OpenUserChallengeData, TfaConfig};
use crate::auth_key::Keyring; use crate::auth_key::{HMACKey, Keyring};
use crate::types::{Authid, RealmRef, Userid, UsernameRef}; use crate::types::{Authid, RealmRef, Userid, UsernameRef};
mod access; mod access;
@ -18,7 +18,7 @@ mod ticket;
use crate::ticket::Ticket; use crate::ticket::Ticket;
use access::verify_csrf_prevention_token; 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}; pub use ticket::{ApiTicket, PartialTicket};
/// Authentication realms are used to manage users: authenticate, change password or remove. /// 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>; fn auth_id_is_active(&self, auth_id: &Authid) -> Result<bool, Error>;
/// CSRF prevention token secret data. /// CSRF prevention token secret data.
fn csrf_secret(&self) -> &[u8]; fn csrf_secret(&self) -> &'static HMACKey;
/// Verify a token secret. /// Verify a token secret.
fn verify_token_secret(&self, token_id: &Authid, token_secret: &str) -> Result<(), Error>; fn verify_token_secret(&self, token_id: &Authid, token_secret: &str) -> Result<(), Error>;

View File

@ -204,6 +204,16 @@ impl HMACKey {
Ok(base64::encode_config(bytes, base64::STANDARD_NO_PAD)) 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 { enum SigningKey {