Security advisory GHSA-3x67-4c2c-w45m: Admidio has a Second-Order SQL Injection via List Configuration (lsc_special_field, lsc_sort, lsc_filter)
Package: admidio/admidio (composer), vulnerable version: v5.0.6, fixed in: 5.0.7
CWE: CWE-89
Files changed by the fix:
modules/groups-roles/mylist.php
src/Roles/Entity/ListConfiguration.php
src/Roles/ValueObject/ConditionParser.php

Fix diff:
diff --git a/modules/groups-roles/mylist.php b/modules/groups-roles/mylist.php
index 8e94b57dc..e8234fc9b 100644
--- a/modules/groups-roles/mylist.php
+++ b/modules/groups-roles/mylist.php
@@ -210,13 +210,6 @@ function createProfileFieldsArray() {
 
     // create a multidimensional array for all columns with the necessary data
     $i = 0;
-    $arrParticipantsInformation = array(
-        'mem_approved' => $gL10n->get('SYS_PARTICIPATION_STATUS'),
-        'mem_usr_id_change' => $gL10n->get('SYS_CHANGED_BY'),
-        'mem_timestamp_change' => $gL10n->get('SYS_CHANGED_AT'),
-        'mem_comment' => $gL10n->get('SYS_COMMENT'),
-        'mem_count_guests' => $gL10n->get('SYS_SEAT_AMOUNT')
-    );
 
     foreach ($gProfileFields->getProfileFields() as $field) {
         // add profile field to user field array
@@ -242,81 +235,33 @@ function createProfileFieldsArray() {
             }
         }
     }
+    
+    $arrAdditionalColumns = array(
+        array('cat_name' => $gL10n->get('SYS_PROFILE_INFORMATION'), 'usf_name_intern' => 'usr_photo', 'usf_name' => $gL10n->get('SYS_PHOTO')),
+        ($gCurrentUser->isAdministratorUsers() ? array('cat_name' => $gL10n->get('SYS_PROFILE_INFORMATION'), 'usf_name_intern' => 'usr_login_name', 'usf_name' => $gL10n->get('SYS_USERNAME')) : []),
+        ($gCurrentUser->isAdministratorUsers() ? array('cat_name' => $gL10n->get('SYS_PROFILE_INFORMATION'), 'usf_name_intern' => 'usr_usr_id_create', 'usf_name' => $gL10n->get('SYS_CREATED_BY')) : []),
+        ($gCurrentUser->isAdministratorUsers() ? array('cat_name' => $gL10n->get('SYS_PROFILE_INFORMATION'), 'usf_name_intern' => 'usr_timestamp_create', 'usf_name' => $gL10n->get('SYS_CREATED_AT')) : []),
+        ($gCurrentUser->isAdministratorUsers() ? array('cat_name' => $gL10n->get('SYS_PROFILE_INFORMATION'), 'usf_name_intern' => 'usr_usr_id_change', 'usf_name' => $gL10n->get('SYS_CHANGED_BY')) : []),
+        ($gCurrentUser->isAdministratorUsers() ? array('cat_name' => $gL10n->get('SYS_PROFILE_INFORMATION'), 'usf_name_intern' => 'usr_timestamp_change', 'usf_name' => $gL10n->get('SYS_CHANGED_AT')) : []),
+        ($gCurrentUser->isAdministratorUsers() ? array('cat_name' => $gL10n->get('SYS_PROFILE_INFORMATION'), 'usf_name_intern' => 'usr_uuid', 'usf_name' => $gL10n->get('SYS_UNIQUE_ID')) : []),
+        array('cat_name' => $gL10n->get('SYS_ROLE_INFORMATION'), 'usf_name_intern' => 'mem_begin', 'usf_name' => $gL10n->get('SYS_MEMBERSHIP_START')),
+        array('cat_name' => $gL10n->get('SYS_ROLE_INFORMATION'), 'usf_name_intern' => 'mem_end', 'usf_name' => $gL10n->get('SYS_MEMBERSHIP_END')),
+        array('cat_name' => $gL10n->get('SYS_ROLE_INFORMATION'), 'usf_name_intern' => 'mem_duration', 'usf_name' => $gL10n->get('SYS_MEMBERSHIP_DURATION')),
+        array('cat_name' => $gL10n->get('SYS_PARTICIPATION_INFORMATION'), 'usf_name_intern' => 'mem_approved', 'usf_name' => $gL10n->get('SYS_PARTICIPATION_STATUS')),
+        array('cat_name' => $gL10n->get('SYS_PARTICIPATION_INFORMATION'), 'usf_name_intern' => 'mem_usr_id_change', 'usf_name' => $gL10n->get('SYS_CHANGED_BY')),
+        array('cat_name' => $gL10n->get('SYS_PARTICIPATION_INFORMATION'), 'usf_name_intern' => 'mem_timestamp_change', 'usf_name' => $gL10n->get('SYS_CHANGED_AT')),
+        array('cat_name' => $gL10n->get('SYS_PARTICIPATION_INFORMATION'), 'usf_name_intern' => 'mem_comment', 'usf_name' => $gL10n->get('SYS_COMMENT')),
+        array('cat_name' => $gL10n->get('SYS_PARTICIPATION_INFORMATION'), 'usf_name_intern' => 'mem_count_guests', 'usf_name' => $gL10n->get('SYS_SEAT_AMOUNT'))
+    );
 
