From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 7BCDC94C21 for ; Fri, 23 Feb 2024 10:26:18 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 6598E1523A for ; Fri, 23 Feb 2024 10:26:18 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Fri, 23 Feb 2024 10:26:17 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 4FE7944EB6 for ; Fri, 23 Feb 2024 10:26:17 +0100 (CET) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Fri, 23 Feb 2024 10:26:15 +0100 Message-Id: To: "Proxmox Backup Server development discussion" From: "Stefan Sterz" X-Mailer: aerc 0.17.0-57-g782a17dfb056 References: <20240215152001.269490-1-s.sterz@proxmox.com> <20240215152001.269490-5-s.sterz@proxmox.com> <0e2e995a-ac9b-4b4a-b7ba-eeb154dfaab5@proxmox.com> <3114362d-e1c8-4107-be0d-61bc0173bc1b@proxmox.com> In-Reply-To: <3114362d-e1c8-4107-be0d-61bc0173bc1b@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.080 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pbs-devel] [PATCH proxmox 04/12] auth-api: move to hmac signing for csrf tokens X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 23 Feb 2024 09:26:18 -0000 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 > > > > 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 =3D parts.pop_front().unwrap(); > >> let sig =3D parts.pop_front().unwrap(); > >> + let sig =3D base64::decode_config(sig, base64::STANDARD_NO_PAD) > >> + .map_err(|e| format_err!("could not base64 decode csrf signat= ure - {e}"))?; > >> > >> let ttime =3D i64::from_str_radix(timestamp, 16) > >> .map_err(|err| format_err!("timestamp format error - {}", err= ))?; > >> > >> - let digest =3D compute_csrf_secret_digest(ttime, secret, userid); > >> - > >> - if digest !=3D 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 b= ack to the > > original verification code below. As you mentioned in your commit messa= ge: > > > >> 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 l= ikely) 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 implementat= ion 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) =3D @_; > > if ($token =3D~ m/^([A-Z0-9]{8}):(\S+)$/) { > my $sig =3D $2; > my $timestamp =3D $1; > my $ttime =3D hex($timestamp); > > my $digest; > if (length($sig) =3D=3D 27) { > # detected sha1 csrf token from older proxy, fallback. FIXME: remove= with 7.0 > $digest =3D Digest::SHA::sha1_base64("$timestamp:$username", $secret= ); > } else { > $digest =3D Digest::SHA::hmac_sha256_base64("$timestamp:$username", = $secret); > } > > my $age =3D time() - $ttime; > return 1 if ($digest eq $sig) && ($age > $min_age) && > ($age < $max_age); > } > > raise("Permission denied - invalid csrf token\n", code =3D> HTTP_UNAU= THORIZED) > if !$noerr; > > return undef; > } > > > I think what you're doing here might be fine after all ;) > > > [0]: https://git.proxmox.com/?p=3Dpve-common.git;a=3Dblob;f=3Dsrc/PVE/Tic= ket.pm;h=3Dce8d5c8c6c158ed8a9c0b8b89bd55a5bc7d01431;hb=3DHEAD#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 versi= on 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 { > >> () =3D> { > >> env!("CARGO_PKG_VERSION_MAJOR") > >> .parse::() > >> .expect("Failed to parse crate major version") > >> }; > >> } > >> > >> #[test] > >> fn check_major() { > >> let v =3D 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 co= uld then refer to the > > things that still need to be changed for a major version bump. A comb= ination with 1. could > > perhaps also work. Though, I realize that this could be quite annoyin= g 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 =3D openssl::sha::Sha256::new(); > >> + let data =3D format!("{:08X}:{}:", ttime, userid); > >> + hasher.update(data.as_bytes()); > >> + hasher.update(&secret.as_bytes()?); > >> + let old_digest =3D hasher.finish(); > >> + > >> + if old_digest.len() !=3D sig.len() && openssl::memcmp::eq(&ol= d_digest, &sig) { > >> + bail!("invalid signature."); > >> + } > > > > This check should IMO be split into two for some finer-grained error ha= ndling - 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 u= sed?) > > 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 pat= ch 03, but these checks > > could perhaps go into a separate patch specifically for the old `comput= e_csrf_secret_digest()` > > function first, so that it also benefits from the usage of constant tim= e comparison. > > This patch could then also be applied separately, of course. > > will do that to, thanks for the suggestion! -- >8 snip 8< --