feat: replaced flaky argon2 with better argon2 crate (#37)

* feat: replaced flaky argon2 with better argon2 crate

* fix: applied cargo fmt nightly

* docs: added comment specifying what the settings for Argon2 mean

* fix: made hashing error a bit more descriptive

* fix: fixed incorrect value for Kib

src/api/client_server/session.rs

@@ -1,5 +1,6 @@
 use super::{DEVICE_ID_LENGTH, TOKEN_LENGTH};
 use crate::{services, utils, Error, Result, Ruma};
+use argon2::{PasswordHash, PasswordVerifier};
 use ruma::{
     api::client::{
         error::ErrorKind,
@@ -9,7 +10,7 @@ use ruma::{
     UserId,
 };
 use serde::Deserialize;
-use tracing::{info, warn};
+use tracing::{error, info, warn};
 
 #[derive(Debug, Deserialize)]
 struct Claims {
@@ -74,9 +75,15 @@ pub async fn login_route(body: Ruma<login::v3::Request>) -> Result<login::v3::Re
                     "The user has been deactivated",
                 ));
             }
-
-            let hash_matches = argon2::verify_encoded(&hash, password.as_bytes()).unwrap_or(false);
-
+            let Ok(parsed_hash) = PasswordHash::new(&hash) else {
+                error!("error while hashing user {}", user_id);
+                return Err(Error::BadServerResponse("could not hash"));
+            };
+            let hash_matches = services()
+                .globals
+                .argon
+                .verify_password(password.as_bytes(), &parsed_hash)
+                .is_ok();
             if !hash_matches {
                 return Err(Error::BadRequest(
                     ErrorKind::Forbidden,

src/database/mod.rs

@@ -6,8 +6,10 @@ use crate::{
     services, utils, Config, Error, PduEvent, Result, Services, SERVICES,
 };
 use abstraction::{KeyValueDatabaseEngine, KvTree};
+use argon2::{password_hash::SaltString, PasswordHasher, PasswordVerifier};
 use directories::ProjectDirs;
 use lru_cache::LruCache;
+use rand::thread_rng;
 use ruma::{
     events::{
         push_rules::{PushRulesEvent, PushRulesEventContent},
@@ -464,11 +466,17 @@ impl KeyValueDatabase {
             if services().globals.database_version()? < 2 {
                 // We accidentally inserted hashed versions of "" into the db instead of just ""
                 for (userid, password) in db.userid_password.iter() {
-                    let password = utils::string_from_bytes(&password);
-
-                    let empty_hashed_password = password.map_or(false, |password| {
-                        argon2::verify_encoded(&password, b"").unwrap_or(false)
-                    });
+                    let salt = SaltString::generate(thread_rng());
+                    let empty_pass = services()
+                        .globals
+                        .argon
+                        .hash_password(b"", &salt)
+                        .expect("our own password to be properly hashed");
+                    let empty_hashed_password = services()
+                        .globals
+                        .argon
+                        .verify_password(&password, &empty_pass)
+                        .is_ok();
 
                     if empty_hashed_password {
                         db.userid_password.insert(&userid, b"")?;

src/lib.rs

@@ -20,9 +20,9 @@ pub use database::KeyValueDatabase;
 pub use service::{pdu::PduEvent, Services};
 pub use utils::error::{Error, Result};
 
-pub static SERVICES: RwLock<Option<&'static Services>> = RwLock::new(None);
+pub static SERVICES: RwLock<Option<&'static Services<'static>>> = RwLock::new(None);
 
-pub fn services() -> &'static Services {
+pub fn services() -> &'static Services<'static> {
     SERVICES
         .read()
         .unwrap()

src/service/globals/mod.rs

@@ -1,4 +1,5 @@
 mod data;
+use argon2::Argon2;
 pub use data::Data;
 use ruma::{
     serde::Base64, OwnedDeviceId, OwnedEventId, OwnedRoomId, OwnedServerName,
@@ -51,7 +52,7 @@ type SyncHandle = (
     Receiver<Option<Result<sync_events::v3::Response>>>, // rx
 );
 
-pub struct Service {
+pub struct Service<'a> {
     pub db: &'static dyn Data,
 
     pub actual_destination_cache: Arc<RwLock<WellKnownMap>>, // actual_destination, host
@@ -77,6 +78,7 @@ pub struct Service {
     pub rotate: RotationHandler,
 
     pub shutdown: AtomicBool,
+    pub argon: Argon2<'a>,
 }
 
 /// Handles "rotation" of long-polling requests. "Rotation" in this context is similar to "rotation" of log files and the like.
@@ -140,7 +142,7 @@ impl Resolve for Resolver {
     }
 }
 
-impl Service {
+impl Service<'_> {
     pub fn load(db: &'static dyn Data, config: Config) -> Result<Self> {
         let keypair = db.load_keypair();
 
@@ -188,7 +190,12 @@ impl Service {
             RoomVersionId::V5,
             RoomVersionId::V11,
         ];
-
+        // 19456 Kib blocks, iterations = 2, parallelism = 1 for more info https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#argon2id
+        let argon = Argon2::new(
+            argon2::Algorithm::Argon2id,
+            argon2::Version::default(),
+            argon2::Params::new(19456, 2, 1, None).expect("valid parameters"),
+        );
         let mut s = Self {
             db,
             config,
@@ -219,6 +226,7 @@ impl Service {
             sync_receivers: RwLock::new(HashMap::new()),
             rotate: RotationHandler::new(),
             shutdown: AtomicBool::new(false),
+            argon,
         };
 
         fs::create_dir_all(s.get_media_folder())?;

src/service/mod.rs

@@ -21,7 +21,7 @@ pub mod transaction_ids;
 pub mod uiaa;
 pub mod users;
 
-pub struct Services {
+pub struct Services<'a> {
     pub appservice: appservice::Service,
     pub pusher: pusher::Service,
     pub rooms: rooms::Service,
@@ -30,13 +30,13 @@ pub struct Services {
     pub users: users::Service,
     pub account_data: account_data::Service,
     pub admin: Arc<admin::Service>,
-    pub globals: globals::Service,
+    pub globals: globals::Service<'a>,
     pub key_backups: key_backups::Service,
     pub media: media::Service,
     pub sending: Arc<sending::Service>,
 }
 
-impl Services {
+impl Services<'_> {
     pub fn build<
         D: appservice::Data
             + pusher::Data

src/service/uiaa/mod.rs

@@ -1,5 +1,6 @@
 mod data;
 
+use argon2::{PasswordHash, PasswordVerifier};
 pub use data::Data;
 
 use ruma::{
@@ -81,8 +82,14 @@ impl Service {
 
                 // Check if password is correct
                 if let Some(hash) = services().users.password_hash(&user_id)? {
-                    let hash_matches =
-                        argon2::verify_encoded(&hash, password.as_bytes()).unwrap_or(false);
+                    let hash_matches = services()
+                        .globals
+                        .argon
+                        .verify_password(
+                            password.as_bytes(),
+                            &PasswordHash::new(&hash).expect("valid hash in database"),
+                        )
+                        .is_ok();
 
                     if !hash_matches {
                         uiaainfo.auth_error = Some(ruma::api::client::error::StandardErrorBody {

src/utils/mod.rs

@@ -1,7 +1,7 @@
 pub mod error;
 
-use crate::{Error, Result};
-use argon2::{Config, Variant};
+use crate::{services, Error, Result};
+use argon2::{password_hash::SaltString, PasswordHasher};
 use rand::prelude::*;
 use ring::digest;
 use ruma::{
@@ -72,14 +72,13 @@ pub fn random_string(length: usize) -> String {
 }
 
 /// Calculate a new hash for the given password
-pub fn calculate_password_hash(password: &str) -> Result<String, argon2::Error> {
-    let hashing_config = Config {
-        variant: Variant::Argon2id,
-        ..Config::owasp2() // m=19456 (19 MiB), t=2, p=1 from https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#argon2id
-    };
-
-    let salt = random_string(32);
-    argon2::hash_encoded(password.as_bytes(), salt.as_bytes(), &hashing_config)
+pub fn calculate_password_hash(password: &str) -> Result<String, argon2::password_hash::Error> {
+    let salt = SaltString::generate(thread_rng());
+    services()
+        .globals
+        .argon
+        .hash_password(password.as_bytes(), &salt)
+        .map(|it| it.to_string())
 }
 
 #[tracing::instrument(skip(keys))]