Сканер сетевого трафика Snort под сканером анализатора кода PVS-Studio

• Введение
• PVS-Studio
• Формирование отчёта с результатами анализа
• Результаты проверки
• Заключение

Snort — это самая используемая система обнаружения вторжений (IDS) в мире. Каждый, кто имеет дело с защитой информации, должен быть с ней знаком. Имеет ли такой крутой инструмент ошибки или потенциальные уязвимости? Давайте попробуем в этом разобраться с помощью статического анализатора PVS-Studio.

Введение

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

Snort — наиболее популярная свободная сетевая система предотвращения вторжений (IPS) и обнаружения вторжений (IDS). У неё открытый исходный код. Snort способна выполнять регистрацию пакетов и в реальном времени осуществлять анализ трафика в IP-сетях, блокировать и предотвращать атаки. Она была создана Мартином Рошем (Martin Roesch) в 1999 году и стала настолько популярной, что сетевой гигант Cisco приобрел её в 2014 году.

На данный момент есть две последние версии Snort. Это Snort 2.9.17 на языке Си и Snort 3.1.1 на С++. В текущей статье мы проанализируем уже давно используемый Snort на Cи, а в следующей – свежий Snort на С++. Подведем итоги и узнаем, какой код является более качественным.

PVS-Studio

PVS-Studio – это инструмент для выявления ошибок и потенциальных уязвимостей в исходном коде программ, написанных на языках С, C++, C# и Java. Работает в 64-битных системах на Windows, Linux и macOS и может анализировать код, предназначенный для 32-битных, 64-битных и встраиваемых ARM платформ. PVS-Studio наиболее эффективно использовать сразу после компиляции. Это позволяет находить ошибки в коде до его тестирования и тратить меньше времени на отладку программы.

Snort версии 2.9.17 написан на языке Cи и предназначен для Linux, поэтому будем использовать PVS-Studio для Linux. Получить инструкцию по установке и запуску анализатора можем здесь и здесь.

Формирование отчёта с результатами анализа

Проект Snort собирается с помощью команды make. После просмотра краткой инструкции достаточно просто найти нужную последовательность команд для проверки такого проекта. В инструкции говорится, что для проверки нам понадобится утилита strace. Итак, что нам нужно сделать?

1) Запустим сборку snort с помощью команды

pvs-studio-analyzer trace – make

2) После успешной сборки начнем анализ с помощью команды:

pvs-studio-analyzer analyze -l path_to_PVS_Studio.lic \
-a GA;OP -o logfile.log -j <N>

где

• path_to_PVS_Studio.lic – пусть к лицензии PVS-Studio (запросить триальный ключ для пробного использования можно тут);
• logfile.log – файл с полным закодированным результатом анализа;
• <N> – число процессоров, которые мы отдадим для анализа;
• -a GA;OP – группы диагностик, используемые для анализа (по умолчанию применяется только GA).

Ниже приведен список всех доступных сейчас и в ближайшем будущем групп диагностик:

• GA – General analysis;
• 64 – 64-bit Analysis;
• OP - Micro-optimizations;
• CS - Customers Specific Requests;
• MISRA – MISRA guidelines;
• AUTOSAR – AUTOSAR guidelines (ожидается);
• OWASP – OWASP guidelines (ожидается).

3) Теперь осталось только конвертировать результат анализа в удобный формат для ознакомления. Здесь нам понадобится утилита Plog Converter. Предлагаю воспользоваться FullHtml отчётом в качестве формата вывода. Он довольно удобен тем, что его можно просматривать на любом устройстве. Возможна сортировка предупреждений по уровням, номерам диагностик, по группам и файлам. Также можно в один клик перейти в любой файл с предупреждением, попав на нужную строку. Если же кликнуть на номер диагностики, то произойдёт переход на страницу с её подробным описанием.

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

Чтобы сгенерировать FullHtml отчёт для всех предупреждений General analysis уровня High и Medium, выполним следующую команду:

plog-converter -a GA:1,2 -t fullhtml logfile.log \
-o path_to_report_dir

где

• GA:1,2 – набор диагностик общего назначения уровня High и Medium,
• path_to_project – путь к папке, в которую будет сгенерирован отчёт.

В этой статье я рассмотрел только предупреждения General analysis, так как их оказалось уже довольно много. Для того чтобы сгенерировать отчёт с предупреждениями Micro-optimizations, можно выполнить команду:

