Network Security Services (NSS) is a set of libraries designed to support cross-platform development of security-enabled client and server applications. It implements cryptographic functions in the Firefox and Chrome browsers, and after a recently found certificate signature verification vulnerability, I decided to take a look at this project too.
We obtained the source code through the following commands:
Since the library is built from the Windows console, I had to use a special utility PVS-Studio Standalone to analyze it. This tool is described in the article PVS-Studio Now Supports Any Build System under Windows and Any Compiler. Easy and Right Out of the Box.
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;
}
....
}The truth table of logical disjunction suggests that if at least one operand is one (like in this case), the condition will always be true.
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
....
}The 'j' variable is used twice in one sequence point. Because of that, you can't predict the result of this expression. To learn more, see the description of the V567 diagnostic.
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); // <=
....
}
....
}In this case, the file shouldn't be closed if the pointer to the file is null. Otherwise, it will cause some troubles. What exactly will happen depends on the handler used to handle such issues (see _set_invalid_parameter_handler() and so on).
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);
....
}The number of format specifiers doesn't correspond to the number of arguments passed to the fprintf() function.
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;
}The 'buf' pointer is still checked for being null. It means that an error may occur in the previous line when passing a null pointer to the strftime() function.
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;
}This is the most dangerous fragment in code responsible for processing confidential information. Since the 'hashed_secret' array is not used anymore after calling the 'memset' function, the compiler is allowed to delete the function call for the sake of optimization, and so the array won't be cleared as intended.
Those were probably the most dangerous errors among all found.
Programmers often don't quite understand the V597 warning. So here you are some additional materials to figure out what this issue is all about:
Here is the list of all such fragments:
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 language standard, shifting a negative number causes undefined behavior.
Another similar fragment:
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);
}
....
}Besides a correct number and zero, difference of unsigned numbers may result in an extremely large value resulting from casting a negative number to unsigned. In this fragment, an incorrect giant value will meet the condition and the 'memset' function will try to clear a huge amount of memory.
However, such an overflow might never occur at all - one can't say for sure what the limits of the range of values the variables in this expression can possibly take are. But the check is too unsafe anyway.
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;And the last thing, a small comment concerning the issue with declaring types which are already declared in system files.
Check the following fragments:
That's not an error of course. But still, why do that?
Security of private data has been treated with especially high attention recently. So let's not forget that software security means as well as intrusion means are developed by humans, and humans tend to make mistakes.
Using static analysis regularly will help you save plenty of time to solve more serious tasks.