# Проверка библиотеки Network Security Services Network Security Services \(NSS\) \- это набор библиотек, разработанных для поддержки кроссплатформенной разработки защищенных клиентских и серверных приложений\. Библиотека используется для криптографии в браузерах Firefox и Chrome, и после недавно найденной уязвимости в проверке подписей сертификатов я решил тоже взглянуть на этот проект\. ![0286_NSS_ru/image1.png](https://import.viva64.com/docx/blog/0286_NSS_ru/image1.png) [Подробнее об уязвимости\.](https://www.kb.cert.org/vuls/id/772676) Исходный код был получен следующими командами: * hg clone https://hg\.mozilla\.org/projects/nspr * hg clone https://hg\.mozilla\.org/projects/nss Так как библиотека собирается из консоли Windows, то для проверки я воспользовался специальной утилитой PVS\-Studio Standalone, которая описана в статье: [PVS\-Studio теперь поддерживает любую сборочную систему на Windows и любой компилятор\. Легко и "из коробки"](https://pvs-studio.ru/ru/blog/posts/0243/)\. ## Результаты проверки [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'dtype \!\= 2 \|\| dtype \!\= 3' is always true\. Probably the '&&' operator should be used here\. crlgen\.c 1172 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v567/) Undefined behavior\. The 'j' variable is modified while being used twice between sequence points\. pk11slot\.c 1934 ```cpp PK11SlotList* PK11_GetAllTokens(....) { .... #if defined( XP_WIN32 ) waste[ j & 0xf] = j++; #endif .... } ``` Переменная 'j' дважды используется в одной точке следования\. В результате невозможно предсказать результат работы такого выражения\. Подробнее в описании диагностики [V567](https://pvs-studio.ru/ru/docs/warnings/v567/)\. [V575](https://pvs-studio.ru/ru/docs/warnings/v575/) The null pointer is passed into 'fclose' function\. Inspect the first argument\. certcgi\.c 608 ```cpp static int get_serial_number(Pair *data) { FILE *serialFile; .... serialFile = fopen(filename, "r"); if (serialFile != NULL) { .... } else { fclose(serialFile); // <= .... } .... } ``` В данном случае, если указатель на файл нулевой, то закрывать файл не надо\. А вот если его закрывать, то это может привести к неприятностям\. Что именно произойдет, зависит от установленного обработчика таких событий \(см\. \_set\_invalid\_parameter\_handler\(\) и так далее\)\. [V576](https://pvs-studio.ru/ru/docs/warnings/v576/) Incorrect format\. A different number of actual arguments is expected while calling 'fprintf' function\. Expected: 3\. Present: 7\. pp\.c 34 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'buf' pointer was utilized before it was verified against nullptr\. Check lines: 1709, 1710\. prtime\.c 1709 ```cpp PR_IMPLEMENT(PRUint32) PR_FormatTime(....) { .... rv = strftime(buf, buflen, fmt, ap); if (!rv && buf && buflen > 0) { buf[0] = '\0'; } return rv; } ``` Указатель 'buf' всё\-таки проверяют на равенство нулю, значит, в предыдущей строчке возможно возникновение ошибки при передаче нулевого указателя функции strftime\(\)\. [V597](https://pvs-studio.ru/ru/docs/warnings/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 ```cpp #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 остаётся непонятым\. Предлагаю дополнительные материалы, раскрывающие суть проблемы: * [Перезаписывать память — зачем?](https://pvs-studio.ru/ru/blog/posts/cpp/k0041/) * [Zero and forget — caveats of zeroing memory in C](http://web.archive.org/web/20160304012102/http:/www.eliteraspberries.com/blog/2012/10/zero-and-forget--caveats-of-zeroing-memory-in-c.html) Список всех таких мест: * 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](https://pvs-studio.ru/ru/docs/warnings/v610/) Undefined behavior\. Check the shift operator '<<\. The left operand '\-1L' is negative\. inflate\.c 1475 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v555/) The expression 'emLen \- reservedLen \- inputLen \> 0' will work as 'emLen \- reservedLen \!\= inputLen'\. rsapkcs\.c 708 ```cpp #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](https://pvs-studio.ru/ru/docs/warnings/v677/) Custom declaration of a standard 'BYTE' type\. The system header file should be used: \#include \. des\.h 15 ```cpp typedef unsigned char BYTE; ``` Напоследок \- небольшое замечание по поводу объявления типов, которые уже определены в системных файлах\. Аналогичные места: * V677 Custom declaration of a standard 'WORD' type\. The system header file should be used: \#include \. arcfour\.c 36 * V677 Custom declaration of a standard 'off\_t' type\. The system header file should be used: \#include \. winfile\.h 34 Конечно, это не ошибка\. Но зачем так делать? ## Заключение В последнее время безопасности персональных данных уделяют особое внимание\. Не будем же забывать, что средства защиты, как и средства взлома, пишутся людьми, а людям свойственно ошибаться\. Используя статический анализ регулярно, можно сэкономить массу времени на решение более полезных задач\. Также контроль за качеством кода можно переложить на других, например, воспользовавшись новой услугой: [регулярный аудит Си/Си\+\+ кода](https://habr.com/ru/company/pvs-studio/blog/233783/)\.