request #24168: Indirect LDAP injection via the ldap_id attribute of …

…a user when checking if it exists This is a follow up to git #tuleap/stable/bd47f29847fcd6a68d359bc8aefb8749bb8a1b7c the initial fix was incomplete. Issue was identified thanks to Psalm taint analysis. Change-Id: I695be7d006e0cabdb9d4804f62772b0d88f3ffc0
Enalean · Nov 23, 2021 · 64e7756 · 64e7756
1 parent 9fa45da
commit 64e7756
Show file tree

Hide file tree

Showing 4 changed files with 9 additions and 1 deletion.
diff --git a/plugins/ldap/include/LDAP_ProjectGroupDao.class.php b/plugins/ldap/include/LDAP_ProjectGroupDao.class.php
@@ -148,6 +148,9 @@ public function doesProjectBindingKeepUsers($project_id)
         return count($this->retrieve($sql)) > 0;
     }
 
+    /**
+     * @psalm-taint-escape ldap DN is stored directly in plugin_ldap_project_group.ldap_group_dn, so we are forced to remove the taint
+     */
     public function getSynchronizedProjects()
     {
         $auto_synchronized_value = $this->da->quoteSmart(LDAP_GroupManager::AUTO_SYNCHRONIZATION);

diff --git a/plugins/ldap/include/LDAP_UserManager.class.php b/plugins/ldap/include/LDAP_UserManager.class.php
@@ -505,7 +505,7 @@ public function checkThreshold($nbr_users_to_suspend, $nbr_all_users)
      */
     public function isUserDeletedFromLdap($row)
     {
-        $ldap_query = $this->ldap->getLDAPParam('eduid') . '=' . $row['ldap_id'];
+        $ldap_query = $this->ldap->getLDAPParam('eduid') . '=' . ldap_escape($row['ldap_id'], '', LDAP_ESCAPE_FILTER);
         $attributes = $this->user_sync->getSyncAttributes($this->ldap);
         $ldapSearch = false;
 

diff --git a/src/common/DB/Compat/Legacy2018/CompatPDODataAccess.php b/src/common/DB/Compat/Legacy2018/CompatPDODataAccess.php
@@ -49,6 +49,7 @@ public function __construct(DBConnection $db_connection)
      *
      * @psalm-taint-sink sql $sql
      * @psalm-taint-sink sql $params
+     * @psalm-taint-source ldap
      */
     public function query($sql, $params = [])
     {
@@ -309,6 +310,7 @@ public function numRows($result)
      * @deprecated
      *
      * @return array Returns an associative array of strings that corresponds to the fetched row, or FALSE if there are no more rows.
+     * @psalm-taint-source ldap
      */
     public function fetch($result)
     {
@@ -330,6 +332,7 @@ public function fetch($result)
      * @param type $result
      *
      * @return type
+     * @psalm-taint-source ldap
      */
     public function fetchArray($result)
     {

diff --git a/src/common/DB/Compat/Legacy2018/LegacyDataAccessInterface.php b/src/common/DB/Compat/Legacy2018/LegacyDataAccessInterface.php
@@ -33,6 +33,7 @@ interface LegacyDataAccessInterface
      *
      * @psalm-taint-sink sql $sql
      * @psalm-taint-sink sql $params
+     * @psalm-taint-source ldap
      */
     public function query($sql, $params = []);
 
@@ -167,6 +168,7 @@ public function numRows($result);
      * @deprecated
      *
      * @return array Returns an associative array of strings that corresponds to the fetched row, or FALSE if there are no more rows.
+     * @psalm-taint-source ldap
      */
     public function fetch($result);