From af4f01b7697d674f94a70beb2966b6d237a0aa68 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Sun, 22 May 2022 10:10:23 +0200 Subject: [PATCH] Check timings when validating an SSO login - exchanging a token twice should not work - exchanging a token more than 30s after its fullfillment should not work - exchanging a pending token should not work - fullfilling a login more than 30min after its creation should not work - also have better errors in some cases --- crates/handlers/src/compat/login.rs | 61 ++++++++++++++++++- .../handlers/src/compat/login_sso_complete.rs | 24 +++++++- crates/storage/src/compat.rs | 32 +++++++--- 3 files changed, 105 insertions(+), 12 deletions(-) diff --git a/crates/handlers/src/compat/login.rs b/crates/handlers/src/compat/login.rs index ee4a8536..a1688529 100644 --- a/crates/handlers/src/compat/login.rs +++ b/crates/handlers/src/compat/login.rs @@ -13,14 +13,15 @@ // limitations under the License. use axum::{response::IntoResponse, Extension, Json}; -use chrono::Duration; +use chrono::{Duration, Utc}; use hyper::StatusCode; use mas_config::MatrixConfig; -use mas_data_model::{CompatSession, Device, TokenType}; +use mas_data_model::{CompatSession, CompatSsoLoginState, Device, TokenType}; use mas_storage::{ compat::{ add_compat_access_token, add_compat_refresh_token, compat_login, get_compat_sso_login_by_token, mark_compat_sso_login_as_exchanged, + CompatSsoLoginLookupError, }, PostgresqlBackend, }; @@ -132,6 +133,12 @@ pub enum RouteError { #[error("login failed")] LoginFailed, + + #[error("login took too long")] + LoginTookTooLong, + + #[error("invalid login token")] + InvalidLoginToken, } impl From for RouteError { @@ -140,6 +147,16 @@ impl From for RouteError { } } +impl From for RouteError { + fn from(e: CompatSsoLoginLookupError) -> Self { + if e.not_found() { + Self::InvalidLoginToken + } else { + Self::Internal(Box::new(e)) + } + } +} + impl IntoResponse for RouteError { fn into_response(self) -> axum::response::Response { match self { @@ -158,6 +175,16 @@ impl IntoResponse for RouteError { error: "Invalid username/password", status: StatusCode::FORBIDDEN, }, + Self::LoginTookTooLong => MatrixError { + errcode: "M_UNAUTHORIZED", + error: "Login token expired", + status: StatusCode::FORBIDDEN, + }, + Self::InvalidLoginToken => MatrixError { + errcode: "M_UNAUTHORIZED", + error: "Invalid login token", + status: StatusCode::FORBIDDEN, + }, } .into_response() } @@ -222,10 +249,38 @@ async fn token_login( token: &str, ) -> Result, RouteError> { let login = get_compat_sso_login_by_token(&mut *txn, token).await?; + + let now = Utc::now(); + match login.state { + CompatSsoLoginState::Pending => { + tracing::error!( + login.data, + "Exchanged a token for a login that was not fullfilled yet" + ); + return Err(RouteError::InvalidLoginToken); + } + CompatSsoLoginState::Fullfilled { fullfilled_at, .. } => { + if now > fullfilled_at + Duration::seconds(30) { + return Err(RouteError::LoginTookTooLong); + } + } + CompatSsoLoginState::Exchanged { exchanged_at, .. } => { + if now > exchanged_at + Duration::seconds(30) { + // TODO: log that session out + tracing::error!( + login.data, + "Login token exchanged a second time more than 30s after" + ); + } + + return Err(RouteError::InvalidLoginToken); + } + } + let login = mark_compat_sso_login_as_exchanged(&mut *txn, login).await?; match login.state { - mas_data_model::CompatSsoLoginState::Exchanged { session, .. } => Ok(session), + CompatSsoLoginState::Exchanged { session, .. } => Ok(session), _ => unreachable!(), } } diff --git a/crates/handlers/src/compat/login_sso_complete.rs b/crates/handlers/src/compat/login_sso_complete.rs index 54a03c98..4f65db2c 100644 --- a/crates/handlers/src/compat/login_sso_complete.rs +++ b/crates/handlers/src/compat/login_sso_complete.rs @@ -21,6 +21,7 @@ use axum::{ Extension, }; use axum_extra::extract::PrivateCookieJar; +use chrono::{Duration, Utc}; use mas_axum_utils::{ csrf::{CsrfExt, ProtectedForm}, FancyError, SessionInfoExt, @@ -29,7 +30,7 @@ use mas_config::Encrypter; use mas_data_model::Device; use mas_router::Route; use mas_storage::compat::{fullfill_compat_sso_login, get_compat_sso_login_by_id}; -use mas_templates::{CompatSsoContext, TemplateContext, Templates}; +use mas_templates::{CompatSsoContext, ErrorContext, TemplateContext, Templates}; use rand::thread_rng; use serde::Serialize; use sqlx::PgPool; @@ -66,6 +67,16 @@ pub async fn get( let login = get_compat_sso_login_by_id(&mut conn, id).await?; + // Bail out if that login session is more than 30min old + if Utc::now() > login.created_at + Duration::minutes(30) { + let ctx = ErrorContext::new() + .with_code("compat_sso_login_expired") + .with_description("This login session expired.".to_string()); + + let content = templates.render_error(&ctx).await?; + return Ok((cookie_jar, Html(content)).into_response()); + } + let ctx = CompatSsoContext::new(login) .with_session(session) .with_csrf(csrf_token.form_value()); @@ -77,6 +88,7 @@ pub async fn get( pub async fn post( Extension(pool): Extension, + Extension(templates): Extension, cookie_jar: PrivateCookieJar, Path(id): Path, Form(form): Form>, @@ -98,6 +110,16 @@ pub async fn post( let login = get_compat_sso_login_by_id(&mut txn, id).await?; + // Bail out if that login session is more than 30min old + if Utc::now() > login.created_at + Duration::minutes(30) { + let ctx = ErrorContext::new() + .with_code("compat_sso_login_expired") + .with_description("This login session expired.".to_string()); + + let content = templates.render_error(&ctx).await?; + return Ok((cookie_jar, Html(content)).into_response()); + } + let redirect_uri = { let mut redirect_uri = login.redirect_uri.clone(); let existing_params = redirect_uri diff --git a/crates/storage/src/compat.rs b/crates/storage/src/compat.rs index 8b1e06dc..55a28653 100644 --- a/crates/storage/src/compat.rs +++ b/crates/storage/src/compat.rs @@ -29,7 +29,7 @@ use crate::{ user::lookup_user_by_username, DatabaseInconsistencyError, IdAndCreationTime, PostgresqlBackend, }; -pub struct CompatAccessTokenLookup { +struct CompatAccessTokenLookup { compat_access_token_id: i64, compat_access_token: String, compat_access_token_created_at: DateTime, @@ -654,12 +654,26 @@ impl TryFrom for CompatSsoLogin { } } +#[derive(Debug, Error)] +#[error("failed to lookup compat SSO login")] +pub enum CompatSsoLoginLookupError { + Database(#[from] sqlx::Error), + Inconsistency(#[from] DatabaseInconsistencyError), +} + +impl CompatSsoLoginLookupError { + #[must_use] + pub fn not_found(&self) -> bool { + matches!(self, Self::Database(sqlx::Error::RowNotFound)) + } +} + #[allow(clippy::too_many_lines)] #[tracing::instrument(skip(executor), err)] pub async fn get_compat_sso_login_by_id( executor: impl PgExecutor<'_>, id: i64, -) -> anyhow::Result> { +) -> Result, CompatSsoLoginLookupError> { let res = sqlx::query_as!( CompatSsoLoginLookup, r#" @@ -693,8 +707,7 @@ pub async fn get_compat_sso_login_by_id( ) .fetch_one(executor) .instrument(tracing::info_span!("Lookup compat SSO login")) - .await - .context("could not lookup compat SSO login")?; + .await?; Ok(res.try_into()?) } @@ -704,7 +717,7 @@ pub async fn get_compat_sso_login_by_id( pub async fn get_compat_sso_login_by_token( executor: impl PgExecutor<'_>, token: &str, -) -> anyhow::Result> { +) -> Result, CompatSsoLoginLookupError> { let res = sqlx::query_as!( CompatSsoLoginLookup, r#" @@ -738,8 +751,7 @@ pub async fn get_compat_sso_login_by_token( ) .fetch_one(executor) .instrument(tracing::info_span!("Lookup compat SSO login")) - .await - .context("could not lookup compat SSO login")?; + .await?; Ok(res.try_into()?) } @@ -750,8 +762,12 @@ pub async fn fullfill_compat_sso_login( mut login: CompatSsoLogin, device: Device, ) -> anyhow::Result> { - // TODO: check if login is in pending state + if !matches!(login.state, CompatSsoLoginState::Pending) { + bail!("sso login in wrong state"); + }; + let mut txn = conn.begin().await.context("could not start transaction")?; + let res = sqlx::query_as!( IdAndCreationTime, r#"