Skip to content

Commit

Permalink
argon2: avoid salt-length related panic (closes #134)
Browse files Browse the repository at this point in the history
Adds some additional advance checks that Argon2 parameters are within
range, and returns errors in the event they are not.

This prevents a panic caused by what was previously an `unimplemented!`
call when converting error types.

The occurrence of the panic in the first place speaks to overall
deficienciesin the `password-hash` crate's error handling strategy.

It's further complicated by the awkwardness of the
`password_hash::Output::init_with` API.

The code in this commit has TODOs for addressing the above, but it'd be
good to open issues upstream on the `password-hash` crate about these
problems, and ideally update the TODOs in the code with references to
the issues.
  • Loading branch information
tarcieri committed Feb 12, 2021
1 parent 25f5be0 commit 549a8a5
Showing 1 changed file with 35 additions and 1 deletion.
36 changes: 35 additions & 1 deletion argon2/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -571,6 +571,22 @@ impl PasswordHasher for Argon2<'_> {
)
.map_err(|_| HasherError::Params(ParamsError::InvalidValue))?;

if MAX_PWD_LENGTH < password.len() {
return Err(HasherError::Password);
}

if !(MIN_SALT_LENGTH..=MAX_SALT_LENGTH).contains(&salt_bytes.len()) {
// TODO(tarcieri): better error types for this case
return Err(HasherError::Crypto);
}

// Validate associated data (optional param)
if MAX_AD_LENGTH < ad.len() {
// TODO(tarcieri): better error types for this case
return Err(HasherError::Crypto);
}

// TODO(tarcieri): improve this API to eliminate redundant checks above
let output = password_hash::Output::init_with(params.output_length, |out| {
hasher
.hash_password_into(algorithm, password, salt_bytes, ad, out)
Expand All @@ -580,7 +596,7 @@ impl PasswordHasher for Argon2<'_> {
Error::OutputTooLong => password_hash::OutputError::TooLong,
// Other cases are not returned from `hash_password_into`
// TODO(tarcieri): finer-grained error types?
_ => unreachable!(),
_ => panic!("unhandled error type: {}", e),
}
})
})?;
Expand Down Expand Up @@ -671,3 +687,21 @@ impl<'a> TryFrom<Params> for ParamsString {
Ok(output)
}
}

#[cfg(all(test, features = "password-hash"))]
mod tests {
use super::{Argon2, Params, PasswordHasher, Salt};

/// Example password only: don't use this as a real password!!!
const EXAMPLE_PASSWORD: &[u8] = b"hunter42";

#[test]
fn decoded_salt_too_short() {
let argon2 = Argon2::default();

// Too short after decoding
let salt = Salt::new("somesalt").unwrap();

let _ = argon2.hash_password(EXAMPLE_PASSWORD, None, None, Params::default(), salt);
}
}

0 comments on commit 549a8a5

Please sign in to comment.