FreeRDP – свободная реализация клиента Remote Desktop Protocol (RDP), протокола, реализующего удаленное управление компьютером, разработанного компанией Microsoft. Проект поддерживает множество платформ, среди которых Windows, Linux, macOS и даже iOS с Android. Этот проект выбран первым в рамках цикла статей, посвященных проверке RDP-клиентов с помощью статического анализатора PVS-Studio.

Немного истории

Проект FreeRDP ^[1] появился после того, как Microsoft открыла спецификации своего проприетарного протокола RDP. На тот момент существовал клиент rdesktop, реализация которого базируется на результатах Reverse Engineering.

В процессе реализации протокола становилось сложнее добавлять новый функционал из-за существовавшей тогда архитектуры проекта. Изменения в ней породили конфликт между разработчиками, что привело к созданию форка rdesktop — FreeRDP. Дальнейшее распространение продукта было ограничено лицензией GPLv2, в результате чего было принято решение о релицензировании на Apache License v2. Однако не все были согласны менять лицензию своего кода, поэтому разработчики решили переписать проект, в результате чего мы имеем современный вид кодовой базы.

Более подробно об истории проекта можно прочесть в заметке официального блога: «The history of the FreeRDP project».

В качестве инструмента для выявления ошибок и потенциальных уязвимостей в коде использовался PVS-Studio ^[2]. Это статический анализатор кода для языков C, C++, C# и Java, доступный на платформах Windows, Linux и macOS.

В статье представлены лишь те ошибки, которые показались мне наиболее интересными.

Утечка памяти

V773 ^[3] The function was exited without releasing the 'cwd' pointer. A memory leak is possible. environment.c 84

DWORD GetCurrentDirectoryA(DWORD nBufferLength, LPSTR lpBuffer)
{
  char* cwd;
  ....
  cwd = getcwd(NULL, 0);
  ....
  if (lpBuffer == NULL)
  {
    free(cwd);
    return 0;
  }

  if ((length + 1) > nBufferLength)
  {
    free(cwd);
    return (DWORD) (length + 1);
  }

  memcpy(lpBuffer, cwd, length + 1);
  return length;
  ....
}

Данный фрагмент был взят из подсистемы winpr, реализующей обертку WINAPI для не-Windows систем, т.е. это легкий аналог Wine. Здесь можно заметить утечку: память, выделенная функцией getcwd, освобождается только при обработке специальных случаев. Для устранения ошибки нужно добавить вызов free после memcpy.

Выход за границы массива

V557 ^[4] Array overrun is possible. The value of 'event->EventHandlerCount' index could reach 32. PubSub.c 117

#define MAX_EVENT_HANDLERS  32

struct _wEventType
{
  ....
  int EventHandlerCount;
  pEventHandler EventHandlers[MAX_EVENT_HANDLERS];
};

int PubSub_Subscribe(wPubSub* pubSub, const char* EventName,
      pEventHandler EventHandler)
{
  ....
  if (event->EventHandlerCount <= MAX_EVENT_HANDLERS)
  {
    event->EventHandlers[event->EventHandlerCount] = EventHandler;
    event->EventHandlerCount++;
  }
  ....
}

В этом примере новый элемент добавляется в список, даже если количество элементов достигло максимального. Здесь достаточно заменить оператор <= на <, чтобы не выходить за границы массива.

Была найдена и другая ошибка такого типа:

• V557 Array overrun is possible. The value of 'iBitmapFormat' index could reach 8. orders.c 2623

Опечатки

Фрагмент 1

V547 ^[5] Expression '!pipe->In' is always false. MessagePipe.c 63

wMessagePipe* MessagePipe_New()
{
  ....
  pipe->In = MessageQueue_New(NULL);
  if (!pipe->In)
    goto error_in;

  pipe->Out = MessageQueue_New(NULL);
  if (!pipe->In) // <=
    goto error_out;
  ....
}

Тут мы видим обычную опечатку: во втором условии выполняется проверка той же переменной, что и в первом. Скорее всего, ошибка появилась в результате неудачного копирования кода.

Фрагмент 2

V760 ^[6] Two identical blocks of text were found. The second block begins from line 771. tsg.c 770

typedef struct _TSG_PACKET_VERSIONCAPS
{
  ....
  UINT16 majorVersion;
  UINT16 minorVersion;
  ....
} TSG_PACKET_VERSIONCAPS, *PTSG_PACKET_VERSIONCAPS;

static BOOL TsProxyCreateTunnelReadResponse(....)
{
  ....
  PTSG_PACKET_VERSIONCAPS versionCaps = NULL;
  ....
  /* MajorVersion (2 bytes) */
  Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
  /* MinorVersion (2 bytes) */
  Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
  ....
}

