Потенциальная уязвимость при получении объекта 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 в репозиторий.

Как мы, разработчики, всё-таки не любим разбираться в проблемах! Но, в любом случае, еще не всё потеряно!
13 ноября 2016, 07:54    Василий Наумкин   G+  
10    1375 +31

Комментарии (28)

  1. Максим 13 ноября 2016, 08:23 # 0
    Есть ли смысл делать еще такую проверку?
    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;
        }
    
    1. Василий Наумкин 13 ноября 2016, 08:31 # +1
      Естественно. Посмотри метод xPDOObject::load и проследи, когда он приведёт к xPDO::getCriteria.
    2. Сергей Шлоков 13 ноября 2016, 08:57 # +6
      Видимо они вовсю пользуются возможностью передавать сырой SQL. Им твоя заплатка поломает сайты.
      А на своих сайтах нам надо повесить предупреждение для хакеров, чтобы они не посылали чистые sql запросы, ведь нужно использовать первичные ключи. Так Джейсон сказал!!!
      Немного конспирологии… А может они специально оставляют эти дырки по просьбе АНБ. :)
      1. Вася 13 ноября 2016, 11:39 # +2
        Вот и форкнули modx
        1. Евгений Борисов 13 ноября 2016, 12:27 # +13
          Вот и официальное подтверждение того, что я понял 2 года назад — MODX наше все. Кто не согласен — сам дурак.
          1. Сергей Шлоков 13 ноября 2016, 13:21 # 0
            Иииииихаааааааа. )) Теперь держимся крепче, чтобы не вывалиться.
            1. Евгений Борисов 13 ноября 2016, 19:19 # +1
              Уже удалили.
              1. Иван Климчук 13 ноября 2016, 20:00 # +6
                Потому что я написал об этом команде, что и просят делать в случае нахождения уязвимостей. Как бы то ни было, можно ныть, что все плохо и лучше не будет, но бля (я тут намеренно не цензурирую свои слова), нахера мы тогда нужны (разработчики)? Как будто все тут пишут идеальный код без факапов каждую минуту всю свою жизнь? Нашел ошибку, укажи, поясни, расскажи подробно, если разработчик не понял сути (а такое бывает, все люди мыслял по разному). Не хочу ругаться, но не понимаю такого отношения.
                1. Евгений Борисов 13 ноября 2016, 20:04 # +1
                  Не хочу ругаться, но не понимаю такого отношения.
                  Чего не понимаешь? Почему я им не написал. Ну да, бяка я. С 2014 года мне ничего не остается, кроме как стебаться…

                  Хотя кто-то, а ты должен знать всю истории и понимать, почему я им принципиально ничего не сообщаю.
          2. Евгений Борисов 13 ноября 2016, 12:43 # +1
            безопасность системы никак не коррелирует с её популярностью и распространением в мире. Примеры: Wordpress и Joomla.
            Кстати, сегодняшнее заявление официально подверждает, что можно тоннами слать баги по каждому компоненту на exploit-db (как это делается для Joomla и WordPress). Если решиться, то думаю всего за 1 день +100 пунктов наберем и убьем всю схему монетизации MODX. Останется сравнивать уже только популярность, т.к. безопасность будет на одном уровне по числу публичных дырок.

            Так что осталось только дождаться сигнала к действию. Чтобы это стало началом конца.
            1. Дмитрий Касаткин 13 ноября 2016, 13:13 # +5
              Здесь, в клубе любителей MODX, думаю всем будет только хуже от такого сценария. Особенно если удастся «убить всю схему монетизации», система вообще может перестать развиваться.

              Не надо нам начала конца, но спасибо, что приглядываете за MODX и за ваш, Евгений, вклад в него. Мы пользуемся вашими наработками с большим удовольствием. Документации уж нет, но хоть репозитории остались)
              1. Василий Наумкин 13 ноября 2016, 19:16 # +7
                Так что осталось только дождаться сигнала к действию. Чтобы это стало началом конца.
                А вот и фигушки. Jason Coward таки вник в вопрос и пообещал срочно принять меры.

                Говорит, были занятые выходные и он просто сразу не въехал в масштаб проблемы (а мой английский, очевидно, не так хорош, чтобы это доступно объяснить).

                В любом случае, скоро должен быть фикс — обновил заметку.
                1. Евгений Борисов 13 ноября 2016, 19:21 # +8
                  Конца света не будет. Расходимся)))
                  1. Bluetenstadt 13 ноября 2016, 20:06 # +2
                    да блин, я ток попкор заготовил
              2. Николай Ланец 13 ноября 2016, 13:24 # 0
                Василий, глянь еще в сторону sortby('1; select 1 ');
                1. Bluetenstadt 13 ноября 2016, 14:31 # 0
                  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...
                  в какой строке вставить или заменить?
                  1. Павел Романов 13 ноября 2016, 14:41 # +1
                    1134 — 1136
                    1. Bluetenstadt 13 ноября 2016, 14:42 # 0
                      спасибо
                  2. Mark Hamstra 13 ноября 2016, 23:21 # +16
                    As I also posted on the other thread, please email security related issues to security@modx.com. For this case there is already a pull request that Jason has merged and is working on further, but please always email security@modx.com with details on possible vulnerabilities. It's not always clear what is happening right away, so we need enough information to reproduce and fix any security related issue.
                    1. Николай 14 ноября 2016, 11:34 # 0
                      Как мы, разработчики, всё-таки не любим разбираться в проблемах! Но, в любом случае, еще не всё потеряно!
                      Вот за такое признание отдельный плюс. Что правда, то правда. Хотя это понятно, уж слишком часто пользователи паникуют, просто по глупости.

                      Наверное стоит иметь возможность голосования за pull request и проблемы. И если большое количество людей паникует, то это уже серьезно.
                      1. Василий Наумкин 14 ноября 2016, 11:36 # +4
                        На GitHub давно сделаны плюсики для issues и pr, только мало кто туда из рядовых пользователей заглядывает.
                        1. Николай 14 ноября 2016, 11:48 # 0
                          Спасибо, реально не знал!
                          1. Вася 14 ноября 2016, 12:04 # 0
                            Я что то не смог найти где ставятся эти плюсики (
                            1. Илья Уткин 14 ноября 2016, 14:47 # +3
                              1. Вася 14 ноября 2016, 14:50 # 0
                                Спасибо! Это находил и ставил не думал, что это и есть плюсики )
                        2. usdm 19 ноября 2016, 12:26 # 0
                          Друзья, подскажите применил решение Василия в файле core/xpdo/xpdo.class.php… и заметил перестал работать компонент редиректор, а именно все редиректы (я сейчас переношу сайт с joomla на modx) из более 2000… все редиректы редиректят на 1ю запись в редиректах…

                          тем кто уже обновился до 2.5.2 проверьте корректно ли работает компонент?
                          1. Марат Марабар 19 ноября 2016, 17:56 # +1
                            Проверил — работает.

                            Попробуй полностью MODX обновить до версии 2.5.2
                            1. usdm 19 ноября 2016, 18:15 # 0
                              спасибо, буду обновляться полностью
                          Вы должны авторизоваться, чтобы оставлять комментарии.