From 015d4ec9cd417f95dc20ee092c3e53bac849807e Mon Sep 17 00:00:00 2001 From: David Goodwin Date: Sat, 23 Dec 2023 21:43:31 +0000 Subject: [PATCH] reindent / reformat; add type hints for some of the app password stuff; try and make sure someone can only remove their own app password (see revokeAppPassword() ) --- functions.inc.php | 5 +- model/Login.php | 44 ++++---- model/TotpPf.php | 68 +++++++------ public/login-mfa.php | 2 +- public/upgrade.php | 1 - public/users/app-passwords.php | 102 ++++++++++++------- public/users/login-mfa.php | 2 +- public/users/totp-exceptions.php | 42 +++++--- public/users/totp.php | 26 ++--- scripts/examples/sync-roundcubemail-totp.php | 16 +-- 10 files changed, 183 insertions(+), 125 deletions(-) diff --git a/functions.inc.php b/functions.inc.php index 182fe8f5..416f55ed 100644 --- a/functions.inc.php +++ b/functions.inc.php @@ -19,10 +19,11 @@ $min_db_version = 1844; # update (at least) before a release with the latest fu /** - * Check if the user already provided a password but not the second factor + * Check if the user already provided a password but not the second factor * @return boolean */ -function authentication_mfa_incomplete() { +function authentication_mfa_incomplete() +{ if (isset($_SESSION['sessid'])) { if (isset($_SESSION['sessid']['mfa_complete'])) { if ($_SESSION['sessid']['mfa_complete'] == false) { diff --git a/model/Login.php b/model/Login.php index 6bc49b3c..d9832855 100644 --- a/model/Login.php +++ b/model/Login.php @@ -183,8 +183,8 @@ class Login } // Write passwords through pipe to command stdin -- provide old password, then new password. - fwrite($pipes[0], $old_password . "\0", 1+strlen($old_password)); - fwrite($pipes[0], $new_password . "\0", 1+strlen($new_password)); + fwrite($pipes[0], $old_password . "\0", 1 + strlen($old_password)); + fwrite($pipes[0], $new_password . "\0", 1 + strlen($new_password)); $output = stream_get_contents($pipes[1]); fclose($pipes[0]); fclose($pipes[1]); @@ -200,9 +200,10 @@ class Login } /** - * @param string $username - * @param string $new_password - * @param string $old_password + * @param string $username - current user + * @param string $password - password for $username + * @param string $app_desc - desc. for the app we're adding + * @param string $app_pass - password for the app we're adding * * All passwords need to be plain text; they'll be hashed appropriately * as per the configuration in config.inc.php @@ -210,7 +211,7 @@ class Login * @return boolean true on success; false on failure * @throws \Exception if invalid user, or db update fails. */ - public function addAppPassword($username, $password, $app_desc, $app_pass): bool + public function addAppPassword(string $username, string $password, string $app_desc, string $app_pass): bool { list(/*NULL*/, $domain) = explode('@', $username); @@ -227,22 +228,26 @@ class Login $app_pass = pacrypt($app_pass); -/* maybe we want this - if (Config::bool('password_expiration')) { - $domain = $this->getUserDomain($username); - if (!is_null($domain)) { - $password_expiration_value = (int)get_password_expiration_value($domain); - $set['password_expiry'] = date('Y-m-d H:i', strtotime("+$password_expiration_value day")); - } - } -*/ + /* maybe we want this + if (Config::bool('password_expiration')) { + $domain = $this->getUserDomain($username); + if (!is_null($domain)) { + $password_expiration_value = (int)get_password_expiration_value($domain); + $set['password_expiry'] = date('Y-m-d H:i', strtotime("+$password_expiration_value day")); + } + } + */ // As PostgeSQL lacks REPLACE we first check and delete any previous rows matching this ip and user $exists = db_query_all('SELECT id FROM mailbox_app_password WHERE username = :username AND description = :description', ['username' => $username, 'description' => $app_desc,]); - if(isset($exists[0])) - foreach($exists as $x) db_delete('mailbox_app_password', 'id', $x['id']); - $result = db_insert('mailbox_app_password', ['username' => $username, 'description' => $app_desc, 'password_hash' => $app_pass ], Array()); + if (isset($exists[0])) { + foreach ($exists as $x) { + db_delete('mailbox_app_password', 'id', $x['id']); + } + } + + $result = db_insert('mailbox_app_password', ['username' => $username, 'description' => $app_desc, 'password_hash' => $app_pass], []); if ($result != 1) { db_log($domain, 'edit_password', "FAILURE: " . $username); @@ -279,7 +284,7 @@ class Login } // Write passwords through pipe to command stdin -- provide old password, then new password. - fwrite($pipes[0], $app_pass . "\0", 1+strlen($app_pass)); + fwrite($pipes[0], $app_pass . "\0", 1 + strlen($app_pass)); $output = stream_get_contents($pipes[0]); fclose($pipes[0]); @@ -292,5 +297,4 @@ class Login return true; } - } diff --git a/model/TotpPf.php b/model/TotpPf.php index e3f5587c..505485f8 100644 --- a/model/TotpPf.php +++ b/model/TotpPf.php @@ -1,4 +1,5 @@ setLabel($username); $totp->setIssuer('Postfix Admin'); - if(Config::read('logo_url')) + if (Config::read('logo_url')) { $totp->setParameter('image', Config::read('logo_url')); + } $QR_content = $totp->getProvisioningUri(); $pTOTP_secret = $totp->getSecret(); unset($totp); @@ -57,7 +59,7 @@ class TotpPf ->validateResult(false) ->build(); $qr_code = base64_encode($QRresult->getString()); - return Array($pTOTP_secret, $qr_code); + return array($pTOTP_secret, $qr_code); } /** @@ -67,8 +69,9 @@ class TotpPf */ public function usesTOTP($username): bool { - if(!(Config::read('totp') == 'YES')) + if (!(Config::read('totp') == 'YES')) { return false; + } $sql = "SELECT totp_secret FROM {$this->table} WHERE username = :username AND active = :active"; @@ -80,8 +83,9 @@ class TotpPf ]; $result = db_query_one($sql, $values); - if (is_array($result) && isset($result['totp_secret']) && $result['totp_secret'] != '') + if (is_array($result) && isset($result['totp_secret']) && $result['totp_secret'] != '') { return true; + } return false; } @@ -103,8 +107,9 @@ class TotpPf ]; $result = db_query_one($sql, $values); - if (!is_array($result) || !isset($result['totp_secret'])) + if (!is_array($result) || !isset($result['totp_secret'])) { return false; + } return $this->checkTOTP($result['totp_secret'], $username, $code); } @@ -120,10 +125,11 @@ class TotpPf { $totp = TOTP::create($secret); - if ( $totp->now() == $code ) + if ( $totp->now() == $code ) { return true; - else + } else { return false; + } } /** @@ -154,7 +160,6 @@ class TotpPf } else { return ''; } - } /** @@ -243,15 +248,17 @@ class TotpPf list($local_part, $domain) = explode('@', $username); - if (!$this->login->login($username, $password)) + if (!$this->login->login($username, $password)) { throw new \Exception(Config::Lang('pPassword_password_current_text_error')); + } - if (authentication_has_role('admin')) + if (authentication_has_role('admin')) { $admin = 1; - elseif (authentication_has_role('global-admin')) + } elseif (authentication_has_role('global-admin')) { $admin = 2; - else + } else { $admin = 0; + } if (empty($Exception_ip)) { $error += 1; @@ -268,22 +275,25 @@ class TotpPf flash_error(Config::Lang('pException_user_entire_domain_error')); } - if ( !($admin==2) && $Exception_user == NULL ) { + if ( !($admin==2) && $Exception_user == null ) { $error += 1; flash_error(Config::Lang('pException_user_global_error')); } - $values = Array('ip' => $Exception_ip, 'username' => $Exception_user, 'description' => $Exception_desc); + $values = array('ip' => $Exception_ip, 'username' => $Exception_user, 'description' => $Exception_desc); - if(!$error){ + if (!$error) { // OK to insert/replace. // As PostgeSQL lacks REPLACE we first check and delete any previous rows matching this ip and user - $exists = db_query_all('SELECT id FROM totp_exception_address WHERE ip = :ip AND username = :username', - ['ip' => $Exception_ip, 'username' => $Exception_user]); - if(isset($exists[0])) - foreach($exists as $x) db_delete('totp_exception_address', 'id', $x['id']); - $result = db_insert('totp_exception_address', $values, Array()); + $exists = db_query_all('SELECT id FROM totp_exception_address WHERE ip = :ip AND username = :username', + ['ip' => $Exception_ip, 'username' => $Exception_user]); + if (isset($exists[0])) { + foreach ($exists as $x) { + db_delete('totp_exception_address', 'id', $x['id']); + } + } + $result = db_insert('totp_exception_address', $values, array()); } if ($result != 1) { @@ -333,17 +343,19 @@ class TotpPf $exception = $this->getException($Exception_id); $error = 0; - if(strpos($exception['username'],'@')) + if (strpos($exception['username'],'@')) { list($Exception_local_part, $Exception_domain) = explode('@', $exception['username']); - else + } else { $Exception_domain = $exception['username']; + } - if (authentication_has_role('global-admin')) + if (authentication_has_role('global-admin')) { $admin = 2; - elseif (authentication_has_role('admin')) + } elseif (authentication_has_role('admin')) { $admin = 1; - else + } else { $admin = 0; + } if ( !$admin && strpos($exception['username'],'@') !== false ) { @@ -351,7 +363,7 @@ class TotpPf throw new \Exception(Config::Lang('pException_user_entire_domain_error')); } - if ( !($admin==2) && $exception['username'] == NULL ) { + if ( !($admin==2) && $exception['username'] == null ) { $error += 1; throw new \Exception(Config::Lang('pException_user_global_error')); } @@ -396,7 +408,7 @@ class TotpPf } /** - * @return array of all exceptions + * @return array of all exceptions */ public function getAllExceptions(): array { @@ -423,7 +435,5 @@ class TotpPf { return db_query_one("SELECT * FROM totp_exception_address WHERE id=$id"); } - - } /* vim: set expandtab softtabstop=4 tabstop=4 shiftwidth=4: */ diff --git a/public/login-mfa.php b/public/login-mfa.php index 351f1546..afad8844 100644 --- a/public/login-mfa.php +++ b/public/login-mfa.php @@ -38,7 +38,7 @@ if (authentication_has_role("admin")) { exit(0); } -if (isset($_GET["abort"]) && $_GET["abort"] == "1" && authentication_mfa_incomplete()){ +if (isset($_GET["abort"]) && $_GET["abort"] == "1" && authentication_mfa_incomplete()) { session_unset(); session_destroy(); session_start(); diff --git a/public/upgrade.php b/public/upgrade.php index 9af3f3c9..bae064f8 100644 --- a/public/upgrade.php +++ b/public/upgrade.php @@ -2324,4 +2324,3 @@ function upgrade_1849_pgsql() "); } } - diff --git a/public/users/app-passwords.php b/public/users/app-passwords.php index 04c10a12..1214c800 100644 --- a/public/users/app-passwords.php +++ b/public/users/app-passwords.php @@ -22,7 +22,7 @@ * fAppDesc * fAppPass * fAppId - * + * */ require_once('../common.php'); @@ -30,18 +30,18 @@ require_once('../common.php'); $smarty = PFASmarty::getInstance(); $smarty->configureTheme('../'); -$username = authentication_get_username(); -$pPassword_text = ""; -$pUser_text = ''; -$pUser = ''; +$username = authentication_get_username(); +$pPassword_text = ""; +$pUser_text = ''; +$pUser = ''; -if (authentication_has_role('global-admin')){ +if (authentication_has_role('global-admin')) { $login = new Login('admin'); $admin = 2; -}elseif (authentication_has_role('admin')){ +} elseif (authentication_has_role('admin')) { $login = new Login('admin'); $admin = 1; -}else{ +} else { $login = new Login('mailbox'); $admin = 0; } @@ -57,30 +57,33 @@ if ($_SERVER['REQUEST_METHOD'] == "POST") { exit(0); } - if(isset($_POST['fAppPass'])){ - $fPass = $_POST['fPassword_current']; - $fAppDesc = $_POST['fAppDesc']; - $fAppPass = $_POST['fAppPass']; - addAppPassword($username, $fPass, $fAppPass, $fAppDesc, $admin, $login, $PALANG); + if (isset($_POST['fAppPass'])) { + $fPass = $_POST['fPassword_current']; + $fAppDesc = $_POST['fAppDesc']; + $fAppPass = $_POST['fAppPass']; + addAppPassword($username, $fPass, $fAppPass, $fAppDesc, $login, $PALANG); } - if(isset($_POST['fAppId'])){ - $fAppId = $_POST['fAppId']; - revokeAppPassword($username, $fAppId, $login, $PALANG); + + if (isset($_POST['fAppId']) && is_numeric($_POST['fAppId'])) { + $fAppId = (int)$_POST['fAppId']; + revokeAppPassword($username, $fAppId, $PALANG); } - } -if($admin==2)$passwords = getAllAppPasswords(); -else $passwords = getAppPasswordsFor($username); - -foreach($passwords as $n => $pass){ - if($pass['username'] == $username) - $passwords[$n]['edit'] = 1; - if($admin == 2) - $passwords[$n]['edit'] = 1; +if ($admin == 2) { + $passwords = getAllAppPasswords(); +} else { + $passwords = getAppPasswordsFor($username); } - +foreach ($passwords as $n => $pass) { + if ($pass['username'] == $username) { + $passwords[$n]['edit'] = 1; + } + if ($admin == 2) { + $passwords[$n]['edit'] = 1; + } +} $smarty->assign('SESSID_USERNAME', $username); @@ -92,8 +95,17 @@ $smarty->assign('smarty_template', 'app-passwords'); $smarty->display('index.tpl'); - -function addAppPassword($username, $fPass, $fAppPass, $fAppDesc, $admin, $login, $PALANG){ +/** + * @param string $username - postfixadmin username + * @param string $fPass - postfixadmin password for username + * @param string $fAppPass - application password + * @param string $fAppDesc - application description + * @param Login $login + * @param array $PALANG + * @return void + */ +function addAppPassword(string $username, string $fPass, string $fAppPass, string $fAppDesc, Login $login, array $PALANG) +{ try { if ($login->addAppPassword($username, $fPass, $fAppDesc, $fAppPass)) { flash_info($PALANG['pAppPassAdd_result_success']); @@ -107,18 +119,38 @@ function addAppPassword($username, $fPass, $fAppPass, $fAppDesc, $admin, $login, } } -function revokeAppPassword($username, $fAppId, $login, $PALANG){ - // No extra password check by design, user might be in a hurry - $result = db_delete('mailbox_app_password', 'id', $fAppId); - if($result == 1)flash_info($PALANG['pTotp_exceptions_revoked']); - else flash_error($PALANG['pPassword_result_error']); +/** + * @todo Why pass Login here? it's not used? + */ +function revokeAppPassword(string $username, int $fAppId, array $PALANG) +{ + // $username should be from $_SESSION and not modifiable by the end user + // we don't want someone to be able to delete someone else's app password by guessing an id... + $rows = db_query('SELECT id FROM mailbox_app_password WHERE id = :id AND username = :username', ['username' => $username, 'id' => $fAppId]); + if (!empty($rows)) { + $result = db_delete('mailbox_app_password', 'id', $rows[0]['id']); + if ($result == 1) { + flash_info($PALANG['pTotp_exceptions_revoked']); + return; + } + } + flash_error($PALANG['pPassword_result_error']); } -function getAllAppPasswords(){ +/** + * @return array + */ +function getAllAppPasswords() +{ return db_query_all("SELECT * FROM mailbox_app_password"); } -function getAppPasswordsFor($username){ +/** + * @param string $username + * @return array + */ +function getAppPasswordsFor($username) +{ return db_query_all("SELECT * FROM mailbox_app_password WHERE username = :username", ['username' => $username]); } diff --git a/public/users/login-mfa.php b/public/users/login-mfa.php index 9fe36bff..da4c6eae 100644 --- a/public/users/login-mfa.php +++ b/public/users/login-mfa.php @@ -35,7 +35,7 @@ if (authentication_has_role("user")) { exit(0); } -if ($_GET["abort"] == "1" && authentication_mfa_incomplete()){ +if ($_GET["abort"] == "1" && authentication_mfa_incomplete()) { session_unset(); session_destroy(); session_start(); diff --git a/public/users/totp-exceptions.php b/public/users/totp-exceptions.php index 8fa39542..48547744 100644 --- a/public/users/totp-exceptions.php +++ b/public/users/totp-exceptions.php @@ -23,7 +23,7 @@ * fDesc * fUser * fId - * + * */ require_once('common.php'); @@ -39,15 +39,15 @@ $pUser = ''; $username = authentication_get_username(); -if (authentication_has_role('global-admin')){ +if (authentication_has_role('global-admin')) { $login = new Login('admin'); $totppf = new TotpPf('admin'); $admin = 2; -}elseif (authentication_has_role('admin')){ +} elseif (authentication_has_role('admin')) { $login = new Login('admin'); $totppf = new TotpPf('admin'); $admin = 1; -}else{ +} else { $login = new Login('mailbox'); $totppf = new TotpPf('mailbox'); $admin = 0; @@ -63,15 +63,15 @@ if ($_SERVER['REQUEST_METHOD'] == "POST") { header("Location: main.php"); exit(0); } - - if(isset($_POST['fPassword_current']) && $_POST['fPassword_current'] != ''){ + + if (isset($_POST['fPassword_current']) && $_POST['fPassword_current'] != '') { $fPass = $_POST['fPassword_current']; $fIp = $_POST['fIp']; $fDesc = $_POST['fDesc']; $fUser = $_POST['fUser']; add_exception($username, $fPass, $fIp, $fDesc, $fUser, $admin, $totppf, $PALANG); } - if(isset($_POST['fId']) && $_POST['fId'] != ''){ + if (isset($_POST['fId']) && $_POST['fId'] != '') { $fId = $_POST['fId']; revoke_exception($username, $fId, $totppf, $PALANG); } @@ -80,19 +80,25 @@ if ($_SERVER['REQUEST_METHOD'] == "POST") { // Generate list of existing exceptions -if($admin==2)$exceptions = $totppf->getAllExceptions(); -else $exceptions = $totppf->getExceptionsFor($username); +if ($admin==2) { + $exceptions = $totppf->getAllExceptions(); +} else { + $exceptions = $totppf->getExceptionsFor($username); +} // User can revoke exceptions for own username // Admins can revoke exceptions for own domain // Global-admin can revoke all exceptions -foreach($exceptions as $n => $ex){ - if($ex['username'] == $username) +foreach ($exceptions as $n => $ex) { + if ($ex['username'] == $username) { $exceptions[$n]['edit'] = 1; - if($admin == 2) + } + if ($admin == 2) { $exceptions[$n]['edit'] = 1; - if($admin==1 && $ex['username'] == $domain) + } + if ($admin==1 && $ex['username'] == $domain) { $exceptions[$n]['edit'] = 1; + } } @@ -108,7 +114,8 @@ $smarty->display('index.tpl'); -function add_exception($username, $fPassword_current, $fException_ip, $fException_desc, $fException_user, $admin, $totppf, $PALANG){ +function add_exception($username, $fPassword_current, $fException_ip, $fException_desc, $fException_user, $admin, $totppf, $PALANG) +{ try { if ($totppf->addException($username, $fPassword_current, $fException_ip, $fException_user, $fException_desc)) { flash_info($PALANG['pTotp_exception_result_success']); @@ -122,10 +129,13 @@ function add_exception($username, $fPassword_current, $fException_ip, $fExceptio } } -function revoke_exception($username, $id, $totppf, $PALANG){ +function revoke_exception($username, $id, $totppf, $PALANG) +{ // No extra password check by design, user might be in a hurry $result = $totppf->deleteException($username, $id); - if($result)flash_info($PALANG['pTotp_exceptions_revoked']); + if ($result) { + flash_info($PALANG['pTotp_exceptions_revoked']); + } } diff --git a/public/users/totp.php b/public/users/totp.php index de9d6cb5..b0fcad85 100644 --- a/public/users/totp.php +++ b/public/users/totp.php @@ -37,18 +37,18 @@ $pTOTP_now = ""; $pPassword_password_text = ""; $pQR_raw = ""; -if (authentication_has_role('admin')){ +if (authentication_has_role('admin')) { $totppf = new TotpPf('admin'); $login = new Login('admin'); $admin = true; -}else{ +} else { $totppf = new TotpPf('mailbox'); $login = new Login('mailbox'); $admin = false; } -// Create new OTP-object +// Create new OTP-object // Generate random secret and resulting QR code list($pTOTP_secret, $pQR_raw) = $totppf->generate($username); @@ -73,15 +73,15 @@ if ($_SERVER['REQUEST_METHOD'] == "POST") { } // Does entered code from 2FA-app match the secret - if($fTOTP_code == '') { + if ($fTOTP_code == '') { $code_checks_out = true; - $fTOTP_secret = NULL; - } - else + $fTOTP_secret = null; + } else { $code_checks_out = $totppf->checkTOTP($fTOTP_secret, $username, $fTOTP_code); + } // Check that user has successfully generated a TOTP with external device - if (!$code_checks_out){ + if (!$code_checks_out) { $secreterror += 1; flash_error($PALANG['pTOTP_code_mismatch']); } @@ -89,7 +89,7 @@ if ($_SERVER['REQUEST_METHOD'] == "POST") { // If TOTP checks out -> store secret in DB if ($secreterror == 0) { try { - if($totppf->changeTOTP_secret($username, $fTOTP_secret, $fPassword_current)){ + if ($totppf->changeTOTP_secret($username, $fTOTP_secret, $fPassword_current)) { flash_info($PALANG['pTotp_stored']); } else { flash_error(Config::Lang_f('pTOTP_secret_result_error', $username)); @@ -98,11 +98,13 @@ if ($_SERVER['REQUEST_METHOD'] == "POST") { flash_error($e->getMessage()); } } - } -if( $totppf->usesTOTP($username)) $smarty->assign('show_form', 'hidden'); -else $smarty->assign('show_form', 'visible'); +if ( $totppf->usesTOTP($username)) { + $smarty->assign('show_form', 'hidden'); +} else { + $smarty->assign('show_form', 'visible'); +} $smarty->assign('SESSID_USERNAME', $username); $smarty->assign('admin', $admin); $smarty->assign('pPassword_password_current_text', $pPassword_password_current_text, false); diff --git a/scripts/examples/sync-roundcubemail-totp.php b/scripts/examples/sync-roundcubemail-totp.php index e20ca874..624abca9 100644 --- a/scripts/examples/sync-roundcubemail-totp.php +++ b/scripts/examples/sync-roundcubemail-totp.php @@ -20,14 +20,14 @@ $stmt->execute(); $result = $stmt->get_result(); if ($result->num_rows == 1) { - echo "Updating TOTP secret for $USERNAME\n"; - $row = $result->fetch_assoc(); - $preferences = unserialize($row['preferences']); - $preferences['twofactor_gauthenticator']['secret'] = $SHARED_SECRET; - $stmt_update = $mysqli->prepare("UPDATE users SET preferences=?"); - $stmt_update->bind_param("s", serialize($preferences)); - $stmt_update->execute(); + echo "Updating TOTP secret for $USERNAME\n"; + $row = $result->fetch_assoc(); + $preferences = unserialize($row['preferences']); + $preferences['twofactor_gauthenticator']['secret'] = $SHARED_SECRET; + $stmt_update = $mysqli->prepare("UPDATE users SET preferences=?"); + $stmt_update->bind_param("s", serialize($preferences)); + $stmt_update->execute(); } else { - echo "Could not find user $USERNAME in Roundcubemail.\n"; + echo "Could not find user $USERNAME in Roundcubemail.\n"; } $mysqli->close();