Checking MatrixSSL with PVS-Studio and Cppcheck

In the article I want to talk about the verification of the MatrixSSL project with C / C ++ static analyzers PVS-Studio and Cppcheck.

I learned about the library from the comment on the Habrahabr website.



Pleased with the presence of "out of the box" project for MS Visual Studio 2010.



For example, to build openSSL from source in Visual C ++, you need to popress a little with a tambourine :). Therefore, many windows developers use openSSL binary builds, such as the Win32 OpenSSL Installation Project .



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

')

Source code open source version can be obtained on the official website. The current version 3.7.1 was analyzed at the time of the analysis of matrixssl-3-7-1-open.tgz .

About the analyzer

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

Test results in PVS-Studio

Memory sweep

V512 A call of the memset function will lead to underflow of the 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; } ... 

According to the code in these three functions, everything is correct and only the part of the array that is used is overwritten, but the analyzer suggests that the buffer to be ordered is 128 bytes in size.



I think everything is fine here, but it's still better to overwrite 64 or 128 bytes for beauty. You can write this:

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

V597 The compiler couldn’t 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 a call to the standard memset () function. For the encryption library, this is probably critical and is a potential hole.



Similar problem areas: 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 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”. In MSDN, about the return of the QueryPerformanceFrequency function is written “If the installed hardware supports a high-resolution performance counter, the return value is nonzero” That is, safer to write 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 is an obvious error: the condition will never be executed as sessionId is declared as an array of 32 bytes and it cannot have an address equal to NULL. The error is certainly not serious. Perhaps this can be called simply a senseless check.



V560 A part of the 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; ... 

Typo: instead of FILE_SHARE_READ | FILE_SHARE_WRITE recorded && turned out 1 && 2 == 1



which is equivalent to one FILE_SHARE_READ.

Possible erroneous condition

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

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

Possible performance drawdown

V814 Decreased performance. The 'strlen' function of the 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) { ... 

Here, the analyzer inside the while () loop detected a call to strlen () for a parameter that does not change, in general it is not optimal, but in this particular case. the constant known at the compilation stage is passed to the strlen () input, then the optimizer in the / O2 mode will generally remove the function call and substitute the constant value calculated at the compilation stage.

Cppcheck check results

This analyzer showed fewer warnings, but there are those that PVS-Studio could not detect.



All of them do not affect the work of the library as they lie in the unit tests are in crypto \ test.

"Control return to the head"

Consecutive return, break, continue, goto or throw statements are unnecessary. So it can be removed.

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

Copy-paste error. At the end of two identical lines: return PS_SUCCESS ;.



A similar typo is in the psSha384Test (void) function.

Memory leak

Memory leak: table



The error is safe in this case, but it's nice that Cppcheck catches it; the code is in the files and looks like this (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 best ordered before they are really needed. If you look at the code in these files, it turns out that the table is not used at all, i.e. then the function call malloc () and at the end of the free (table) function are superfluous.

Conclusion

I am a developer of FlylinkDC ++ and for more than two years I have been using the PVS-Studio analyzer, which was presented to us as an Open Source project. The analyzer has repeatedly helped to localize various errors, both in its code and in the code of third-party libraries. Thanks to regular checks, the FlylinkDC ++ code has become a bit more reliable and safer. And that's great.