[pbs-devel] [PATCH proxmox 07/12] sys: crypt: add helper to allow upgrading hashes

[Stefan Sterz]

`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 <s.sterz at proxmox.com>
---
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<String,
     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.
+pub fn upgrade_hash(hash: &str) -> Result<String, Error> {
+    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::<String>()
+    } 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::<Vec<&str>>()
+                .join(&HASH_SEPARATOR.to_string())
+        );
+    }
+
+    Ok(to_return)
+}
+
 /// Encrypt a pasword using sha256 hashing method
 pub fn encrypt_pw(password: &str) -> Result<String, Error> {
     // 8*32 = 256 bits security (128+ recommended, see `man crypt(5)`)
@@ -154,7 +204,19 @@ pub fn encrypt_pw(password: &str) -> Result<String, Error> {

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