В статье я хочу рассказать о проверке проекта MatrixSSL статическими анализаторами C/C++ PVS-Studio и Cppcheck.
Статья написана Павлом Пименовым, являющимся автором открытого peer-to-peer клиента FlylinkDC++. Статья публикуется в нашем блоге с его разрешения.
Порадовало наличие "из коробки" проекта для MS Visual Studio 2010.
Например, чтобы собрать openSSL из исходников в Visual C++ нужно немного поприседать с бубном :). Поэтому многие разработчики под windows используют готовые бинарные сборки openSSL, такие как Win32 OpenSSL Installation Project.
MatrixSSL - альтернативная библиотека алгоритмов шифрования распространяемым под лицензией GNU (также возможно получение коммерческой поддержки).
Исходный код open source версии можно получить на официальном сайте. Анализу подвергалась актуальная версия 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;
}
...По коду в этих трех функция все корректно и затирается только часть массива, которая используется, но анализатор подсказывает о возможно избыточном размере заказанного буфер размером 128 байт.
Думаю, здесь всё хорошо, но всё равно лучше для красоты затирать 64 или 128 байт. Можно написать так:
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;
}
...Оптимизатор выкидывает вызов стандартной функции memset(). Для библиотеки шифрования вероятно это критично и является потенциальной дырой.
Аналогичные проблемные места: 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 объявлена как "1". В MSDN про возврат функции QueryPerformanceFrequency написано "If the installed hardware supports a high-resolution performance counter, the return value is nonzero" Т.е. надежнее написать 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;
}
...Тут явная ошибка: условие никогда не выполнится так как sessionId объявлен как массив из 32 байт и у него не может быть адреса равного NULL. Ошибка конечно не серьезная. Пожалуй, это можно назвать и просто лишней бессмысленной проверкой.
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;
...Опечатка: вместо FILE_SHARE_READ | FILE_SHARE_WRITE записали && получилось 1 && 2 == 1
что эквивалентно одному 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) {
...Тут анализатор внутри цикла while() обнаружил вызов strlen() для параметра который не меняется, в общем случае это не оптимально, но в данном конкретном т.к. на вход strlen() передается константа известная на этапе компиляции, то оптимизатор в режиме /O2 вообще уберет вызов функции и подставит значение константы вычисленное на этапе компиляции.
Этот анализатор показал меньше предупреждений, но есть те которые PVS-Studio не смог детектировать.
Все они на работу библиотеки не влияют так как лежат в юнит-тестах относятся в 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;
}
...Ошибка copy-paste. В конце две одинаковых строчки: return PS_SUCCESS;.
Аналогичная опечатка находится в функции psSha384Test(void).
Memory leak: table
Неопасная в данном случае ошибка, но приятно что Cppcheck ее ловит код находится в файлах и выглядит так (copy-paste):
...
table = malloc(tsize * sizeof(uint32));
if ((sfd = fopen("perfstat.txt", "w")) == NULL) {
return PS_FAILURE;
}
...Ресурсы лучше заказывать перед тем когда они действительно необходимы. Если посмотреть код в этих файлах, то окажется что table вообще не используется, т.е. тут вызов функции malloc() и в конце функции free(table) лишние.
Я явлюсь разработчиком FlylinkDC++ и уже более двух лет использую анализатор PVS-Studio, который нам подарили как Open Source проекту. Анализатор неоднократно помогал локализовать различные ошибки, как в своем коде, так и в коде сторонних библиотек. Благодаря регулярным проверкам код FlylinkDC++ стал немного надёжней и безопасней. И это замечательно.