From aff68120366db15297754508f1b8345d5b806fbc Mon Sep 17 00:00:00 2001 From: johnnyq Date: Wed, 4 Nov 2026 14:01:40 -0500 Subject: [PATCH] Security: hold an agent who must set up MFA to the set up page on every request, not just at sign in, and fix the client portal two-step gate, which a path added to the URL or a second action sent with the set up one got past - both now go through requestIsMfaSetup(), which checks the script actually running and every key in the request; View as contact is refused while an agent still owes MFA set up, and agents are emailed when MFA is turned on --- agent/user/post/profile.php | 5 +++++ client/includes/check_login.php | 9 +++++++-- client/post.php | 6 ++---- functions/auth.php | 25 +++++++++++++++++++++++++ functions/portal.php | 10 ++++++++-- includes/load_user_session.php | 11 +++++++++++ 6 files changed, 58 insertions(+), 8 deletions(-) diff --git a/agent/user/post/profile.php b/agent/user/post/profile.php index 7c455068c..3990089d8 100644 --- a/agent/user/post/profile.php +++ b/agent/user/post/profile.php @@ -157,6 +157,11 @@ if (isset($_POST['enable_mfa'])) { logAudit("User Account", "Edit", "$session_name enabled MFA on their account"); + // Setting MFA up needs only the password, so whoever holds it can do it - the email + // is how the real owner finds out an authenticator was added (the portal sends one too) + $me = userRow($session_user_id); + userNotifyChange($me['user_name'], $me['user_email'], 'multi-factor authentication was turned on'); + flashAlert("Multi-factor authentication is on - you will be asked for a code when you sign in"); // Clear the mfa_token from the session to avoid re-use. diff --git a/client/includes/check_login.php b/client/includes/check_login.php index 719e8122d..8520a2e8f 100644 --- a/client/includes/check_login.php +++ b/client/includes/check_login.php @@ -105,9 +105,14 @@ if (!$portal_view_as) { } // Two-step sign-in required for password accounts (Admin > Client Portal) and -// not set up yet: Your account until it is, and nothing else +// not set up yet: Your account until it is, and nothing else. The one gate for the +// pages and for client/post.php, judged on the script really running and every key +// in the request (requestIsMfaSetup()). $portal_mfa_setup_needed = !$portal_view_as && intval($config_client_portal_require_mfa ?? 0) === 1 && $session_auth_method === 'local' && !$session_mfa_on; -if ($portal_mfa_setup_needed && !in_array(basename($_SERVER['PHP_SELF']), ['profile.php', 'post.php'], true)) { +if ($portal_mfa_setup_needed && !requestIsMfaSetup([ + 'client/profile.php' => [[['mfa'], []]], + 'client/post.php' => [[[], ['csrf_token', 'portal_mfa_enable', 'verify_code']], [['logout'], []]], +])) { redirect("/client/profile.php?mfa=setup"); } diff --git a/client/post.php b/client/post.php index b487e89ef..7966a3aec 100644 --- a/client/post.php +++ b/client/post.php @@ -25,10 +25,8 @@ if (portalViewingAs() && !isset($_GET['export_statement_pdf'])) { redirect(); } -// Two-step sign-in required and not set up yet: setting it up, or signing out -if ($portal_mfa_setup_needed && !isset($_POST['portal_mfa_enable']) && !isset($_GET['logout'])) { - redirect('profile.php?mfa=setup'); -} +// Two-step sign-in required and not set up yet: client/includes/check_login.php has +// already turned away everything but setting it up, or signing out if (isset($_POST['add_ticket'])) { diff --git a/functions/auth.php b/functions/auth.php index 5b6670ec7..9c6308cb6 100644 --- a/functions/auth.php +++ b/functions/auth.php @@ -5,6 +5,31 @@ // When provided a module name (e.g. module_support), returns the associated permission level (false=none, 1=read, 2=write, 3=full). // The role's levels are read once a request - a page asks dozens of times. +/* + * Is this request one of the few a session may make while it still has to set up MFA? + * + * $allowed maps a script, from the app root, to the request shapes it may take - each a + * [GET keys, POST keys] pair, and every key in the request has to be one of them. The + * script is the file actually running, never PHP_SELF: a path tacked onto the end of a + * URL (client/tickets.php/profile.php) changes PHP_SELF but not what runs. And the keys + * are checked whole, because post handlers run one after another - a request that carries + * the set-up action and another one alongside it must not get the other one through. + */ +function requestIsMfaSetup(array $allowed) { + $root = realpath(dirname(__DIR__)); + $script = realpath((string) ($_SERVER['SCRIPT_FILENAME'] ?? '')); + if ($root === false || $script === false || strpos($script, $root . DIRECTORY_SEPARATOR) !== 0) { + return false; + } + $script = str_replace(DIRECTORY_SEPARATOR, '/', substr($script, strlen($root) + 1)); + foreach ($allowed[$script] ?? [] as [$get_keys, $post_keys]) { + if (!array_diff(array_keys($_GET), $get_keys) && !array_diff(array_keys($_POST), $post_keys)) { + return true; + } + } + return false; +} + function lookupUserPermission($module) { global $mysqli, $session_is_admin, $session_user_role; static $levels = []; diff --git a/functions/portal.php b/functions/portal.php index 8b93b8dea..36ab0bb88 100644 --- a/functions/portal.php +++ b/functions/portal.php @@ -343,11 +343,17 @@ function portalViewAsAccess($agent_user_id, $contact_id) { if (intval($config_client_portal_enable) !== 1 || !$agent_user_id || !$contact_id) { return null; } - $agent = mysqli_fetch_assoc(mysqli_query($mysqli, "SELECT user_name, user_password, user_type, user_status, user_archived_at, user_role_id, role_is_admin - FROM users LEFT JOIN user_roles ON role_id = user_role_id WHERE user_id = $agent_user_id LIMIT 1")); + $agent = mysqli_fetch_assoc(mysqli_query($mysqli, "SELECT user_name, user_password, user_type, user_status, user_archived_at, user_role_id, role_is_admin, + user_token, user_config_force_mfa + FROM users LEFT JOIN user_roles ON role_id = user_role_id LEFT JOIN user_settings ON user_settings.user_id = users.user_id + WHERE users.user_id = $agent_user_id LIMIT 1")); if (!$agent || intval($agent['user_type']) !== 1 || intval($agent['user_status']) !== 1 || $agent['user_archived_at'] !== null) { return null; } + // Still has to set up the MFA their account requires (includes/load_user_session.php) + if (intval($agent['user_config_force_mfa'] ?? 0) === 1 && trim((string) $agent['user_token']) === '') { + return null; + } $admin = intval($agent['role_is_admin'] ?? 0) === 1; $level = function ($module) use ($mysqli, $admin, $agent) { if ($admin) { diff --git a/includes/load_user_session.php b/includes/load_user_session.php index dfb8cdd44..fe446ff55 100644 --- a/includes/load_user_session.php +++ b/includes/load_user_session.php @@ -64,6 +64,17 @@ if (!hash_equals($session_stamp_current, (string) ($_SESSION['user_session_stamp redirect("/login.php"); } +// MFA required on this account and not set up yet: setting it up, or signing out, is all +// this session may do - every other page, modal, AJAX call and POST goes back to the set +// up page. Signing in only sends them there; this is what keeps them there, including when +// an admin requires MFA, or resets it, on an agent who is already signed in. +if ($session_user_config_force_mfa === 1 && trim((string) $session_token) === '' && !requestIsMfaSetup([ + 'agent/user/mfa_enforcement.php' => [[[], []]], + 'agent/user/post.php' => [[[], ['csrf_token', 'enable_mfa', 'verify_code']], [['logout'], []]], +])) { + redirect("/agent/user/mfa_enforcement.php"); +} + // Load user client permissions (allow + deny lists) $user_client_access_sql = "SELECT client_id, permission_type FROM user_client_permissions WHERE user_id = $session_user_id"; $user_client_access_result = mysqli_query($mysqli, $user_client_access_sql);