Настоящей статьей мы продолжаем серию обзоров, посвященных обнаружению уязвимостей в open-source проектах с помощью статического анализатора кода AppChecker.


В рамках этой серии рассматриваются наиболее часто встречающиеся дефекты в программном коде, которые могут привести к серьезным уязвимостям. В этой статье мы остановимся на широком классе дефектов типа "Expression Issues" и рассмотрим их на примерах на языках PHP и Java.



В международной классификации CWE данный тип дефектов известен как CWE-569: Expression Issues. К нему относятся различные ошибки в логических выражениях в коде программы. Частным случаем дефекта такого класса является дефект «Присваивание вместо сравнения».



Присваивание вместо сравнения (CWE-481: Assigning instead of Comparing) – дефект, как видно из названия, заключающийся в том, что вместо оператора сравнения в коде программы используется оператор присваивания. Подобная ошибка в коде ведет к некорректной работе программы и может привести к серьезной бреши в безопасности.


Страх совершения подобных ошибок довел того, что появилась так называемая нотация Йоды (Yoda notation), требующая написании константы или вызова функции слева от оператора сравнения: «7 == j» вместо привычного «j == 7». Давайте посмотрим код. Например, такой:


??pswd=GetText();
??if (consthash=hash(pswd)); //авторизовать пользователя

Предположим, что pswd – переменная, в которой хранится введенный пользователем пароль. В условном операторе должно происходить сравнение хэша от пароля с заранее заданным значением. Если эти значения совпадают, считается, что пароль совпал, и пользователь получает доступ. Однако в этом примере вместо оператора сравнения «==» стоит оператор присваивания «=». В этом случае переменной consthash присвоится значение hash(pswd), и поскольку эта операция выполнена успешно, программа посчитает, что условие выполняется, что, в свою очередь, приведет к авторизации пользователя, независимо от введенного им пароля.


Казалось бы, что данный пример достаточно искусственный и вряд ли может встретиться в серьезных программных продуктах. Однако, анализ opensource проектов показал, что этот дефект встречается довольно часто. Обычно такие дефекты возникают из-за невнимательности разработчика. Подобные дефекты можно обнаруживать с помощью сигнатурно-эвристического анализа.


Логично предположить, что существует и обратный дефект: сравнение вместо присваивания. Действительно, классификация CWE определяет этот тип дефектов, как CWE-482: Comparing instead of Assigning.


Далее приведем пример реального кода, в котором наоборот вместо операции присваивания используется операция сравнения.


if ($mValueCount == floor($mValueCount)) {
     ...
} else { 
     $mValueCount == floor($mValueCount);
     ... 
}

