[pbs-devel] [PATCH proxmox 04/12] auth-api: move to hmac signing for csrf tokens
Max Carrara
m.carrara at proxmox.com
Tue Feb 20 13:54:31 CET 2024
On 2/19/24 17:02, Max Carrara wrote:
> On 2/15/24 16:19, Stefan Sterz wrote:
>> 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>
>
> I like the overall approach of this series quite a lot so far! However,
> I'm not entirely sure if introducing a breaking change here is what we
> actually want, though I'm curious what others are thinking.
>
> There are some more comments inline.
>
>> ---
>> 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,
>> + )? {
>
> The check above bothers me somewhat in particular, since we just fall back to the
> original verification code below. As you mentioned in your commit message:
>
>> 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. [...]
>
>> 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.
>
> So, technically, it would still be possible for an attacker to forge a signature
> during the transition period, because the condition above (most, most likely) fails
> anyway.
>
> (Also, you made a quick comment on the side about this off-list, but I fail to recall
> it at the moment, so I apologize if you've already mentioned this!)
>
> I feel like it would be more practical to separate the HMAC implementation out as a
> separate API and mark the current one as #[deprecated] (or similar) and provide an
> upgrade path for implementors of this crate.
... regarding the fallback mechanism, I've found something [0] that might be of interest:
sub verify_csrf_prevention_token {
my ($secret, $username, $token, $min_age, $max_age, $noerr) = @_;
if ($token =~ m/^([A-Z0-9]{8}):(\S+)$/) {
my $sig = $2;
my $timestamp = $1;
my $ttime = hex($timestamp);
my $digest;
if (length($sig) == 27) {
# detected sha1 csrf token from older proxy, fallback. FIXME: remove with 7.0
$digest = Digest::SHA::sha1_base64("$timestamp:$username", $secret);
} else {
$digest = Digest::SHA::hmac_sha256_base64("$timestamp:$username", $secret);
}
my $age = time() - $ttime;
return 1 if ($digest eq $sig) && ($age > $min_age) &&
($age < $max_age);
}
raise("Permission denied - invalid csrf token\n", code => HTTP_UNAUTHORIZED)
if !$noerr;
return undef;
}
I think what you're doing here might be fine after all ;)
[0]: https://git.proxmox.com/?p=pve-common.git;a=blob;f=src/PVE/Ticket.pm;h=ce8d5c8c6c158ed8a9c0b8b89bd55a5bc7d01431;hb=HEAD#l29
>
>> + // legacy token verification code
>> + // TODO: remove once all dependent products had a major version release (PBS)
>
> Somewhat off-topic, but I feel that we should have guards in place for things that need
> to be removed in future versions so that we don't miss them, even if it ends up being
> a bit of a chore in the end.
>
> There are two ideas I had in mind:
>
> 1. Marker comments in a certain format that are scanned by a tool; tool emits
> warnings / messages for those comments
> --> not sure how "convenient" or adaptable to peoples' workflow this might be though
>
> 2. Automatic compile time checks for cargo env vars (etc.), for example:
>
>> macro_rules! crate_version {
>> () => {
>> env!("CARGO_PKG_VERSION_MAJOR")
>> .parse::<u32>()
>> .expect("Failed to parse crate major version")
>> };
>> }
>>
>> #[test]
>> fn check_major() {
>> let v = crate_version!();
>>
>> if v > 3 {
>> panic!("Encountered major version bump [...]")
>> }
>> }
>
> Putting this into a separate test in PBS would cause PBS to fail when running `make build`
> on a newer major version than 3 (the current one in this case). We could then refer to the
> things that still need to be changed for a major version bump. A combination with 1. could
> perhaps also work. Though, I realize that this could be quite annoying for some when working
> on things unrelated to the checks for the next PBS major release.
>
>> + 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.");
>> + }
>
> This check should IMO be split into two for some finer-grained error handling - meaning,
> one `bail!()` for different `.len()`s and one if `old_digest` and `sig` are equal.
>
> Speaking of, should `old_digest` and `sig` be equal here..? Unless I'm mistaken, the
> digest and signature must be of equal length *and* be equal in order to be correct, right?
> Or am I misunderstanding? (Do we want to fail if an old hash is being used?)
>
> It's great though that `openssl::memcmp::eq()` is used here like in patch 03, but these checks
> could perhaps go into a separate patch specifically for the old `compute_csrf_secret_digest()`
> function first, so that it also benefits from the usage of constant time comparison.
> This patch could then also be applied separately, of course.
>
>> }
>>
>> 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 {
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
More information about the pbs-devel
mailing list