Связующее звено сегодняшней статьи отличается от обычного. Это не один проект, для которого был проведён анализ исходного кода, а ряд срабатываний одного и того же диагностического правила в нескольких разных проектах. В чём здесь интерес? В том, что некоторые из рассмотренных фрагментов кода содержат ошибки, воспроизводимые при работе с приложением, а другие – и вовсе уязвимости (CVE). Кроме того, в конце статьи немного порассуждаем на тему дефектов безопасности.
Все ошибки, которые будут рассмотрены сегодня в статье, имеют схожий паттерн:
Тем не менее, все фрагменты, которые будут рассмотрены, содержат в себе ошибки и уязвимы к подстроенному вводу. Так как данные принимаются от пользователя, который и может нарушить логику исполнения приложения, был велик соблазн попробовать что-нибудь сломать. Что я и сделал.
Все проблемы, приведённые ниже, были обнаружены статическим анализатором PVS-Studio, который ищет ошибки в коде не только для C, C++, но и для C#, Java.
Конечно, найти проблему статическим анализатором – хорошо, но найти и воспроизвести – это уже совершенно другой уровень удовольствия. :)
Первый подозрительный фрагмент кода был обнаружен в коде модуля fs_cli.exe, входящего в состав дистрибутива FreeSWITCH:
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 CWE-20 Unchecked tainted data is used in index: 'strlen(command_buf)'.
Анализатор предупреждает о подозрительном обращении по индексу к массиву command_buf. Подозрительным оно считается по той причине, что в качестве индекса используются непроверенные внешние данные. Внешние – потому что получены через функцию fgets из потока stdin. Непроверенными - так как никакой проверки перед использованием выполнено не было. Выражение fgets(command_buf, ....) != command_buf - не в счёт, так как таким образом мы проверяем только факт получения данных, но не их содержимое.
Проблема данного кода в том, что при определённых условиях произойдёт запись '\0' за пределы массива, что приведёт к возникновению неопределённого поведения. Для этого достаточно ввести строку нулевой длины (строку нулевой длины с точки зрения языка Си, то есть такую, в которой первым символом будет '\0').
Давайте прикинем, что произойдёт, если передать на вход строку нулевой длины:
Упс!
Интересно здесь то, что это предупреждение анализатора вполне себе можно "пощупать руками". Для того, чтобы повторить проблему, нужно:
Немного покопавшись в исходниках, я составил конкретную последовательность воспроизведения проблемы:
Ниже приводится видеозапись воспроизведения проблемы:
В проекте NcFTP была обнаружена аналогичная проблема, только встретилась она уже в двух местах. Так как код выглядит похоже, рассмотрим только одно проблемное место:
static int NcFTPConfirmResumeDownloadProc(....)
{
....
if (fgets(newname, sizeof(newname) - 1, stdin) == NULL)
newname[0] = '\0';
newname[strlen(newname) - 1] = '\0';
....
}Предупреждение PVS-Studio: V1010 CWE-20 Unchecked tainted data is used in index: 'strlen(newname)'.
Здесь, в отличии от примера из FreeSWITCH, код написан хуже и больше подвержен проблемам. Например, запись '\0' происходит вне зависимости от того, успешно произошло считывание с использованием fgets или нет. То есть здесь даже больше возможностей того, как можно нарушить нормальную логику исполнения. Пойдём проверенным путём – через строки нулевой длины.
Воспроизводится проблема немного сложнее, чем в случае с FreeSWITCH. Последовательность шагов описана ниже:
Воспроизведение проблемы также записано на видео:
В проекте OpenLDAP (точнее – в одной из сопутствующих утилит) наступили на те же грабли, что и во FreeSWITCH. Попытка удаления символа переноса строки происходит только при условии, что строка была считана успешно, но защиты от строк нулевой длины также нет.
Фрагмент кода:
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 CWE-20 Unchecked tainted data is used in index: 'strlen(buf)'.
Выкинем лишнее, чтобы суть проблемы стала более очевидна:
while (.... && fgets(buf, sizeof(buf), fp) != NULL) {
buf[ strlen( buf ) - 1 ] = '\0';
....
}Этот код лучше, чем в NcFTP, но всё равно является уязвимым. Если при запросе fgets передать на вход строку нулевой длины:
Несмотря на то, что ошибки, разобранные выше, достаточно интересны (они стабильно воспроизводятся, и их можно 'пощупать' (разве что до воспроизведения проблемы на OpenLDAP у меня руки не дотянулись)), уязвимостями их назвать нельзя хотя бы по той причине, что проблемам не присвоены CVE-идентификаторы.
Тем не менее, такой же паттерн проблемы имеют и некоторые настоящие уязвимости. Оба фрагмента кода, приведённые ниже, относятся к проекту libidn.
Фрагмент кода:
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 CWE-20 Unchecked tainted data is used in index: 'strlen(readbuf)'.
Ситуация похожа, за исключением того, что в отличии от предыдущих примеров, где осуществлялась запись по индексу -1, здесь происходит чтение. Тем не менее, это всё ещё undefined behavior. Данная ошибка удостоилась собственного идентификатора CVE (CVE-2015-8948).
После обнаружения проблемы данный код изменили следующим образом:
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.
Предупреждение PVS-Studio: V1010 CWE-20 Unchecked tainted data is used in index: 'strlen(line)'.
С очередной попытки проблему поправили, добавив проверку длины входной строки:
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 может помочь в поиске уязвимостей?". Там есть интересные примеры уязвимостей, которые выглядят как простые ошибки. Дополнительно, в той статье я немного рассказал о терминологии и почему статический анализ – must have, если вас волнует тема безопасности.
Давайте посмотрим на статистику по количеству обнаруженных за последние 10 лет уязвимостей, чтобы оценить ситуацию. Данные я взял с сайта CVE Details.
Интересная вырисовывается ситуация. До 2014 года количество зафиксированных CVE не превышало отметки в 6000 единиц, а начиная с – уже не опускалось ниже. Но самым интересным здесь, конечно, выглядит статистика за 2017 год – абсолютный лидер (14714 единиц). Что касается текущего – 2018 – года, он хоть ещё не закончился, но уже бьёт рекорды – 15310 единиц.
Значит ли это, что весь новый софт дырявый как решето? Не думаю, и вот почему:
Так что складывающуюся тенденцию нельзя назвать исключительно отрицательной – издатели больше заботятся об информационной безопасности, инструменты поиска проблем совершенствуются, а всё это, несомненно, позитивно.
Значит ли это, что можно расслабиться и "не париться"? Думаю, нет. Если вас беспокоит тема безопасности ваших приложений, следует принимать как можно больше мер по обеспечению безопасности. Особенно это актуально, если исходный код находится в открытом доступе, так как он:
Не хочу сказать, что не нужно переводить свои проекты под open source. Просто следует не забывать о надлежащих мерах контроля качества / безопасности.
Является ли статический анализ такой дополнительной мерой? Да. Статический анализ хорошо справляется с обнаружением потенциальных уязвимостей, которые в дальнейшем могут стать вполне реальными.
Мне кажется (допускаю, что ошибаюсь), что многие считают уязвимости явлением достаточно высокоуровневым. И да, и нет. Проблемы в коде, которые кажутся простыми ошибками программирования, тоже вполне могут быть серьёзными уязвимостями. Опять же, некоторые примеры таких уязвимостей приведены в упоминавшейся ранее статье. Не следует недооценивать 'простые' ошибки.
Не забывайте, что входные данные могут иметь нулевую длину, и это тоже нужно учитывать.
Выводы по поводу того, является ли вся шумиха с уязвимостями просто шумихой, или же проблема существует, делайте сами.
Со своей стороны, разве что предложу попробовать на своём проекте PVS-Studio, если вы ещё этого не сделали.
Всех благ!