Pull to refresh
280.71
PVS-Studio
Static Code Analysis for C, C++, C# and Java

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

Reading time 7 min
Views 6.7K

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 остаётся непонятым. Предлагаю дополнительные материалы, раскрывающие суть проблемы:Список всех таких мест:
  • V597 The compiler could delete the 'memset' function call, which is used to flush 'ctx' object. The RtlSecureZeroMemory() function should be used to erase the private data. sha512.c 503
  • V597 The compiler could delete the 'memset' function call, which is used to flush 'ctx' object. The RtlSecureZeroMemory() function should be used to erase the private data. sha512.c 605
  • V597 The compiler could delete the 'memset' function call, which is used to flush 'ctx' object. The RtlSecureZeroMemory() function should be used to erase the private data. sha512.c 1307
  • V597 The compiler could delete the 'memset' function call, which is used to flush 'ctx' object. The RtlSecureZeroMemory() function should be used to erase the private data. sha512.c 1423
  • V597 The compiler could delete the 'memset' function call, which is used to flush 'cx' object. The RtlSecureZeroMemory() function should be used to erase the private data. md5.c 209
  • V597 The compiler could delete the 'memset' function call, which is used to flush 'ctx' object. The RtlSecureZeroMemory() function should be used to erase the private data. sha_fast.c 416
  • V597 The compiler could delete the 'memset' function call, which is used to flush 'lastBlock' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. cts.c 141
  • V597 The compiler could delete the 'memset' function call, which is used to flush 'lastBlock' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. cts.c 299
  • V597 The compiler could delete the 'memset' function call, which is used to flush 'data' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. drbg.c 300
  • V597 The compiler could delete the 'memset' function call, which is used to flush 'inputhash' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. drbg.c 450
  • V597 The compiler could delete the 'memset' function call, which is used to flush 'localDigestData' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. dsa.c 417
  • V597 The compiler could delete the 'memset' function call, which is used to flush 'U' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. pqg.c 422
  • V597 The compiler could delete the 'memset' function call, which is used to flush 'sha1' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. pqg.c 423
  • V597 The compiler could delete the 'memset' function call, which is used to flush 'sha2' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. pqg.c 424
  • V597 The compiler could delete the 'memset' function call, which is used to flush 'U' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. pqg.c 471
  • V597 The compiler could delete the 'memset' function call, which is used to flush 'data' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. pqg.c 1208
  • V597 The compiler could delete the 'memset' function call, which is used to flush 'state' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. tlsprfalg.c 86
  • V597 The compiler could delete the 'memset' function call, which is used to flush 'outbuf' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. tlsprfalg.c 87
  • V597 The compiler could delete the 'memset' function call, which is used to flush 'newdeskey' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. pkcs11c.c 943
  • V597 The compiler could delete the 'memset' function call, which is used to flush 'randomData' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. pk11merge.c 298
  • V597 The compiler could delete the 'memset' function call, which is used to flush 'keyData' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. sslcon.c 2151
  • V597 The compiler could delete the 'memset' function call, which is used to flush 'randbuf' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. keystuff.c 113

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, сдвиг отрицательного числа приводит к неопределённому поведению.

Ещё похожее место:
  • V610 Undefined behavior. Check the shift operator '<<=. The left operand is negative ('cipher' = [-1..15]). strsclnt.c 1115

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;

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

Аналогичные места:
  • V677 Custom declaration of a standard 'WORD' type. The system header file should be used: #include <WinDef.h>. arcfour.c 36
  • V677 Custom declaration of a standard 'off_t' type. The system header file should be used: #include <WCHAR.H or SYS\TYPES.H>. winfile.h 34

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

Заключение


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

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

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


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

Прочитали статью и есть вопрос?
Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio и CppCat, версия 2014. Пожалуйста, ознакомьтесь со списком.
Tags:
Hubs:
+16
Comments 22
Comments Comments 22

Articles

Information

Website
pvs-studio.com
Registered
Founded
2008
Employees
31–50 employees