From 585bbbd33ebfb4a5d7801539cc17871983f26121 Mon Sep 17 00:00:00 2001 From: jim-p Date: Tue, 27 Aug 2019 13:19:30 -0400 Subject: [PATCH] User & Group Manager: Improve Deny Config Write Handling. Fixes #9259 * Denies all changes if a user has the Deny Config Write privilege. Previously it only denied the config write but some OS operations were performed. * Sets an input error so the user is notified that their attempt failed. * Hides the add and delete buttons so read only users don't see the option to perform those actions (but are still denied if they submit the form through other means) (cherry picked from commit acd7e5601ac6bc8b079bd6ea7f8b637a5ec89b5f) --- src/usr/local/www/system_groupmanager.php | 36 +++++++++++--- .../www/system_groupmanager_addprivs.php | 16 +++++- src/usr/local/www/system_usermanager.php | 49 ++++++++++++++----- .../local/www/system_usermanager_addprivs.php | 16 +++++- 4 files changed, 94 insertions(+), 23 deletions(-) diff --git a/src/usr/local/www/system_groupmanager.php b/src/usr/local/www/system_groupmanager.php index c15ffb67e8..96aff76b55 100644 --- a/src/usr/local/www/system_groupmanager.php +++ b/src/usr/local/www/system_groupmanager.php @@ -60,7 +60,21 @@ function admin_groups_sort() { usort($a_group, "cpusercmp"); } -if ($_POST['act'] == "delgroup") { +/* + * Check user privileges to test if the user is allowed to make changes. + * Otherwise users can end up in an inconsistent state where some changes are + * performed and others denied. See https://redmine.pfsense.org/issues/9259 + */ +phpsession_begin(); +$guiuser = getUserEntry($_SESSION['Username']); +$read_only = (is_array($guiuser) && userHasPrivilege($guiuser, "user-config-readonly")); +phpsession_end(); + +if (!empty($_POST) && $read_only) { + $input_errors = array(gettext("Insufficient privileges to make the requested change (read only).")); +} + +if (($_POST['act'] == "delgroup") && !$read_only) { if (!isset($id) || !isset($_REQUEST['groupname']) || !isset($a_group[$id]) || @@ -84,7 +98,7 @@ if ($_POST['act'] == "delgroup") { syslog($logging_level, "{$logging_prefix}: {$savemsg}"); } -if ($_POST['act'] == "delpriv") { +if (($_POST['act'] == "delpriv") && !$read_only) { if (!isset($id) || !isset($a_group[$id])) { pfSenseHeader("system_groupmanager.php"); @@ -124,7 +138,7 @@ if ($act == "edit") { } } -if (isset($_POST['dellall_x'])) { +if (isset($_POST['dellall_x']) && !$read_only) { $del_groups = $_POST['delete_check']; $deleted_groups = array(); @@ -153,7 +167,7 @@ if (isset($_POST['dellall_x'])) { } } -if (isset($_POST['save'])) { +if (isset($_POST['save']) && !$read_only) { unset($input_errors); $pconfig = $_POST; @@ -264,7 +278,7 @@ if (isset($_POST['save'])) { } function build_priv_table() { - global $a_group, $id; + global $a_group, $id, $read_only; $privhtml = '
'; $privhtml .= ''; @@ -288,7 +302,9 @@ function build_priv_table() { $user_has_root_priv = true; } $privhtml .= ''; - $privhtml .= ''; + if (!$read_only) { + $privhtml .= ''; + } $privhtml .= ''; } @@ -309,7 +325,9 @@ function build_priv_table() { $privhtml .= ''; $privhtml .= ''; return($privhtml); @@ -378,7 +396,7 @@ if (!($act == "new" || $act == "edit")) { @@ -393,10 +411,12 @@ if (!($act == "new" || $act == "edit")) { '; $privhtml .= '
" href="?act=edit&groupid="> - + " href="?act=delgroup&groupid=&groupname=" usepost>
'; @@ -506,7 +520,7 @@ function build_priv_table() { } $privhtml .= ''; $privhtml .= '
'; - if (!$group) { + if (!$group && !$read_only) { $privhtml .= ''; } @@ -534,14 +548,16 @@ function build_priv_table() { $privhtml .= ''; $privhtml .= ''; return($privhtml); } function build_cert_table() { - global $a_user, $id; + global $a_user, $id, $read_only; $certhtml = '
'; $certhtml .= ''; @@ -566,8 +582,10 @@ function build_cert_table() { $certhtml .= ''; $certhtml .= ''; $certhtml .= ''; $certhtml .= ''; $i++; @@ -580,7 +598,9 @@ function build_cert_table() { $certhtml .= ''; $certhtml .= ''; return($certhtml); @@ -657,7 +677,7 @@ foreach ($a_user as $i => $userent): @@ -669,6 +689,8 @@ foreach ($a_user as $i => $userent): diff --git a/src/usr/local/www/system_usermanager_addprivs.php b/src/usr/local/www/system_usermanager_addprivs.php index d48e23489e..d5b875536d 100644 --- a/src/usr/local/www/system_usermanager_addprivs.php +++ b/src/usr/local/www/system_usermanager_addprivs.php @@ -56,7 +56,21 @@ if (!is_array($a_user['priv'])) { $spriv_list = $priv_list; uasort($spriv_list, "compare_by_name"); -if ($_POST['save']) { +/* + * Check user privileges to test if the user is allowed to make changes. + * Otherwise users can end up in an inconsistent state where some changes are + * performed and others denied. See https://redmine.pfsense.org/issues/9259 + */ +phpsession_begin(); +$guiuser = getUserEntry($_SESSION['Username']); +$read_only = (is_array($guiuser) && userHasPrivilege($guiuser, "user-config-readonly")); +phpsession_end(); + +if (!empty($_POST) && $read_only) { + $input_errors = array(gettext("Insufficient privileges to make the requested change (read only).")); +} + +if ($_POST['save'] && !$read_only) { unset($input_errors); $pconfig = $_POST;
' . htmlspecialchars($cert['descr']) . $revokedstr . '' . htmlspecialchars($ca['descr']) . ''; - $certhtml .= ''; + if (!$read_only) { + $certhtml .= ''; + } $certhtml .= '
" href="?act=edit&userid="> - + " href="?act=deluser&userid=&username=" usepost>