-    // after the profile fields add some special profile information e.g. username, photo, created at ...
-    $javascriptCode .= '
-        userFields[' . ++$i . '] = {
-            "cat_name": "' . $gL10n->get('SYS_PROFILE_INFORMATION') . '",
-            "usf_name": "' . $gL10n->get('SYS_PHOTO') . '",
-            "usf_name_intern": "usr_photo"
-        };';
-
-    // administrator could export the uuid of each user to identify the user later at the import
-    if ($gCurrentUser->isAdministratorUsers()) {
-        $javascriptCode .= '
-            userFields[' . ++$i . '] = {
-                "cat_name": "' . $gL10n->get('SYS_PROFILE_INFORMATION') . '",
-                "usf_name": "' . $gL10n->get('SYS_USERNAME') . '",
-                "usf_name_intern": "usr_login_name"
-            };
-
-            userFields[' . ++$i . '] = {
-                "cat_name": "' . $gL10n->get('SYS_PROFILE_INFORMATION') . '",
-                "usf_name": "' . $gL10n->get('SYS_CREATED_BY') . '",
-                "usf_name_intern": "usr_usr_id_create"
-                };
-
-            userFields[' . ++$i . '] = {
-                "cat_name": "' . $gL10n->get('SYS_PROFILE_INFORMATION') . '",
-                "usf_name": "' . $gL10n->get('SYS_CREATED_AT') . '",
-                "usf_name_intern": "usr_timestamp_create"
-                };
-
-            userFields[' . ++$i . '] = {
-                "cat_name": "' . $gL10n->get('SYS_PROFILE_INFORMATION') . '",
-                "usf_name": "' . $gL10n->get('SYS_CHANGED_BY') . '",
-                "usf_name_intern": "usr_usr_id_change"
-                };
 
-            userFields[' . ++$i . '] = {
-                "cat_name": "' . $gL10n->get('SYS_PROFILE_INFORMATION') . '",
-                "usf_name": "' . $gL10n->get('SYS_CHANGED_AT') . '",
-                "usf_name_intern": "usr_timestamp_change"
-                };
-
-            userFields[' . ++$i . '] = {
-                "cat_name": "' . $gL10n->get('SYS_PROFILE_INFORMATION') . '",
-                "usf_name": "' . $gL10n->get('SYS_UNIQUE_ID') . '",
-                "usf_name_intern": "usr_uuid"
-                };';
-    }
-
-    $javascriptCode .= '
-        userFields[' . ++$i . '] = {
-            "cat_name": "' . $gL10n->get('SYS_ROLE_INFORMATION') . '",
-            "usf_name": "' . $gL10n->get('SYS_MEMBERSHIP_START') . '",
-            "usf_name_intern": "mem_begin"
-        };
-
-        userFields[' . ++$i . '] = {
-            "cat_name": "' . $gL10n->get('SYS_ROLE_INFORMATION') . '",
-            "usf_name": "' . $gL10n->get('SYS_MEMBERSHIP_END') . '",
-            "usf_name_intern": "mem_end"
-        };
-
-        userFields[' . ++$i . '] = {
-            "cat_name": "' . $gL10n->get('SYS_ROLE_INFORMATION') . '",
-            "usf_name": "' . $gL10n->get('SYS_MEMBERSHIP_DURATION') . '",
-            "usf_name_intern": "mem_duration"
-        };';
-
-    // add new category with participant information of events
-    foreach ($arrParticipantsInformation as $memberStatus => $columnName) {
+    // after the profile fields add some special profile information e.g. username, photo, created at ...
+    foreach ($arrAdditionalColumns as $column) {
         $javascriptCode .= '
             userFields[' . ++$i . '] = {
-                "cat_name" : "' . $gL10n->get('SYS_PARTICIPATION_INFORMATION') . '",
-                "usf_name" : "' . $columnName . '",
-                "usf_name_intern" : "' . $memberStatus . '",
+                "cat_name" : "' . $column['cat_name'] . '",
+                "usf_name" : "' . $column['usf_name'] . '",
+                "usf_name_intern" : "' . $column['usf_name_intern'] . '",
             };';
     }
 
diff --git a/src/Roles/Entity/ListConfiguration.php b/src/Roles/Entity/ListConfiguration.php
index 73fd48693..4298259ed 100644
--- a/src/Roles/Entity/ListConfiguration.php
+++ b/src/Roles/Entity/ListConfiguration.php
@@ -49,6 +49,26 @@ class ListConfiguration extends Entity
      */
     protected bool $showLeaders = false;
 
+    /**
+     * @var array<string> Array with all allowed special fields.
+     */
+    protected array $allowedSpecialColumns = [
+        'usr_login_name',
+        'usr_photo',
+        'usr_timestamp_change',
+        'usr_timestamp_create',
+        'usr_usr_id_change',
+        'usr_usr_id_create',
+        'usr_uuid',
+        'mem_approved',
+        'mem_begin',
+        'mem_comment',
+        'mem_count_guests',
+        'mem_duration',
+        'mem_end',
+        'mem_timestamp_change',
+        'mem_usr_id_change'
+    ];
 
     /**
      * Constructor that will create an object to handle the configuration of lists.
@@ -67,7 +87,8 @@ public function __construct(Database $database, $listID = 0)
 
     /**
      * Add new column to a column array. The number of the column will be the maximum number of the current
-     * array plus one. The special field usr_uuid could only be added by users with the right to edit all users.
+     * array plus one. The special fields must be part from the list of allowed columns. Also, the special field
+     * usr_uuid could only be added by users with the right to edit all users.
      * @param int|string $field Usf-Id of a profile field or the name of a special field.
      * @param int $number Optional the number of the column. This is useful if the list already exists
      *                           and maybe the profile field changed the position within the list.
@@ -85,6 +106,11 @@ public function addColumn(int|string $field, int $number = 0, string $sort = '',
             $number = count($this->columns) + 1;
         }
 
+        // check if field is a number or a string and if the field is allowed
+        if (is_string($field) && !in_array($field, $this->allowedSpecialColumns, true)) {
+            return false;
+        }
+
         // can join max. 61 tables
         // Passed parameters must be set carefully
         if (strlen($field) === 0 || $field === 0 || count($this->columns) >= 57) {
@@ -96,6 +122,10 @@ public function addColumn(int|string $field, int $number = 0, string $sort = '',
             return false;
         }
 
+        if ($sort !== '' && strtoupper($sort) !== 'ASC' && strtoupper($sort) !== 'DESC') {
+            $sort = '';
+        }
+
         // If column doesn't exist create an object
         if (!array_key_exists($number, $this->columns)) {
             $this->columns[$number] = new ListColumns($this->db);
@@ -644,7 +674,7 @@ public function getSearchConditions(): array
      *                                   This setting could be combined with **showFormerMembers** or **showRelationTypes**.
      *                                 - **showFormerMembers**: Set to true if role members or members of the organization
      *                                   should be shown and also former members should be listed
-     *                                 - **showRelationTypes**: An array with relation types. The sql will be expanded with
+     *                                 - **showRelationTypes**: An array with relation types. The SQL will be expanded with
      *                                   all users who are in such a relationship to the selected role users.
      *                                 - **showUserUUID** : If set to true the first column of the SQL will be the usr_uuid.
      *                                 - **showLeaderFlag** : If set to true, the first columns of the SQL will be
@@ -704,7 +734,7 @@ public function getSQL(array $options = array()): string
         }
 
         if ($sqlRoleUUIDs === '') {
-            // create the role UUIDs part of the sql
+            // create the role UUIDs part of the SQL
             if (count($optionsAll['showRolesMembers']) > 0) {
                 $sqlRoleUUIDs = '(\'' . implode('\', \'', $optionsAll['showRolesMembers']) . '\')';
             } else {
@@ -739,6 +769,12 @@ public function getSQL(array $options = array()): string
             } else {
                 // Special fields like usr_photo, mem_begin ...
                 $sqlColumnName = $listColumn->getValue('lsc_special_field');
+
+                // if the special field isn't allowed, then continue with the next column
+                if (!in_array($sqlColumnName, $this->allowedSpecialColumns, true)) {
+                    continue;
+                }
+
                 // Handle a special case for membership fields
                 switch ($sqlColumnName) {
                     case 'mem_duration':
@@ -795,7 +831,7 @@ public function getSQL(array $options = array()): string
 
             // Handle the conditions for the columns
             if ($optionsAll['useConditions'] && (string)$listColumn->getValue('lsc_filter') !== '') {
-                $value = $listColumn->getValue('lsc_filter');
+                $value = $listColumn->getValue('lsc_filter', 'database');
 
                 // custom profile field
                 if ($lscUsfId > 0) {
@@ -862,7 +898,7 @@ public function getSQL(array $options = array()): string
                 } else {
                     $columnName = $dbColumnName;
                 }
-                $sqlWhere .= $parser->makeSqlStatement($value, $columnName, $type, $gProfileFields->getPropertyById($lscUsfId, 'usf_name')); // TODO Exception handling
+                $sqlWhere .= $parser->makeSqlStatement($value, $columnName, $type, $gProfileFields->getPropertyById($lscUsfId, 'usf_name'), $this->db); // TODO Exception handling
             }
         }
 
diff --git a/src/Roles/ValueObject/ConditionParser.php b/src/Roles/ValueObject/ConditionParser.php
index 65632a2c3..acad96479 100644
--- a/src/Roles/ValueObject/ConditionParser.php
+++ b/src/Roles/ValueObject/ConditionParser.php
@@ -1,13 +1,14 @@
 <?php
 namespace Admidio\Roles\ValueObject;
 
+use Admidio\Infrastructure\Database;
 use Admidio\Infrastructure\Exception;
 use Admidio\Infrastructure\Utils\StringUtils;
 use DateInterval;
 use DateTime;
 
 /**
- * @brief Creates from a custom condition syntax a sql condition
+ * @brief Creates from a custom condition syntax a SQL condition
  *
  * The user can write a condition in a special syntax. This class will parse
  * that condition and creates a valid SQL statement which can be used in
@@ -33,17 +34,13 @@ class ConditionParser
      */
     private string $srcCond = '';
     /**
-     * @var string The destination string with the valid sql statement
+     * @var string The destination string with the valid SQL statement
      */
     private string $destCond = '';
     /**
-     * @var string Stores the sql statement if a record should not exist when user wants to exclude a column
+     * @var string Stores the SQL statement if a record should not exist when user wants to exclude a column
      */
     private string $notExistsSql = '';
-    /**
-     * @var bool Flag if there is an open quote in this condition that must be closed before the next condition will be parsed
-     */
-    private bool $openQuotes = false;
 
     /**
      * constructor that will initialize variables
@@ -52,20 +49,6 @@ public function __construct()
     {
     }
 
-    /**
-     * Ends the "DestCondition"
-     */
-    private function endDestCond()
-    {
-        if ($this->openQuotes) {
-            // always set quote marks for a value because some fields are a varchar in db
-            // but should only be filled with integer
-            $this->destCond .= '\' ';
-        }
-
-        $this->destCond .= ' ) ';
-    }
-
     /**
      * Creates a valid date format **YYYY-MM-DD** for the SQL statement
      * @param string $date The not formatted date from user input e.g. **12.04.2012**
@@ -101,7 +84,6 @@ private function getFormatDate(string $date, string $operator): string
                     $dateFrom = $dateObj->format('Y-m-d');
 
                     $ageCondition = ' BETWEEN \'' . $dateFrom . '\' AND \'' . $dateTo . '\'';
-                    $this->openQuotes = false;
                     break;
                 case '}':
                     // search for dates that are older than the age
@@ -152,7 +134,7 @@ private static function isDateSearch(string $columnType, string $sourceCondition
     }
 
     /**
-     * Stores a sql statement that checks if a record in a table does exist or not exist.
+     * Stores a SQL statement that checks if a record in a table does exist or not exist.
      * This must bei a full subselect that starts with SELECT. The statement is used if
      * a condition with EMPTY or NOT EMPTY is used.
      * @param string $sqlStatement String with the full subselect
@@ -161,7 +143,7 @@ private static function isDateSearch(string $columnType, string $sourceCondition
      * $parser->setNotExistsStatement('SELECT 1 FROM adm_user_data WHERE usd_usr_id = 1 AND usd_usf_id = 9');
      * ```
      */
-    public function setNotExistsStatement(string $sqlStatement)
+    public function setNotExistsStatement(string $sqlStatement): void
     {
         $this->notExistsSql = $sqlStatement;
     }
@@ -172,22 +154,23 @@ public function setNotExistsStatement(string $sqlStatement)
      * @param string $columnName The name of the database column for which the condition should be created
      * @param string $columnType The type of the column. Valid types are **string**, **int**, **date** and **checkbox**
      * @param string $fieldName The name of the profile field. This is used for error output to the end user
+     * @param Database $db Database connection for possible database queries
      * @return string Returns a valid SQL string with the condition for that column
      * @throws Exception
      */
-    public function makeSqlStatement(string $sourceCondition, string $columnName, string $columnType, string $fieldName): string
+    public function makeSqlStatement(string $sourceCondition, string $columnName, string $columnType, string $fieldName, Database $db): string
     {
         $conditionComplete = $this->startDestCond($columnType, $columnName, $sourceCondition);
         if ($conditionComplete) {
             return $this->destCond;
         }
 
-        $this->openQuotes = false;    // set to true if quotes for conditions are open
         $startCondition   = true;     // Indicates that a new condition has been started
         $newCondition     = true;     // a new word is searched for in text fields -> new condition
         $startOperand     = false;    // Indicates whether <>= has already been specified for numeric or date fields
         $date             = '';       // Variable stores the entire date for date fields
         $operator         = '=';      // saves the actual operator, if no operator is set then = will be default
+        $conditionValue   = '';       // saves the actual condition value for fields
 
         $this->makeStandardCondition($sourceCondition);
         $srcCondArray = str_split($this->srcCond);
@@ -211,6 +194,10 @@ public function makeSqlStatement(string $sourceCondition, string $columnName, st
                     }
 
                     $startCondition = true;
+                    if ($conditionValue !== '') {
+                        $this->destCond .= $db->escapeString($conditionValue);
+                        $conditionValue = '';
+                    }
                 }
             }
             // Comparison of the values is processed here
