February 2, 2015 at 16:03Checking MatrixSSL with PVS-Studio and Сppcheck
In the article I want to talk about checking the MatrixSSL project with the static analyzers C / C ++ PVS-Studio and Cppcheck. I learned about the library from a comment on the Habrahabr website.
I was pleased with the out of the box project for MS Visual Studio 2010.
For example, to build openSSL from the source in Visual C ++, you need to squat a little with a tambourine :). Therefore, many developers for windows use ready-made openSSL binaries, 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).
The source code of the open source version can be obtained on the official website. The latest version 3.7.1 was analyzed at the time of analysis 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. (used version of PVS-Studio 5.21).
- Cppcheck is a free, open source static analyzer (Cppcheck version 1.68 was used).
Test Results in PVS-Studio
Memory stripping
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;
}
...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 tells you about the possibly excess size of the ordered buffer of 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 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 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 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". In MSDN, about the return of the QueryPerformanceFrequency function, it says "If the installed hardware supports a high-resolution performance counter, the return value is nonzero" it’s more reliable 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 a clear error: the condition will never be fulfilled since sessionId is declared as an array of 32 bytes and it cannot have an address equal to NULL. The mistake is certainly not serious. Perhaps this can be called just an extra 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;
...Typo: instead of FILE_SHARE_READ | FILE_SHARE_WRITE wrote && it 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 excessive or contains a 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 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) {
...Here, the analyzer inside the while () loop found a strlen () call for a parameter that does not change, in the general case this is not optimal, but in this particular case the constant known at the compilation stage is passed to the input of strlen (), then the optimizer in the / O2 mode will generally remove the function call and substitute the constant value calculated at the compilation stage.
Check Results in Cppcheck
This analyzer showed fewer warnings, but there are those that PVS-Studio could not detect.
All of them do not affect the operation of the library since they are in unit tests and are in crypto \ test.
"Control return to 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;
}
...Copy-paste error. At the end, two identical lines: return PS_SUCCESS ;.
A similar typo is in the psSha384Test (void) function.
Memory leak
Memory leak: table A
non-dangerous error 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 truly needed. If you look at the code in these files, it turns out that table is not used at all, i.e. here the call to the malloc () function and at the end of the free (table) function are unnecessary.
Conclusion
I am a FlylinkDC ++ developer and have been using the PVS-Studio analyzer for more than two years, 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 little more reliable and secure. And that's great.