From 75a42c27db119305df4278f77d742fb62acf321a Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Wed, 6 Mar 2024 20:51:43 -0700 Subject: [PATCH] Migrate from scrypt to sha256. (#8969) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reduces the server time to compute the hash from 40ms to 5µs, which should remove this as a noticable chunk of CPU time in production. (An attacker who has access to our database will now need only 10^54 years of CPU time instead of 10^58 to brute force a token). Release Notes: - Improved sign in latency by 40ms. --- Cargo.lock | 2 + Cargo.toml | 2 + crates/collab/Cargo.toml | 2 + crates/collab/src/auth.rs | 198 +++++++++++++++--- crates/collab/src/db/queries/access_tokens.rs | 18 ++ crates/rpc/Cargo.toml | 2 +- 6 files changed, 197 insertions(+), 27 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 591d136d6a..ed386a0bff 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2223,6 +2223,7 @@ dependencies = [ "aws-sdk-s3", "axum", "axum-extra", + "base64 0.13.1", "call", "channel", "chrono", @@ -2272,6 +2273,7 @@ dependencies = [ "settings", "sha2 0.10.7", "sqlx", + "subtle", "telemetry_events", "text", "theme", diff --git a/Cargo.toml b/Cargo.toml index 5aef34228f..963901d40d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -105,6 +105,7 @@ assets = { path = "crates/assets" } assistant = { path = "crates/assistant" } audio = { path = "crates/audio" } auto_update = { path = "crates/auto_update" } +base64 = "0.13" breadcrumbs = { path = "crates/breadcrumbs" } call = { path = "crates/call" } channel = { path = "crates/channel" } @@ -252,6 +253,7 @@ shellexpand = "2.1.0" smallvec = { version = "1.6", features = ["union"] } smol = "1.2" strum = { version = "0.25.0", features = ["derive"] } +subtle = "2.5.0" sysinfo = "0.29.10" tempfile = "3.9.0" thiserror = "1.0.29" diff --git a/crates/collab/Cargo.toml b/crates/collab/Cargo.toml index 0e4df30987..0f4b706b5a 100644 --- a/crates/collab/Cargo.toml +++ b/crates/collab/Cargo.toml @@ -23,6 +23,7 @@ aws-config = { version = "1.1.5" } aws-sdk-s3 = { version = "1.15.0" } axum = { version = "0.6", features = ["json", "headers", "ws"] } axum-extra = { version = "0.4", features = ["erased-json"] } +base64.workspace = true chrono.workspace = true clock.workspace = true clickhouse.workspace = true @@ -48,6 +49,7 @@ serde_derive.workspace = true serde_json.workspace = true sha2.workspace = true sqlx = { version = "0.7", features = ["runtime-tokio-rustls", "postgres", "json", "time", "uuid", "any"] } +subtle.workspace = true rustc-demangle.workspace = true telemetry_events.workspace = true text.workspace = true diff --git a/crates/collab/src/auth.rs b/crates/collab/src/auth.rs index 59141e252e..26f6ede3d3 100644 --- a/crates/collab/src/auth.rs +++ b/crates/collab/src/auth.rs @@ -9,14 +9,15 @@ use axum::{ response::IntoResponse, }; use prometheus::{exponential_buckets, register_histogram, Histogram}; -use rand::thread_rng; use scrypt::{ - password_hash::{PasswordHash, PasswordHasher, PasswordVerifier, SaltString}, + password_hash::{PasswordHash, PasswordVerifier}, Scrypt, }; use serde::{Deserialize, Serialize}; +use sha2::Digest; use std::sync::OnceLock; use std::{sync::Arc, time::Instant}; +use subtle::ConstantTimeEq; #[derive(Clone, Debug, Default, PartialEq, Eq)] pub struct Impersonator(pub Option); @@ -115,8 +116,7 @@ pub async fn create_access_token( ) -> Result { const VERSION: usize = 1; let access_token = rpc::auth::random_token(); - let access_token_hash = - hash_access_token(&access_token).context("failed to hash access token")?; + let access_token_hash = hash_access_token(&access_token); let id = db .create_access_token( user_id, @@ -132,23 +132,15 @@ pub async fn create_access_token( })?) } -fn hash_access_token(token: &str) -> Result { - // Avoid slow hashing in debug mode. - let params = if cfg!(debug_assertions) { - scrypt::Params::new(1, 1, 1).unwrap() - } else { - scrypt::Params::new(14, 8, 1).unwrap() - }; - - Ok(Scrypt - .hash_password( - token.as_bytes(), - None, - params, - &SaltString::generate(thread_rng()), - ) - .map_err(anyhow::Error::new)? - .to_string()) +/// Hashing prevents anyone with access to the database being able to login. +/// As the token is randomly generated, we don't need to worry about scrypt-style +/// protection. +fn hash_access_token(token: &str) -> String { + let digest = sha2::Sha256::digest(token); + format!( + "$sha256${}", + base64::encode_config(digest, base64::URL_SAFE) + ) } /// Encrypts the given access token with the given public key to avoid leaking it on the way @@ -190,15 +182,27 @@ pub async fn verify_access_token( if token_user_id != user_id { return Err(anyhow!("no such access token"))?; } - - let db_hash = PasswordHash::new(&db_token.hash).map_err(anyhow::Error::new)?; let t0 = Instant::now(); - let is_valid = Scrypt - .verify_password(token.token.as_bytes(), &db_hash) - .is_ok(); + + let is_valid = if db_token.hash.starts_with("$scrypt$") { + let db_hash = PasswordHash::new(&db_token.hash).map_err(anyhow::Error::new)?; + Scrypt + .verify_password(token.token.as_bytes(), &db_hash) + .is_ok() + } else { + let token_hash = hash_access_token(&token.token); + db_token.hash.as_bytes().ct_eq(token_hash.as_ref()).into() + }; + let duration = t0.elapsed(); log::info!("hashed access token in {:?}", duration); metric_access_token_hashing_time.observe(duration.as_millis() as f64); + + if is_valid && db_token.hash.starts_with("$scrypt$") { + let new_hash = hash_access_token(&token.token); + db.update_access_token_hash(db_token.id, &new_hash).await?; + } + Ok(VerifyAccessTokenResult { is_valid, impersonator_id: if db_token.impersonated_user_id.is_some() { @@ -208,3 +212,145 @@ pub async fn verify_access_token( }, }) } + +#[cfg(test)] +mod test { + use rand::thread_rng; + use scrypt::password_hash::{PasswordHasher, SaltString}; + use sea_orm::EntityTrait; + + use super::*; + use crate::db::{access_token, NewUserParams}; + + #[gpui::test] + async fn test_verify_access_token(cx: &mut gpui::TestAppContext) { + let test_db = crate::db::TestDb::postgres(cx.executor().clone()); + let db = test_db.db(); + + let user = db + .create_user( + "example@example.com", + false, + NewUserParams { + github_login: "example".into(), + github_user_id: 1, + }, + ) + .await + .unwrap(); + + let token = create_access_token(&db, user.user_id, None).await.unwrap(); + assert!(matches!( + verify_access_token(&token, user.user_id, &db) + .await + .unwrap(), + VerifyAccessTokenResult { + is_valid: true, + impersonator_id: None, + } + )); + + let old_token = create_previous_access_token(user.user_id, None, &db) + .await + .unwrap(); + + let old_token_id = serde_json::from_str::(&old_token) + .unwrap() + .id; + + let hash = db + .transaction(|tx| async move { + Ok(access_token::Entity::find_by_id(old_token_id) + .one(&*tx) + .await?) + }) + .await + .unwrap() + .unwrap() + .hash; + assert!(hash.starts_with("$scrypt$")); + + assert!(matches!( + verify_access_token(&old_token, user.user_id, &db) + .await + .unwrap(), + VerifyAccessTokenResult { + is_valid: true, + impersonator_id: None, + } + )); + + let hash = db + .transaction(|tx| async move { + Ok(access_token::Entity::find_by_id(old_token_id) + .one(&*tx) + .await?) + }) + .await + .unwrap() + .unwrap() + .hash; + assert!(hash.starts_with("$sha256$")); + + assert!(matches!( + verify_access_token(&old_token, user.user_id, &db) + .await + .unwrap(), + VerifyAccessTokenResult { + is_valid: true, + impersonator_id: None, + } + )); + + assert!(matches!( + verify_access_token(&token, user.user_id, &db) + .await + .unwrap(), + VerifyAccessTokenResult { + is_valid: true, + impersonator_id: None, + } + )); + } + + async fn create_previous_access_token( + user_id: UserId, + impersonated_user_id: Option, + db: &Database, + ) -> Result { + let access_token = rpc::auth::random_token(); + let access_token_hash = previous_hash_access_token(&access_token)?; + let id = db + .create_access_token( + user_id, + impersonated_user_id, + &access_token_hash, + MAX_ACCESS_TOKENS_TO_STORE, + ) + .await?; + Ok(serde_json::to_string(&AccessTokenJson { + version: 1, + id, + token: access_token, + })?) + } + + fn previous_hash_access_token(token: &str) -> Result { + // Avoid slow hashing in debug mode. + let params = if cfg!(debug_assertions) { + scrypt::Params::new(1, 1, 1).unwrap() + } else { + scrypt::Params::new(14, 8, 1).unwrap() + }; + + Ok(Scrypt + .hash_password( + token.as_bytes(), + None, + params, + &SaltString::generate(thread_rng()), + ) + .map_err(anyhow::Error::new)? + .to_string()) + } +} diff --git a/crates/collab/src/db/queries/access_tokens.rs b/crates/collab/src/db/queries/access_tokens.rs index af58d51a33..f251cdacba 100644 --- a/crates/collab/src/db/queries/access_tokens.rs +++ b/crates/collab/src/db/queries/access_tokens.rs @@ -55,4 +55,22 @@ impl Database { }) .await } + + /// Retrieves the access token with the given ID. + pub async fn update_access_token_hash( + &self, + id: AccessTokenId, + new_hash: &str, + ) -> Result { + self.transaction(|tx| async move { + Ok(access_token::Entity::update(access_token::ActiveModel { + id: ActiveValue::unchanged(id), + hash: ActiveValue::set(new_hash.into()), + ..Default::default() + }) + .exec(&*tx) + .await?) + }) + .await + } } diff --git a/crates/rpc/Cargo.toml b/crates/rpc/Cargo.toml index a4af21df11..be37140e60 100644 --- a/crates/rpc/Cargo.toml +++ b/crates/rpc/Cargo.toml @@ -19,7 +19,7 @@ test-support = ["collections/test-support", "gpui/test-support"] [dependencies] anyhow.workspace = true async-tungstenite = "0.16" -base64 = "0.13" +base64.workspace = true collections.workspace = true futures.workspace = true gpui = { workspace = true, optional = true }