Checking MatrixSSL with PVS-Studio and Cppcheck

Pavel Pimenov
Articles: 1



In this article, I'm going to tell you about a check of the MatrixSSL project done with the static analyzers for C/C++ code PVS-Studio and Cppcheck.

Picture 1

The article is written by Pavel Pimenov, the author of the open peer-to-peer client FlylinkDC++. The article is published in our blog by his permission.

What I liked about the MatrixSSL project was that it came with the MS Visual Studio 2010 version available "out-of-the-box".

You know, in order to be able to build openSSL from source files for Visual C++, you usually have to dance around with a shaman's drum for a while :). That's why many Windows developers use ready binary openSSL builds such as Win32 OpenSSL Installation Project.

MatrixSSL is an alternative library of cryptographic algorithms distributed under the GNU license (commercial support is also available).

The source code of the open-source version can be downloaded from the official site. We analyzed the current version 3.7.1 (matrixssl-3-7-1-open.tgz).

About the analyzers

  • PVS-Studio is a commercial static analyzer detecting errors in source code of C/C++/C++11 applications (we used version PVS-Studio 5.21).
  • Cppcheck is a free open-source analyzer (we used version Cppcheck 1.68).

Analysis results by PVS-Studio

Memory clearing

V512 A call of the 'memset' function will lead to underflow of the buffer 'ctx->pad'. hmac.c 136, 222, 356

