From 848af4cecde79bc1cc63edce3a215326b3b2ad9f Mon Sep 17 00:00:00 2001 From: CEbbinghaus Date: Wed, 19 Feb 2025 17:54:50 +1100 Subject: [PATCH] TOTP label verification (#3419) * Adding TOTP Label verification (for both empty and duplicate) --- proto/src/internal/credupdate.rs | 1 + server/core/src/https/v1.rs | 2 + server/core/src/https/views/reset.rs | 28 +++++- .../credential_update_add_totp_partial.html | 15 ++- server/lib/src/credential/mod.rs | 7 ++ server/lib/src/idm/credupdatesession.rs | 96 ++++++++++++++++++- tools/cli/src/cli/person.rs | 14 +++ 7 files changed, 154 insertions(+), 9 deletions(-) diff --git a/proto/src/internal/credupdate.rs b/proto/src/internal/credupdate.rs index 5a7fa66d6..8fe2fa8f4 100644 --- a/proto/src/internal/credupdate.rs +++ b/proto/src/internal/credupdate.rs @@ -130,6 +130,7 @@ pub enum CURegState { None, TotpCheck(TotpSecret), TotpTryAgain, + TotpNameTryAgain(String), TotpInvalidSha1, BackupCodes(Vec), Passkey(CreationChallengeResponse), diff --git a/server/core/src/https/v1.rs b/server/core/src/https/v1.rs index c410a4b5d..30de387b8 100644 --- a/server/core/src/https/v1.rs +++ b/server/core/src/https/v1.rs @@ -1396,6 +1396,7 @@ pub async fn credential_update_update( return Err(WebError::InternalServerError(errmsg)); } }; + let session_token = match serde_json::from_value(cubody[1].clone()) { Ok(val) => val, Err(err) => { @@ -1406,6 +1407,7 @@ pub async fn credential_update_update( }; debug!("session_token: {:?}", session_token); debug!("scr: {:?}", scr); + state .qe_r_ref .handle_idmcredentialupdate(session_token, scr, kopid.eventid) diff --git a/server/core/src/https/views/reset.rs b/server/core/src/https/views/reset.rs index 36cea8ffb..195e042a6 100644 --- a/server/core/src/https/views/reset.rs +++ b/server/core/src/https/views/reset.rs @@ -210,6 +210,8 @@ pub(crate) struct TotpInit { pub(crate) struct TotpCheck { wrong_code: bool, broken_app: bool, + bad_name: bool, + taken_name: Option, } #[derive(Template)] @@ -599,6 +601,25 @@ pub(crate) async fn add_totp( let cu_session_token = get_cu_session(&jar).await?; let check_totpcode = u32::from_str(&new_totp_form.check_totpcode).unwrap_or_default(); + let swapped_handler_trigger = + HxResponseTrigger::after_swap([HxEvent::new("addTotpSwapped".to_string())]); + + // If the user has not provided a name or added only spaces we exit early + if new_totp_form.name.trim().is_empty() { + return Ok(( + swapped_handler_trigger, + AddTotpPartial { + totp_init: None, + totp_name: "".into(), + totp_value: new_totp_form.check_totpcode.clone(), + check: TotpCheck { + bad_name: true, + ..Default::default() + }, + }, + ) + .into_response()); + } let cu_status = if new_totp_form.ignore_broken_app { // Cope with SHA1 apps because the user has intended to do so, their totp code was already verified @@ -624,6 +645,10 @@ pub(crate) async fn add_totp( wrong_code: true, ..Default::default() }, + CURegState::TotpNameTryAgain(val) => TotpCheck { + taken_name: Some(val.clone()), + ..Default::default() + }, CURegState::TotpInvalidSha1 => TotpCheck { broken_app: true, ..Default::default() @@ -646,9 +671,6 @@ pub(crate) async fn add_totp( new_totp_form.check_totpcode.clone() }; - let swapped_handler_trigger = - HxResponseTrigger::after_swap([HxEvent::new("addTotpSwapped".to_string())]); - Ok(( swapped_handler_trigger, AddTotpPartial { diff --git a/server/core/templates/credential_update_add_totp_partial.html b/server/core/templates/credential_update_add_totp_partial.html index e9df734e6..3f18e6e32 100644 --- a/server/core/templates/credential_update_add_totp_partial.html +++ b/server/core/templates/credential_update_add_totp_partial.html @@ -19,7 +19,8 @@ Incorrect TOTP code - Please try again + (% else if check.bad_name %) +
+
    +
  • The name you provided was empty or blank. Please provide a proper name
    • +
+
+ (% else if let Some(name) = check.taken_name %) +
+
    +
  • The name "((name))" is either invalid or already taken, Please pick a different one
    • +
+
(% endif %) diff --git a/server/lib/src/credential/mod.rs b/server/lib/src/credential/mod.rs index aad5e8ca2..a6c4380ef 100644 --- a/server/lib/src/credential/mod.rs +++ b/server/lib/src/credential/mod.rs @@ -702,6 +702,13 @@ impl Credential { } } + pub(crate) fn has_totp_by_name(&self, label: &str) -> bool { + match &self.type_ { + CredentialType::PasswordMfa(_, totp, _, _) => totp.contains_key(label), + _ => false, + } + } + pub(crate) fn new_from_generatedpassword(pw: Password) -> Self { Credential { type_: CredentialType::GeneratedPassword(pw), diff --git a/server/lib/src/idm/credupdatesession.rs b/server/lib/src/idm/credupdatesession.rs index 227f438d5..9d4ce0299 100644 --- a/server/lib/src/idm/credupdatesession.rs +++ b/server/lib/src/idm/credupdatesession.rs @@ -86,6 +86,7 @@ enum MfaRegState { None, TotpInit(Totp), TotpTryAgain(Totp), + TotpNameTryAgain(Totp, String), TotpInvalidSha1(Totp, Totp, String), Passkey(Box, PasskeyRegistration), #[allow(dead_code)] @@ -98,6 +99,7 @@ impl fmt::Debug for MfaRegState { MfaRegState::None => "MfaRegState::None", MfaRegState::TotpInit(_) => "MfaRegState::TotpInit", MfaRegState::TotpTryAgain(_) => "MfaRegState::TotpTryAgain", + MfaRegState::TotpNameTryAgain(_, _) => "MfaRegState::TotpNameTryAgain", MfaRegState::TotpInvalidSha1(_, _, _) => "MfaRegState::TotpInvalidSha1", MfaRegState::Passkey(_, _) => "MfaRegState::Passkey", MfaRegState::AttestedPasskey(_, _) => "MfaRegState::AttestedPasskey", @@ -273,6 +275,7 @@ pub enum MfaRegStateStatus { None, TotpCheck(TotpSecret), TotpTryAgain, + TotpNameTryAgain(String), TotpInvalidSha1, BackupCodes(HashSet), Passkey(CreationChallengeResponse), @@ -283,8 +286,9 @@ impl fmt::Debug for MfaRegStateStatus { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let t = match self { MfaRegStateStatus::None => "MfaRegStateStatus::None", - MfaRegStateStatus::TotpCheck(_) => "MfaRegStateStatus::TotpCheck(_)", + MfaRegStateStatus::TotpCheck(_) => "MfaRegStateStatus::TotpCheck", MfaRegStateStatus::TotpTryAgain => "MfaRegStateStatus::TotpTryAgain", + MfaRegStateStatus::TotpNameTryAgain(_) => "MfaRegStateStatus::TotpNameTryAgain", MfaRegStateStatus::TotpInvalidSha1 => "MfaRegStateStatus::TotpInvalidSha1", MfaRegStateStatus::BackupCodes(_) => "MfaRegStateStatus::BackupCodes", MfaRegStateStatus::Passkey(_) => "MfaRegStateStatus::Passkey", @@ -389,6 +393,7 @@ impl Into for CredentialUpdateSessionStatus { MfaRegStateStatus::None => CURegState::None, MfaRegStateStatus::TotpCheck(c) => CURegState::TotpCheck(c), MfaRegStateStatus::TotpTryAgain => CURegState::TotpTryAgain, + MfaRegStateStatus::TotpNameTryAgain(label) => CURegState::TotpNameTryAgain(label), MfaRegStateStatus::TotpInvalidSha1 => CURegState::TotpInvalidSha1, MfaRegStateStatus::BackupCodes(s) => { CURegState::BackupCodes(s.into_iter().collect()) @@ -469,6 +474,9 @@ impl From<&CredentialUpdateSession> for CredentialUpdateSessionStatus { MfaRegState::TotpInit(token) => MfaRegStateStatus::TotpCheck( token.to_proto(session.account.name.as_str(), session.issuer.as_str()), ), + MfaRegState::TotpNameTryAgain(_, name) => { + MfaRegStateStatus::TotpNameTryAgain(name.clone()) + } MfaRegState::TotpTryAgain(_) => MfaRegStateStatus::TotpTryAgain, MfaRegState::TotpInvalidSha1(_, _, _) => MfaRegStateStatus::TotpInvalidSha1, MfaRegState::Passkey(r, _) => MfaRegStateStatus::Passkey(r.as_ref().clone()), @@ -1899,7 +1907,22 @@ impl IdmServerCredUpdateTransaction<'_> { match &session.mfaregstate { MfaRegState::TotpInit(totp_token) | MfaRegState::TotpTryAgain(totp_token) + | MfaRegState::TotpNameTryAgain(totp_token, _) | MfaRegState::TotpInvalidSha1(totp_token, _, _) => { + if session + .primary + .as_ref() + .map(|cred| cred.has_totp_by_name(label)) + .unwrap_or_default() + || label.trim().is_empty() + || !Value::validate_str_escapes(label) + { + // The user is trying to add a second TOTP under the same name. Lets save them from themselves + session.mfaregstate = + MfaRegState::TotpNameTryAgain(totp_token.clone(), label.into()); + return Ok(session.deref().into()); + } + if totp_token.verify(totp_chal, ct) { // It was valid. Update the credential. let ncred = session @@ -3368,10 +3391,39 @@ mod tests { .credential_primary_check_totp(&cust, ct, chal + 1, "totp") .expect("Failed to update the primary cred totp"); - assert!(matches!( - c_status.mfaregstate, - MfaRegStateStatus::TotpTryAgain - )); + assert!( + matches!(c_status.mfaregstate, MfaRegStateStatus::TotpTryAgain), + "{:?}", + c_status.mfaregstate + ); + + // Check that the user actually put something into the label + let c_status = cutxn + .credential_primary_check_totp(&cust, ct, chal, "") + .expect("Failed to update the primary cred totp"); + + assert!( + matches!( + c_status.mfaregstate, + MfaRegStateStatus::TotpNameTryAgain(ref val) if val == "" + ), + "{:?}", + c_status.mfaregstate + ); + + // Okay, Now they are trying to be smart... + let c_status = cutxn + .credential_primary_check_totp(&cust, ct, chal, " ") + .expect("Failed to update the primary cred totp"); + + assert!( + matches!( + c_status.mfaregstate, + MfaRegStateStatus::TotpNameTryAgain(ref val) if val == " " + ), + "{:?}", + c_status.mfaregstate + ); let c_status = cutxn .credential_primary_check_totp(&cust, ct, chal, "totp") @@ -3383,6 +3435,40 @@ mod tests { _ => false, }); + { + let c_status = cutxn + .credential_primary_init_totp(&cust, ct) + .expect("Failed to update the primary cred password"); + + // Check the status has the token. + let totp_token: Totp = match c_status.mfaregstate { + MfaRegStateStatus::TotpCheck(secret) => Some(secret.try_into().unwrap()), + _ => None, + } + .expect("Unable to retrieve totp token, invalid state."); + + trace!(?totp_token); + let chal = totp_token + .do_totp_duration_from_epoch(&ct) + .expect("Failed to perform totp step"); + + // They tried to add a second totp under the same name + let c_status = cutxn + .credential_primary_check_totp(&cust, ct, chal, "totp") + .expect("Failed to update the primary cred totp"); + + assert!( + matches!( + c_status.mfaregstate, + MfaRegStateStatus::TotpNameTryAgain(ref val) if val == "totp" + ), + "{:?}", + c_status.mfaregstate + ); + + assert!(cutxn.credential_update_cancel_mfareg(&cust, ct).is_ok()) + } + // Should be okay now! drop(cutxn); diff --git a/tools/cli/src/cli/person.rs b/tools/cli/src/cli/person.rs index ce14fdf5b..87c2ad765 100644 --- a/tools/cli/src/cli/person.rs +++ b/tools/cli/src/cli/person.rs @@ -831,6 +831,13 @@ async fn totp_enroll_prompt(session_token: &CUSessionToken, client: &KanidmClien let label: String = Input::new() .with_prompt("TOTP Label") + .validate_with(|input: &String| -> Result<(), &str> { + if input.trim().is_empty() { + Err("Label cannot be empty") + } else { + Ok(()) + } + }) .interact_text() .expect("Failed to interact with interactive session"); @@ -919,6 +926,13 @@ async fn totp_enroll_prompt(session_token: &CUSessionToken, client: &KanidmClien eprintln!("Incorrect TOTP code entered. Please try again."); continue; } + Ok(CUStatus { + mfaregstate: CURegState::TotpNameTryAgain(label), + .. + }) => { + eprintln!("{label} is either invalid or already taken. Please try again."); + continue; + } Ok(CUStatus { mfaregstate: CURegState::TotpInvalidSha1, ..