Потенциальная уязвимость при получении объекта xPDO

Привет, друзья! Настало время подвести некоторые итоги по новости недельной давности.

Если кто не в курсе, в xPDO, а соотвественно, и в MODX обнаружилась уязвимость, позволяющая проводить слепые SQL инъекции и ломать сайты. Точнее как, обнаружилась… Всегда там была, и кому нужно — давно это знали.

Суть в том, что при получении объекта xPDO можно указать вторым параметром любую строку, и она не фильтруется.

$modx->getObject('modResource', 'тут любой SQL код')

Этот код выполнит произвольный SQL запрос, потому что «фича, а не бага».

Правда, про эту фичу нет ни слова в документации, где говорят только о

The criteria can be a primary key value, an array of primary key values (for multiple primary key objects) or an xPDOCriteria object.

и никаких сырых SQL выражений.


То есть, если вы вдруг думаете, что xPDO что-то за вас отфильтрует, когда вы пишите

$modx->getObject('modResource', $_REQUEST['pageId'])

то вы очень серьёзно заблуждаетесь.
И смена префикса БД никак не поможет, что уже было продемонстрировано товарищем zenit.

Решение очевидно — всегда приводить эту строку к числу:

$modx->getObject('modResource', (int)$_REQUEST['pageId'])

Это хорошо работает с объектами, у которых первичным ключом является id.

Окей, а что делать с объектами, у которых ключ является строкой, например, объект modContext? А тут нужно принудительно передавать ассоциативный массив в качестве ключа

$modx->getObject('modContext', array('key' => $_REQUEST['ctx']))

Тогда xPDO экранирует полученную строку.

И самое смешное, что авторы MODX об этом давно знают, и никакой проблемы не видят. Вот, еще в 2014 году было массовое исправление контроллеров админки, с принудительной проверкой первичного ключа. А до этого, соотвественно, и в менеджере можно было свободно резвиться. Кстати, не факт, что и сейчас всё закрыто.

Так что же делать нам, разработчикам и пользователям дополнений, у которых теперь неизвестно сколько точек потенциального взлома на сайте? Я предлагаю менять ядро системы.

Недоработка, на мой взгляд, в том, что когда в метод xPDO::getObject вторым параметром приходит не объект xPDOQuery, то он этот объект создаёт вот так:

public function getCriteria($className, $type= null, $cacheFlag= true) {
    return $this->newQuery($className, $type, $cacheFlag);
}

То есть, пихает в условие любые данные, с инъекциями или без — ему неважно.

Честно говоря, я очень сомневаюсь, что большинство разработчиков отправляет в getObject именно SQL запросы, вместо первичного ключа, поэтому предлагаю поменять метод в файле core/xpdo/xpdo.class.php:

public function getCriteria($className, $type = null, $cacheFlag = true)
{
    $c = $this->newQuery($className);
    $c->cacheFlag = $cacheFlag;
    if (!empty($type)) {
        if ($type instanceof xPDOCriteria) {
            $c->wrap($type);
        } elseif (is_scalar($type)) {
            if ($pk = $this->getPK($className)) {
                $c->where(array($pk => $type));
            }
        } else {
            $c->where($type);
        }
    }

    return $c;
}

Теперь любое условие, если это не объект и не массив, приводится к использованию в качестве нормального ключа. Все SQL инъекции превращаются в экранированные строки. Возможно, и это получится обойти, но у меня пока не вышло.
Если же кому-то нужно получить объект с джоинами и сложными условиями — они, как и прежде, вручную соберут $modx->newQuery и передадут его в getObject, как обычно.

Кстати говоря, в Laravel для сырых запросов есть отдельные RAW методы, которые позволяют разработчику сразу для себя решить, где он использует чистый SQL, а где нет.

Исправления от автора xPDO ждать не стоит, этот вопрос с нами надолго.

См. P.P.S.

Я долго думал, писать об этом или нет — ведь такая новость обратит внимание на уязвимость. Но:
1. «Фича» эта существует с момента создания самого xPDO — и кому нужно, давно про неё знают
2. Радикально решать вопрос никто не собирается

Так что мы можем протестировать мою заплатку на ваших сайтах (на своих я её уже везде использую), найти возможные косяки в работе, а позже выпустить её в виде хотфикса-дополнения для всех.

Наверняка это не закрывает все лазейки, но против самых простых и тупых — однозначно поможет.

P.S. Здесь можно поплакать, какая теперь MODX плохая и небезопасная, однако нужно понимать, что безопасность системы никак не коррелирует с её популярностью и распространением в мире. Примеры: Wordpress и Joomla.

Все свои дополнения я уже обновил и вам советую поступить так же.

P.P.S. После небольшой беседы, Jason Coward вник в вопрос и пообещал внести исправления как можно скорее. Я со своей стороны отправил PR в репозиторий.

Как мы, разработчики, всё-таки не любим разбираться в проблемах! Но, в любом случае, еще не всё потеряно!

public function getObject($className, $criteria= null, $cacheFlag= true) {
        $instance= null;
        if ($criteria !== null) {
            $instance = $this->call($className, 'load', array(& $this, $className, $criteria, $cacheFlag));
        }
        return $instance instanceof $classname ? $instance : null;
    }

public function getCriteria($className, $type = null, $cacheFlag = true)
{
    $c = $this->newQuery($className);
    $c->cacheFlag = $cacheFlag;
    if (!empty($type)) {
        if ($type instanceof xPDOCriteria) {
            $c->wrap($type);
        } elseif (is_array($type)) {  etc...