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

[Max Carrara <m.carrara@proxmox.com>]

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@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.

> +        // 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@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 {