Еще одна опечатка: комментарий к коду подразумевает, что из потока должна прийти minorVersion, однако считывание происходит в переменную с именем majorVersion. Тем не менее, я не знаком с протоколом, так что это лишь предположение.

Фрагмент 3

V524 ^[7] It is odd that the body of 'trio_index_last' function is fully equivalent to the body of 'trio_index' function. triostr.c 933

/**
   Find first occurrence of a character in a string.
   ....
 */
TRIO_PUBLIC_STRING char *
trio_index
TRIO_ARGS2((string, character),
     TRIO_CONST char *string,
     int character)
{
  assert(string);
  return strchr(string, character);
}

/**
   Find last occurrence of a character in a string.
   ....
 */
TRIO_PUBLIC_STRING char *
trio_index_last
TRIO_ARGS2((string, character),
     TRIO_CONST char *string,
     int character)
{
  assert(string);
  return strchr(string, character);
}

Судя по комментарию, функция trio_index находит первое совпадение символа в строке, когда trio_index_last — последнее. Но тела этих функций идентичны! Скорее всего, это опечатка, и в функции trio_index_last нужно использовать strrchr вместо strchr. Тогда поведение будет ожидаемым.

Фрагмент 4

V769 ^[8] The 'data' pointer in the expression equals nullptr. The resulting value of arithmetic operations on this pointer is senseless and it should not be used. nsc_encode.c 124

static BOOL nsc_encode_argb_to_aycocg(NSC_CONTEXT* context,
                                      const BYTE* data,
                                      UINT32 scanline)
{
  ....
  if (!context || data || (scanline == 0))
    return FALSE;
  ....
  src = data + (context->height - 1 - y) * scanline;
  ....
}

Похоже, здесь случайно пропустили оператор отрицания ! рядом с data. Странно, что это осталось незамеченным.

Фрагмент 5

V517 ^[9] The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 213, 222. rdpei_common.c 213

BOOL rdpei_write_4byte_unsigned(wStream* s, UINT32 value)
{
  BYTE byte;

  if (value <= 0x3F)
  {
    ....
  }
  else if (value <= 0x3FFF)
  {
    ....
  }
  else if (value <= 0x3FFFFF)
  {
    byte = (value >> 16) & 0x3F;
    Stream_Write_UINT8(s, byte | 0x80);
    byte = (value >> 8) & 0xFF;
    Stream_Write_UINT8(s, byte);
    byte = (value & 0xFF);
    Stream_Write_UINT8(s, byte);
  }
  else if (value <= 0x3FFFFF)
  {
    byte = (value >> 24) & 0x3F;
    Stream_Write_UINT8(s, byte | 0xC0);
    byte = (value >> 16) & 0xFF;
    Stream_Write_UINT8(s, byte);
    byte = (value >> 8) & 0xFF;
    Stream_Write_UINT8(s, byte);
    byte = (value & 0xFF);
    Stream_Write_UINT8(s, byte);
  }
  ....
}

Последние два условия одинаковы: видимо, кто-то забыл проверить их после копирования. По коду заметно, что последняя часть работает с четырехбайтными значениями, поэтому можно предположить, что последнее условие должно быть value <= 0x3FFFFFFF.

Была найдена и другая ошибка такого типа:

• V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 169, 173. file.c 169

Проверка входных данных

Фрагмент 1

V547 ^[5] Expression 'strcat(target, source) != NULL' is always true. triostr.c 425

TRIO_PUBLIC_STRING int
trio_append
TRIO_ARGS2((target, source),
     char *target,
     TRIO_CONST char *source)
{
  assert(target);
  assert(source);
  
  return (strcat(target, source) != NULL);
}

Проверка результата выполнения функции в этом примере некорректна. Функция strcat возвращает указатель на конечный вариант строки, т.е. первый переданный параметр. В данном случае это target. Однако если он равен NULL, то проверять его поздно, так как в функции strcat произойдёт его разыменование.

Фрагмент 2

V547 ^[5] Expression 'cache' is always true. glyph.c 730

typedef struct rdp_glyph_cache rdpGlyphCache;

struct rdp_glyph_cache
{
  ....
  GLYPH_CACHE glyphCache[10];
  ....
};

void glyph_cache_free(rdpGlyphCache* glyphCache)
{
  ....
  GLYPH_CACHE* cache = glyphCache->glyphCache;

  if (cache)
  {
    ....
  }
  ....
}

В этом случае переменной cache присваивается адрес статического массива glyphCache->glyphCache. Таким образом, проверку if (cache) можно опустить.

