ProHoster > блог > адміністраванне > Праверка FreeRDP з дапамогай аналізатара PVS-Studio

Праверка FreeRDP з дапамогай аналізатара PVS-Studio

Праверка FreeRDP з дапамогай аналізатара PVS-Studio
FreeRDP - адкрытая рэалізацыя Remote Desktop Protocol (RDP), пратаколу, які рэалізуе выдаленае кіраванне кампутарам, распрацаванага кампаніяй Microsoft. Праект падтрымлівае мноства платформ, сярод якіх Windows, Linux, macOS і нават iOS з Android. Гэты праект абраны першым у рамках цыклу артыкулаў, прысвечаных праверцы RDP-кліентаў з дапамогай статычнага аналізатара PVS-Studio.

Трохі гісторыі

Праект FreeRDP з'явіўся пасля таго, як Microsoft адкрыла спецыфікацыі свайго прапрыетарнага пратакола RDP. На той момант існаваў кліент rdesktop, рэалізацыя якога грунтуецца на выніках Reverse Engineering.

У працэсе рэалізацыі пратакола станавілася больш складана дадаваць новы функцыянал з-за існаваўшай тады архітэктуры праекта. Змены ў ёй спарадзілі канфлікт паміж распрацоўшчыкамі, што прывяло да стварэння форка rdesktop – FreeRDP. Далейшае распаўсюджванне прадукта было абмежавана ліцэнзіяй GPLv2, у выніку чаго было прынята рашэнне аб рэліцэнзаванні на Apache License v2. Аднак не ўсе былі згодны мяняць ліцэнзію свайго кода, таму распрацоўшчыкі вырашылі перапісаць праект, у выніку чаго мы маем сучасны від кодавай базы.

Больш падрабязна пра гісторыю праекта можна прачытаць у нататцы афіцыйнага блога: "The history of the FreeRDP project".

У якасці прылады для выяўлення памылак і патэнцыйных уразлівасцяў у кодзе выкарыстоўваўся PVS-студыя. Гэта статычны аналізатар кода для моваў C, C++, C# і Java, даступны на платформах Windows, Linux і macOS.

У артыкуле прадстаўлены толькі тыя памылкі, якія падаліся мне найболей цікавымі.

Уцечка памяці

V773 Функцыя была завершана без выканання '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, вызваляецца толькі пры апрацоўцы спецыяльных выпадкаў. Для ўхілення памылкі трэба дадаць выклік бясплатна пасля memcpy.

Выхад за межы масіва

V557 Array overrun is possible. Вага '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. Вага 'iBitmapFormat' index could reach 8. orders.c 2623

памылкі друку

Фрагмент 1

V547 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 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 Гэта адпачынак, які функцыі функцыі 'trio_index_last' з'яўляецца цалкам equivalible функцыяй 'trio_index'. 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 The 'data' pointer in expression equals nullptr. Паказвае значэнне арытметычных дзеянняў на гэтым плане не мае патрэбы і не павінна быць выкарыстана. 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;
  ....
}

Падобна, тут выпадкова прапусцілі аператар адмаўлення ! побач з gegevens. Дзіўна, што гэта засталося незаўважаным.

Фрагмент 5

V517 use of 'if (A) {…} else if (A) {…}' pattern was detected. Там ёсць верагоднасць лагічнай рашучасці. 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 З дапамогай 'if (A) {…} else if (A) {…}' pattern was detected. Там ёсць верагоднасць лагічнай рашучасці. Check lines: 169, 173. file.c 169

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

Фрагмент 1

V547 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 вяртае паказальнік на канчатковы варыянт радка, г.зн. першы перададзены параметр. У дадзеным выпадку гэта мішэнь. Аднак калі ён роўны NULL, то правяраць яго позна, бо ў функцыі strcat адбудзецца яго разнайменаванне.

Фрагмент 2

V547 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)
  {
    ....
  }
  ....
}