plog-converter -a OP:1,2,3 -t fullhtml path_to_project.log \
-o path_to_report_dir

Вернёмся к первому отчёту. Откроем его в любом браузере и приступим к проверке.

Результаты проверки

Предупреждение N1 – И да, и нет равно нет

V560 A part of conditional expression is always false: !p->tcph. sp_rpc_check.c 285

V560 A part of conditional expression is always false: !p->udph. sp_rpc_check.c 286

#define IsTCP(p) (IsIP(p) && p->tcph)
#define IsUDP(p) (IsIP(p) && p->udph)
int CheckRpc(void *option_data, Packet *p)
{
  ....
  if (!p->iph_api || (IsTCP(p) && !p->tcph)
                  || (IsUDP(p) && !p->udph))
  {
    return 0; /* if error occured while ip header
               * was processed, return 0 automagically.  */
  }
  ....
}

С виду вполне логичное условие после раскрытия макроса теряет смысл. PVS-Studio сообщает нам, что выражение !p->tcph всегда ложно, но почему? Дело в том, что если условие внутри макроса выполнилось, то p->tcph уже точно не равен нулю. Раскрыв макрос, мы получим:

((IsIP(p) && p->tcph) && !p->tcph)

Данное выражение всегда будет принимать значение false, так как x && !x = 0. Точно такая же ситуация и со строкой кода ниже:

((IsIP(p) && p->tcph) && !p->ucph)

Наверное, это совсем не то, что хотел автор. Иначе бы он оставил только условие if (!p->iph_api). Функция не проверяет, является ли переменная p TCP или UDP, и поэтому может работать не совсем корректно.

Предупреждение N2 – Опасный макрос

V634 The priority of the '*' operation is higher than that of the '<<' operation. It's possible that parentheses should be used in the expression. bug34427.c 160

#define PM_EXP2(A) 1 << A

int process_val(const u_int8_t *data, u_int32_t data_len,
                               u_int32_t *retvalue, ....) 
{
  *retvalue = 0;
  ....
  /* Now find the actual value */
  for (; i < data_len; i++) {
    *retvalue += data[i] * PM_EXP2(8 * (data_len - i - 1));
  }
  return(0);
}

Анализатор предупреждает, что после подстановки макроса результирующее выражение может вычисляться некорректно. Сначала будет выполнено умножение на 1, а уже затем побитовый сдвиг на выражение в скобках. Повезло, что в данной строке выражение x * 1 << y эквивалентно x * (1 << y). Если слева или справа от макроса будет стоять /, %, +, - или прочие операции с приоритетом большим, чем у <<, либо внутри макроса будет стоять операция с приоритетом меньшим, чем у <<, выражение будет вычисляться неправильно. Всегда следует оборачивать макрос и его аргументы скобками, чтобы избежать проблем в будущем. Так, правильным будет:

Define PM_EXP2(A) (1 << (A))

Этот же опасный макрос удачно используется и в файле misc_ber.c (строка 97).

Предупреждение N3 – Нечистоплотный компилятор

V597 The compiler could delete the 'memset' function call, which is used to flush 'ThisFmt' object. The memset_s() function should be used to erase the private data. ftpp_ui_config.c 251

void ftpp_ui_config_reset_ftp_cmd_format(FTP_PARAM_FMT *ThisFmt)
{
  ....
  memset(ThisFmt, 0, sizeof(FTP_PARAM_FMT));
  free(ThisFmt);
}

Одной из важных задач компилятора является оптимизация. Зачем записывать что-то туда, где оно не будет использоваться? Функция memset будет удалена и, возможно, приватные данные останутся не затертыми. Анализатор предлагает использовать memset_s, чтобы все работало так, как вы и задумали. Эту функцию компилятор не тронет. Как безопасно очищать приватные данные, можно посмотреть здесь.

Ещё одна небезопасная очистка есть тут: spo_log_tcpdump.c 485

Предупреждение N4 – Неопределенность

V595 The 'ssd' pointer was utilized before it was verified against nullptr. Check lines: 900, 910. dce2_smb2.c 900

void DCE2_Smb2Process(DCE2_SmbSsnData *ssd)
{
  const SFSnortPacket *p = ssd->sd.wire_pkt;
  ....
  if (ssd && ssd->pdu_state != DCE2_SMB_PDU_STATE__RAW_DATA)
  {
    ....
  }
  ....
}

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

