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 BA2CE918F0 for ; Thu, 15 Feb 2024 16:20:19 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id EA3501673D for ; Thu, 15 Feb 2024 16:20: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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Thu, 15 Feb 2024 16:20:15 +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 824DD4844A for ; Thu, 15 Feb 2024 16:20:15 +0100 (CET) From: Stefan Sterz To: pbs-devel@lists.proxmox.com Date: Thu, 15 Feb 2024 16:19:56 +0100 Message-Id: <20240215152001.269490-8-s.sterz@proxmox.com> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20240215152001.269490-1-s.sterz@proxmox.com> References: <20240215152001.269490-1-s.sterz@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.081 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: [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: Thu, 15 Feb 2024 15:20:19 -0000 `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. 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 Result { + 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!" + ))?; + + let old_setting = if current.starts_with("$5$") { + let till = current.rfind('$').ok_or_else(|| { + format_err!("upgrading hash failed, could not separate hash from settings!") + })?; + current.chars().take(till).collect::() + } 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 { + bail!("upgrading hash failed, current hash function is unknown"); + }; + + 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) +} + /// 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 { /// 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); +} -- 2.39.2