У гэтым выпадку зменнай кэш прысвойваецца адрас статычнага масіва glyphCache->glyphCache. Такім чынам, праверку if (cache) можна апусціць.

Памылка кіравання рэсурсамі

V1005 The resource была запатрабаваная шляхам 'CreateFileA' функцыі, але была выканана шляхам запуску '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 Дадатковыя expressions of 'if' statements размешчаны вакол сябе ўсе іншыя 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 null pointer is passed in '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);
  ....
}

У функцыю бясплатна можна перадаваць нулявы паказальнік і аналізатар пра гэта ведае. Але калі выяўляецца сітуацыя, пры якой паказальнік заўсёды перадаецца нулявым, як у гэтым фрагменце, будзе выдадзена папярэджанне.

паказальнік mszGroupsA першапачаткова роўны NULL і больш нідзе не ініцыялізуецца. Адзіная галіна кода, дзе паказальнік мог ініцыялізавацца, з'яўляецца недасяжнай.

Былі і іншыя паведамленні гэта тыпу:

  • V575 null pointer з'яўляецца простым 'function'. Inspect the first argument. license.c 790
  • V575 null pointer з'яўляецца 'праца' функцый. Inspect the first argument. rdpsnd_alsa.c 575

Хутчэй за ўсё, падобныя забытыя зменныя ўзнікаюць падчас рэфактарынгу і іх можна проста выдаліць.

Магчымае перапаўненне

V1028 Possible overflow. Consider casting operands, не лічыцца. 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));
  ....
}

Прывядзенне выніку да доўга не з'яўляецца абаронай ад перапаўнення, бо само вылічэнне адбываецца з выкарыстаннем тыпу INT.

Разнайменне паказальніка ў ініцыялізацыі

V595 The 'context' pointer быў выкарыстаны да таго, што быў зняволены да 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;
  ....
}

Тут паказальнік кантэкст разназываецца ў ініцыялізацыі - раней, чым адбываецца яго праверка.

Былі знойдзены і іншыя памылкі такога тыпу:

  • V595 The 'ntlm' pointer быў выкарыстаны перад тым, што ён быў зняволены да nullptr. Check lines: 236, 255. ntlm.c 236
  • V595 The 'context' pointer быў выкарыстаны перад тым, што ён быў зняволены да nullptr. Check lines: 1003, 1007. rfx.c 1003
  • V595 The 'rdpei' pointer быў выкарыстаны перад тым, што ён быў зняволены да nullptr. Check lines: 176, 180. rdpei_main.c 176
  • V595 The 'gdi' pointer быў выкарыстаны перад тым, што ён быў зняволены да nullptr. Check lines: 121, 123. xf_gfx.c 121

Бессэнсоўная ўмова

V547 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 Incorrect format. Паняцце, якое выконвае 220-дзённы argument функцыі 'sscanf'. Пытанне да ненадпісанага тыпу тыпу з'яўляецца выяўленым. proxy.c XNUMX

V560 Apart 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, Але зменная да поўдня мае тып INT. Далей мы бачым падазроную праверку: умова справа не мае сэнсу, бо напачатку ідзе параўнанне з адзінкай. Не ведаю, што меў на ўвазе аўтар гэтага кода, але тут відавочна нешта не так.

Неўпарадкаваныя праверкі

V547 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-студыя. Былі таксама і ілжывыя спрацоўванні, праца над якімі дапаможа палепшыць аналізатар. Тым не менш, статычны аналіз важны, калі вы хочаце не толькі павысіць якасць кода, але і скараціць час на пошук памылак, і PVS-Studio можа ў гэтым дапамагчы.

Праверка FreeRDP з дапамогай аналізатара PVS-Studio

Калі хочаце падзяліцца гэтым артыкулам з англамоўнай аўдыторыяй, то прашу выкарыстаць спасылку на пераклад: Sergey Larin. Checking FreeRDP with PVS-Studio

Крыніца: habr.com

Дадаць каментар