...
// crypto\digest\digest.h
typedef struct {
#ifdef USE_SHA384
  unsigned char  pad[128];
#else
  unsigned char  pad[64];
#endif  

int32 psHmacMd5Final(psHmacContext_t *ctx, unsigned char *hash)
{ 
  memset(ctx->pad, 0x0, 64);
  return MD5_HASH_SIZE;
}
...

The code of all the three functions is alright and only the used part of the array is cleared, but the analyzer warns that the size of the requested buffer - 128 bytes - is probably too large.

I think it's OK here but still it's better to clear either 64 or 128 bytes just for the code to look neat. You can write it, for example, like this:

memset(ctx->pad, 0x0, sizeof(ctx->pad));

V597 The compiler could delete the 'memset' function call, which is used to flush 'tmp' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. aes.c 1139

...
int32 psAesEncrypt(psCipherContext_t *ctx, unsigned char *pt,
           unsigned char *ct, uint32 len)
{
  unsigned char  tmp[MAXBLOCKSIZE];
        .....
  memset(tmp, 0x0, sizeof(tmp));
  return len;
}
...

The optimizer throws away the call of the standard memset() function. I guess it may be critical for a crypto library and is a potential break.

Other similar issues: aes.c 1139, aes.c 1190, aes.c 1191, des3.c 1564, des3.c 1609, des3.c 1610, corelib.c 304, pkcs.c 1625, pkcs.c 1680, pkcs.c 1741

V676 It is incorrect to compare the variable of BOOL type with TRUE. Correct expression is: 'QueryPerformanceFrequency(& hiresFreq) == FALSE'. osdep.c 52, 55

...
#define  PS_TRUE  1
#define  PS_FALSE   0  
int osdepTimeOpen(void)
{
  if (QueryPerformanceFrequency(&hiresFreq) != PS_TRUE) {
    return PS_FAILURE;
  }
  if (QueryPerformanceCounter(&hiresStart) != PS_TRUE) {
    return PS_FAILURE;
  }
...

PS_TRUE is declared as "1". MSDN says the following about the return value of the QueryPerformanceFrequency function: "If the installed hardware supports a high-resolution performance counter, the return value is nonzero" So, a safer way to write it is QueryPerformanceCounter() == PS_FALSE

V547 Expression '(id = ssl->sessionId) == ((void *) 0)' is always false. Pointer 'id = ssl->sessionId' != NULL. matrixssl.c 2061

...
typedef struct ssl {
        ...
  unsigned char  sessionIdLen;
  unsigned char  sessionId[SSL_MAX_SESSION_ID_SIZE];

int32 matrixUpdateSession(ssl_t *ssl)
{
#ifndef USE_PKCS11_TLS_ALGS
  unsigned char  *id;
  uint32  i;

  if (!(ssl->flags & SSL_FLAGS_SERVER)) {
    return PS_ARG_FAIL;
  }
  if ((id = ssl->sessionId) == NULL) {
    return PS_ARG_FAIL;
  }
...

There's an obvious error here: The condition will never be fulfilled because sessionld is declared as an array of 32 bytes and can't have a NULL address. This error is not critical of course and could probably be viewed just as an excessive pointless check.

V560 A part of conditional expression is always true: 0x00000002. osdep.c 265

...
#define FILE_SHARE_READ                 0x00000001  
#define FILE_SHARE_WRITE                0x00000002  

  if ((hFile = CreateFileA(fileName, GENERIC_READ,
      FILE_SHARE_READ && FILE_SHARE_WRITE, NULL, OPEN_EXISTING,
      FILE_ATTRIBUTE_NORMAL, NULL)) == INVALID_HANDLE_VALUE) {
    psTraceStrCore("Unable to open %s\n", (char*)fileName);
        return PS_PLATFORM_FAIL;
...

We have a typo here: Instead of FILE_SHARE_READ | FILE_SHARE_WRITE, the programmer wrote && and got 1 && 2 == 1

which is equivalent to one FILE_SHARE_READ.

Probably incorrect condition

V590 Consider inspecting the '* c != 0 && * c == 1' expression. The expression is excessive or contains a misprint. ssldecode.c 3539

...
    if (*c != 0 && *c == 1) {
#ifdef USE_ZLIB_COMPRESSION
      ssl->inflate.zalloc = NULL;
...

Probable performance drop

V814 Decreased performance. The 'strlen' function was called multiple times inside the body of a loop. x509.c 226

...
  memset(current, 0x0, sizeof(psList_t));
  chFileBuf = (char*)fileBuf;
  while (fileBufLen > 0) {
  if (((start = strstr(chFileBuf, "-----BEGIN")) != NULL) &&
...
      start += strlen("CERTIFICATE-----");
      if (current == NULL) {
...

In this code, inside the while() loop, the analyzer detected a call of the strlen() function for a parameter which doesn't change. Generally it is not optimal but in this particular case since the strlen() function receives a constant known at the compilation stage, the optimizer in the /O2 mode will remove the function call completely and substitute it with the constant's value calculated at the compilation stage.

Analysis results by Cppcheck

This analyzer generated fewer warnings but there were some among them which PVS-Studio had failed to diagnose.

None of them affect the library's work as they all refer to unit-tests in crypto\test.

"Finishing return-shot in the head"

Consecutive return, break, continue, goto or throw statements are unnecessary. The second statement can never be executed, and so should be removed.

...

int32 psSha224Test(void)
{
  runDigestTime(&ctx, HUGE_CHUNKS, SHA224_ALG);
  
     return PS_SUCCESS;
  return PS_SUCCESS;
}
...

This is a copy-paste error. There are two identical lines at the end: return PS_SUCCESS;.

Another typo of this kind can be found in the function psSha384Test(void).

Memory leak

Memory leak: table

This issue is non-critical in this case but it's nice to see that Cppcheck can catch it. The code is inside files and looks as follows (copy-paste):

  • crypto\test\eccperf\eccperf.c
  • crypto\test\rsaperf\rsaperf.c
...
  table = malloc(tsize * sizeof(uint32));  
  if ((sfd = fopen("perfstat.txt", "w")) == NULL) {
    return PS_FAILURE;
  }
...

Resources are better to be requested right before they are really necessary. If you look at the code in those files, you will see that the table is not used at all, that is, the call of the malloc() function as well as the call of the free(table) function at the end are just excessive.

Conclusion

I am a FlylinkDC++ developer and I've been using the PVS-Studio analyzer granted to us as an open-source project for more than two years now. The analyzer more than once helped us find various bugs both in our own code and third-party libraries' code. Thanks to regular checks, FlylinkDC++'s code has become much more stable and safe. And that's wonderful!



Use PVS-Studio to search for bugs in C, C++, and C# code

We offer you to check your project code with PVS-Studio. Just one bug found in the project will show you the benefits of the static code analysis methodology better than a dozen of the articles.

goto PVS-Studio;

Pavel Pimenov
Articles: 1


Do you make errors in the code?

Check your code
with PVS-Studio

Static code analysis
for C, C++, and C#

goto PVS-Studio;
On our website we use a cookie to collect information of a technical nature.
If you do not agree, please leave the site. Learn More →
Do not show again