Ошибка управления ресурсами

V1005 ^[10] The resource was acquired using 'CreateFileA' function but was released using incompatible 'fclose' function. certificate.c 447

BOOL certificate_data_replace(rdpCertificateStore* certificate_store,
                              rdpCertificateData* certificate_data)
{
  HANDLE fp;
  ....
  fp = CreateFileA(certificate_store->file, GENERIC_READ | GENERIC_WRITE, 0,
                   NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
  ....
  if (size < 1)
  {
    CloseHandle(fp);
    return FALSE;
  }
  ....
  if (!data)
  {
    fclose(fp);
    return FALSE;
  }
  ....
}

Дескриптор файла fp, созданный вызовом функции CreateFile, по ошибке закрыт функцией fclose из стандартной библиотеки, а не CloseHandle.

Одинаковые условия

V581 ^[11] The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 269, 283. ndr_structure.c 283

void NdrComplexStructBufferSize(PMIDL_STUB_MESSAGE pStubMsg,
      unsigned char* pMemory, PFORMAT_STRING pFormat)
{
  ....
  if (conformant_array_description)
  {
    ULONG size;
    unsigned char array_type;
    array_type = conformant_array_description[0];
    size = NdrComplexStructMemberSize(pStubMsg, pFormat);
    WLog_ERR(TAG, "warning: NdrComplexStructBufferSize array_type: "
      "0x%02X unimplemented", array_type);
    NdrpComputeConformance(pStubMsg, pMemory + size,
      conformant_array_description);
    NdrpComputeVariance(pStubMsg, pMemory + size,
      conformant_array_description);
    MaxCount = pStubMsg->MaxCount;
    ActualCount = pStubMsg->ActualCount;
    Offset = pStubMsg->Offset;
  }

  if (conformant_array_description)
  {
    unsigned char array_type;
    array_type = conformant_array_description[0];
    pStubMsg->MaxCount = MaxCount;
    pStubMsg->ActualCount = ActualCount;
    pStubMsg->Offset = Offset;
    WLog_ERR(TAG, "warning: NdrComplexStructBufferSize array_type: "
      "0x%02X unimplemented", array_type);
  }
  ....
}

Возможно, этот пример не является ошибкой. Однако оба условия содержат одинаковые сообщения, одно из которых, скорее всего, можно убрать.

Очистка нулевых указателей

V575 ^[12] The null pointer is passed into 'free' function. Inspect the first argument. smartcard_pcsc.c 875

WINSCARDAPI LONG WINAPI PCSC_SCardListReadersW(
  SCARDCONTEXT hContext,
  LPCWSTR mszGroups,
  LPWSTR mszReaders,
  LPDWORD pcchReaders)
{
  LPSTR mszGroupsA = NULL;
  ....
  mszGroups = NULL; /* mszGroups is not supported by pcsc-lite */

  if (mszGroups)
    ConvertFromUnicode(CP_UTF8,0, mszGroups, -1, 
                       (char**) &mszGroupsA, 0,
                       NULL, NULL);

  status = PCSC_SCardListReaders_Internal(hContext, mszGroupsA,
                                          (LPSTR) &mszReadersA,
                                          pcchReaders);

  if (status == SCARD_S_SUCCESS)
  {
    ....
  }

  free(mszGroupsA);
  ....
}

В функцию free можно передавать нулевой указатель и анализатор об этом знает. Но если выявляется ситуация, при которой указатель всегда передаётся нулевым, как в этом фрагменте, будет выдано предупреждение.

Указатель mszGroupsA изначально равен NULL и больше нигде не инициализируется. Единственная ветвь кода, где указатель мог инициализироваться, является недостижимой.

Были и другие сообщения это типа:

• V575 The null pointer is passed into 'free' function. Inspect the first argument. license.c 790
• V575 The null pointer is passed into 'free' function. Inspect the first argument. rdpsnd_alsa.c 575

Скорее всего, подобные забытые переменные возникают в процессе рефакторинга и их можно просто удалить.

Возможное переполнение

V1028 ^[13] Possible overflow. Consider casting operands, not the result. makecert.c 1087

// openssl/x509.h
ASN1_TIME *X509_gmtime_adj(ASN1_TIME *s, long adj);

struct _MAKECERT_CONTEXT
{
  ....
  int duration_years;
  int duration_months;
};

typedef struct _MAKECERT_CONTEXT MAKECERT_CONTEXT;

int makecert_context_process(MAKECERT_CONTEXT* context, ....)
{
  ....
  if (context->duration_months)
    X509_gmtime_adj(after, (long)(60 * 60 * 24 * 31 *
      context->duration_months));
  else if (context->duration_years)
    X509_gmtime_adj(after, (long)(60 * 60 * 24 * 365 *
      context->duration_years));
  ....
}

Приведение результата к long не является защитой от переполнения, так как само вычисление происходит с использованием типа int.

Разыменование указателя в инициализации

V595 ^[14] The 'context' pointer was utilized before it was verified against nullptr. Check lines: 746, 748. gfx.c 746

static UINT gdi_SurfaceCommand(RdpgfxClientContext* context,
                               const RDPGFX_SURFACE_COMMAND* cmd)
{
  ....
  rdpGdi* gdi = (rdpGdi*) context->custom;

  if (!context || !cmd)
    return ERROR_INVALID_PARAMETER;
  ....
}

Здесь указатель context разыменовывается в инициализации — раньше, чем происходит его проверка.

Были найдены и другие ошибки такого типа:

• V595 The 'ntlm' pointer was utilized before it was verified against nullptr. Check lines: 236, 255. ntlm.c 236
• V595 The 'context' pointer was utilized before it was verified against nullptr. Check lines: 1003, 1007. rfx.c 1003
• V595 The 'rdpei' pointer was utilized before it was verified against nullptr. Check lines: 176, 180. rdpei_main.c 176
• V595 The 'gdi' pointer was utilized before it was verified against nullptr. Check lines: 121, 123. xf_gfx.c 121

Бессмысленное условие

V547 ^[5] Expression 'rdp->state >= CONNECTION_STATE_ACTIVE' is always true. connection.c 1489

int rdp_server_transition_to_state(rdpRdp* rdp, int state)
{
  ....
  switch (state)
  {
    ....
    case CONNECTION_STATE_ACTIVE:
      rdp->state = CONNECTION_STATE_ACTIVE;          // <=
      ....
      if (rdp->state >= CONNECTION_STATE_ACTIVE)     // <=
      {
        IFCALLRET(client->Activate, client->activated, client);

        if (!client->activated)
          return -1;
      }
    ....
  }
  ....
}

Легко заметить, что первое условие не имеет смысла из-за присваивания соответствующего значения ранее.

Некорректный разбор строки

V576 ^[15] Incorrect format. Consider checking the third actual argument of the 'sscanf' function. A pointer to the unsigned int type is expected. proxy.c 220

V560 ^[16] A part of conditional expression is always true: (rc >= 0). proxy.c 222

static BOOL check_no_proxy(....)
{
  ....
  int sub;
  int rc = sscanf(range, "%u", &sub);

  if ((rc == 1) && (rc >= 0))
  {
    ....
  }
  ....
}

Анализатор для этого фрагмента выдает сразу 2 предупреждения. Спецификатор %u ожидает переменную типа unsigned int, но переменная sub имеет тип int. Далее мы видим подозрительную проверку: условие справа не имеет смысла, так как в начале идет сравнение с единицей. Не знаю, что имел в виду автор этого кода, но тут явно что-то не так.

Неупорядоченные проверки

V547 ^[5] Expression 'status == 0x00090314' is always false. ntlm.c 299

BOOL ntlm_authenticate(rdpNtlm* ntlm, BOOL* pbContinueNeeded)
{
  ....
  if (status != SEC_E_OK)
  {
    ....
    return FALSE;
  }

  if (status == SEC_I_COMPLETE_NEEDED)            // <=
    status = SEC_E_OK;
  else if (status == SEC_I_COMPLETE_AND_CONTINUE) // <=
    status = SEC_I_CONTINUE_NEEDED;
  ....
}

Отмеченные условия будут всегда ложны, так как выполнение дойдет до второго условия только в том случае, когда status == SEC_E_OK. Правильный код может выглядеть так:

if (status == SEC_I_COMPLETE_NEEDED)
  status = SEC_E_OK;
else if (status == SEC_I_COMPLETE_AND_CONTINUE)
  status = SEC_I_CONTINUE_NEEDED;
else if (status != SEC_E_OK)
{
  ....
  return FALSE;
}

Заключение

Таким образом, проверка проекта выявила множество проблем, но только наиболее интересная их часть была описана в статье. Разработчики проекта могут сами проверить проект, запросив временный ключ лицензии на сайте PVS-Studio ^[17]. Были также и ложные срабатывания, работа над которыми поможет улучшить анализатор. Тем не менее, статический анализ важен, если вы хотите не только повысить качество кода, но и сократить время на поиск ошибок, и PVS-Studio может в этом помочь.

^[18]

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Sergey Larin. Checking FreeRDP with PVS-Studio ^[18]

Автор: cerg2010cerg2010

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