Другое предупреждение этой диагностики:

V595 The 'it' pointer was utilized before it was verified against nullptr. Check lines: 158, 160. u2spewfoo.c 158

static inline void free_iterator(u2iterator *it) 
{
  if(it->file) fclose(it->file);
  if(it->filename) free(it->filename);
  if(it) free(it);
}

Анализатор снова заметил странное поведение. Здесь также уверенность в том, что указатель на что-то указывает, потерялась по мере выполнения кода. Проверку it на nullptr следовало бы сделать в самом начале.

Проблема разыменовывания нулевого указателя популярна среди разработчиков на С\С++. Так и в этом проекте нашлось ещё 15 подобных предупреждений. Там тоже не все однозначно. Вот половина из них:

• The 'bm_variable_name' pointer was utilized before it was verified against nullptr. Check lines: 113, 128. sf_snort_plugin_byte.c 113
• V595 The 'cursor' pointer was utilized before it was verified against nullptr. Check lines: 293, 302. sf_snort_plugin_pcre.c 293
• V595 The 'configNext' pointer was utilized before it was verified against nullptr. Check lines: 782, 788. spp_imap.c 782
• V595 The 'sub->entries' pointer was utilized before it was verified against nullptr. Check lines: 193, 197. sfrt_dir.c 193
• V595 The 'sub->lengths' pointer was utilized before it was verified against nullptr. Check lines: 191, 207. sfrt_dir.c 191
• The 'configNext' pointer was utilized before it was verified against nullptr. Check lines: 778, 784. spp_pop.c 778
• V595 The 'configNext' pointer was utilized before it was verified against nullptr. Check lines: 809, 816. spp_smtp.c 809
• V595 The 'pmd' pointer was utilized before it was verified against nullptr. Check lines: 1754, 1761. fpcreate.c 1754

Предупреждение N5 – Очистить пустоту

V575 The null pointer is passed into 'free' function. Inspect the first argument. sdf_us_ssn.c 202

int ParseSSNGroups(....)
{
  FILE *ssn_file;
  char *contents;
  ....
  contents = (char *)malloc(length + 1);
  if (contents == NULL)
  {
    _dpd.logMsg("Sensitive Data preprocessor: Failed to allocate memory "
      "for SSN groups.\n");

    fclose(ssn_file);
    free(contents); // <=
    return -1;
  }
  ....
  free(contents);
  return 0;
}

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

Предупреждение N6 – Не поделили место

V519 The 'port_array[5061 / 8]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 327, 328. sip_config.c 328

#define PORT_INDEX(port) port / 8
#define SIP_PORT 5060
#define SIPS_PORT 5061

static void SIP_ParsePortList(char **ptr, uint8_t *port_array)
{
  ....
  /* If the user specified ports, remove SIP_PORT for now since
   * it now needs to be set explicitly. */
  port_array[PORT_INDEX(SIP_PORT)] = 0;
  port_array[PORT_INDEX(SIPS_PORT)] = 0;
  ....
}

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

Предупреждение N7 – Не на своем месте

V713 The pointer 'fileEntry->context' was utilized in the logical expression before it was verified against nullptr in the same logical expression. file_segment_process.c 393

static inline int _process_one_file_segment(void* p, 
                          FileEntry *fileEntry, ....)
{
  ....
    if ((fileEntry->context->file_state.sig_state == FILE_SIG_FLUSH)
      && fileEntry->context 
      && fileEntry->context->sha256)
    {
      free(fileEntry->context->sha256);
      fileEntry->context->sha256 = NULL;
    }
  ....
}

В одном и том же условном выражении сначала разыменовывается указатель, а затем проверяется на nullptr. Это серьезная опечатка, которая приведёт работу программы к краху. Наверное, уставший разработчик по невнимательности вставил дополнительное условие в самое начало выражения, а не в середину или конец. Правильно было бы его записать так:

if ( fileEntry->context 
  && fileEntry->context->file_state.sig_state == FILE_SIG_FLUSH
  && fileEntry->context->sha256)

Другой возможный правильный вариант:

