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 DE67797D54 for ; Wed, 6 Mar 2024 13:36:22 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E7E2D16EFD for ; Wed, 6 Mar 2024 13:36:21 +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 ; Wed, 6 Mar 2024 13:36:19 +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 3146E48845 for ; Wed, 6 Mar 2024 13:36:19 +0100 (CET) From: Stefan Sterz To: pbs-devel@lists.proxmox.com Date: Wed, 6 Mar 2024 13:36:03 +0100 Message-Id: <20240306123609.164021-7-s.sterz@proxmox.com> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20240306123609.164021-1-s.sterz@proxmox.com> References: <20240306123609.164021-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.074 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 v2 06/12] sys: crypt: move to yescrypt for password hashing 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: Wed, 06 Mar 2024 12:36:22 -0000 previously we used `sha256scrypt` for password hashing. while this may by safe if used with the correct parameters, we used the default parameters which are considered unsafe. according to `man crypt(5)`: > The default CPU time cost parameter is 5000, which is too low for > modern hardware. hence, we needed to adapt this code anyway. conveniently, verification with crypt also works for older hashes as the parameters for the hashing function are encoded in the output of crypt. so this is a drop in replacement that will simply use yescrypt for new hashes while old hashes will still verify properly. this commit also adds a wrapper for `crypt_gensalt_rn` to more easily generate correctly formatted salt strings. this is also useful for switching the cpu time hardness parameter, as otherwise we'd need to encode that ourselves. Signed-off-by: Stefan Sterz --- proxmox-sys/src/crypt.rs | 132 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 129 insertions(+), 3 deletions(-) diff --git a/proxmox-sys/src/crypt.rs b/proxmox-sys/src/crypt.rs index ab42685..fa10911 100644 --- a/proxmox-sys/src/crypt.rs +++ b/proxmox-sys/src/crypt.rs @@ -1,6 +1,9 @@ //! Rust bindings for libcrypt +//! +//! this may fail if we ever pull in dependencies that also link with libcrypt. we may eventually +//! want to switch to pure rust re-implementations of libcrypt. -use std::ffi::CStr; +use std::ffi::{CStr, CString}; use anyhow::{bail, Error}; @@ -9,6 +12,18 @@ const CRYPT_OUTPUT_SIZE: usize = 384; const CRYPT_MAX_PASSPHRASE_SIZE: usize = 512; const CRYPT_DATA_RESERVED_SIZE: usize = 767; const CRYPT_DATA_INTERNAL_SIZE: usize = 30720; +const CRYPT_GENSALT_OUTPUT_SIZE: usize = 192; + +// the hash prefix selects the password hashing method, currently this is yescrypt. check `man +// crypt(5)` for more info +pub const HASH_PREFIX: &str = "$y$"; + +// the cpu cost of the password hashing function. depends on the hashing function, see`man +// crypt_gensalt(3)` and `man crypt(5) for more info +// +// `5` selects a good medium cpu time hardness that seems to be widely used by e.g. Debian +// see `YESCRYPT_COST_FACTOR` in `/etc/login.defs` +const HASH_COST: u64 = 5; #[repr(C)] struct crypt_data { @@ -49,15 +64,90 @@ pub fn crypt(password: &[u8], salt: &[u8]) -> Result { if status.is_null() { bail!("internal error: crypt_r returned null pointer"); } + + // according to man crypt(3): + // + // > Upon error, crypt_r, crypt_rn, and crypt_ra write an invalid hashed passphrase to the + // > output field of their data argument, and crypt writes an invalid hash to its static + // > storage area. This string will be shorter than 13 characters, will begin with a ‘*’, + // > and will not compare equal to setting. + if data.output.first().is_none() || Some(&('*' as i8)) == data.output.first() { + bail!("internal error: crypt_r returned invalid hash"); + } CStr::from_ptr(&data.output as *const _) }; Ok(String::from(res.to_str()?)) } +/// Rust wrapper around `crypt_gensalt_rn` from libcrypt. Useful to generate salts for crypt. +/// +/// - `prefix`: The prefix that selects the hashing method to use (see `man crypt(5)`) +/// - `count`: The CPU time cost parameter (e.g., for `yescrypt` between 1 and 11, see `man +/// crypt(5)`) +/// - `rbytes`: The byte slice that contains cryptographically random bytes for generating the salt +pub fn crypt_gensalt(prefix: &str, count: u64, rbytes: &[u8]) -> Result { + #[link(name = "crypt")] + extern "C" { + #[link_name = "crypt_gensalt_rn"] + fn __crypt_gensalt_rn( + prefix: *const libc::c_char, + count: libc::c_ulong, + // `crypt_gensalt_rn`'s signature expects a char pointer here, which would be a pointer + // to an `i8` slice in rust. however, this is interpreted as raw bytes that are used as + // entropy, which in rust usually is a `u8` slice. so use this signature to avoid a + // pointless transmutation (random bytes are random, whether interpreted as `i8` or + // `u8`) + rbytes: *const u8, + nrbytes: libc::c_int, + output: *mut libc::c_char, + output_size: libc::c_int, + ) -> *mut libc::c_char; + } + + let prefix = CString::new(prefix)?; + + #[allow(clippy::useless_conversion)] + let mut output = [libc::c_char::from(0); CRYPT_GENSALT_OUTPUT_SIZE]; + + let status = unsafe { + __crypt_gensalt_rn( + prefix.as_ptr(), + count, + rbytes.as_ptr(), + rbytes.len().try_into()?, + output.as_mut_ptr(), + output.len().try_into()?, + ) + }; + + if status.is_null() { + bail!("internal error: crypt_gensalt_rn returned a null pointer"); + } + + // according to man crypt_gensalt_rn(3): + // + // > Upon error, in addition to returning a null pointer, crypt_gensalt and crypt_gensalt_rn + // > will write an invalid setting string to their output buffer, if there is enough space; + // > this string will begin with a ‘*’ and will not be equal to prefix. + // + // while it states that this is "in addition" to returning a null pointer, this isn't how + // `crypt_r` seems to behave (sometimes only setting an invalid hash) so add this here too just + // in case. + if output.first().is_none() || Some(&('*' as i8)) == output.first() { + bail!("internal error: crypt_gensalt_rn could not create a valid salt"); + } + + let res = unsafe { CStr::from_ptr(output.as_ptr()) }; + + Ok(res.to_str()?.to_string()) +} + /// Encrypt a pasword using sha256 hashing method pub fn encrypt_pw(password: &str) -> Result { - let salt = crate::linux::random_data(8)?; - let salt = format!("$5${}$", base64::encode_config(&salt, base64::CRYPT)); + // 8*32 = 256 bits security (128+ recommended, see `man crypt(5)`) + let salt = crate::linux::random_data(32)?; + + let salt = crypt_gensalt(HASH_PREFIX, HASH_COST, &salt)?; crypt(password.as_bytes(), salt.as_bytes()) } @@ -70,3 +160,39 @@ pub fn verify_crypt_pw(password: &str, enc_password: &str) -> Result<(), Error> } Ok(()) } + +#[test] +fn test_hash_and_verify_passphrase() { + let phrase = "supersecretpassphrasenoonewillguess"; + + let hash = encrypt_pw(phrase).expect("could not hash test password"); + verify_crypt_pw(phrase, &hash).expect("could not verify test password"); +} + +#[test] +#[should_panic] +fn test_wrong_passphrase_fails() { + let phrase = "supersecretpassphrasenoonewillguess"; + + let hash = encrypt_pw(phrase).expect("could not hash test password"); + verify_crypt_pw("nope", &hash).expect("could not verify test password"); +} + +#[test] +fn test_old_passphrase_hash() { + let phrase = "supersecretpassphrasenoonewillguess"; + // `$5$` -> sha256crypt, our previous default implementation + let hash = "$5$bx7fjhlS8yMPM3Nc$yRgB5vyoTWeRcYn31RFTg5hAGyTInUq.l0HqLKzRuRC"; + + verify_crypt_pw(phrase, hash).expect("could not verify test password"); +} + +#[test] +#[should_panic] +fn test_old_hash_wrong_passphrase_fails() { + let phrase = "nope"; + // `$5$` -> sha256crypt, our previous default implementation + let hash = "$5$bx7fjhlS8yMPM3Nc$yRgB5vyoTWeRcYn31RFTg5hAGyTInUq.l0HqLKzRuRC"; + + verify_crypt_pw(phrase, hash).expect("could not verify test password"); +} -- 2.39.2