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;Напоследок - небольшое замечание по поводу объявления типов, которые уже определены в системных файлах.
Аналогичные места:
Конечно, это не ошибка. Но зачем так делать?
В последнее время безопасности персональных данных уделяют особое внимание. Не будем же забывать, что средства защиты, как и средства взлома, пишутся людьми, а людям свойственно ошибаться.
Используя статический анализ регулярно, можно сэкономить массу времени на решение более полезных задач. Также контроль за качеством кода можно переложить на других, например, воспользовавшись новой услугой: регулярный аудит Си/Си++ кода.