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

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

On Tue Feb 20, 2024 at 1:54 PM CET, Max Carrara wrote:
> 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@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.
> >
> >> ---

-- >8 snip 8< --

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

alright, then i'll leave it as is. i did think about whether some kind
of csrf token "versioning" would make sense, if only through the length
of the token. however, that's just security by obscurity as having the
right length comes along with using a broken old hashing method. so
yeah, i think this is fine.

the alternative is not having a fallback at all and breaking all open
session once on upgrade. but basically we should be able to remove this
check even between minor versions since we don't support version
skipping to my knowledge. sessions are only valid for two hours and
usually we don't release those versions *that* quickly ;)

> >
> >> +        // 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.
> >

something like that would be good yeah, your example of that fallback
that should have been removed with pve 7 is a great example. but until
we have a universal way of doing something like that, i think this is
off-topic for this series.

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

as discussed off-list: we should avoid very spcific error messages in
this case. usually that is good practice as it makes debugging easier.
however, here it just give more information to a potential attacker. i'm
not even sure we should return an "invalid signature" error message
here, rather a "csrf token is invalid" for all failure cases would
probably be best. but since we are already here, changing it would also
give more information to a potential attacker.

> > 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?)
> >

argh yeah, that is my mistake sorry, will fix that up in a v2!

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

will do that to, thanks for the suggestion!

-- >8 snip 8< --