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 63A31934D1 for ; Mon, 19 Feb 2024 19:50:14 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 43B85348D4 for ; Mon, 19 Feb 2024 19:50:14 +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 ; Mon, 19 Feb 2024 19:50:12 +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 78D9E43998 for ; Mon, 19 Feb 2024 19:50:12 +0100 (CET) Message-ID: <667ea9d3-4d72-4012-bcd5-c4a283e4bed9@proxmox.com> Date: Mon, 19 Feb 2024 19:50:10 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: pbs-devel@lists.proxmox.com References: <20240215152001.269490-1-s.sterz@proxmox.com> <20240215152001.269490-8-s.sterz@proxmox.com> From: Max Carrara In-Reply-To: <20240215152001.269490-8-s.sterz@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.006 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 07/12] sys: crypt: add helper to allow upgrading hashes 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: Mon, 19 Feb 2024 18:50:14 -0000 On 2/15/24 16:19, Stefan Sterz wrote: > `upgrade_hash` function allows us to transparently upgrade a password > hash to a newer hash function. thus, we can get rid of insecure hashes > even without the user needing to log in or reset their password. > > it works by retaining only the settings of the previous hash and not the > hash itself. the new hash is a salted hash of the previous hash, > including the settings. > > the `verify_crypt_pw` function is also adapted to deal with the new > string format. it verifies hashes whether they've been upgraded or not. > > Signed-off-by: Stefan Sterz > --- > this is a far from ideal method of upgrading hashes. as the input to the > new hash function has several well-known parts, it may break the > security assumptions of a newer password hashing function. it could be > possible that finding collisions is made easier compared with re-hashing > the original password. hence, only do this as a stop-gap meassure. > > also, my choice of `HASH_SEPARATOR` is possibly not ideal. i welcome > some bike-shedding there if we want to consider this approach at all. I know you've meticulously tested this, you gave me a demonstration of this as well, but it still makes me feel uneasy nevertheless - IMHO it is long overdue that we upgrade our hashes, but there must be a cleaner way to do this that doesn't involve keeping those amalgams of crypt hashes around. There are quite a few comments inline, but I do want to mention that I realize that this took you a lot of work, so I highly commend your efforts on this. > > proxmox-sys/src/crypt.rs | 131 ++++++++++++++++++++++++++++++++++++++- > 1 file changed, 129 insertions(+), 2 deletions(-) > > diff --git a/proxmox-sys/src/crypt.rs b/proxmox-sys/src/crypt.rs > index 3313f66..ad715ff 100644 > --- a/proxmox-sys/src/crypt.rs > +++ b/proxmox-sys/src/crypt.rs > @@ -5,7 +5,7 @@ > > use std::ffi::{CStr, CString}; > > -use anyhow::{bail, Error}; > +use anyhow::{bail, format_err, Error}; > > // from libcrypt1, 'lib/crypt.h.in' > const CRYPT_OUTPUT_SIZE: usize = 384; > @@ -25,6 +25,14 @@ pub const HASH_PREFIX: &str = "$y$"; > // see `YESCRYPT_COST_FACTOR` in `/etc/login.defs` > const HASH_COST: u64 = 5; > > +// for password hash upgrading > +// chosen because we don't use it in our configs, nor should it ever be returned by crypt as the > +// man page states (`man crypt(3)`): > +// > +// > It will be entirely printable ASCII, and will not contain whitespace or the characters ‘:’, > +// > ‘;’, ‘*’, ‘!’, or ‘\’. See crypt(5) for more detail on the format of hashed passphrases. > +const HASH_SEPARATOR: char = ';'; > + > #[repr(C)] > struct crypt_data { > output: [libc::c_char; CRYPT_OUTPUT_SIZE], > @@ -142,6 +150,48 @@ pub fn crypt_gensalt(prefix: &str, count: u64, rbytes: &[u8]) -> Result Ok(res.to_str()?.to_string()) > } > > +/// Upgrades an existing hash with the latest hash function > +/// > +/// Upgraded hashes have the following format: > +/// > +/// `{old_settings}{HASH_SEPARATOR}{old_settings}{HASH_SEPARATOR}{new_hash}` > +/// > +/// This allows us to upgrade hashes in place while keeping the settings which are necessary for > +/// verifying passwords. By keeping the oldest settings first, we can also simply replace them once > +/// a user logs back in and we can re-hash the proper password. While upgrading the hashes will look ugly no matter what, I think this could be wrapped into a more maintainable API. Here's a rough draft of an alternative approach: #[non_exhaustive] // technically not necessary enum HashType<'a> { #[deprecated = "support for legacy type hashes (sha256crypt) will be removed soon"] Legacy(Cow<'a, str>), Upgraded(Cow<'a, str>), // possible other variants } impl<'a> HashType<'a> { fn new(s: &'a str) -> Result { todo!("parse type of hash here") } fn try_upgrade(&self) -> Result { todo!("try to rehash here if necessary") } fn into_inner(self) -> Cow<'a, str> { match self { HashType::Legacy(s) => s, HashType::Upgraded(s) => s, } } // as_str(), to_bytes(), to_owned(), etc. } // for good measure ;P impl<'a> TryFrom<&'a str> for HashType<'a> { type Error = Error; fn try_from(value: &'a str) -> Result { Self::new(value) } } This enum would allow us to represent different types of hashes as different variants, which means we don't have to check whether the `&str` is prefixed with a certain magic constant in different places. Only if `HashType::new()` returns `Ok()` we know that our `&str` does in fact represent a valid hash for our uses. This also reduces the amount of awkward error handling at usage sites. Furthermore, this enum is supposed to remain *strictly* private in case we need to change its implementation (e.g. add another hash type, if that ever happens; fix a bug, etc.). However, since we want to expose its functionality, we need a wrapper type: pub struct CryptHash<'a> { inner: HashType<'a>, } impl<'a> CryptHash<'a> { pub fn new(s: &'a str) -> Result { HashType::new(s).map(|inner| Self { inner }) } pub fn try_upgrade(&self) -> Result { self.inner .try_upgrade() .map(|new_inner| Self { inner: new_inner }) } // other operations // also as_str(), to_bytes(), to_owned(), etc. } The `CryptHash` struct above just wraps the `HashType` enum and exposes its underlying mechanisms via a small API. All implementation details regarding (re-)hashing and other things can therefore hidden behind a small abstraction. This is, as mentioned before, really just a rough draft - the `Cow<'a, &str>`s can be put elsewhere / omitted if desired, or we could decide to always allocate, etc. Whatever it might look like in the end, I think it's important we pack this away neatly, so that use sites can consistently rely on it without having to worry about implementation details. This function here would then be implemented on `CryptHash` - since we know whether a hash is valid or not when we call `CryptHash::new()` ... > +pub fn upgrade_hash(hash: &str) -> Result { ... this would be eliminated: > + let mut hashes = hash.split(HASH_SEPARATOR).rev().peekable(); > + let current = hashes.next().ok_or(format_err!( > + "upgrading hash failed, could not get current hash!" > + ))?; > + ... this here too: > + let old_setting = if current.starts_with("$5$") { ... this here as well: > + let till = current.rfind('$').ok_or_else(|| { > + format_err!("upgrading hash failed, could not separate hash from settings!") > + })?; > + current.chars().take(till).collect::() ... we also know here whether the hash is upgraded or of some other variant: > + } else if current.starts_with(HASH_PREFIX) { > + // we already have an an upgraded hash, no need to process it > + return Ok(hash.to_string()); > + } else { ... this here could maybe (?) be eliminated as well (if it's possible to determine the hash function being used when `CryptHash` is instantiated, that is): > + bail!("upgrading hash failed, current hash function is unknown"); > + }; > + ... this (with the remaining things above) would then go into `CryptHash::try_upgrade()`: > + let new_hash = encrypt_pw(current)?; > + let mut to_return = format!("{old_setting}{HASH_SEPARATOR}{new_hash}"); > + > + if hashes.peek().is_some() { > + to_return = format!( > + "{}{HASH_SEPARATOR}{to_return}", > + hashes > + .collect::>() > + .join(&HASH_SEPARATOR.to_string()) > + ); > + } > + > + Ok(to_return) > +} > + One thing I'm unsure of is whether we can avoid storing concatenated `crypt` settings (those separated by `HASH_SEPARATOR`) that way - ideally we should avoid that IMO and just upgrade from one hash type to another, but I'm not 100% if that's possible, so please let me know what you think! This here could maybe also be added as constructor-fn to that type (e.g. `CryptHash::new_from_password()`) > /// Encrypt a pasword using sha256 hashing method > pub fn encrypt_pw(password: &str) -> Result { > // 8*32 = 256 bits security (128+ recommended, see `man crypt(5)`) > @@ -154,7 +204,19 @@ pub fn encrypt_pw(password: &str) -> Result { > Also a good candidate for a method: > /// Verify if an encrypted password matches > pub fn verify_crypt_pw(password: &str, enc_password: &str) -> Result<(), Error> { > - let verify = crypt(password.as_bytes(), enc_password.as_bytes())?; > + let verify = enc_password > + .split(HASH_SEPARATOR) > + .try_fold(password.to_string(), |password, salt| { > + crypt(password.as_bytes(), salt.as_bytes()) > + }) > + .map_err(|e| format_err!("could not verify password, hashing failed - {e}"))?; > + > + let enc_password = enc_password > + .split(HASH_SEPARATOR) > + .last() > + .ok_or(format_err!( > + "could not verify password, current hash could not be retrieved!" > + ))?; > > // `openssl::memcmp::eq()`'s runtime does not depend on the content of the arrays only the > // length, this makes it harder to exploit timing side-channels. > @@ -202,3 +264,68 @@ fn test_old_hash_wrong_passphrase_fails() { > > verify_crypt_pw(phrase, hash).expect("could not verify test password"); > } > + > +#[test] > +fn test_simple_upgraded_hash_verifies() { > + let phrase = "supersecretpassphrasenoonewillguess"; > + // `$5$` -> sha256crypt, our previous default implementation > + let hash = "$5$bx7fjhlS8yMPM3Nc$yRgB5vyoTWeRcYn31RFTg5hAGyTInUq.l0HqLKzRuRC"; > + let new_hash = upgrade_hash(hash).expect("could not upgrade test hash"); > + > + assert!(new_hash > + .split(HASH_SEPARATOR) > + .last() > + .expect("could not get last upgraded hash") > + .starts_with(HASH_PREFIX)); > + > + verify_crypt_pw(phrase, &new_hash).expect("could not verify test password"); > +} > + > +#[test] > +fn test_upgrading_passphrase_hash() { > + let phrase = "supersecretpassphrasenoonewillguess"; > + let hash = [ > + // scrypt hash settings of the initial password hash > + "$7$CU..../....t5ffrYy0ejA2e4irwo2JM.".to_string(), > + // sha256crypt hash of the previous scrypt hash > + "$5$UFxVcRPYnVIvUw9r$6fMExxbmSklsO/XucEkRCh/ZMivQle0ssRp9mc.YKz2".to_string(), > + ] > + .join(&HASH_SEPARATOR.to_string()); > + > + let new_hash = upgrade_hash(&hash).expect("could not upgrade test password"); > + assert!(new_hash > + .split(HASH_SEPARATOR) > + .last() > + .expect("could not get last upgraded hash") > + .starts_with(HASH_PREFIX)); > + verify_crypt_pw(phrase, &new_hash).expect("could not verify test password"); > +} > + > +#[test] > +#[should_panic] > +fn test_upgraded_hash_wrong_passphrase_fails() { > + let hash = [ > + // scrypt hash settings of the initial password hash > + "$7$CU..../....t5ffrYy0ejA2e4irwo2JM.".to_string(), > + // sha256crypt hash of the previous scrypt hash > + "$5$UFxVcRPYnVIvUw9r$6fMExxbmSklsO/XucEkRCh/ZMivQle0ssRp9mc.YKz2".to_string(), > + ] > + .join(&HASH_SEPARATOR.to_string()); > + > + let new_hash = upgrade_hash(&hash).expect("could not upgrade test password"); > + assert!(new_hash > + .split(HASH_SEPARATOR) > + .last() > + .expect("could not get last upgraded hash") > + .starts_with(HASH_PREFIX)); > + verify_crypt_pw("nope", &new_hash).expect("could not verify test password"); > +} > + > +#[test] > +fn test_current_hash_is_not_upgraded() { > + let phrase = "supersecretpassphrasenoonewillguess"; > + let hash = encrypt_pw(phrase).expect("could not create test password hash"); > + > + let new_hash = upgrade_hash(&hash).expect("could not upgrade test password"); > + assert_eq!(hash, new_hash); > +} The tests otherwise look fine to me; though, I'll have do admit, the `.split()`ing and `.join()`ing still feels quite off to me... > -- > 2.39.2 > > > > _______________________________________________ > pbs-devel mailing list > pbs-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel