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

[Max Carrara <m.carrara@proxmox.com>]

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 <s.sterz@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.

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

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<Self, Error> {
        todo!("parse type of hash here")
    }

    fn try_upgrade(&self) -> Result<Self, Error> {
        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, Self::Error> {
        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<Self, Error> {
        HashType::new(s).map(|inner| Self { inner })
    }

    pub fn try_upgrade(&self) -> Result<Self, Error> {
        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<String, Error> {
    ... 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::<String>()

    ... 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::<Vec<&str>>()
> +                .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<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> {
> 

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