# Стреляем в ногу, обрабатывая входные данные Связующее звено сегодняшней статьи отличается от обычного\. Это не один проект, для которого был проведён анализ исходного кода, а ряд срабатываний одного и того же диагностического правила в нескольких разных проектах\. В чём здесь интерес? В том, что некоторые из рассмотренных фрагментов кода содержат ошибки, воспроизводимые при работе с приложением, а другие – и вовсе уязвимости \(CVE\)\. Кроме того, в конце статьи немного порассуждаем на тему дефектов безопасности\. ![0599_Vulnerabilities_ru/image1.png](https://import.viva64.com/docx/blog/0599_Vulnerabilities_ru/image1.png) ## Краткое предисловие Все ошибки, которые будут рассмотрены сегодня в статье, имеют схожий паттерн: * программа принимает данные из потока _stdin_; * выполняется проверка успешности считывания данных; * если данные считаны успешно, из строки удаляется символ переноса\. Тем не менее, все фрагменты, которые будут рассмотрены, содержат в себе ошибки и уязвимы к подстроенному вводу\. Так как данные принимаются от пользователя, который и может нарушить логику исполнения приложения, был велик соблазн попробовать что\-нибудь сломать\. Что я и сделал\. Все проблемы, приведённые ниже, были обнаружены [статическим анализатором PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/), который ищет ошибки в коде не только для C, C\+\+, но и для C\#, Java\. Конечно, найти проблему статическим анализатором – хорошо, но найти и воспроизвести – это уже совершенно другой уровень удовольствия\. :\) ## FreeSWITCH Первый подозрительный фрагмент кода был обнаружен в коде модуля _fs\_cli\.exe_, входящего в состав дистрибутива FreeSWITCH: ```cpp static const char *basic_gets(int *cnt) { .... int c = getchar(); if (c < 0) { if (fgets(command_buf, sizeof(command_buf) - 1, stdin) != command_buf) { break; } command_buf[strlen(command_buf)-1] = '\0'; /* remove endline */ break; } .... } ``` **Предупреждение PVS\-Studio**: [V1010](https://pvs-studio.ru/ru/docs/warnings/v1010/) [CWE\-20](https://cwe.mitre.org/data/definitions/20.html) Unchecked tainted data is used in index: 'strlen\(command\_buf\)'\. Анализатор предупреждает о подозрительном обращении по индексу к массиву _command\_buf_\. Подозрительным оно считается по той причине, что в качестве индекса используются непроверенные внешние данные\. Внешние – потому что получены через функцию _fgets_ из потока _stdin_\. Непроверенными \- так как никакой проверки перед использованием выполнено не было\. Выражение _fgets\(command\_buf, \.\.\.\.\) \!\= command\_buf_ \- не в счёт, так как таким образом мы проверяем только факт получения данных, но не их содержимое\. Проблема данного кода в том, что при определённых условиях произойдёт запись '\\0' за пределы массива, что приведёт к возникновению неопределённого поведения\. Для этого достаточно ввести строку нулевой длины \(строку нулевой длины с точки зрения языка Си, то есть такую, в которой первым символом будет '\\0'\)\. Давайте прикинем, что произойдёт, если передать на вход строку нулевой длины: * _fgets\(command\_buf, \.\.\.\.\)_ \-\> _command\_buf_; * _fgets\(\.\.\.\.\) \!\= command\_buf_ \-\> _false_ \(_then_\-ветвь оператора _if_ игнорируется\); * _strlen\(command\_buf\)_ \-\> _0_; * _command\_buf\[strlen\(command\_buf\) \- 1\]_ \-\> _command\_buf\[\-1\]_\. Упс\! Интересно здесь то, что это предупреждение анализатора вполне себе можно "пощупать руками"\. Для того, чтобы повторить проблему, нужно: * довести исполнение программы до данной функции; * подстроить ввод таким образом, чтобы вызов _getchar\(\)_ вернул отрицательное значение; * передать для функции _fgets_ строку с терминальным нулём в начале, которую та должна успешно считать\. Немного покопавшись в исходниках, я составил конкретную последовательность воспроизведения проблемы: * Запускаем _fs\_cli\.exe_ в batch\-режиме \(_fs\_cli\.exe \-b_\)\. Отмечу, что для выполнения дальнейших шагов подключение _fs\_cli\.exe_ к серверу должно пройти успешно\. Для этого достаточно, например, локально запустить _FreeSwitchConsole\.exe_ от имени администратора\. * Осуществляем ввод, чтобы вызов _getchar\(\)_ вернул отрицательное значение\. * Вводим строку с терминальным нулём в начале \(например, '\\0Oooops'\)\. * \.\.\.\. * PROFIT\! Ниже приводится видеозапись воспроизведения проблемы: ## NcFTP В проекте NcFTP была обнаружена аналогичная проблема, только встретилась она уже в двух местах\. Так как код выглядит похоже, рассмотрим только одно проблемное место: ```cpp static int NcFTPConfirmResumeDownloadProc(....) { .... if (fgets(newname, sizeof(newname) - 1, stdin) == NULL) newname[0] = '\0'; newname[strlen(newname) - 1] = '\0'; .... } ``` **Предупреждение PVS\-Studio**: [V1010](https://pvs-studio.ru/ru/docs/warnings/v1010/) [CWE\-20](https://cwe.mitre.org/data/definitions/20.html) Unchecked tainted data is used in index: 'strlen\(newname\)'\. Здесь, в отличии от примера из FreeSWITCH, код написан хуже и больше подвержен проблемам\. Например, запись '\\0' происходит вне зависимости от того, успешно произошло считывание с использованием _fgets_ или нет\. То есть здесь даже больше возможностей того, как можно нарушить нормальную логику исполнения\. Пойдём проверенным путём – через строки нулевой длины\. Воспроизводится проблема немного сложнее, чем в случае с FreeSWITCH\. Последовательность шагов описана ниже: * запуск и подключение к серверу, с которого можно загрузить файл\. Я, например, использовал _speedtest\.tele2\.net_ \(в итоге команда запуска приложения выглядит так: _ncftp\.exe ftp://speedtest\.tele2\.net_\); * загрузка файла с сервера\. Локально уже должен существовать файл с таким же именем, но с другими свойствами\. Можно, например, загрузить файл с сервера, изменить его, и попробовать снова выполнить команду загрузки \(например, _get 512KB\.zip_\); * на вопрос о выборе действия ответить строкой, начинающейся с символа 'N' \(например, _Now let's have some fun_\); * ввести '\\0' \(или что\-нибудь поинтереснее\); * \.\.\.\. * PROFIT\! Воспроизведение проблемы также записано на видео: ## OpenLDAP В проекте OpenLDAP \(точнее – в одной из сопутствующих утилит\) наступили на те же грабли, что и во FreeSWITCH\. Попытка удаления символа переноса строки происходит только при условии, что строка была считана успешно, но защиты от строк нулевой длины также нет\. Фрагмент кода: ```cpp int main( int argc, char **argv ) { char buf[ 4096 ]; FILE *fp = NULL; .... if (....) { fp = stdin; } .... if ( fp == NULL ) { .... } else { while ((rc == 0 || contoper) && fgets(buf, sizeof(buf), fp) != NULL) { buf[ strlen( buf ) - 1 ] = '\0'; /* remove trailing newline */ if ( *buf != '\0' ) { rc = dodelete( ld, buf ); if ( rc != 0 ) retval = rc; } } } .... } ``` **Предупреждение PVS\-Studio**: [V1010](https://pvs-studio.ru/ru/docs/warnings/v1010/) [CWE\-20](https://cwe.mitre.org/data/definitions/20.html) Unchecked tainted data is used in index: 'strlen\(buf\)'\. Выкинем лишнее, чтобы суть проблемы стала более очевидна: ```cpp while (.... && fgets(buf, sizeof(buf), fp) != NULL) { buf[ strlen( buf ) - 1 ] = '\0'; .... } ``` Этот код лучше, чем в NcFTP, но всё равно является уязвимым\. Если при запросе _fgets_ передать на вход строку нулевой длины: * _fgets\(buf, \.\.\.\.\)_ \-\> _buf_; * _fgets\(\.\.\.\.\) \!\= NULL_ \-\> _true_ \(начинает исполняться тело цикла _while_\); * _strlen\(buf\) \- 1_ \-\> _0 \- 1_ \-\> _\-1_; * _buf\[\-1\] \= '\\0'_\. ## libidn Несмотря на то, что ошибки, разобранные выше, достаточно интересны \(они стабильно воспроизводятся, и их можно 'пощупать' \(разве что до воспроизведения проблемы на OpenLDAP у меня руки не дотянулись\)\), уязвимостями их назвать нельзя хотя бы по той причине, что проблемам не присвоены CVE\-идентификаторы\. Тем не менее, такой же паттерн проблемы имеют и некоторые настоящие уязвимости\. Оба фрагмента кода, приведённые ниже, относятся к проекту libidn\. Фрагмент кода: ```cpp int main (int argc, char *argv[]) { .... else if (fgets (readbuf, BUFSIZ, stdin) == NULL) { if (feof (stdin)) break; error (EXIT_FAILURE, errno, _("input error")); } if (readbuf[strlen (readbuf) - 1] == '\n') readbuf[strlen (readbuf) - 1] = '\0'; .... } ``` **Предупреждение PVS\-Studio**: [V1010](https://pvs-studio.ru/ru/docs/warnings/v1010/) [CWE\-20](https://cwe.mitre.org/data/definitions/20.html) Unchecked tainted data is used in index: 'strlen\(readbuf\)'\. Ситуация похожа, за исключением того, что в отличии от предыдущих примеров, где осуществлялась запись по индексу _\-1_, здесь происходит чтение\. Тем не менее, это всё ещё undefined behavior\. Данная ошибка удостоилась собственного идентификатора CVE \([CVE\-2015\-8948](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2015-8948)\)\. После обнаружения проблемы данный код изменили следующим образом: ```cpp int main (int argc, char *argv[]) { .... else if (getline (&line, &linelen, stdin) == -1) { if (feof (stdin)) break; error (EXIT_FAILURE, errno, _("input error")); } if (line[strlen (line) - 1] == '\n') line[strlen (line) - 1] = '\0'; .... } ``` Немного удивлены? Бывает\. Новая уязвимость, соответствующий идентификатор CVE: [CVE\-2016\-6262](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2016-6262)\. **Предупреждение PVS\-Studio**: [V1010](https://pvs-studio.ru/ru/docs/warnings/v1010/) [CWE\-20](https://cwe.mitre.org/data/definitions/20.html) Unchecked tainted data is used in index: 'strlen\(line\)'\. С очередной попытки проблему поправили, добавив проверку длины входной строки: ```cpp if (strlen (line) > 0) if (line[strlen (line) - 1] == '\n') line[strlen (line) - 1] = '\0'; ``` Давайте взглянем на даты\. Коммит, 'закрывающий' CVE\-2015\-8948 – _10\.08\.2015_\. Коммит, закрывающий CVE\-2016\-62\-62 – _14\.01\.2016_\. То есть разница между приведёнными исправлениями составляет _5 месяцев_\! Вот тут\-то вспоминаешь про такое преимущество статического анализа, как обнаружение ошибок на ранних этапах написания кода\.\.\. ## Статический анализ и безопасность Дальше примеров кода не будет, вместо этого – статистика и рассуждения\. В этом разделе мнение автора может не совпадать с мнением читателя куда больше, чем раньше в этой статье\. : \) **Примечание**\. Рекомендую ознакомиться с другой статьёй на схожую тему – "[Как PVS\-Studio может помочь в поиске уязвимостей?](https://pvs-studio.ru/ru/blog/posts/cpp/0514/)"\. Там есть интересные примеры уязвимостей, которые выглядят как простые ошибки\. Дополнительно, в той статье я немного рассказал о терминологии и почему статический анализ – must have, если вас волнует тема безопасности\. Давайте посмотрим на статистику по количеству обнаруженных за последние 10 лет уязвимостей, чтобы оценить ситуацию\. Данные я взял с сайта [CVE Details](https://www.cvedetails.com/)\. ![0599_Vulnerabilities_ru/image2.png](https://import.viva64.com/docx/blog/0599_Vulnerabilities_ru/image2.png) Интересная вырисовывается ситуация\. До 2014 года количество зафиксированных CVE не превышало отметки в 6000 единиц, а начиная с – уже не опускалось ниже\. Но самым интересным здесь, конечно, выглядит статистика за 2017 год – абсолютный лидер \(14714 единиц\)\. Что касается текущего – 2018 – года, он хоть ещё не закончился, но уже бьёт рекорды – 15310 единиц\. Значит ли это, что весь новый софт дырявый как решето? Не думаю, и вот почему: * Повышенный интерес к теме уязвимостей\. Наверняка, даже если вы не очень близки непосредственно к теме безопасности, неоднократно приходилось натыкаться на статьи, заметки, доклады и видео, посвящённые теме безопасности\. Иначе говоря, создался некий "хайп"\. Плохо ли это? Пожалуй, нет\. В конечном итоге всё сводится к тому, что разработчики больше заботятся о безопасности приложений, что хорошо\. * Увеличение количества разрабатываемых приложений\. Больше кода – больше вероятность возникновения какой\-нибудь уязвимости, которая пополнит собой статистику\. * Улучшение инструментов поиска уязвимостей и обеспечения качества кода\. Больше спрос \-\> больше предложение\. Анализаторы, фаззеры и прочие инструменты становятся всё более совершенными, что играет на руку тем, кто хочется искать уязвимости \(вне зависимости от того, с какой стороны баррикад они находятся\)\. Так что складывающуюся тенденцию нельзя назвать исключительно отрицательной – издатели больше заботятся об информационной безопасности, инструменты поиска проблем совершенствуются, а всё это, несомненно, позитивно\. Значит ли это, что можно расслабиться и "не париться"? Думаю, нет\. Если вас беспокоит тема безопасности ваших приложений, следует принимать как можно больше мер по обеспечению безопасности\. Особенно это актуально, если исходный код находится в открытом доступе, так как он: * более подвержен встраиванию уязвимостей со стороны; * более подвержен 'зондированию' теми 'джентльменами', которых интересуют дыры в вашем приложении с целью их эксплуатации\. Хотя и доброжелатели в таком случае помочь вам смогут больше\. Не хочу сказать, что не нужно переводить свои проекты под open source\. Просто следует не забывать о надлежащих мерах контроля качества / безопасности\. Является ли статический анализ такой дополнительной мерой? Да\. Статический анализ хорошо справляется с обнаружением потенциальных уязвимостей, которые в дальнейшем могут стать вполне реальными\. Мне кажется \(допускаю, что ошибаюсь\), что многие считают уязвимости явлением достаточно высокоуровневым\. И да, и нет\. Проблемы в коде, которые кажутся простыми ошибками программирования, тоже вполне могут быть серьёзными уязвимостями\. Опять же, некоторые примеры таких уязвимостей приведены в [упоминавшейся ранее статье](https://pvs-studio.ru/ru/blog/posts/cpp/0514/)\. Не следует недооценивать 'простые' ошибки\. ## Заключение Не забывайте, что входные данные могут иметь нулевую длину, и это тоже нужно учитывать\. Выводы по поводу того, является ли вся шумиха с уязвимостями просто шумихой, или же проблема существует, делайте сами\. Со своей стороны, разве что предложу попробовать на своём проекте [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/download/), если вы ещё этого не сделали\. Всех благ\!