Проверяем MatrixSSL с помощью PVS-Studio и Сppcheck

В статье я хочу рассказать о проверке проекта MatrixSSL статическими анализаторами C/C++ PVS-Studio и Cppcheck.
О библиотеке узнал из комментария ^[1] на сайте Хабрахабр.

Порадовало наличие «из коробки» проекта для MS Visual Studio 2010.

Например, чтобы собрать openSSL из исходников в Visual C++ нужно немного поприседать с бубном :). Поэтому многие разработчики под windows используют готовые бинарные сборки openSSL, такие как Win32 OpenSSL Installation Project ^[2].

MatrixSSL ^[3] — альтернативная библиотека алгоритмов шифрования распространяемым под лицензией GNU (также возможно получение коммерческой поддержки).

Исходный код open source версии можно получить на официальном сайте. анализу подвергалась версия актуальная версия 3.7.1 на момент анализа matrixssl-3-7-1-open.tgz ^[4].

Об анализаторе

• PVS-Studio ^[5] — коммерческий статический анализатор, выявляющий ошибки в исходном коде приложений на языке C/C++/C++11. (использовалась версия PVS-Studio 5.21).
• Cppcheck ^[6] — бесплатный статический анализатор с открытым исходным кодом (использовалась версия Cppcheck 1.68).

Результаты проверки в PVS-Studio

Зачистка памяти

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

...
// cryptodigestdigest.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 ^[8] 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 ^[9] 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 ^[10] 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 ^[11] 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 %sn", (char*)fileName);
        return PS_PLATFORM_FAIL;
...

Опечатка: вместо FILE_SHARE_READ | FILE_SHARE_WRITE записали && получилось 1 && 2 == 1

что эквивалентно одному FILE_SHARE_READ.

Возможно ошибочное условие

V590 ^[12] 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 ^[13] 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 вообще уберет вызов функции и подставит значение константы вычисленное на этапе компиляции.

Результаты проверки в Cppcheck

Этот анализатор показал меньше предупреждений, но есть те которые PVS-Studio не смог детектировать.

Все они на работу библиотеки не влияют так как лежат в юнит-тестах относятся в cryptotest.

«Контрольный return в голову»

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):

• cryptotesteccperfeccperf.c
• cryptotestrsaperfrsaperf.c

...
  table = malloc(tsize * sizeof(uint32));  
  if ((sfd = fopen("perfstat.txt", "w")) == NULL) {
    return PS_FAILURE;
  }
...

Ресурсы лучше заказывать перед тем когда они действительно необходимы. Если посмотреть код в этих файлах, то окажется что table вообще не используется, т.е. тут вызов функции malloc() и в конце функции free(table) лишние.

Заключение

Я явлюсь разработчиком FlylinkDC++ ^[14] и уже более двух лет использую анализатор PVS-Studio, который нам подарили как Open Source проекту. Анализатор неоднократно помогал локализовать различные ошибки, как в своем коде, так и в коде сторонних библиотек. Благодаря регулярным проверкам код FlylinkDC++ стал немного надёжней и безопасней. И это замечательно.

Автор: pavel_pimenov

Источник ^[15]