if ((fileEntry->context->file_state.sig_state == FILE_SIG_FLUSH)
  && fileEntry->context->something 
  && fileEntry->context->sha256

Программа не устаёт. Статический анализатор всегда просматривает каждый участок кода с одинаковой внимательностью и предупреждает об ошибках или странностях в коде. Попробуйте PVS-Studio сами и убедитесь в этом.

Предупреждение N8 – Вечный двигатель

V654 The condition '!done' of loop is always true. log.c 207

void PrintNetData(....)
{
  int done;           /* flag */
  ....

  /* initialization */
  done = 0;
  ....

  /* loop thru the whole buffer */
  while(!done)
    {
      ....
    }
  ....
}

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

Предупреждение N9 – Дважды проверь!

V501 There are identical sub-expressions '!info->sip.__in6_u.__u6_addr32[0]' to the left and to the right of the '&&' operator. pkt_tracer.c 160

V501 There are identical sub-expressions '!info->dip.__in6_u.__u6_addr32[0]' to the left and to the right of the '&&' operator. pkt_tracer.c 167

static inline void debugParse(...., DebugSessionConstraints *info)
{
  ....
  if (!info->sip.s6_addr32[0] && !info->sip.s6_addr32[0] &&
      !info->sip.s6_addr16[4] && info->sip.s6_addr16[5] == 0xFFFF)
  {
    saf = AF_INET;
  }
  else
    saf = AF_INET6;  
  if (!info->dip.s6_addr32[0] && !info->dip.s6_addr32[0] &&
      !info->dip.s6_addr16[4] && info->dip.s6_addr16[5] == 0xFFFF)
  {
    daf = AF_INET;
  }
  else
    daf = AF_INET6;
  ....
}

В одной и той же функции дважды встречается двойная проверка !info->sip.s6_addr32[0]. Работать от этого функция лучше не станет, а вот упустить важное условие может. Скорее всего, была допущена опечатка в одном условном выражении, которая была скопирована и во второе. Правильным вариантом могло быть:

!info->sip.s6_addr32[0] && !info->sip.s6_addr32[1]

или

!info->sip.s6_addr32[0] && !info->sip.s6_addr16[0]

или что-то ещё. Разработчику стоит проверить этот код. Функция может работать несовсем корректно.

Точно такой же фрагмент кода, с такими же предупреждениями был найден в файле fw_appid.c:

• V501. There are identical sub-expressions '!info->sip.__in6_u.__u6_addr32[0]' to the left and to the right of the '&&' operator. fw_appid.c 864
• V501 There are identical sub-expressions '!info->dip.__in6_u.__u6_addr32[0]' to the left and to the right of the '&&' operator. fw_appid.c 871

Предупреждение N10 – Закрыт навсегда

V621 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. snort_stream_tcp.c 2316

V654 The condition 'i < 0' of loop is always false. snort_stream_tcp.c 2316

#define DEFAULT_PORTS_SIZE 0

static void StreamParseTcpArgs(....)
{
  int i;
  ....
    for (i = 0; i < DEFAULT_PORTS_SIZE; i++)
    {
      ....
    }
  ....
}

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

Предупреждение N11 – Утечка памяти

В начале рассмотрим два макроса: BNFA_MALLOC и BNFA_FREE.

Макрос BNFA_MALLOC раскрывается следующим образом:

#define BNFA_MALLOC(n,memory) bnfa_alloc(n,&(memory))
static void * bnfa_alloc( int n, int * m )
{
   void * p = calloc(1,n);
   if( p )
   {
     if(m)
     {
         m[0] += n;
     }
   }
   return p;
}

А макрос BNFA_FREE скрывает в себе:

#define BNFA_FREE(p,n,memory) bnfa_free(p,n,&(memory))
static void bnfa_free( void *p, int n, int * m )
{
   if( p )
   {
       free(p);
       if(m)
       {
          m[0] -= n;
       }
   }
}

Теперь посмотрим предупреждение PVS-Studio:

V773 The function was exited without releasing the 'pi' pointer. A memory leak is possible. bnfa_search.c 1168

static
int _bnfa_conv_list_to_csparse_array(bnfa_struct_t * bnfa)
{
  bnfa_state_t    * ps; /* transition list */
  bnfa_state_t    * pi; /* state indexes into ps */
  bnfa_state_t      ps_index = 0;
  unsigned       nps;
  ....

  ps = BNFA_MALLOC(nps*sizeof(bnfa_state_t),
    bnfa->nextstate_memory);
  if (!ps)
  {
    return -1;
  }
  bnfa->bnfaTransList = ps;

  pi = BNFA_MALLOC(bnfa->bnfaNumStates*sizeof(bnfa_state_t),
    bnfa->nextstate_memory); // <=
  if (!pi)
  {
    return -1;
  }
  ....
  if (ps_index > nps)
  {
    return -1; // <=
  }
  ....
  BNFA_FREE(pi,bnfa->bnfaNumStates*sizeof(bnfa_state_t),
    bnfa->nextstate_memory);
  return 0;
}

Есть два указателя: ps и pi. Анализатор ругается только на pi. Почему же? Дело в том, что участок памяти, выделенный для ps, запомнил другой, внешний для функции указатель bnfa->bnfaTransList. В этой функции не производится очистка ни bnfa->bnfaTransList, ни ps, а значит, эта память вероятней всего используется и очищается в другом месте программы. Совсем иначе дело обстоит с указателем pi. Память, занятая pi, очищается в конце функции с помощью функции BNFA_FREE. Однако этого не произойдёт, если выполнится условие ps_index > nps. Тогда функция завершится без очистки. Чтобы функция работала корректно, нужно продублировать функцию очистки pi и в тело этого условного оператора.

Похожая ситуация встретилась и в другом месте:

V773 The function was exited without releasing the 'ips_port_filter_list' pointer. A memory leak is possible. parser.c 1854

Предупреждение N12 – Бессмысленная проверка

V547 Expression 'rval != - 6' is always true. output_base.c 219

#define OUTPUT_SUCCESS 0
#define OUTPUT_ERROR -1
#define OUTPUT_ERROR_EXISTS -6
static int register_module(....)
{
  ....
  int rval;
  if ((rval = register_plugin(current_dm)) 
                        != OUTPUT_SUCCESS)
    {
      if (rval != OUTPUT_ERROR_EXISTS) // <=
      {
        fprintf(stderr, "%s: Failed to register OUTPUT plugin.\n",
          current_dm->name);
      }
      return OUTPUT_ERROR;
    }
  ....
}

Заглянем в функцию register_plugin:

static int register_plugin(const Output_Module_t *dm)
{
  if (....)
  {
    ....
    return OUTPUT_ERROR;
  }
  ....
  return OUTPUT_SUCCESS;
}

Анализатор видит, что rval принимает результат функции, а функция может возвращать либо 0, либо -1. Следовательно, rval не может иметь значение -6. Условие if (rval != OUTPUT_ERROR_EXISTS) бессмысленно. rval гарантированно имеет значение -1. Разработчику стоит обратить внимание на этот код. Возможно, нужно проверять другую переменную, или есть опечатка в функции register_plugin.

Подобный случай был обнаружен ещё в одном месте:

V547 Expression 'ret == - 2' is always false. base.c 344

#define OUTPUT_SUCCESS          0
#define OUTPUT_ERROR           -1
#define OUTPUT_ERROR_NOMEM     -2
#define OUTPUT_ERROR_INVAL     -5

int output_load(const char *directory)
{
  ....
  ret = output_load_module(dirpath);
  if (ret == OUTPUT_SUCCESS)
  {
    DEBUG_WRAP(DebugMessage(DEBUG_INIT, 
      "Found module %s\n", de->d_name););
  }
  else if (ret == OUTPUT_ERROR_NOMEM) // <=
  {
    closedir(dirp);
    return OUTPUT_ERROR_NOMEM;
  }
  ....
}

Функция output_load_module возвращает одно из значений: -5, -1, 0. Это значит, что условие ret == -2 всегда будет ложным. Следует проверить условие или функцию. Возможно, есть опечатка.

На этом закончились предупреждения уровня High. Он включает предупреждения с максимальным уровнем достоверности. Они часто указывают на ошибки, требующие немедленного исправления. Уровень Medium содержит менее достоверные предупреждения, на которые все же стоит обратить пристальное внимание. Давайте посмотрим, что удалось обнаружить диагностикам уровня Medium.

Предупреждение N13 – Макросная упаковка

V1004 The 'ppm_pt' pointer wras used unsafely after it was verified against nullptr. Check lines: 361, 362. detect.c 362

ppm_pkt_timer_t  *ppm_pt = NULL;

int Preprocess(Packet * p)
{
  ....
  if( PPM_PKTS_ENABLED() )
  {
    PPM_GET_TIME();
    PPM_TOTAL_PKT_TIME();
    PPM_ACCUM_PKT_TIME();
    ....
  }
  ....
}

#define PPM_TOTAL_PKT_TIME() \
    if( ppm_pt) \
{ \
    ppm_pt->tot = \
      ppm_cur_time - ppm_pt->start - ppm_pt->subtract; \
}

#define PPM_ACCUM_PKT_TIME() \
snort_conf->ppm_cfg.tot_pkt_time += ppm_pt->tot;

Функция Preprocess почти полностью состоит из макросов, в которые упакованы инструкции выполнения программы. Это очень усложняет читабельность кода. Вероятность запутаться, что-то недоглядеть и сделать ошибку повышается. Так и случилось. Два макроса, выполняющие определенные действия стоят друг за другом. Раскрывая их, можно обнаружить, что в первом случае ppm_pt проверяют на nullptr, а во втором – уже нет. Это совсем нелогично. Если ppm_pt будет нулевым, программа все равно упадёт.

Предупреждение N14 – Отладочный код

V547 Expression 'found_offset' is always true. sf_snort_plugin_pcre.c 202

static int pcre_test(...., int *found_offset)
{
  ....
  *found_offset = -1;
  ....

  if (found_offset)
  {
    *found_offset = ovector[1];
    DEBUG_WRAP(DebugMessage(DEBUG_PATTERN_MATCH,
                            "Setting buffer and found_offset: %p %d\n",
                            buf, found_offset););
  }
  return matched;
}

Проверка не имеет смысла. Если по адресу указателя было записано значение, то указатель не нулевой. А если он не нулевой, то значение будет перезаписано. Скорее всего, строчка *found_offset = -1 лишняя и была добавлена при отладке программы. Если found_offset будет являться нулевым указателем, то программа завершится аварийно.

В другом месте диагностика выдала следующее предупреждение:

V547 Expression 'sipMsg->status_code > 0' is always true. sip_dialog.c 806

int SIP_updateDialog(SIPMsg *sipMsg,
                     SIP_DialogList *dList,
                     SFSnortPacket *p      )
{
  int ret;
  ....
  if (sipMsg->status_code == 0)
    {
    ret = SIP_processRequest(....);
    }
  else if (sipMsg->status_code > 0)
    {
    ret = SIP_processResponse(....);
    }
  else
    {
    ret = SIP_FAILURE;
    }
  ....
}

Все бы хорошо, но sipMsg->status_code имеет тип uint16_t. Если этот элемент структуры SIPMsg не равен нулю, то он может быть только больше нуля. Условие в первом else избыточно, а тело второго оператора else недостижимо. Тут нет ошибки, есть только избыточный код. Его следует избегать, чтобы в дальнейшем на его изучение и доработку не тратилось время разработчика.

Подобное предупреждение было найдено ещё в 32 местах.

Предупреждение N15 – Избыточность или опечатка?

V560 A part of conditional expression is always true: hnode. spp_frag3.c 4366

static int Frag3Prune(FragTracker *not_me)
{
  SFXHASH_NODE *hnode;
  ....
  while (....)
  {
    hnode = sfxhash_lru_node(f_cache);
    if (!hnode)
    {
      break;
    }

    if (hnode && hnode->data == not_me)  // <=
  }
  ....
}

Проверка на нулевой указатель hnode тут не нужна. Если hnode будет нулевым, условие и так будет пропущено. Может, допущена опечатка и нужно было проверить какое-то поле объекта *hnode?

Подобное предупреждение было найдено ещё в 39 местах.

Предупреждение N16 – Дублированное условие

V581 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 300, 308. sf_snort_plugin_pcre.c 308

static int pcreMatchInternal(...., const uint8_t **cursor)
{
  const uint8_t *buffer_start;
  int pcre_offset;
  int pcre_found;
  ....
  if (pcre_found)
  {
    if (cursor)
    {
      *cursor = buffer_start + pcre_offset;
    }
  }

  if (pcre_found)
    return RULE_MATCH;
  ....
}

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

Предупреждение N17 – break или return?

V1001 The 'portsweep' variable is assigned but is not used by the end of the function. spp_sfportscan.c 596

static int PortscanAlertTcp(PS_PROTO *proto, ....)
{
  ....
  int portsweep = 0;

  if (!proto)
    return -1;

  switch (proto->alerts)
  {
  case PS_ALERT_ONE_TO_ONE:
    ....
    break;

  case PS_ALERT_ONE_TO_ONE_DECOY:
    ....
    break;

  case PS_ALERT_PORTSWEEP:
    ....
    portsweep = 1;
    break;

  case PS_ALERT_DISTRIBUTED:
    ....
    break;

  case PS_ALERT_ONE_TO_ONE_FILTERED:
    ....
    break;

  case PS_ALERT_ONE_TO_ONE_DECOY_FILTERED:
    ....
    break;

  case PS_ALERT_PORTSWEEP_FILTERED:
    ....
    portsweep = 1; // <=
    return 0;

  case PS_ALERT_DISTRIBUTED_FILTERED:
    ....
    break;

  default:
    return 0;
  }
  ....
}

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

Предупреждение N18 – Если ноль не равен нулю

V1048 The 'ret' variable was assigned the same value. sf_snort_plugin_loop.c 142

V1048 The 'ret' variable was assigned the same value. sf_snort_plugin_loop.c 148

int LoopInfoInitialize(...., Rule *rule, LoopInfo *loopInfo)
{
  int ret;

  /* Initialize the dynamic start, end, increment fields */
  ret = DynamicElementInitialize(rule, loopInfo->start);
  if (ret)
  {
    return ret;
  }
  ret = DynamicElementInitialize(rule, loopInfo->end);
  if (ret)
  {
    return ret;
  }
  ret = DynamicElementInitialize(rule, loopInfo->increment);
  if (ret)
  {
    return ret;
  }
  ....
}

Ниже приведено определение функции DynamicElementInitialize. Стоит обратить внимание на возвращаемое значение.

int DynamicElementInitialize(Rule *rule, DynamicElement *element)
{
  void *memoryLocation;

  if (!rule->ruleData)
  {
    DynamicEngineFatalMessage("ByteExtract variable '%s' "
      "in rule [%d:%d] is used before it is defined.\n", 
      element->refId, rule->info.genID, rule->info.sigID);
  }

  switch (element->dynamicType)
  {
  case DYNAMIC_TYPE_INT_REF:
    memoryLocation = sfghash_find((SFGHASH*)rule->ruleData,
                                           element->refId);
    if (memoryLocation)
    {
       element->data.dynamicInt = memoryLocation;
    }
    else
    {
      element->data.dynamicInt = NULL;
      DynamicEngineFatalMessage("ByteExtract variable '%s' "
        "in rule [%d:%d] is used before it is defined.\n",
        element->refId, rule->info.genID, rule->info.sigID);
      //return -1;
    }
    break;
  case DYNAMIC_TYPE_INT_STATIC:
  default:
    /* nothing to do, its static */
    break;
  }

  return 0;  // <=
}

Функция DynamicElementInitialize всегда возвращает 0, поэтому проверять возвращаемое значение ret в функции LoopInfoInitialize нет никакого смысла. Нет смысла вообще что-то возвращать, если есть всего один вариант значения. Ранее разработчики, возможно, экспериментировали со значением -1 (об этом свидетельствует закомментированный код), но сейчас этот код не используется.

Подобное предупреждение встречалась ещё в 15 местах.

В результате проверки системы обнаружения вторжения Snort c помощью статического анализатора PVS-Studio было обнаружено 35 потенциально опасных мест или ошибок, а также около 100 участков кода, которые требуют внимания разработчиков. Есть вероятность, что они работают совсем не так, как было задумано. Это не так много для 470 000 строк кода на языке Си. Разработчики проекта Snort хорошо выполнили свою работу. Они сделали свой продукт максимально продуманным и с минимальным числом ошибок. Использовав PVS-Studio, они бы могли сделать код ещё более качественным и, скорее всего, сэкономить довольно большую часть времени, потраченную на отладку.

В следующей статье мы проанализируем Snort, написанный на C++, и сравним результаты двух анализов. Это позволит увидеть, какие паттерны ошибок больше характерны для Си, а какие – для C++. Также посмотрим, стал ли код более качественным или расширение функционала привело к большему числу ошибок.

Заключение

PVS-Studio – удобный и полезный инструмент для разработчика. Он приходит на помощь, когда в человеческий мозг перестают влезать многоуровневые зависимости в коде, когда программист теряет внимательность в результате усталости, когда модифицируются большие файлы и не все тонкости работы программы можно легко заметить, чтобы дописать код корректно. Статический анализатор – это программа, которая будет покорно и всегда внимательно проверять код. Используйте PVS-Studio во время разработки, и вы сэкономите часть своего времени и нервов.