diff --git a/CHANGELOG b/CHANGELOG index 26d66cc6..afd6358c 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,11 @@ +Unreleased: + * Fix security: identifiers passed to escapeField() are now really escaped (backticks doubled on MySQL, double quotes doubled on PostgreSQL, strict whitelist on SphinxQL) + * Fix security: Repository::getBy() order keys and Repository::get() primary-key arrays are checked against the metadata, unknown columns or directions throw instead of reaching the SQL + * Fix security: setTimezone() refuses values that are not a timezone name, and Pgsql connect() quotes every DSN value (a password with a space or quote no longer breaks the DSN nor injects an option) + * Fix security: the connection key used by ConnectionPool no longer contains the password in clear text + * Fix Mysqli: values inlined by execute() are quoted with apostrophes, so sql_mode=ANSI_QUOTES no longer turns them into identifiers + * Fix Pgsql: getInsertedId() throws when lastval() fails + 4.1.1 (2026-10-07): * Fix UnitOfWork: saving a managed entity without any change removes it from the queue. It stayed queued, kept in memory until detach() or reset() (a leak in worker mode), and a later change made without pushSave() was written by the next process() * Fix security: Pgsql\Driver::getInsertedIdForSequence() passes the sequence name as a query parameter instead of concatenating it in the SQL diff --git a/src/Ting/Driver/Mysqli/Driver.php b/src/Ting/Driver/Mysqli/Driver.php index 6e047e4c..72c6b5b0 100755 --- a/src/Ting/Driver/Mysqli/Driver.php +++ b/src/Ting/Driver/Mysqli/Driver.php @@ -148,7 +148,7 @@ public static function getConnectionKey(array $connectionConfig, $database) $connectionConfig['host'] . '|' . $connectionConfig['port'] . '|' . $connectionConfig['user'] . '|' . - $connectionConfig['password']; + hash('sha256', (string) $connectionConfig['password']); } /** @@ -324,7 +324,8 @@ protected function quoteValue($value) "boolean" => (int) $value, "integer", "double" => $value, "NULL" => 'null', - default => '"' . $this->connection->real_escape_string($value) . '"', + // Apostrophes : en mode sql_mode=ANSI_QUOTES les guillemets désignent un identifiant, pas une chaîne + default => "'" . $this->connection->real_escape_string((string) $value) . "'", }; } @@ -409,7 +410,8 @@ public function ifIsNotConnected(callable $callback) */ public function escapeField($field) { - return '`' . $field . '`'; + // Un identifiant MySQL se protège en doublant les accents graves qu'il contient + return '`' . str_replace('`', '``', (string) $field) . '`'; } /** @@ -525,6 +527,9 @@ public function setTimezone($timezone) if ($this->currentTimezone === $timezone) { return; } + if ($timezone !== null && preg_match('/^[A-Za-z0-9_+\\-\/:]{1,64}$/', (string) $timezone) !== 1) { + throw new DriverException('Invalid timezone value: ' . $timezone); + } $value = $timezone; $query = 'SET time_zone = "%s";'; diff --git a/src/Ting/Driver/Pgsql/Driver.php b/src/Ting/Driver/Pgsql/Driver.php index 9aba51da..0aeee43f 100755 --- a/src/Ting/Driver/Pgsql/Driver.php +++ b/src/Ting/Driver/Pgsql/Driver.php @@ -104,7 +104,7 @@ public static function getConnectionKey(array $connectionConfig, $database) $connectionConfig['host'] . '|' . $connectionConfig['port'] . '|' . $connectionConfig['user'] . '|' . - $connectionConfig['password'] . '|' . + hash('sha256', (string) $connectionConfig['password']) . '|' . $database; } @@ -118,10 +118,19 @@ public static function getConnectionKey(array $connectionConfig, $database) */ public function connect($hostname, $username, $password, $port) { - $this->dsn = 'host=' . $hostname . ' user=' . $username . ' password=' . $password . ' port=' . $port; + // Chaque valeur est quotée : un mot de passe avec espace ou apostrophe ne casse plus le DSN et ne peut pas y injecter une option (sslmode=...) + $this->dsn = 'host=' . $this->quoteDsnValue($hostname) + . ' user=' . $this->quoteDsnValue($username) + . ' password=' . $this->quoteDsnValue($password) + . ' port=' . $this->quoteDsnValue($port); return $this; } + private function quoteDsnValue($value): string + { + return "'" . addcslashes((string) $value, "\\'") . "'"; + } + /** * Close the connection to the database * @return $this @@ -177,11 +186,11 @@ public function setDatabase($database) return $this; } - $resource = pg_connect($this->dsn . ' dbname=' . $database); + $resource = pg_connect($this->dsn . ' dbname=' . $this->quoteDsnValue($database)); $this->database = $database; if ($resource === false) { - $dsn = preg_replace('/((user|password)=[^\s]+)/', '$2=', $this->dsn); + $dsn = preg_replace('/((user|password)=\'(?:[^\'\\\\]|\\\\.)*\')/', '$2=', $this->dsn); throw new DriverException('Connect Error: ' . $dsn . ' dbname=' . $database); } $this->connection = $resource; @@ -370,7 +379,8 @@ public function ifIsNotConnected(callable $callback) */ public function escapeField($field) { - return '"' . $field . '"'; + // Un identifiant PostgreSQL se protège en doublant les guillemets qu'il contient + return '"' . str_replace('"', '""', (string) $field) . '"'; } /** @@ -419,6 +429,9 @@ public function rollback() public function getInsertedId() { $resultResource = pg_query($this->connection, 'SELECT lastval()'); + if ($resultResource === false) { + throw new QueryException(pg_last_error($this->connection) . ' (Query: SELECT lastval())'); + } $row = pg_fetch_row($resultResource); return (int) $row[0]; } @@ -516,6 +529,9 @@ public function setTimezone($timezone) if ($this->currentTimezone === $timezone) { return; } + if ($timezone !== null && preg_match('/^[A-Za-z0-9_+\\-\/:]{1,64}$/', (string) $timezone) !== 1) { + throw new DriverException('Invalid timezone value: ' . $timezone); + } $value = $timezone; $query = 'SET timezone = "%s";'; if ($timezone === null) { @@ -540,6 +556,7 @@ public function reconnect(): bool $this->currentCharset = null; $this->setCharset($charset); } + } catch (DriverException) { return false; } diff --git a/src/Ting/Driver/SphinxQL/Driver.php b/src/Ting/Driver/SphinxQL/Driver.php index 06365756..e4493cc8 100644 --- a/src/Ting/Driver/SphinxQL/Driver.php +++ b/src/Ting/Driver/SphinxQL/Driver.php @@ -50,6 +50,11 @@ protected function quoteValue($value) */ public function escapeField($field) { + // SphinxQL ne protège pas les identifiants : on ne laisse passer que des noms simples + if (preg_match('/^[A-Za-z_][A-Za-z0-9_]*(\\.[A-Za-z_][A-Za-z0-9_]*)?$/', (string) $field) !== 1) { + throw new \InvalidArgumentException('Invalid SphinxQL identifier: ' . $field); + } + return $field; } } diff --git a/src/Ting/Query/Generator.php b/src/Ting/Query/Generator.php index d272fdb1..960805c0 100644 --- a/src/Ting/Query/Generator.php +++ b/src/Ting/Query/Generator.php @@ -407,18 +407,19 @@ protected function updateSQLWithOrderAndLimit(string &$sql, DriverInterface $dri */ protected function generateOrder(array $orderList, DriverInterface $driver) { - $fields = $this->escapeFields(array_keys($orderList), $driver); - $orderClause = ''; $orderCriteria = []; - $i = 0; - foreach ($orderList as $value) { - $value = strtoupper($value); - if (\in_array($value, ['ASC', 'DESC'])) { - $orderCriteria[] = $fields[$i] . ' ' . $value; + foreach ($orderList as $column => $direction) { + // Une colonne de tri inconnue de la table est refusée : ORDER BY ne doit jamais recevoir une valeur libre + if (\in_array((string) $column, $this->fields, true) === false) { + throw new \InvalidArgumentException(sprintf('Unknown column "%s" in order clause', $column)); } - $i++; + $direction = strtoupper((string) $direction); + if (\in_array($direction, ['ASC', 'DESC'], true) === false) { + throw new \InvalidArgumentException(sprintf('Invalid order direction "%s" for column "%s"', $direction, $column)); + } + $orderCriteria[] = $driver->escapeField($column) . ' ' . $direction; } if (count($orderList) > 0) { @@ -436,6 +437,6 @@ protected function generateOrder(array $orderList, DriverInterface $driver) */ protected function generateLimit($limit) { - return ' LIMIT ' . $limit; + return ' LIMIT ' . (int) $limit; } } diff --git a/src/Ting/Repository/Metadata.php b/src/Ting/Repository/Metadata.php index e3eebbd4..d9efd17f 100644 --- a/src/Ting/Repository/Metadata.php +++ b/src/Ting/Repository/Metadata.php @@ -604,8 +604,33 @@ public function getByCriteriaWithOrderAndLimit( $fields ); $criteriaColumn = $this->getColumnsFromCriteria($criteria); + // Les clés de tri sont des propriétés de l'entité, traduites en colonnes et vérifiées comme les critères + $orderByColumn = $this->getOrderColumns($orderBy); - return $queryGenerator->getByCriteriaWithOrderAndLimit($criteriaColumn, $collectionFactory, $forceMaster, $orderBy, $limit); + return $queryGenerator->getByCriteriaWithOrderAndLimit($criteriaColumn, $collectionFactory, $forceMaster, $orderByColumn, $limit); + } + + /** + * Translate order keys (entity property or column name) into column names and refuse unknown ones. + * + * @param array $orderBy + * @return array + * @throws ValueException + */ + protected function getOrderColumns(array $orderBy) + { + $orderByColumn = []; + foreach ($orderBy as $key => $direction) { + if (isset($this->fieldsByProperty[$key]) === true) { + $orderByColumn[$this->fieldsByProperty[$key]['columnName']] = $direction; + } elseif (isset($this->fields[$key]) === true) { + $orderByColumn[$key] = $direction; + } else { + throw new ValueException(sprintf('Undefined property or column %s in your order clause', $key)); + } + } + + return $orderByColumn; } /** @@ -624,9 +649,19 @@ protected function getPrimariesKeyValuesAsArray($originalValue) } else { throw new \CCMBenchmark\Ting\Exception('Incorrect format for primaries'); } - } else { - return $originalValue; } + + // Les clés (propriété de l'entité ou nom de colonne) sont traduites en colonnes et doivent être des primaires + $primariesKeyValue = []; + foreach ($originalValue as $key => $value) { + $columnName = isset($this->fieldsByProperty[$key]) === true ? $this->fieldsByProperty[$key]['columnName'] : $key; + if (isset($this->primaries[$columnName]) === false) { + throw new ValueException(sprintf('Undefined primary %s in your criteria', $key)); + } + $primariesKeyValue[$columnName] = $value; + } + + return $primariesKeyValue; } /** diff --git a/tests/units/Ting/Driver/Mysqli/Driver.php b/tests/units/Ting/Driver/Mysqli/Driver.php index 10280a6d..b1fc8ea3 100644 --- a/tests/units/Ting/Driver/Mysqli/Driver.php +++ b/tests/units/Ting/Driver/Mysqli/Driver.php @@ -352,7 +352,7 @@ public function testExecuteShouldOnlyReplaceParameters() ->mock($driverFake) ->call('query') ->withIdenticalArguments("SELECT 'Bouh:Ting', ' ::Ting', ADDTIME('23:59:59', '1:1:1') ' - . ' FROM Bouh WHERE id = 3 AND login = \"Sylvain\" AND is_banned = 0") + . ' FROM Bouh WHERE id = 3 AND login = 'Sylvain' AND is_banned = 0") ->once(); } @@ -580,6 +580,8 @@ public function testEscapeFieldShouldEscapeField() ->if($driver = new \CCMBenchmark\Ting\Driver\Mysqli\Driver($mockDriver)) ->string($driver->escapeField('Bouh')) ->isIdenticalTo('`Bouh`') + ->string($driver->escapeField('Bo`uh')) + ->isIdenticalTo('`Bo``uh`') ; } diff --git a/tests/units/Ting/Driver/Pgsql/Driver.php b/tests/units/Ting/Driver/Pgsql/Driver.php index 6dd60c0e..aec7310d 100644 --- a/tests/units/Ting/Driver/Pgsql/Driver.php +++ b/tests/units/Ting/Driver/Pgsql/Driver.php @@ -145,7 +145,7 @@ public function testSetDatabaseShouldCompleteGeneratedDsnByConnect() ->then($driver->setDatabase('database.test')) ->string($outerDsn) ->isIdenticalTo( - 'host=hostname.test user=user.test password=password.test port=1234 dbname=database.test' + "host='hostname.test' user='user.test' password='password.test' port='1234' dbname='database.test'" ); } @@ -285,6 +285,8 @@ public function testEscapeFieldShouldReturnEscapedField() ->if($driver = new \CCMBenchmark\Ting\Driver\Pgsql\Driver()) ->string($driver->escapeField('Bouh')) ->isIdenticalTo('"Bouh"') + ->string($driver->escapeField('Bo"uh')) + ->isIdenticalTo('"Bo""uh"') ; } diff --git a/tests/units/Ting/Query/Generator.php b/tests/units/Ting/Query/Generator.php index 18c45e69..7bb0e63d 100644 --- a/tests/units/Ting/Query/Generator.php +++ b/tests/units/Ting/Query/Generator.php @@ -230,7 +230,7 @@ public function testGetByCriteriaWithArrayValueAndOrderShouldReturnAQuery() ['id', 'population'] ) ) - ->object($generator->getByCriteriaWithOrderAndLimit(['name' => ['Xavier','Olivier']], $services->get('CollectionFactory'), true, ['name' => 'ASC'])) + ->object($generator->getByCriteriaWithOrderAndLimit(['name' => ['Xavier','Olivier']], $services->get('CollectionFactory'), true, ['population' => 'ASC'])) ->isInstanceOf(\CCMBenchmark\Ting\Query\Query::class) ->mock($this->mockConnection) ->call('master') @@ -240,6 +240,31 @@ public function testGetByCriteriaWithArrayValueAndOrderShouldReturnAQuery() } + public function testGetByCriteriaWithUnknownOrderColumnShouldRaiseException() + { + $services = new \CCMBenchmark\Ting\Services(); + + $this + ->if( + $generator = new \CCMBenchmark\Ting\Query\Generator( + $this->mockConnection, + $this->mockQueryFactory, + '', + 'table', + ['id', 'population'] + ) + ) + ->exception(function () use ($generator, $services): void { + $generator->getByCriteriaWithOrderAndLimit([], $services->get('CollectionFactory'), true, ['id` -- ' => 'ASC']); + }) + ->isInstanceOf(\InvalidArgumentException::class) + ->exception(function () use ($generator, $services): void { + $generator->getByCriteriaWithOrderAndLimit([], $services->get('CollectionFactory'), true, ['id' => 'ASC; DROP TABLE t']); + }) + ->isInstanceOf(\InvalidArgumentException::class) + ; + } + public function testGetByCriteriaWithArrayValueAndOrderLimitShouldReturnAQuery() { $services = new \CCMBenchmark\Ting\Services(); @@ -254,7 +279,7 @@ public function testGetByCriteriaWithArrayValueAndOrderLimitShouldReturnAQuery() ['id', 'population'] ) ) - ->object($generator->getByCriteriaWithOrderAndLimit(['name' => ['Xavier','Olivier']], $services->get('CollectionFactory'), true, ['name' => 'ASC'], 1)) + ->object($generator->getByCriteriaWithOrderAndLimit(['name' => ['Xavier','Olivier']], $services->get('CollectionFactory'), true, ['population' => 'ASC'], 1)) ->isInstanceOf(\CCMBenchmark\Ting\Query\Query::class) ->mock($this->mockConnection) ->call('master')