Проверка библиотеки Network Security Services


Network Security Services (NSS) — это набор библиотек, разработанных для поддержки кроссплатформенной разработки защищенных клиентских и серверных приложений. Библиотека используется для криптографии в браузерах Firefox и Chrome, и после недавно найденной уязвимости в проверке подписей сертификатов, я решил тоже взглянуть на этот проект.

Подробнее об уязвимости.

Исходный код был получен следующими командами:Так как библиотека собирается из консоли Windows, то для проверки я воспользовался специальной утилитой PVS-Studio Standalone, которая описана в статье: PVS-Studio теперь поддерживает любую сборочную систему на Windows и любой компилятор. Легко и «из коробки».

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


V547 Expression 'dtype != 2 || dtype != 3' is always true. Probably the '&&' operator should be used here. crlgen.c 1172
static SECStatus
crlgen_setNextDataFn_field(...., unsigned short dtype)
{
  ....
  if (dtype != CRLGEN_TYPE_DIGIT ||                    //<==
      dtype != CRLGEN_TYPE_DIGIT_RANGE) {              //<==
        crlgen_PrintError(crlGenData->parsedLineNum,
          "range value should have "
          "numeric or numeric range values.\n");
    return SECFailure;
  }
  ....
}

Из таблицы истинности логического сложения следует, что если хотя бы один операнд – единица (как в этом случае), то условие будет всегда истинным.

V567 Undefined behavior. The 'j' variable is modified while being used twice between sequence points. pk11slot.c 1934
PK11SlotList* PK11_GetAllTokens(....)
{
  ....
  #if defined( XP_WIN32 ) 
    waste[ j & 0xf] = j++; 
  #endif
  ....
}

Переменная 'j' дважды используется в одной точке следования. В результате невозможно предсказать результат работы такого выражения. Подробнее в описании диагностики V567.

V575 The null pointer is passed into 'fclose' function. Inspect the first argument. certcgi.c 608
static int get_serial_number(Pair  *data)
{
  FILE *serialFile;
  ....
  serialFile = fopen(filename, "r");
  if (serialFile != NULL) {
  ....
  } else {
    fclose(serialFile);                  //<==
    ....
  }
  ....
}

В данном случае, если указатель на файл нулевой, то закрывать файл не надо. А вот если его закрывать, то это может привести к неприятностям. Что именно произойдет, зависит от установленного обработчика таких событий (см. _set_invalid_parameter_handler() и так далее).

V576 Incorrect format. A different number of actual arguments is expected while calling 'fprintf' function. Expected: 3. Present: 7. pp.c 34
static void Usage(char *progName)
{
  ....
  fprintf(stderr, "%-14s (Use either the long type name or "
    "the shortcut.)\n", "", SEC_CT_CERTIFICATE_ID,
    SEC_CT_PKCS7, SEC_CT_CRL, SEC_CT_NAME);
  ....
}

Количество спецификаторов формата не соответствует количеству передаваемых аргументов функции fprintf().

V595 The 'buf' pointer was utilized before it was verified against nullptr. Check lines: 1709, 1710. prtime.c 1709
PR_IMPLEMENT(PRUint32) PR_FormatTime(....)
{
  ....
  rv = strftime(buf, buflen, fmt, ap);
  if (!rv && buf && buflen > 0) {
    buf[0] = '\0';
  }
  return rv;
}

Указатель 'buf' всё-таки проверяют на равенство нулю, значит, в предыдущей строчке возможно возникновение ошибки при передаче нулевого указателя функции strftime().

V597 The compiler could delete the 'memset' function call, which is used to flush 'hashed_secret' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. alghmac.c 87
#define PORT_Memset memset

SECStatus
HMAC_Init( HMACContext * cx, const SECHashObject *hash_obj,
           const unsigned char *secret,
           unsigned int secret_len, PRBool isFIPS)
{
  ....
  PORT_Memset(hashed_secret, 0, sizeof hashed_secret);   //<==
  if (cx->hash != NULL)
    cx->hashobj->destroy(cx->hash, PR_TRUE);
  return SECFailure;
}

Самое опасное место для кода, отвечающего за обработку секретной информации. Так как массив 'hashed_secret' больше не используется после вызова функции 'memset', то компилятор может удалить вызов функции для оптимизации, и массив не будет обнулён, как планировалось.

Это, пожалуй, самые серьезные ошибки из всех найденных.

Часто предупреждение V597 остаётся непонятым. Предлагаю дополнительные материалы, раскрывающие суть проблемы:Список всех таких мест:
V610 Undefined behavior. Check the shift operator '<<. The left operand '-1L' is negative. inflate.c 1475
long ZEXPORT inflateMark(strm)
z_streamp strm;
{
  struct inflate_state FAR *state;

  if (strm == Z_NULL || strm->state == Z_NULL)
    return -1L << 16;
  state = (struct inflate_state FAR *)strm->state;
  ....
}

Согласно стандарту языка C++11, сдвиг отрицательного числа приводит к неопределённому поведению.

Ещё похожее место:
V555 The expression 'emLen — reservedLen — inputLen > 0' will work as 'emLen — reservedLen != inputLen'. rsapkcs.c 708
#define PORT_Memset memset

static SECStatus
eme_oaep_encode(unsigned char * em,
                unsigned int emLen,
                const unsigned char * input,
                unsigned int inputLen,
                HASH_HashType hashAlg,
                HASH_HashType maskHashAlg,
                const unsigned char * label,
                unsigned int labelLen,
                const unsigned char * seed,
                unsigned int seedLen)
{
  ....
  /* Step 2.b - Generate PS */
    if (emLen - reservedLen - inputLen > 0) {
        PORT_Memset(em + 1 + (hash->length * 2), 0x00,
                    emLen - reservedLen - inputLen);
    }
  ....
}

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

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

V677 Custom declaration of a standard 'BYTE' type. The system header file should be used: #include <WinDef.h>. des.h 15
typedef unsigned char BYTE;

Напоследок — небольшое замечание по поводу объявления типов, которые уже определены в системных файлах.

Аналогичные места:
Конечно, это не ошибка. Но зачем так делать?

Заключение


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

Используя статический анализ регулярно, можно сэкономить массу времени на решение более полезных задач. Также контроль за качеством кода можно переложить на других, например, воспользовавшись новой услугой: регулярный аудит Си/Си++ кода.

Эта статья на английском


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Analyzing the Network Security Services Library.

Прочитали статью и есть вопрос?
Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio и CppCat, версия 2014. Пожалуйста, ознакомьтесь со списком.
@SvyatoslavMC
08.10.2014 12:27 UTC
Первоисточник