CVE-2021-25956: Other (NVD-CWE-Other)
value: In “Dolibarr” application, v3.3.beta1_20121221 to v13.0.2 have “Modify” access for admin level users to change other user’s details but fails to validate already existing “Login” name, while renaming the user “Login”. This leads to complete account takeover of the victim user. This happens since the password gets overwritten for the victim user having a similar login name.
CWE description: NVD is only using a subset of CWE for mapping instead of the entire CWE, and the weakness type is not covered by that subset.
CVSS v3 base score: 7.2
Published: 2021-08-17T15:15Z
Repository: https://github.com/Dolibarr/dolibarr
Language: PHP
Files changed: htdocs/langs/en_US/errors.lang, htdocs/user/card.php, htdocs/user/class/user.class.php
Commit date: 2021-06-13 16:15:05 +0200
Commit message: FIX Broken Access Control reported by Ahsan Aziz.
Diff: diff --git a/htdocs/langs/en_US/errors.lang b/htdocs/langs/en_US/errors.lang
index f2d9106107b0..c4e8a1d72264 100644
--- a/htdocs/langs/en_US/errors.lang
+++ b/htdocs/langs/en_US/errors.lang
@@ -9,10 +9,11 @@ ErrorBadMXDomain=Email %s seems incorrect (domain has no valid MX record)
 ErrorBadUrl=Url %s is incorrect
 ErrorBadValueForParamNotAString=Bad value for your parameter. It appends generally when translation is missing.
 ErrorRefAlreadyExists=Reference <b>%s</b> already exists.
 ErrorLoginAlreadyExists=Login %s already exists.
 ErrorGroupAlreadyExists=Group %s already exists.
+ErrorEmailAlreadyExists=Email %s already exists.
 ErrorRecordNotFound=Record not found.
 ErrorFailToCopyFile=Failed to copy file '<b>%s</b>' into '<b>%s</b>'.
 ErrorFailToCopyDir=Failed to copy directory '<b>%s</b>' into '<b>%s</b>'.
 ErrorFailToRenameFile=Failed to rename file '<b>%s</b>' into '<b>%s</b>'.
 ErrorFailToDeleteFile=Failed to remove file '<b>%s</b>'.
diff --git a/htdocs/user/card.php b/htdocs/user/card.php
index 0b67853b5538..a78425bb596c 100644
--- a/htdocs/user/card.php
+++ b/htdocs/user/card.php
@@ -442,20 +442,26 @@ if (empty($reshook)) {
 					$object->fk_warehouse = GETPOST('fk_warehouse', 'int');
 				}
 
 				$object->lang = GETPOST('default_lang', 'aZ09');
 
