Skip to content

Commit

Permalink
A......... [ZBXNEXT-1898] implemented strong cryptography for encodin…
Browse files Browse the repository at this point in the history 
…g frontend passwords

* commit '4f94c93e5d170fff84b68bd8619aa4dd6eac7ec6':
  .......... [ZBXNEXT-1898] fixed Selenium test, changed password hash
  A......... [ZBXNEXT-1898] removed useless array keys from createSession() method call; made this method static
  .......... [ZBXNEXT-1898] fixed dbupgrate after resolved conflicts
  A......... [ZBXNEXT-1898] removed password from sid calculation
  A......... [ZBXNEXT-1898] used "static" keyword for verifyPassword() function
  A......... [ZBXNEXT-1898] added type hinting for second argument of verifyPassword() function
  A......... [ZBXNEXT-1898] fixed parameters order in hash_equals() function
  A......... [ZBXNEXT-1898] fixed PHPDoc of the verifyPassword method
  A......... [ZBXNEXT-1898] renamed method to verifyPassword
  A......... [ZBXNEXT-1898] removed trailing spaces
  A......... [ZBXNEXT-1898] implemented strong cryptography for encoding frontend passwords
  • Loading branch information
Vasily Goncharenko committed Dec 18, 2019
2 parents aec04e4 + 4f94c93 commit 3c4b81c
Show file tree
Hide file tree
Showing 9 changed files with 62 additions and 30 deletions.
1 change: 1 addition & 0 deletions ChangeLog.d/feature/ZBXNEXT-1898
View file
@@ -0,0 +1 @@
A......... [ZBXNEXT-1898] implemented strong cryptography for encoding frontend passwords (vasilijs)
6 changes: 3 additions & 3 deletions create/src/data.tmpl
View file
Expand Up @@ -18,9 +18,9 @@
--

TABLE |users
FIELDS|userid|alias|name |surname |passwd |url|autologin|autologout|lang |refresh|type|theme |rows_per_page|
ROW |1 |Admin|Zabbix|Administrator|5fce1b3e34b520afeffb37ce08c7cd66| |1 |0 |en_GB|30s |3 |default|50 |
ROW |2 |guest| | |d41d8cd98f00b204e9800998ecf8427e| |0 |15m |en_GB|30s |1 |default|50 |
FIELDS|userid|alias|name |surname |passwd |url|autologin|autologout|lang |refresh|type|theme |rows_per_page|
ROW |1 |Admin|Zabbix|Administrator|$2y$10$92nDno4n0Zm7Ej7Jfsz8WukBfgSS/U0QkIuu8WkJPihXBb2A1UrEK| |1 |0 |en_GB|30s |3 |default|50 |
ROW |2 |guest| | |$2y$10$89otZrRNmde97rIyzclecuk6LwKAsHN0BcvoOKGjbT.BwMBfm7G06| |0 |15m |en_GB|30s |1 |default|50 |

TABLE |hstgrp
FIELDS|groupid|name |internal|flags|
Expand Down
4 changes: 2 additions & 2 deletions create/src/schema.tmpl
View file
Expand Up @@ -27,7 +27,7 @@ FIELD |userid |t_id | |NOT NULL |0
FIELD |alias |t_varchar(100) |'' |NOT NULL |0
FIELD |name |t_varchar(100) |'' |NOT NULL |0
FIELD |surname |t_varchar(100) |'' |NOT NULL |0
FIELD |passwd |t_varchar(32) |'' |NOT NULL |0
FIELD |passwd |t_varchar(60) |'' |NOT NULL |0
FIELD |url |t_varchar(255) |'' |NOT NULL |0
FIELD |autologin |t_integer |'0' |NOT NULL |0
FIELD |autologout |t_varchar(32) |'15m' |NOT NULL |0
Expand Down Expand Up @@ -1705,4 +1705,4 @@ UNIQUE |1 |tls_psk_identity
TABLE|dbversion||
FIELD |mandatory |t_integer |'0' |NOT NULL |
FIELD |optional |t_integer |'0' |NOT NULL |
ROW |4050011 |4050011
ROW |4050012 |4050012
49 changes: 35 additions & 14 deletions frontends/php/include/classes/api/services/CUser.php
View file
Expand Up @@ -298,7 +298,9 @@ private function validateCreate(array &$users) {
* If user is created without a password (e.g. for GROUP_GUI_ACCESS_LDAP), store an empty string
* as his password in database.
*/
$user['passwd'] = (array_key_exists('passwd', $user)) ? md5($user['passwd']) : '';
$user['passwd'] = array_key_exists('passwd', $user)
? password_hash($user['passwd'], PASSWORD_BCRYPT, ['cost' => ZBX_BCRYPT_COST])
: '';
}
unset($user);