Как видно, по ошибке программиста выражения в условии и теле условного оператора идентичны, хотя во втором случае явно должен быть оператор присваивания. Этот фрагмент кода взят из библиотеки для работы с документами с помощью PHP – PHPExcel версии 1.8.1. Мы уведомили разработчиков об обнаруженном с помощью AppChecker дефекте, и уже выпущен патч, в котором этот дефект был исправлен (https://github.com/PHPOffice/PHPExcel/pull/710/files).


Еще один пример такого типа дефектов был найден в библиотеке для работы с MP3, которая входит в состав некоторых CMS, в частности wordpress и написанной на языке PHP – getID3-1.9.10:


if (ord($frame_ownerid) === 0) {
     $frame_ownerid == '';
}

Как видно из этого фрагмента кода в теле условного оператора происходит сравнение, хотя по логике должно быть присваивание. Мы уведомили разработчиков об этом дефекте, и они оперативно этот дефект исправили (https://github.com/JamesHeinrich/getID3/pull/57/files).


Тем не менее, класс «Дефекты в выражениях» не ограничивается этими двумя дефектами. Другим примером может являться дефект «Выражение всегда верно» (CWE-571: Expression is always true) и «Выражение всегда ложно» (CWE-570: Expression is always false). Как очевидно из названия, эти дефекты подразумевают написание условия в условном операторе, которое будет всегда верно/всегда ложно, что в свою очередь означает, что в условном операторе в данном участке кода нет никакой надобности.


В качестве примера рассмотрим фрагмент кода CMS с открытым исходным кодом Chamilo LMS 1.10.4, написанной на языке PHP:


if ($row['item_type'] != 'dokeos_chapter' || $row['item_type'] != 'dokeos_module') {

Выражение в условном операторе верно всегда, поскольку для него необходимо, чтобы одно и то же значение 'item_type' было либо не равно 'dokeos_chapter', либо не равно 'dokeos_module'. Таким образом, единственным вариантом, при котором это выражение будет ложным – когда 'item_type' одновременно равен обоим, в общем случае, разным значениям. Разработчики были уведомлены о дефекте и оперативно его исправили (https://github.com/chamilo/chamilo-lms/pull/1109/files). Как оказалось, в выражении должен стоять оператор «&&» вместо «||».


В качестве обратного примера, рассмотрим платформу для электронной коммерции с открытым исходным кодом OpenCart 2.2.0.0:


if ($chr == 252 && $chr == 253) {

Очевидно, что $chr не может быть одновременно равен 252 и 253. Этот дефект разработчики также устранили (https://github.com/opencart/opencart/pull/4231/files). Как оказалось, в выражении должен стоять оператор «||» вместо «&&».


Так же к дефектам вида «Expression Issues» можно отнести случаи, когда обе части бинарного выражения идентичны (в частном случае это приводит к самоприсваиванию). Рассмотрим следующий фрагмент кода на Java:


if (s2.getClass() != s2.getClass()) return false;

Как видно из этого фрагмента кода, s2.getClass() сравнивается сам с собой, в результате чего условие всегда будет ложным. Вероятнее всего это просто опечатка программиста, но случай далеко не искусственный. Данный пример взят из google sageTV – кроссплатформенной системы управления медиа-данными. Разработчики были уведомлены об этом дефекте и оперативно его исправили (https://github.com/google/sagetv/commit/93144762681a5f441ad011564ff8309095d9ca31).


Очевидно, что такие дефекты также не зависят от языка программирования. При помощи AppChecker такие дефекты обнаруживаются для всех поддерживаемых языков.


Следующий пример на языке PHP взят из OpenEMR-4.2.1:


if ( !empty($obx24) || !empty($obx24) || !empty($obx25) )

Как видно из этого фрагмента кода, значение $obx24 проверяется дважды, хотя по логике в первом случае должно быть $obx23. Подобная невнимательность разработчика может привести к серьезным проблемам в работе программы. С этим согласны и разработчики OpenEMR – они оперативно исправили этот дефект, когда мы им о нем сообщили (https://github.com/openemr/openemr/pull/179).


Подобные дефекты могут оставаться незамеченными очень долго, но программа при этом будет работать некорректно, а в некоторых случаях, как мы уже выяснили, и вовсе может привести к уязвимостям безопасности. Стоит отметить, что дефект является логическим, а потому независимым от языка программирования.


В этой статье были рассмотрены дефекты типа «Expression Issues». Несмотря на простоту и кажущуюся банальность такого рода дефектов, они встречаются достаточно часто, как в open-source, так и в коммерческих проектах.


PS>
Бесплатную версию AppChecker можно скачать с нашего сайта: https://file.cnpo.ru/index.php/s/o1cLkNrUX4plHMV

Поделиться с друзьями
-->

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


  1. jigpuzzled
    25.01.2017 15:23
    -7

    Мораль статьи такова: пишите нормальные тесты, тогда такие ошибки найдутся сами без всяких тулзов. А если деньги негде деть(я так понял полная версия платная) то купите подписку на Scrutinizer.


    1. Andrey2008
      25.01.2017 16:17
      +9

      Да, вот только в тестах тоже бывают ошибки. Я неоднократно приводил такие примеры в своих статьях. И даже отдельная статья есть: "Как статический анализ дополняет TDD".

      Да и не все ошибки тестами можно выявить. Вот как, например, написать тест вот на такое? Как проверить, что память обнулена?

      Так что скорее следует говорить, что разные методологии дополняют друг друга. Но никак о том, что вот этот способ — серебряная пуля.


      1. Alex_T666
        26.01.2017 11:34

        Но никак о том, что вот этот способ — серебряная пуля.


        Как говориться +100500. А то посмотришь иногда на сами тесты, оторопь берет. Вопрос — зачем ты столько времени на это убивал? Чтобы написать заветное «покрытие 99%»?

        Как обычно из крайности в крайность, лишь бы «на поезд не опоздать», в тренд попасть.


    1. kalessil
      25.01.2017 18:01
      +5

      Scrutinizer что-то, конечно, находит, но для интереса попробуйте Php Inspections (EA Extended) (плагин для PhpStorm), посмотрите что нормальные анализаторы могут.

      Ну и PVS-Studio, конечно, вообще молодцы и дело говорят.


    1. lany
      26.01.2017 04:24
      +5

      Даже в тех местах, которые легко покрываются тестами, платный статический анализатор (если он действительно лучше бесплатного аналога) легко окупится. Написание тестов на каждый branch в коде — занятие утомительное и отнимает кучу времени. Проще поднять покрытие тестами с 60% до 80%, чем с 90% до 95%. Мой опенсорсный проект покрыт на 98.6% по строкам кода, я также заморачивался мутационным тестированием (по PITest показатель около 94%) и знаю, о чём говорю. При этом у меня мегабайт исходников и полмегабайта тестов. И это работа в свободное время для души, поэтому я упоролся. По грубым прикидкам написание тестов заняло треть времени работы над проектом. Готовы ли вы на написание тестов потратить 33% времени разработчиков? Именно разработчиков, а не специально выделенных тестеров, которые за еду работают. Чтобы добиться максимального покрытия, придётся и код частенько рефакторить, это не для тестера работа. Если вы готовы, то скорее всего это вам некуда деньги девать. У продукта должно быть вменяемое соотношение качества и трудозатрат. Если вы не пишете систему управления атомным реактором, то важнее выйти вовремя на рынок, чем выпустить продукт без единой ошибки. Однако в отличие от написания тестов использование статического анализатора вам планку качества поднимет практически нахаляву. В крупном и даже среднем проекте вылавливание тех же багов тестами потребовало бы на порядок больше денежных затрат (при пересчёте из человеко-часов в рубли), чем покупка и использование статического анализатора.


  1. Andrey2008
    25.01.2017 16:42
    +10

    О, оказывается сколько мы разных CWE ловим… Правда, что ли, PVS-Studio Secure пора делать… :)

    Для интересующихся этой темой, выписал ссылки на примеры реальных ошибок для перечисленных здесь CWE:

    1. CWE-481: Assigning instead of Comparing: V559
    2. CWE-482: Comparing instead of Assigning: косвенно найдёт V607 (см. например баг в Boost)
    3. CWE-571: Expression is always true/CWE-570: Expression is always false: V547, V654, V3063, V637, V3022

    Так что все эти проблемы совершенно реальны и портят жизнь программистов.


  1. SerafimArts
    25.01.2017 21:04

    Эм, а это нормально что вы поддерживаете, цитата "PHP 4/5", а на дворе уже 7.1?


    P.S. Дополняя коммент jigpuzzled — Scruinizer абсолютно бесплатный для OpenSource.


  1. vovochkin
    26.01.2017 08:41

    Скажите, пожалуйста, а удалось ли исправить проблему из комментария?
    Сейчас я дополнил код, теперь он выглядит

    так:
    bool CheckPointer(int value)
    {
       int* intPtr(new int(value));
       std::cout << std::endl << *intPtr << std::endl;
       return true;
    }
    class A {
            int x;
            public:
                void bar() {
                    std::cout << x << "Test!\n";
                }
    };
    
    int main()
    {
        CheckPointer(5);
        A* a;
        a->bar();
    
        int adminPassword(123456);
        int currentUserPassword(123);
        if (currentUserPassword = adminPassword)
        {
            std::cout << "You are admin";
        }
        else
        {
            std::cout << "You are not admin";
        }
        return 0;
    }
    


    1. facet
      27.01.2017 18:51

      Дефект, который у Вас в примере — это CWE-457: "Use of Uninitialized Variable". Подобные дефекты AppChecker обнаруживает в настоящее время для языка PHP. Возьмем к примеру тот же Chamilo LMS, о котором написано в статье. Там есть, например, вот такой код:


      $ldapuser = extldap_authenticate($login, 'nopass', true); 
      if ($ldap_user !== false) {
         $chamilo_user = extldap_get_chamilo_user($ldapuser); 
      ..
      }

      Здесь как раз используется неинициализированная переменная — значение присваивается переменной $ldapuser, сравнивается $ldap_user, а потом используется $ldapuser.
      Этот дефект разработчики также исправили после того как мы им о нем сообщили.


      В настоящее время мы дорабатываем продукт, чтобы находить этот дефект и для C/C++


      1. SerafimArts
        27.01.2017 19:39
        +2

        Описанная проблема ловится сама в совершенно бесплатных Scrutinizer и плагине EA (что на порядок удобнее, т.к. всё видно в самой IDE):



        В связи с этим и вопрос, т.к. вы продаёте продукт, протестировать я его не могу (не готов тратить столько времени на регистрацию и ожидание аппрува), да ещё и для устаревших версий php (http://php.net/supported-versions.php), какие вообще у вас конкурентные преимущества?


        1. facet
          30.01.2017 18:51
          +2

          Конкурентные преимущества AppChecker:


          • Осуществляет поиск свыше 100 типов дефектов кодирования.
          • Классифицирует потенциально опасные конструкции в соответствии со стандартом CWE.
          • Web-интерфейс позволяет проводить совместный аудит кода несколькими экспертами, а высокое качество разбора исходных текстов позволяет снизить число ложных срабатываний.
          • Гибкая конфигурация анализируемых проектов позволяет учитывать влияние таких особенностей языков программирования, как например директивы прекомпиляции (в C/C++).
          • Интеграция с СУВ и CI


        1. kalessil
          31.01.2017 18:01

          SerafimArts, вот ещё пара проектов, которые стоит посмотреть:

          • https://www.exakat.io/ (рекомендую посмотреть — разные анализаторы ловят зачастую разные проблемы)
          • https://www.sonarqube.org/ (фокус на code quality)


          1. SerafimArts
            31.01.2017 19:21

            Не вижу у Exakat возможность указания ветки при запросе ссылки на репу — это вообще возможно?


            1. SerafimArts
              31.01.2017 20:00

              P.S. Написали мне письмо:


              Hello,
              We have tried to process the code at ...., but it is too big.
              May be you can try a smaller repository? There are too many PHP files. Try keeping 1/4 of the PHP scripts

              Получается Exakat вообще не вариант для простеньких сайтиков (это был обычный офф сайтик laravel, там только статьи, да документация, ничего особо серьёзного), я уж не говорю про крупные проекты.


              1. kalessil
                01.02.2017 10:45

                — Отдельно для каждой ветки — не пробовал, спросил: нет, проверяется текущая ветка.
                — Это ограничения пробной версии, большие проекты тоже можно проверять. Проект приватный или опенсорсный (может ссылку дадите)?


                1. SerafimArts
                  01.02.2017 11:34

                  1. kalessil
                    01.02.2017 20:35

                    О, то что надо. Я пошел узнавать, отпишусь как мне ответят)