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.
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.
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.
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;
...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.
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.
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: 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):
...
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.
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!