-				if (!empty($conf->multicompany->enabled)) {
-					if (GETPOST("superadmin")) {
-						$object->entity = 0;
-					} elseif (!empty($conf->global->MULTICOMPANY_TRANSVERSE_MODE)) {
-						$object->entity = 1; // all users in master entity
+				// Do we update also ->entity ?
+				if (!empty($conf->multicompany->enabled)) {	// If multicompany is not enabled, we never update the entity of a user.
+					if (!empty($conf->global->MULTICOMPANY_TRANSVERSE_MODE)) {
+						$object->entity = 1; // all users are in master entity
 					} else {
-						$object->entity = (!GETPOST('entity', 'int') ? 0 : GETPOST('entity', 'int'));
+						// A user should not be able to move a user into another entity. Only superadmin should be able to do this.
+						if ($user->entity == 0 && $user->admin) {
+							if (GETPOST("superadmin")) {
+								// We try to set the user as superadmin.
+								$object->entity = 0;
+							} else {
+								// We try to change the entity of user
+								$object->entity = (GETPOSTISSET('entity') ? GETPOSTINT('entity') : $object->entity);
+							}
+						}
 					}
-				} else {
-					$object->entity = (!GETPOST('entity', 'int') ? 0 : GETPOST('entity', 'int'));
 				}
 
 				// Fill array 'array_options' with data from add form
 				$ret = $extrafields->setOptionalsFromPost(null, $object);
 				if ($ret < 0) {
diff --git a/htdocs/user/class/user.class.php b/htdocs/user/class/user.class.php
index ec09aafeef5f..1a2098619aec 100644
--- a/htdocs/user/class/user.class.php
+++ b/htdocs/user/class/user.class.php
@@ -1660,10 +1660,36 @@ class User extends CommonObject
 			return -1;
 		}
 
 		$this->db->begin();
 
+		// Check if login already exists in same entity or into entity 0.
+		if ($this->oldcopy->login != $this->login) {
+			$sqltochecklogin = "SELECT COUNT(*) as nb FROM ".MAIN_DB_PREFIX."user WHERE entity IN (".((int) $this->entity).", 0) AND login = '".$this->db->escape($this->login)."'";
+			$resqltochecklogin = $this->db->query($sqltochecklogin);
+			if ($resqltochecklogin) {
+				$objtochecklogin = $this->db->fetch_object($resqltochecklogin);
+				if ($objtochecklogin && $objtochecklogin->nb > 0) {
+					$langs->load("errors");
+					$this->error = $langs->trans("ErrorLoginAlreadyExists", $this->login);
+					return -1;
+				}
+			}
+		}
+		if ($this->email !== '' && $this->oldcopy->email != $this->email) {
+			$sqltochecklogin = "SELECT COUNT(*) as nb FROM ".MAIN_DB_PREFIX."user WHERE entity IN (".((int) $this->entity).", 0) AND email = '".$this->db->escape($this->email)."'";
+			$resqltochecklogin = $this->db->query($sqltochecklogin);
+			if ($resqltochecklogin) {
+				$objtochecklogin = $this->db->fetch_object($resqltochecklogin);
+				if ($objtochecklogin && $objtochecklogin->nb > 0) {
+					$langs->load("errors");
+					$this->error = $langs->trans("ErrorEmailAlreadyExists", $this->email);
+					return -1;
+				}
+			}
+		}
+
 		// Update datas
 		$sql = "UPDATE ".MAIN_DB_PREFIX."user SET";
 		$sql .= " civility = '".$this->db->escape($this->civility_code)."'";
 		$sql .= ", lastname = '".$this->db->escape($this->lastname)."'";
 		$sql .= ", firstname = '".$this->db->escape($this->firstname)."'";
@@ -1713,11 +1739,11 @@ class User extends CommonObject
 		}
 		if (isset($this->salaryextra) || $this->salaryextra != '') {
 			$sql .= ", salaryextra= ".($this->salaryextra != '' ? "'".$this->db->escape($this->salaryextra)."'" : "null");
 		}
 		$sql .= ", weeklyhours= ".($this->weeklyhours != '' ? "'".$this->db->escape($this->weeklyhours)."'" : "null");
-		$sql .= ", entity = '".$this->db->escape($this->entity)."'";
+		$sql .= ", entity = ".((int) $this->entity);
 		$sql .= ", default_range = ".($this->default_range > 0 ? $this->default_range : 'null');
 		$sql .= ", default_c_exp_tax_cat = ".($this->default_c_exp_tax_cat > 0 ? $this->default_c_exp_tax_cat : 'null');
 		$sql .= ", fk_warehouse = ".($this->fk_warehouse > 0 ? $this->fk_warehouse : "null");
 		$sql .= ", lang = ".($this->lang ? "'".$this->db->escape($this->lang)."'" : "null");
 		$sql .= " WHERE rowid = ".$this->id;

Vulnerable code: 				if (!empty($conf->multicompany->enabled)) {
					if (GETPOST("superadmin")) {
						$object->entity = 0;
					} elseif (!empty($conf->global->MULTICOMPANY_TRANSVERSE_MODE)) {
						$object->entity = 1; // all users in master entity
						$object->entity = (!GETPOST('entity', 'int') ? 0 : GETPOST('entity', 'int'));
				} else {
					$object->entity = (!GETPOST('entity', 'int') ? 0 : GETPOST('entity', 'int'));
		$sql .= ", entity = '".$this->db->escape($this->entity)."'";
Fixed code: ErrorEmailAlreadyExists=Email %s already exists.
				// Do we update also ->entity ?
				if (!empty($conf->multicompany->enabled)) {	// If multicompany is not enabled, we never update the entity of a user.
					if (!empty($conf->global->MULTICOMPANY_TRANSVERSE_MODE)) {
						$object->entity = 1; // all users are in master entity
						// A user should not be able to move a user into another entity. Only superadmin should be able to do this.
						if ($user->entity == 0 && $user->admin) {
							if (GETPOST("superadmin")) {
								// We try to set the user as superadmin.
								$object->entity = 0;
							} else {
								// We try to change the entity of user
								$object->entity = (GETPOSTISSET('entity') ? GETPOSTINT('entity') : $object->entity);
							}
						}
		// Check if login already exists in same entity or into entity 0.
		if ($this->oldcopy->login != $this->login) {
			$sqltochecklogin = "SELECT COUNT(*) as nb FROM ".MAIN_DB_PREFIX."user WHERE entity IN (".((int) $this->entity).", 0) AND login = '".$this->db->escape($this->login)."'";
			$resqltochecklogin = $this->db->query($sqltochecklogin);
			if ($resqltochecklogin) {
				$objtochecklogin = $this->db->fetch_object($resqltochecklogin);
				if ($objtochecklogin && $objtochecklogin->nb > 0) {
					$langs->load("errors");
					$this->error = $langs->trans("ErrorLoginAlreadyExists", $this->login);
					return -1;
				}
			}
		}
		if ($this->email !== '' && $this->oldcopy->email != $this->email) {
			$sqltochecklogin = "SELECT COUNT(*) as nb FROM ".MAIN_DB_PREFIX."user WHERE entity IN (".((int) $this->entity).", 0) AND email = '".$this->db->escape($this->email)."'";
			$resqltochecklogin = $this->db->query($sqltochecklogin);
			if ($resqltochecklogin) {
				$objtochecklogin = $this->db->fetch_object($resqltochecklogin);
				if ($objtochecklogin && $objtochecklogin->nb > 0) {
					$langs->load("errors");
					$this->error = $langs->trans("ErrorEmailAlreadyExists", $this->email);
					return -1;
				}
			}
		}

		$sql .= ", entity = ".((int) $this->entity);