Предлагаем вашему вниманию цикл статей, посвященных рекомендациям по написанию качественного кода на примере ошибок, найденных в проекте Chromium. Это пятая часть, которая будет посвящена ошибкам использования непроверенных или неправильно проверенных данных. Очень большое количество уязвимостей существуют благодаря как раз использованию непроверенных данных, что делает данную тему интересной и актуальной.

На самом деле, причиной уязвимости может стать ошибка почти любого типа, даже обыкновенная опечатка. Собственно, если найденная ошибка классифицируется согласно Common Weakness Enumeration, то значит она является потенциальной уязвимостью.

Анализатор PVS-Studio, начиная с версии 6.21, научился классифицировать найденные ошибки согласно Common Weakness Enumeration и назначать им соответствующий CWE ID.

Возможно читатели уже обратили внимание, что в предыдущих статьях помимо номера предупреждения Vxxx я приводил ещё и CWE ID. Это значит, что рассмотренные ранее ошибки теоретически могут стать причиной уязвимости. Вероятность этого невелика, но она есть. Что интересно, нам удалось сопоставить тот или иной CWE ID почти с каждым предупреждением, выдаваемым PVS-Studio. Это значит, что сами того не планируя, мы создали анализатор, который способен выявлять большое количество weaknesses :).

Вывод. Анализатор PVS-Studio помогает заранее предотвращать многие виды уязвимостей. Публикация на эту тему: "Как PVS-Studio может помочь в поиске уязвимостей?".

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

Итак, давайте рассмотрим, какие дефекты безопасности я заметил в процессе разбора отчета, выданного PVS-Studio для проекта Chromium. Как я писал в вводной статье, отчёт я смотрел достаточно бегло, поэтому могут быть и другие, незамеченные мной ошибки. Задача статьи в общих чертах показать, как какие-то ошибки могут приводить к тому, что программа начинает обрабатывать некорректные или непроверенные данные. Я пока не определился, как лучше называть такие данные, и пока буду использовать термин «недостоверные данные».

Примеры ошибок

Проект Chromium.

InstallUtil::ConditionalDeleteResult
InstallUtil::DeleteRegistryValueIf(....) {
  ....
  ConditionalDeleteResult delete_result = NOT_FOUND;
  ....
  if (....) {
    LONG result = key.DeleteValue(value_name);
    if (result != ERROR_SUCCESS) {
      ....
      delete_result = DELETE_FAILED;
    }
    delete_result = DELETED;
  }
  return delete_result;
}

Предупреждение PVS-Studio: V519 CWE-563 The 'delete_result' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 381, 383. install_util.cc 383

Функция возвращает некорректный статус. В результате, другие части программы будут считать, что функция успешно удалила некое значение. Ошибка в том, что статус DELETE_FAILED всегда заменяется на статус DELETED.

Ошибку можно исправить, добавив ключевое слово else:

if (result != ERROR_SUCCESS) {
  ....
  delete_result = DELETE_FAILED;
} else {
  delete_result = DELETED;
}

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

Библиотека PDFium (используется в Chromium).

CPVT_WordRange Intersect(const CPVT_WordRange& that) const {
  if (that.EndPos < BeginPos || that.BeginPos > EndPos ||
      EndPos < that.BeginPos || BeginPos > that.EndPos) {
    return CPVT_WordRange();
  }
  return CPVT_WordRange(std::max(BeginPos, that.BeginPos),
                        std::min(EndPos, that.EndPos));
}

Предупреждения PVS-Studio:

• V501 CWE-570 There are identical sub-expressions 'that.BeginPos > EndPos' to the left and to the right of the '||' operator. cpvt_wordrange.h 46
• V501 CWE-570 There are identical sub-expressions 'that.EndPos < BeginPos' to the left and to the right of the '||' operator. cpvt_wordrange.h 46

Условие написано неправильно. Чтобы легче было заметить ошибку, сократим условие:

if (E2 < B1 || B2 > E1 || E1 < B2 || B1 > E2)

Обратите внимание, что (E2 < B1) и (B1 > E2), это одно и то же. Аналогично (B2 > E1), это то же самое, что и (E1 < B2).

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

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

