Skip to content

Commit

Permalink
Protect emails and is_active flag of users with higher rights
Browse files Browse the repository at this point in the history
  • Loading branch information
@cedric-anne @trasher
cedric-anne authored and trasher committed Nov 6, 2024
1 parent 2bf5dc7 commit b753b76
Show file tree
Hide file tree
Showing 4 changed files with 275 additions and 21 deletions.
128 changes: 128 additions & 0 deletions phpunit/functional/UserEmailTest.php
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
<?php

/**
* ---------------------------------------------------------------------
*
* GLPI - Gestionnaire Libre de Parc Informatique
*
* http://glpi-project.org
*
* @copyright 2015-2024 Teclib' and contributors.
* @copyright 2003-2014 by the INDEPNET Development Team.
* @licence https://www.gnu.org/licenses/gpl-3.0.html
*
* ---------------------------------------------------------------------
*
* LICENSE
*
* This file is part of GLPI.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <https://www.gnu.org/licenses/>.
*
* ---------------------------------------------------------------------
*/

namespace tests\units;

use DbTestCase;
use Session;
use User;
use UserEmail;

class UserEmailTest extends DbTestCase
{
public function testCan(): void
{
$users_passwords = [
TU_USER => TU_PASS,
'glpi' => 'glpi',
'tech' => 'tech',
'normal' => 'normal',
'post-only' => 'postonly',
];

$users_matrix = [
TU_USER => [
TU_USER => true,
'glpi' => true,
'tech' => true,
'normal' => true,
'post-only' => true,
],
'glpi' => [
TU_USER => true,
'glpi' => true,
'tech' => true,
'normal' => true,
'post-only' => true,
],
'tech' => [
TU_USER => false,
'glpi' => false,
'tech' => true,
'normal' => false, // has some more rights somewhere
'post-only' => true,
],
'normal' => [
TU_USER => false,
'glpi' => false,
'tech' => false,
'normal' => true,
'post-only' => true,
],
'post-only' => [
TU_USER => false,
'glpi' => false,
'tech' => false,
'normal' => false,
'post-only' => true,
]
];

foreach ($users_matrix as $login => $targer_users_names) {
$this->login($login, $users_passwords[$login]);

foreach ($targer_users_names as $target_user_name => $can) {
$target_user_id = \getItemByTypeName(User::class, $target_user_name, true);

$input = [
'users_id' => $target_user_id,
'email' => \bin2hex(\random_bytes(16)) . '@example.org',
];

$item = new UserEmail();
$this->assertEquals(
$can && Session::haveRight(User::$rightname, CREATE),
$item->can($item->getID(), CREATE, $input)
);

$item = $this->createItem(UserEmail::class, $input);
$this->assertEquals(
$can && Session::haveRight(User::$rightname, READ),
$item->can($item->getID(), READ)
);

$this->assertEquals(
$can && Session::haveRight(User::$rightname, UPDATE),
$item->can($item->getID(), UPDATE)
);

$this->assertEquals(
$can && Session::haveRight(User::$rightname, DELETE),
$item->can($item->getID(), DELETE)
);
}
}
}
}
86 changes: 86 additions & 0 deletions phpunit/functional/UserTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
use Monolog\Logger;
use Profile_User;
use QuerySubQuery;
use User;

/* Test for inc/user.class.php */

Expand Down Expand Up @@ -556,6 +557,91 @@ public function testPrepareInputForUpdatePassword(array $input, $expected, ?arra
$this->assertSame($expected, $result);
}