Expand Down Expand Up @@ -440,7 +442,7 @@ private function validateUpdate(array &$users, array &$db_users = null) {
self::exception(ZBX_API_ERROR_PARAMETERS, _('Not allowed to set password for user "guest".'));
}

$user['passwd'] = md5($user['passwd']);
$user['passwd'] = password_hash($user['passwd'], PASSWORD_BCRYPT, ['cost' => ZBX_BCRYPT_COST]);
}
}
unset($user);
Expand Down Expand Up @@ -1190,7 +1192,7 @@ public function login(array $user) {
break;

case ZBX_AUTH_INTERNAL:
if (md5($user['password']) !== $db_user['passwd']) {
if (!self::verifyPassword($user['password'], $db_user)) {
self::exception(ZBX_API_ERROR_PERMISSIONS, _('Login name or password is incorrect.'));
}
break;
Expand Down Expand Up @@ -1229,14 +1231,38 @@ public function login(array $user) {

// Start session.
unset($db_user['passwd']);
$db_user = $this->createSession($user, $db_user);
$db_user = self::createSession($user['user'], $db_user);
self::$userData = $db_user;

$this->addAuditDetails(AUDIT_ACTION_LOGIN, AUDIT_RESOURCE_USER);

return array_key_exists('userData', $user) && $user['userData'] ? $db_user : $db_user['sessionid'];
}

/**
* @param string $password User-specified password.
* @param array $db_user Saved user profile.
* @param string $db_user['passwd'] Saved password hash.
* @param int $db_user['userid'] User id.
*
* @return bool
*/
private static function verifyPassword($password, array $db_user) {
if (strlen($db_user['passwd']) > ZBX_MD5_SIZE) {
return password_verify($password, $db_user['passwd']);
}
elseif (hash_equals($db_user['passwd'], md5($password))) {
DB::update('users', [
'values' => ['passwd' => password_hash($password, PASSWORD_BCRYPT, ['cost' => ZBX_BCRYPT_COST])],
'where' => ['userid' => $db_user['userid']]
]);

return true;
}

return false;
}

/**
* Method is ONLY for internal use!
* Login user by alias. Return array with user data.
Expand All @@ -1257,10 +1283,7 @@ public function loginHttp($alias, $api_call = true) {
);

unset($db_user['passwd']);
$db_user = $this->createSession([
'user' => $alias,
'password' => mt_rand()
], $db_user);
$db_user = self::createSession($alias, $db_user);
self::$userData = $db_user;

$this->addAuditDetails(AUDIT_ACTION_LOGIN, AUDIT_RESOURCE_USER);
Expand Down Expand Up @@ -1458,15 +1481,13 @@ protected function addRelatedObjects(array $options, array $result) {
/**
* Initialize session for user. Returns user data array with valid sessionid.
*
* @param array $user Authentication credentials.
* @param string $user['user'] User alias value.
* @param string $user['password'] User password, is used in sessionid generation.
* @param array $db_user User data from database.
* @param string $alias User alias value.
* @param array $db_user User data from database.
*
* @return array
*/
private function createSession($user, $db_user) {
$db_user['sessionid'] = md5(microtime().md5($user['password']).$user['user'].mt_rand());
private static function createSession($alias, $db_user) {
$db_user['sessionid'] = md5(microtime().$alias.mt_rand());

DB::insert('sessions', [[
'sessionid' => $db_user['sessionid'],
Expand Down
5 changes: 4 additions & 1 deletion frontends/php/include/defines.inc.php
View file
Expand Up @@ -21,14 +21,17 @@
define('ZABBIX_VERSION', '5.0.0alpha1');
define('ZABBIX_API_VERSION', '5.0.0');
define('ZABBIX_EXPORT_VERSION', '4.4');
define('ZABBIX_DB_VERSION', 4050011);
define('ZABBIX_DB_VERSION', 4050012);

define('ZABBIX_COPYRIGHT_FROM', '2001');
define('ZABBIX_COPYRIGHT_TO', '2019');

define('ZBX_LOGIN_ATTEMPTS', 5);
define('ZBX_LOGIN_BLOCK', 30); // sec

define('ZBX_BCRYPT_COST', 10);
define('ZBX_MD5_SIZE', 32);

define('ZBX_SESSION_NAME', 'zbx_sessionid'); // Session cookie name for Zabbix front-end.

define('ZBX_KIBIBYTE', '1024');
Expand Down
2 changes: 1 addition & 1 deletion frontends/php/include/schema.inc.php
View file
Expand Up @@ -29,7 +29,7 @@
'passwd' => [
'null' => false,
'type' => DB::FIELD_TYPE_CHAR,
'length' => 32,
'length' => 60,
'default' => '',
],
'url' => [
Expand Down
14 changes: 7 additions & 7 deletions frontends/php/tests/selenium/data/data_test.sql
View file
Expand Up @@ -1415,19 +1415,19 @@ INSERT INTO items (itemid, hostid, interfaceid, type, value_type, name, key_, de
INSERT INTO items (itemid, hostid, interfaceid, type, value_type, name, key_, delay, history, status, params, description, flags, posts, headers) VALUES (15092, 15003, 15005, 0, 2, 'item_testPageHistory_CheckLayout_Eventlog_2' , 'eventlog[item_testpagehistory_checklayout, 2]' , '30s', '90d', 0, '', 'The following url should be clickable: https://zabbix.com', 0, '', '');

-- testPageUsers, testFormLogin
INSERT INTO users (userid, alias, passwd, autologin, autologout, lang, refresh, type, theme, attempt_failed, attempt_clock, rows_per_page) VALUES (3, 'test-user', '5fce1b3e34b520afeffb37ce08c7cd66', 0, 0, 'en_GB', 30, 1, 'default', 0, 0, 50);
INSERT INTO users (userid, alias, passwd, autologin, autologout, lang, refresh, type, theme, attempt_failed, attempt_clock, rows_per_page) VALUES (3, 'test-user', '$2y$10$rHT2NCiJY3ly7ORLdqWbceGrDZ3YsR6gHSb5KAZAtc4QNbs6rFAaC', 0, 0, 'en_GB', 30, 1, 'default', 0, 0, 50);
INSERT INTO users_groups (id, usrgrpid, userid) VALUES (5, 8, 3);
INSERT INTO users (userid, alias, passwd, autologin, autologout, lang, refresh, type, theme, attempt_failed, attempt_clock, rows_per_page) VALUES (6, 'user-for-blocking', '5fce1b3e34b520afeffb37ce08c7cd66', 0, 0, 'en_GB', 30, 1, 'default', 0, 0, 50);
INSERT INTO users (userid, alias, passwd, autologin, autologout, lang, refresh, type, theme, attempt_failed, attempt_clock, rows_per_page) VALUES (6, 'user-for-blocking', '$2y$10$XuoeDz5QsPSjhZm9Ht.J8ujgX5en.X5QNSzLa3aYn04l9u6bu1p2u', 0, 0, 'en_GB', 30, 1, 'default', 0, 0, 50);
INSERT INTO users_groups (id, usrgrpid, userid) VALUES (8, 8, 6);
INSERT INTO users (userid, alias, passwd, autologin, autologout, lang, refresh, type, theme, attempt_failed, attempt_clock, rows_per_page) VALUES (7, 'disabled-user', '5fce1b3e34b520afeffb37ce08c7cd66', 0, 0, 'en_GB', 30, 1, 'default', 0, 0, 50);
INSERT INTO users (userid, alias, passwd, autologin, autologout, lang, refresh, type, theme, attempt_failed, attempt_clock, rows_per_page) VALUES (7, 'disabled-user', '$2y$10$7BRRzklg43VciDKXPsLq7uUqFehlgdTGzr/WfQXDFtACVYWFiRjjO', 0, 0, 'en_GB', 30, 1, 'default', 0, 0, 50);
INSERT INTO users_groups (id, usrgrpid, userid) VALUES (9, 9, 7);
INSERT INTO users (userid, alias, passwd, autologin, autologout, lang, refresh, type, theme, attempt_failed, attempt_clock, rows_per_page) VALUES (8, 'no-access-to-the-frontend', '5fce1b3e34b520afeffb37ce08c7cd66', 0, 0, 'en_GB', 30, 1, 'default', 0, 0, 50);
INSERT INTO users (userid, alias, passwd, autologin, autologout, lang, refresh, type, theme, attempt_failed, attempt_clock, rows_per_page) VALUES (8, 'no-access-to-the-frontend', '$2y$10$cExfysPEJsHzIoCkoUVH..XM0OSIwIQPE1sob2UgXDcH1Iyw8Wtny', 0, 0, 'en_GB', 30, 1, 'default', 0, 0, 50);
INSERT INTO users_groups (id, usrgrpid, userid) VALUES (10, 12, 8);

-- testUrlUserPermissions
INSERT INTO users (userid, alias, passwd, autologin, autologout, lang, refresh, type, theme, attempt_failed, attempt_clock, rows_per_page) VALUES (4, 'admin-zabbix', '5fce1b3e34b520afeffb37ce08c7cd66', 0, 0, 'en_GB', 30, 2, 'default', 0, 0, 50);
INSERT INTO users (userid, alias, passwd, autologin, autologout, lang, refresh, type, theme, attempt_failed, attempt_clock, rows_per_page) VALUES (4, 'admin-zabbix', '$2y$10$HuvU0X0vGitK8YhwyxILbOVU6oxYNF.BqsOhaieVBvDiGlxgxriay', 0, 0, 'en_GB', 30, 2, 'default', 0, 0, 50);
INSERT INTO users_groups (id, usrgrpid, userid) VALUES (6, 7, 4);
INSERT INTO users (userid, alias, passwd, autologin, autologout, lang, refresh, type, theme, attempt_failed, attempt_clock, rows_per_page) VALUES (5, 'user-zabbix', '5fce1b3e34b520afeffb37ce08c7cd66', 0, 0, 'en_GB', 30, 1, 'default', 0, 0, 50);
INSERT INTO users (userid, alias, passwd, autologin, autologout, lang, refresh, type, theme, attempt_failed, attempt_clock, rows_per_page) VALUES (5, 'user-zabbix', '$2y$10$MZQTU3/7XsECy1DbQqvn/eaoPoMDgMYJ7Ml1wYon1dC0NfwM9E3zu', 0, 0, 'en_GB', 30, 1, 'default', 0, 0, 50);
INSERT INTO users_groups (id, usrgrpid, userid) VALUES (7, 7, 5);

-- testPageDashboard Favorites
Expand Down Expand Up @@ -1496,7 +1496,7 @@ INSERT INTO problem_tag (problemtagid,eventid,tag,value) VALUES (93,93,'Service'
-- Tag based permissions
INSERT INTO usrgrp (usrgrpid, name) VALUES (90, 'Selenium user group for tag permissions AAA');
INSERT INTO usrgrp (usrgrpid, name) VALUES (91, 'Selenium user group for tag permissions BBB');
INSERT INTO users (userid, alias, passwd, autologin, autologout, lang, refresh, type, theme, attempt_failed, attempt_clock, rows_per_page) VALUES (90, 'Tag-user', '5fce1b3e34b520afeffb37ce08c7cd66', 0, 0, 'en_GB', 30, 1, 'default', 0, 0, 50);
INSERT INTO users (userid, alias, passwd, autologin, autologout, lang, refresh, type, theme, attempt_failed, attempt_clock, rows_per_page) VALUES (90, 'Tag-user', '$2y$10$UpgaksQrfBNgJVTZ8Zy53eVE6gaRcGhh1WQZojBAw2GGGh3ZXIoSi', 0, 0, 'en_GB', 30, 1, 'default', 0, 0, 50);
INSERT INTO users_groups (id, usrgrpid, userid) VALUES (90, 90, 90);
INSERT INTO users_groups (id, usrgrpid, userid) VALUES (91, 91, 90);
-- Tag based permissions: host group, host, item, two triggers
Expand Down
3 changes: 1 addition & 2 deletions frontends/php/tests/selenium/testFormUserProfile.php
View file
Expand Up @@ -103,8 +103,7 @@ public function testFormProfile_PasswordChange($data) {
switch ($data['expected']) {
case TEST_GOOD:
$this->zbxTestCheckHeader('Global view');
$row = DBfetch(DBselect("select passwd from users where alias='".PHPUNIT_LOGIN_NAME."'"));
$this->assertEquals(md5($data['password1']), $row['passwd']);
$this->zbxTestWaitUntilMessageTextPresent('msg-good' , 'User updated');
break;
case TEST_BAD:
$this->zbxTestWaitUntilMessageTextPresent('msg-bad' , $data['error_msg']);
Expand Down
8 changes: 8 additions & 0 deletions src/libs/zbxdbupgrade/dbupgrade_4050.c
View file
Expand Up @@ -125,6 +125,13 @@ static int DBpatch_4050011(void)
return SUCCEED;
}

static int DBpatch_4050012(void)
{
const ZBX_FIELD field = {"passwd", "", NULL, NULL, 60, ZBX_TYPE_CHAR, ZBX_NOTNULL, 0};

return DBmodify_field_type("users", &field, NULL);
}

#endif

DBPATCH_START(4050)
Expand All @@ -140,5 +147,6 @@ DBPATCH_ADD(4050006, 0, 1)
DBPATCH_ADD(4050007, 0, 1)
DBPATCH_ADD(4050010, 0, 1)
DBPATCH_ADD(4050011, 0, 1)
DBPATCH_ADD(4050012, 0, 1)

DBPATCH_END()

0 comments on commit 3c4b81c

Please sign in to comment.