To get a trial key
fill out the form below
Team License (standard version)
Enterprise License (extended version)
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Request our prices
New License
License Renewal
--Select currency--
USD
EUR
GBP
RUB
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Message submitted.

Your message has been sent. We will email you at


If you haven't received our response, please do the following:
check your Spam/Junk folder and click the "Not Spam" button for our message.
This way, you won't miss messages from our team in the future.

>
>
>
Analyzing the Network Security Services…

Analyzing the Network Security Services Library

Oct. 8, 2014

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.

0286_NSS/image1.png

More about the vulnerability.

We obtained the source code through the following commands:

  • hg clone https://hg.mozilla.org/projects/nspr
  • hg clone https://hg.mozilla.org/projects/nss

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.

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

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:

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

According to the C++11 language standard, shifting a negative number causes undefined behavior.

Another similar fragment:

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

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:

  • 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

That's not an error of course. But still, why do that?

Conclusion

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.

Popular related articles
Characteristics of PVS-Studio Analyzer by the Example of EFL Core Libraries, 10-15% of False Positives

Date: 07.31.2017

Author: Andrey Karpov

After I wrote quite a big article about the analysis of the Tizen OS code, I received a large number of questions concerning the percentage of false positives and the density of errors (how many erro…
The way static analyzers fight against false positives, and why they do it

Date: 03.20.2017

Author: Andrey Karpov

In my previous article I wrote that I don't like the approach of evaluating the efficiency of static analyzers with the help of synthetic tests. In that article, I give the example of a code fragment…
Free PVS-Studio for those who develops open source projects

Date: 12.22.2018

Author: Andrey Karpov

On the New 2019 year's eve, a PVS-Studio team decided to make a nice gift for all contributors of open-source projects hosted on GitHub, GitLab or Bitbucket. They are given free usage of PVS-Studio s…
PVS-Studio for Java

Date: 01.17.2019

Author: Andrey Karpov

In the seventh version of the PVS-Studio static analyzer, we added support of the Java language. It's time for a brief story of how we've started making support of the Java language, how far we've co…
Technologies used in the PVS-Studio code analyzer for finding bugs and potential vulnerabilities

Date: 11.21.2018

Author: Andrey Karpov

A brief description of technologies used in the PVS-Studio tool, which let us effectively detect a large number of error patterns and potential vulnerabilities. The article describes the implementati…
Appreciate Static Code Analysis!

Date: 10.16.2017

Author: Andrey Karpov

I am really astonished by the capabilities of static code analysis even though I am one of the developers of PVS-Studio analyzer myself. The tool surprised me the other day as it turned out to be sma…
The Last Line Effect

Date: 05.31.2014

Author: Andrey Karpov

I have studied many errors caused by the use of the Copy-Paste method, and can assure you that programmers most often tend to make mistakes in the last fragment of a homogeneous code block. I have ne…
The Evil within the Comparison Functions

Date: 05.19.2017

Author: Andrey Karpov

Perhaps, readers remember my article titled "Last line effect". It describes a pattern I've once noticed: in most cases programmers make an error in the last line of similar text blocks. Now I want t…
The Ultimate Question of Programming, Refactoring, and Everything

Date: 04.14.2016

Author: Andrey Karpov

Yes, you've guessed correctly - the answer is "42". In this article you will find 42 recommendations about coding in C++ that can help a programmer avoid a lot of errors, save time and effort. The au…
Static analysis as part of the development process in Unreal Engine

Date: 06.27.2017

Author: Andrey Karpov

Unreal Engine continues to develop as new code is added and previously written code is changed. What is the inevitable consequence of ongoing development in a project? The emergence of new bugs in th…

Comments (0)

Next comments

This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
This website uses cookies and other technology to provide you a more personalized experience. By continuing the view of our web-pages you accept the terms of using these files. If you don't want your personal data to be processed, please, leave this site.
Learn More →
Accept