Вначале следует показать, как объявлены некоторые типы. Если этого не сделать, то код будет не очень понятен.

typedef signed int Rune;
enum
{
  UTFmax = 4,
  Runesync = 0x80,
  Runeself = 0x80,
  Runeerror = 0xFFFD,
  Runemax = 0x10FFFF,
};

А теперь функция с аномалией.

char*
utfrune(const char *s, Rune c)
{
  long c1;
  Rune r;
  int n;

  if(c < Runesync)    /* not part of utf sequence */
    return strchr((char*)s, c);

  for(;;) {
    c1 = *(unsigned char*)s;
    if(c1 < Runeself) {  /* one byte rune */
      if(c1 == 0)
        return 0;
      if(c1 == c)                // <=
        return (char*)s;
      s++;
      continue;
    }
    n = chartorune(&r, s);
    if(r == c)
      return (char*)s;
    s += n;
  }
  return 0;
}

Анализатор PVS-Studio выдаёт предупреждение на строчку, которую я отметил комментарием "// <=". Сообщение: V547 CWE-570 Expression 'c1 == c' is always false. rune.cc 247

Давайте попробуем разобраться, почему условие всегда ложно. Вначале обратите внимание на эти строки:

if(c < Runesync)
  return strchr((char*)s, c);

Если переменная c < 0x80, то функция прекратит свою работу. Если функция не завершит свою работу, а продолжит её, то можно точно сказать, что переменная c >= 0x80.

Теперь смотрим на условие:

if(c1 < Runeself)

Условие (c1 == c), отмеченное комментарием "// <=", выполняется только в том случае, если c1 < 0x80.

Итак, вот что мы знаем про значения переменных:

• c >= 0x80
• c1 < 0x80

Отсюда следует, что условие c1 == c всегда ложно. А ведь это очень подозрительно. Получается, что функция utfrune в библиотеке регулярных выражений работает не так, как запланировано. Последствия такой ошибки непредсказуемы.

Видеокодек LibVPX (используется в Chromium).

#define VP9_LEVELS 14

extern const Vp9LevelSpec vp9_level_defs[VP9_LEVELS];

typedef enum {
  ....
  LEVEL_MAX = 255
} VP9_LEVEL;

static INLINE int log_tile_cols_from_picsize_level(
  uint32_t width, uint32_t height)
{
  int i;
  const uint32_t pic_size = width * height;
  const uint32_t pic_breadth = VPXMAX(width, height);
  for (i = LEVEL_1; i < LEVEL_MAX; ++i) {
   if (vp9_level_defs[i].max_luma_picture_size >= pic_size &&
       vp9_level_defs[i].max_luma_picture_breadth >= pic_breadth)
   {
     return get_msb(vp9_level_defs[i].max_col_tiles);
   }
  }
  return INT_MAX;
}

Предупреждения PVS-Studio:

• V557 CWE-119 Array overrun is possible. The value of 'i' index could reach 254. vp9_encoder.h 931
• V557 CWE-119 Array overrun is possible. The value of 'i' index could reach 254. vp9_encoder.h 932
• V557 CWE-119 Array overrun is possible. The value of 'i' index could reach 254. vp9_encoder.h 933

Массив vp9_level_defs состоит из 14 элементов. В цикле переменная i, используемая в качестве индекса массива, изменяется от 0 до 254. Итог: происходит выход за границу массива.

Хорошо, если этот код приведёт к Access Violation. Но на практике, скорее всего, начнут обрабатываться какие-то случайные данные, расположенные после массива vp9_level_defs.

Другая схожая ошибка использования данных за пределами массива встретилась мне в библиотеке SQLite (используется в Chromium).

Вначале обратите внимание, что массив yy_shift_ofst содержит в себе 455 элементов.

static const short yy_shift_ofst[] = {
  /*   0 */ 355, 888, 1021, 909, 1063, 1063, 1063, 1063, 20, -19,
  ....
  /* 450 */ 1440, 1443, 1538, 1542, 1562,
}

Также интерес для нас представляют два макроса:

#define YY_SHIFT_COUNT    (454)
#define YY_MIN_REDUCE     993