public function testPrepareInputForUpdateSensitiveFields(): void
{
$users_passwords = [
TU_USER => TU_PASS,
'glpi' => 'glpi',
'tech' => 'tech',
'normal' => 'normal',
'post-only' => 'postonly',
];

$users_matrix = [
TU_USER => [
TU_USER => true,
'glpi' => true,
'tech' => true,
'normal' => true,
'post-only' => true,
],
'glpi' => [
TU_USER => true,
'glpi' => true,
'tech' => true,
'normal' => true,
'post-only' => true,
],
'tech' => [
TU_USER => false,
'glpi' => false,
'tech' => true,
'normal' => false, // has some more rights somewhere
'post-only' => true,
],
'normal' => [
TU_USER => false,
'glpi' => false,
'tech' => false,
'normal' => true,
'post-only' => true,
],
'post-only' => [
TU_USER => false,
'glpi' => false,
'tech' => false,
'normal' => false,
'post-only' => true,
]
];

$inputs = [
'api_token' => \bin2hex(\random_bytes(16)),
'_reset_api_token' => true,
'cookie_token' => \bin2hex(\random_bytes(16)),
'password_forget_token' => \bin2hex(\random_bytes(16)),
'personal_token' => \bin2hex(\random_bytes(16)),
'_reset_personal_token' => true,
'_useremails' => ['[email protected]', '[email protected]'],
'_emails' => ['[email protected]', '[email protected]'],
'is_active' => false,
];

foreach ($users_matrix as $login => $targer_users_names) {
$this->login($login, $users_passwords[$login]);

foreach ($targer_users_names as $target_user_name => $can) {
$target_user = \getItemByTypeName(User::class, $target_user_name);

foreach ($inputs as $key => $value) {
$output = $target_user->prepareInputForUpdate(['id' => $target_user->getID(), $key => $value]);
$this->assertEquals($can, \array_key_exists($key, $output));
}
}
}

// Filtering of sensitive fields is not done if no session is active (cron case)
$this->logout();
foreach ([TU_USER, 'glpi', 'tech', 'normal', 'post-only'] as $target_user_name) {
$target_user = \getItemByTypeName(User::class, $target_user_name);

foreach ($inputs as $key => $value) {
$output = $target_user->prepareInputForUpdate(['id' => $target_user->getID(), $key => $value]);
$this->assertEquals(true, \array_key_exists($key, $output));
}
}
}

public function testPost_addItem()
{
$this->login();
Expand Down
48 changes: 27 additions & 21 deletions src/User.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1036,29 +1036,35 @@ public function prepareInputForUpdate($input)
unset($input["password"]);
}

// prevent changing tokens and emails from users with lower rights
$protected_input_keys = [
'api_token',
'_reset_api_token',
'cookie_token',
'password_forget_token',
'personal_token',
'_reset_personal_token',

'_useremails',
];
if (!isCommandLine()) {
// Disallow `_emails` input unless on CLI context (e.g. LDAP sync command).
$protected_input_keys[] = '_emails';
}
if (
count(array_intersect($protected_input_keys, array_keys($input))) > 0
&& !Session::isCron() // cron context is considered safe
&& (int) $input['id'] !== Session::getLoginUserID()
&& !$this->currentUserHaveMoreRightThan($input['id'])
Session::getLoginUserID() !== false
&& ((int) $input['id']) !== Session::getLoginUserID()
) {
foreach ($protected_input_keys as $input_key) {
unset($input[$input_key]);
// Security checks to prevent an unathorized user to update sensitive fields of another user.
// These checks are done only if a "user" session is active.
$protected_input_keys = [
// Security tokens
'api_token',
'_reset_api_token',
'cookie_token',
'password_forget_token',
'personal_token',
'_reset_personal_token',

// Prevent changing emails that could then be used to get the password reset token
'_useremails',
'_emails',

// Prevent disabling another user account
'is_active',
];
if (
count(array_intersect($protected_input_keys, array_keys($input))) > 0
&& !$this->currentUserHaveMoreRightThan($input['id'])
) {
foreach ($protected_input_keys as $input_key) {
unset($input[$input_key]);
}
}
}

Expand Down
34 changes: 34 additions & 0 deletions src/UserEmail.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,40 @@ public static function getTypeName($nb = 0)
return _n('Email', 'Emails', $nb);
}

public function canChildItem($methodItem, $methodNotItem)
{
$users_id = $this->input['users_id'] ?? $this->fields['users_id'] ?? null;
if ($users_id !== null && !$this->canAlterUserEmails((int) $users_id)) {
return false;
}

return parent::canChildItem($methodItem, $methodNotItem);
}

/**
* Indicates whether the current user can alter the email addresses from the target user.
*
* @param int $target_user_id
* @return bool
*/
private function canAlterUserEmails(int $target_user_id): bool
{
$session_user_id = Session::getLoginUserID();

if ($session_user_id === false) {
// No active user session, action is made by a cron or a system routine, no need to check.
return true;
}

if ($target_user_id === $session_user_id) {
// Email is attached to the current user, no need to check.
return true;
}

// Current user can alter target user's emails only if he has more rights.
$user = new User();
return $user->currentUserHaveMoreRightThan($target_user_id);
}

/**
* Get default email for user. If no default email get first one
Expand Down

0 comments on commit b753b76

Please sign in to comment.