@@ -221,6 +208,10 @@ public function makeSqlStatement(string $sourceCondition, string $columnName, st
                 if (!$startCondition) {
                     $this->destCond .= ' AND ' . $columnName . ' ';
                     $startCondition = true;
+                    if ($conditionValue !== '') {
+                        $this->destCond .= $db->escapeString($conditionValue);
+                        $conditionValue = '';
+                    }
                 }
 
                 switch ($character) {
@@ -287,10 +278,6 @@ public function makeSqlStatement(string $sourceCondition, string $columnName, st
                 }
 
                 if ($character !== '_' && $character !== '#') {
-                    // always set quote marks for a value because some fields are a varchar in db
-                    // but should only be filled with integer
-                    $this->destCond  .= ' \'';
-                    $this->openQuotes = true;
                     $startOperand     = true;
                 }
             } elseif ($character === ' ') {
@@ -301,24 +288,21 @@ public function makeSqlStatement(string $sourceCondition, string $columnName, st
                     if ($columnType === 'date' && $date !== '') {
                         $formatDate = $this->getFormatDate($date, $operator);
                         if ($formatDate !== '') {
-                            $this->destCond .= $formatDate;
+                            $this->destCond .= $db->escapeString($formatDate);
                         } else {
                             throw new Exception('SYS_NOT_VALID_DATE_FORMAT', array($fieldName));
                         }
                         $date = '';
                     }
 
-                    if ($this->openQuotes) {
-                        // always set quote marks for a value because some fields are a varchar in db
-                        // but should only be filled with integer
-                        $this->destCond  .= '\' ';
-                        $this->openQuotes = false;
-                    }
-
                     $newCondition = true;
+                    if ($conditionValue !== '') {
+                        $this->destCond .= $db->escapeString($conditionValue);
+                        $conditionValue = '';
+                    }
                 }
             } else {
-                // neues Suchwort, aber noch keine Bedingung
+                // New search term, but no criteria yet
 
                 if ($newCondition && !$startCondition) {
                     if ($columnType === 'string') {
@@ -326,15 +310,13 @@ public function makeSqlStatement(string $sourceCondition, string $columnName, st
                     } else {
                         $this->destCond .= ' AND ' . $columnName . ' = ';
                     }
-                    $this->openQuotes = false;
                 } elseif ($newCondition && !$startOperand) {
                     // first condition of this column
                     if ($columnType === 'string') {
-                        $this->destCond .= ' LIKE \'';
+                        $this->destCond .= ' LIKE ';
                     } else {
-                        $this->destCond .= ' = \'';
+                        $this->destCond .= ' = ';
                     }
-                    $this->openQuotes = true;
                 }
 
                 // Append character to target string
@@ -344,7 +326,7 @@ public function makeSqlStatement(string $sourceCondition, string $columnName, st
                     // if numeric field than only numeric characters are allowed
                     throw new Exception('SYS_NOT_NUMERIC', array($fieldName));
                 } else {
-                    $this->destCond .= $character;
+                    $conditionValue .= $character;
                 }
 
                 $newCondition   = false;
@@ -357,13 +339,16 @@ public function makeSqlStatement(string $sourceCondition, string $columnName, st
         if ($columnType === 'date' && $date !== '') {
             $formatDate = $this->getFormatDate($date, $operator);
             if ($formatDate !== '') {
-                $this->destCond .= $formatDate;
+                $this->destCond .= $db->escapeString($formatDate);
             } else {
                 throw new Exception('SYS_NOT_VALID_DATE_FORMAT', array($fieldName));
             }
         }
 
-        $this->endDestCond();
+        if ($conditionValue !== '') {
+            $this->destCond .= $db->escapeString($conditionValue);
+        }
+        $this->destCond .= ' ) ';
 
         return $this->destCond;
     }