Макрос YY_SHIFT_COUNT определяет максимальный индекс, который можно использовать для доступа к элементам массива yy_shift_ofst. Он равен не 455, а 454, так как нумерация элементов идёт с 0.

Макрос YY_MIN_REDUCE, равный 993, никакого отношения к размеру массива yy_shift_ofst не имеет.

Функция, содержащая слабую проверку:

static unsigned int yy_find_shift_action(....)
{
  int i;
  int stateno = pParser->yytos->stateno;

  if( stateno>=YY_MIN_REDUCE ) return stateno;      // <=

  assert( stateno <= YY_SHIFT_COUNT );

  do {
    i = yy_shift_ofst[stateno];                     // <=
  ....
}

Предупреждение PVS-Studio: V557 CWE-125 Array overrun is possible. The value of 'stateno' index could reach 992. sqlite3.c 138802

В функции сделана защита, что индекс при доступе к массиву не должен быть больше определённого значения. Из-за опечатки, или по другой причине, используется не та константа. Следовало использовать константу равную 454, а вместо этого значение индекса сравнивается с 993.

В результате, возможен доступ за границу массива и чтение произвольных недостоверных данных.

Примечание. Ниже есть правильный assert, но он не поможет в Release-версии.

Скорее всего, проверку надо переписать так:

if (stateno > YY_SHIFT_COUNT)
{
  assert(false);
  return stateno;
}

Проект ICU (используется в Chromium).

UVector*
ZoneMeta::createMetazoneMappings(const UnicodeString &tzid) {
  UVector *mzMappings = NULL;
  ....
  if (U_SUCCESS(status)) {
    ....
    if (U_SUCCESS(status)) {
      ....
      while (ures_hasNext(rb)) {
        ....
        if (mzMappings == NULL) {
          mzMappings = new UVector(
            deleteOlsonToMetaMappingEntry, NULL, status);
          if (U_FAILURE(status)) {
            delete mzMappings;
            uprv_free(entry);
            break;
          }
        }
        ....
      }
      ....
    }
  }
  ures_close(rb);
  return mzMappings;
}

Предупреждение PVS-Studio: V774 CWE-416 The 'mzMappings' pointer was used after the memory was released. zonemeta.cpp 713

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

if (U_FAILURE(status)) {
  delete mzMappings;
  mzMappings = nullptr;
  uprv_free(entry);
  break;
}

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

В следующей функции проекта Chromium неправильно реализована защита от отрицательных значений.

void AXPlatformNodeWin::HandleSpecialTextOffset(LONG* offset) {
  if (*offset == IA2_TEXT_OFFSET_LENGTH) {
    *offset = static_cast<LONG>(GetText().length());
  } else if (*offset == IA2_TEXT_OFFSET_CARET) {
    int selection_start, selection_end;
    GetSelectionOffsets(&selection_start, &selection_end);
    if (selection_end < 0)
      *offset = 0;
    *offset = static_cast<LONG>(selection_end);
  }
}

Предупреждение PVS-Studio: V519 CWE-563 The '* offset' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 3543, 3544. ax_platform_node_win.cc 3544

Если значение переменной selection_end отрицательное, то функция должна вернуть 0. Однако из-за опечатки 0 записывается не туда, куда нужно. Правильный код должен быть таким:

if (selection_end < 0)
  selection_end = 0;
*offset = static_cast<LONG>(selection_end);

Из-за этой ошибки функция может вернуть отрицательное число, хотя не должна. Это отрицательное число, которое может «просочиться» сквозь проверку, и есть недостоверные данные.

Другие ошибки

Если честно, мне не очень нравятся примеры, которые я привёл в предыдущем разделе статьи. Их мало, и они не очень хорошо отражают суть ошибок использования недостоверных данных. Думаю, со временем я сделаю отдельную статью, где покажу более яркие примеры ошибок, собрав их из разных открытых проектов.

Кстати, в статью можно было включить больше примеров ошибок, но я их уже «потратил» при написании предыдущих статей, а повторяться не хочется. Например, в статье «Chromium: опечатки», был вот такой фрагмент:

  if(!posX->hasDirtyContents() ||
     !posY->hasDirtyContents() ||
     !posZ->hasDirtyContents() ||
     !negX->hasDirtyContents() ||
     !negY->hasDirtyContents() ||          // <=
     !negY->hasDirtyContents())            // <=

Из-за этой опечатки не проверяется объект, на который ссылается указатель negZ. В результате программа будет работать с недостоверными данными.

Также в этой статье я не стал рассматривать ситуации, когда недостоверные (испорченные) данные возникают из-за отсутствия проверки указателя, который возвращает функция malloc. Если функция malloc вернула NULL, это вовсе не значит, что возможна только ошибка разыменования нулевого указателя. Существуют и более коварные ситуации. Схематично они выглядят так:

int *ptr = (int *)malloc(100 * sizeof(int));
ptr[1234567] = 42;

Здесь не будет разыменования нулевого указателя. Здесь произойдёт запись данных непонятно куда и разрушение каких-то данных.

Это интересная история и я посвящу ей следующую отдельную статью.

Рекомендации

К возникновению и использованию недостоверных (непроверенных, испорченных) данных приводят разнообразнейшие ошибки. Какого-то универсального совета здесь быть не может. Можно, конечно, написать: не делайте в коде ошибок! Но проку от такого совета нет :).

