Skip to content

Commit

Permalink
Add LDAP filter for the filter to prevent security problem (#29)
Browse files Browse the repository at this point in the history 
* Fix many security problems, include couples with LDAP
  • Loading branch information
@ddurieux
ddurieux authored Feb 14, 2024
1 parent 92440cd commit 191adc9
Show file tree
Hide file tree
Showing 7 changed files with 160 additions and 132 deletions.
14 changes: 5 additions & 9 deletions front/login.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,19 +45,15 @@
}
}

$_POST = array_map('stripslashes', $_POST);

//Do login and checks
//$user_present = 1;
$rawLogin = '';
$rawPassword = '';
if (isset($_SESSION['namfield']) && isset($_POST[$_SESSION['namfield']])) {
$login = $_POST[$_SESSION['namfield']];
} else {
$login = '';
$rawLogin = $_UPOST[$_SESSION['namfield']];
}
if (isset($_SESSION['pwdfield']) && isset($_POST[$_SESSION['pwdfield']])) {
$password = Toolbox::unclean_cross_side_scripting_deep($_POST[$_SESSION['pwdfield']]);
} else {
$password = '';
$rawPassword = $_UPOST[$_SESSION['pwdfield']];
}
// Manage the selection of the auth source (local, LDAP id, MAIL id)
if (isset($_POST['auth'])) {
Expand All @@ -81,7 +77,7 @@


// now we can continue with the process...
if ($auth->login($login, $password, (isset($_REQUEST["noAUTO"])?$_REQUEST["noAUTO"]:false), $remember, $login_auth)) {
if ($auth->login($rawLogin, $rawPassword, (isset($_REQUEST["noAUTO"])?$_REQUEST["noAUTO"]:false), $remember, $login_auth)) {
Auth::redirectIfAuthenticated();
} else {
// we have done at least a good login? No, we exit.
Expand Down
81 changes: 43 additions & 38 deletions inc/auth.class.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -218,19 +218,23 @@ function connection_imap($host, $login, $pass) {
* Based on GRR auth system
*
* @param string $ldap_method ldap_method array to use
* @param string $login User Login
* @param string $password User Password
* @param string $rawLogin User Login
* @param string $rawPassword User Password
*
* @return string basedn of the user / false if not founded
*/
function connection_ldap($ldap_method, $login, $password) {
function connection_ldap($ldap_method, $rawLogin, $rawPassword) {

// we prevent some delay...
if (empty($ldap_method['host'])) {
return false;
}

$this->ldap_connection = AuthLDAP::tryToConnectToServer($ldap_method, $login, $password);
$this->ldap_connection = AuthLDAP::tryToConnectToServer(
$ldap_method,
Toolbox::clean_LDAP_filter($rawLogin),
$rawPassword
);
$this->user_found = false;

if ($this->ldap_connection) {
Expand All @@ -250,7 +254,7 @@ function connection_ldap($ldap_method, $login, $password) {
'search_parameters' => $params,
'user_params' => [
'method' => AuthLDAP::IDENTIFIER_LOGIN,
'value' => $login
'value' => $rawLogin
],
'condition' => $ldap_method['condition'],
'user_dn' => $this->user_dn
Expand All @@ -263,7 +267,7 @@ function connection_ldap($ldap_method, $login, $password) {

$dn = $infos['dn'];
$this->user_found = $dn != '';
if ($this->user_found && @ldap_bind($this->ldap_connection, $dn, $password)) {
if ($this->user_found && @ldap_bind($this->ldap_connection, $dn, $rawPassword)) {

//Hook to implement to restrict access by checking the ldap directory
if (Plugin::doHookFunction("restrict_ldap_auth", $infos)) {
Expand Down Expand Up @@ -356,12 +360,12 @@ static function getPasswordHash($pass) {
* with an eventual error message
*
* @global DBmysql $DB
* @param string $name User Login
* @param string $password User Password
* @param string $rawName User Login
* @param string $rawPassword User Password
*
* @return boolean user in GLPI DB with the right password
*/
function connection_db($name, $password) {
function connection_db($rawName, $rawPassword) {
global $CFG_GLPI, $DB;

$pass_expiration_delay = (int)$CFG_GLPI['password_expiration_delay'];
Expand Down Expand Up @@ -390,7 +394,7 @@ function connection_db($name, $password) {
],
'FROM' => User::getTable(),
'WHERE' => [
'name' => $name,
'name' => addslashes($rawName),
'authtype' => self::DB_GLPI,
'auths_id' => 0,
]
Expand All @@ -402,7 +406,7 @@ function connection_db($name, $password) {
$row = $result->next();
$password_db = $row['password'];

if (self::checkPassword($password, $password_db)) {
if (self::checkPassword($rawPassword, $password_db)) {
// Disable account if password expired
if (-1 !== $pass_expiration_delay && -1 !== $lock_delay
&& $row['lock_date'] < $_SESSION['glpi_currenttime']) {
Expand All @@ -426,16 +430,16 @@ function connection_db($name, $password) {
];
// Set glpiID to allow password update
$_SESSION['glpiID'] = $input['id'];
$input['password'] = $password;
$input['password2'] = $password;
$input['password'] = $rawPassword;
$input['password2'] = $rawPassword;
$user = new User();
$user->update($input);
}
$this->user->getFromDBByCrit(['id' => $row['id']]);
$this->extauth = 0;
$this->user_present = 1;
$this->user->fields["authtype"] = self::DB_GLPI;
$this->user->fields["password"] = $password;
$this->user->fields["password"] = $rawPassword;

// apply rule rights on local user
$rules = new RuleRightCollection();
Expand Down Expand Up @@ -501,6 +505,7 @@ function getAlternateAuthSystemsUserLogin($authtype = 0) {
$CFG_GLPI["ssovariables_id"]);
$login_string = '';
// MoYo : checking REQUEST create a security hole for me !
// => ddurieux: this variable will be addslashes later
if (isset($_SERVER[$ssovariable])) {
$login_string = $_SERVER[$ssovariable];
}
Expand Down Expand Up @@ -680,15 +685,15 @@ function addToError($message) {
/**
* Manage use authentication and initialize the session
*
* @param string $login_name Login
* @param string $login_password Password
* @param boolean $noauto (false by default)
* @param string $rawLoginName Login
* @param string $rawLoginPassword Password
* @param boolean $noauto (false by default)
* @param bool $remember_me
* @param string $login_auth Type of auth - id of the auth
* @param string $login_auth Type of auth - id of the auth
*
* @return boolean (success)
*/
function login($login_name, $login_password, $noauto = false, $remember_me = false, $login_auth = '') {
function login($rawLoginName, $rawLoginPassword, $noauto = false, $remember_me = false, $login_auth = '') {
global $DB, $CFG_GLPI;

$this->getAuthMethods();
Expand All @@ -697,8 +702,8 @@ function login($login_name, $login_password, $noauto = false, $remember_me = fal
//In case the user was deleted in the LDAP directory
$user_deleted_ldap = false;

// Trim login_name : avoid LDAP search errors
$login_name = trim($login_name);
// Trim rawLoginName : avoid LDAP search errors
$rawLoginName = trim($rawLoginName);

// manage the $login_auth (force the auth source of the user account)
$this->user->fields["auths_id"] = 0;
Expand All @@ -722,9 +727,9 @@ function login($login_name, $login_password, $noauto = false, $remember_me = fal
if ($this->getAlternateAuthSystemsUserLogin($authtype)
&& !empty($this->user->fields['name'])) {
// Used for log when login process failed
$login_name = $this->user->fields['name'];
$rawLoginName = trim($this->user->fields['name']);
$this->auth_succeded = true;
$this->user_present = $this->user->getFromDBbyName(addslashes($login_name));
$this->user_present = $this->user->getFromDBbyName(addslashes($rawLoginName));
$this->extauth = 1;
$user_dn = false;

Expand Down Expand Up @@ -775,7 +780,7 @@ function login($login_name, $login_password, $noauto = false, $remember_me = fal
'condition' => $ldap_method["condition"],
'user_params' => [
'method' => AuthLDAP::IDENTIFIER_LOGIN,
'value' => $login_name
'value' => $rawLoginName
],
]);
} catch (\RuntimeException $e) {
Expand All @@ -785,7 +790,7 @@ function login($login_name, $login_password, $noauto = false, $remember_me = fal
if ($user_dn) {
$this->user_found = true;
$this->user->fields['auths_id'] = $ldap_method['id'];
$this->user->getFromLDAP($ds, $ldap_method, $user_dn['dn'], $login_name,
$this->user->getFromLDAP($ds, $ldap_method, $user_dn['dn'], $rawLoginName,
!$this->user_present);
break;
}
Expand Down Expand Up @@ -814,7 +819,7 @@ function login($login_name, $login_password, $noauto = false, $remember_me = fal
}
}
// Reset to secure it
$this->user->fields['name'] = $login_name;
$this->user->fields['name'] = $rawLoginName;
$this->user->fields["last_login"] = $_SESSION["glpi_currenttime"];

} else {
Expand All @@ -823,16 +828,16 @@ function login($login_name, $login_password, $noauto = false, $remember_me = fal
}

if (!$this->auth_succeded) {
if (empty($login_name) || strstr($login_name, "\0")
|| empty($login_password) || strstr($login_password, "\0")) {
if (empty($rawLoginName) || strstr($rawLoginName, "\0")
|| empty($rawLoginPassword) || strstr($rawLoginPassword, "\0")) {
$this->addToError(__('Empty login or password'));
} else {

// Try connect local user if not yet authenticated
if (empty($login_auth)
|| $this->user->fields["authtype"] == $this::DB_GLPI) {
$this->auth_succeded = $this->connection_db(addslashes($login_name),
$login_password);
$this->auth_succeded = $this->connection_db(addslashes($rawLoginName),
$rawLoginPassword);
}

// Try to connect LDAP user if not yet authenticated
Expand All @@ -843,11 +848,11 @@ function login($login_name, $login_password, $noauto = false, $remember_me = fal
|| $this->user->fields["authtype"] == $this::LDAP) {

if (Toolbox::canUseLdap()) {
AuthLDAP::tryLdapAuth($this, $login_name, $login_password,
AuthLDAP::tryLdapAuth($this, $rawLoginName, $rawLoginPassword,
$this->user->fields["auths_id"]);
if (!$this->auth_succeded && !$this->user_found) {
$search_params = [
'name' => addslashes($login_name),
'name' => $DB->escape($rawLoginName),
'authtype' => $this::LDAP];
if (!empty($login_auth)) {
$search_params['auths_id'] = $this->user->fields["auths_id"];
Expand All @@ -866,8 +871,8 @@ function login($login_name, $login_password, $noauto = false, $remember_me = fal
|| $this->user->fields["authtype"] == $this::MAIL) {
AuthMail::tryMailAuth(
$this,
$login_name,
$login_password,
$rawLoginName,
$rawLoginPassword,
$this->user->fields["auths_id"]
);
}
Expand Down Expand Up @@ -939,21 +944,21 @@ function login($login_name, $login_password, $noauto = false, $remember_me = fal
if ($this->auth_succeded) {
if (GLPI_DEMO_MODE) {
// not translation in GLPI_DEMO_MODE
Event::log(-1, "system", 3, "login", $login_name." log in from ".$ip);
Event::log(-1, "system", 3, "login", addslashes($rawLoginName)." log in from ".$ip);
} else {
//TRANS: %1$s is the login of the user and %2$s its IP address
Event::log(-1, "system", 3, "login", sprintf(__('%1$s log in from IP %2$s'),
$login_name, $ip));
addslashes($rawLoginName), $ip));
}

} else {
if (GLPI_DEMO_MODE) {
Event::log(-1, "system", 3, "login", "login",
"Connection failed for " . $login_name . " ($ip)");
"Connection failed for " . addslashes($rawLoginName) . " ($ip)");
} else {
//TRANS: %1$s is the login of the user and %2$s its IP address
Event::log(-1, "system", 3, "login", sprintf(__('Failed login for %1$s from IP %2$s'),
$login_name, $ip));
addslashes($rawLoginName), $ip));
}
}
}
Expand Down
Loading

0 comments on commit 191adc9

Please sign in to comment.