Geekly Articles each Day
📜 ⬆️ ⬇️

Check Network Security Services Library


Network Security Services (NSS) is a set of libraries designed to support cross-platform development of secure client and server applications. The library is used for cryptography in Firefox and Chrome browsers, and after a recently found vulnerability in verifying certificate signatures, I decided to also take a look at this project.

More on vulnerability.

The source code was obtained by the following commands:Since the library is built from the Windows console, for testing I used the special utility PVS-Studio Standalone, which is described in the article: PVS-Studio now supports any build system on Windows and any compiler. Easy and out of the box .

Test results


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; } .... }

From the truth table of logical addition it follows that if at least one operand is one (as in this case), then the condition will always be true.
')
V567 Undefined behavior. The j variable is modified while it is being used. pk11slot.c 1934
 PK11SlotList* PK11_GetAllTokens(....) { .... #if defined( XP_WIN32 ) waste[ j & 0xf] = j++; #endif .... }

The variable 'j' is used twice in one sequence point. As a result, it is impossible to predict the result of the operation of such an expression. See details in the V567 diagnostic description .

V575 The null pointer is passed to '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); //<== .... } .... }

In this case, if the pointer to the file is zero, then it is not necessary to close the file. But if you close it, it can lead to trouble. What exactly happens will depend on the installed handler for such events (see _set_invalid_parameter_handler () and so on).

V576 Incorrect format. A different number of actual arguments is expected while calling the '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); .... }

The number of format specifiers does not match the number of arguments passed to the fprintf () function.

V595 ' bu f pointer pointer pointer 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; }

The 'buf' pointer is still checked for equality to zero, which means that an error may occur in the previous line when passing the zero pointer to the strftime () function.

V597 The compiler couldn’t 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; }

The most dangerous place for the code responsible for processing sensitive information. Since the array hashed_secret is no longer used after calling the memset function, the compiler can remove the function call for optimization, and the array will not be zeroed as planned.

These are perhaps the most serious errors of all found.

Often the V597 warning remains misunderstood. I propose additional materials that reveal the essence of the problem:List of all such places:
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; .... }

According to the C ++ 11 standard, a negative number shift leads to undefined behavior.

Another similar place:
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); } .... }

The result of the difference of unsigned numbers, in addition to the correct and zero values, can also be an incredibly large number, resulting from the reduction of a negative number to an unsigned type. In this fragment, an incorrect giant value will satisfy the verification condition, and the 'memset' function will attempt to reset a huge amount of memory.

However, such an overflow may never occur at all - it is difficult to judge what the limiting values ​​of the variables used in the expression are. But the check is in any case very unreliable.

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;

Finally - a small note about the declaration of types that are already defined in the system files.

Similar places:
Of course, this is not a mistake. But why do so?

Conclusion


Recently, the security of personal data has been given special attention. Let's not forget that the means of protection, as well as the means of hacking, are written by people, and people tend to make mistakes.

Using static analysis regularly, you can save a lot of time on solving more useful tasks. Also, control over the quality of the code can be shifted to others, for example, using the new service: regular auditing of C / C ++ code .

This article is in English.


If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Analyzing the Network Security Services Library .

Read the article and have a question?
Often our articles are asked the same questions. We collected the answers to them here: Answers to questions from readers of articles about PVS-Studio and CppCat, version 2014 . Please review the list.

Source: https://habr.com/ru/post/239629/

More articles:


All Articles