Так зачем тогда я вообще писал эту статью и выделил этот тип ошибок?

Чтобы вы знали о них. Знание о существовании какой-то проблемы уже само по себе помогает предотвратить её. Если кто-то не знает о какой-то проблеме, не значит, что её нет. Хорошей иллюстрацией будет вот эта картинка:

Что же можно всё-таки посоветовать:

1. Обновляйте используемые в вашем проекте библиотеки. В новых версиях могут быть исправлены различные ошибки, которые представляют собой уязвимости. Впрочем, надо признать, что уязвимость может появиться как раз в новой версии, а в старой отсутствовать. Но все равно, более хорошим решением будет обновлять библиотеки. Про старые уязвимости знает больше людей, чем про новые.
2. Тщательно проверяйте все входные данные, особенно поступающие откуда-то извне. Например, очень тщательно должны проверяться все данные, поступающие откуда-то по сети.
3. Используйте различные инструменты проверки кода. Например, проекту Chromium явно не хватает использования статического анализатора PVS-Studio :).
4. Объясняйте коллегам, что "простая ошибка при кодировании — не значит нестрашная ошибка". Если ваша команда разрабатывает ответственные приложения, то вы должны максимально сосредоточиться на качестве кода и уничтожать все, даже безобидные на вид ошибки.

Примечание про PVS-Studio

Как я уже сказал, анализатор PVS-Studio уже помогает предотвратить появление уязвимости, обнаруживая ошибки ещё на этапе написания кода. Но мы хотим большего и вскоре серьезно усовершенствуем PVS-Studio, введя в Data Flow анализ понятие «использование непроверенных данных».

Мы даже уже зарезервировали специальный номер для этой важной диагностики: V1010. Диагностика позволит выявлять ошибки, когда данные были получены из ненадёжного источника (например, присланы по сети) и используются без должной проверки. Отсутствие всех необходимых проверок входных данных часто является причиной обнаружения уязвимости в приложениях. Подробнее про это мы недавно писали в статье "PVS-Studio 2018: CWE, Java, RPG, macOS, Keil, IAR, MISRA" (см. раздел «Потенциальные уязвимости, CWE»).

Новая диагностика позволит существенно усилить анализатор в выявлении потенциальных уязвимостей. Скорее всего, диагностика V1010 будет соответствовать идентификатору CWE-20 (Improper Input Validation).

Заключение

Предлагаю вам и вашим коллегам почитать на нашем сайте статью "42 рекомендации". После неё программист не превратится в эксперта по безопасности, но узнает много нового и полезного. Особенно эти статьи будут полезны разработчикам, только недавно освоившим язык C или C++ и не подозревающим насколько глубока кроличья нора, в которую они попали.

Я планирую обновить «42 совета» и превратить их в «50 советов». Поэтому приглашаю подписываться на мой твиттер @Code_Analysis и наш RSS канал, чтобы не пропустить эту и другие интересные статьи в нашем блоге.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Chromium: Use of Untrusted Data.