ProHoster > blog > Amministrazzjoni > Iċċekkjar FreeRDP bl-użu tal-analizzatur PVS-Studio

Iċċekkjar FreeRDP bl-użu tal-analizzatur PVS-Studio

Iċċekkja FreeRDP billi tuża l-analizzatur PVS-Studio
FreeRDP hija implimentazzjoni open-source tal-Remote Desktop Protocol (RDP), protokoll żviluppat minn Microsoft għall-kontroll remot tal-kompjuter. Il-proġett jappoġġja diversi pjattaformi, inklużi Windows, Linux, macOS u anke iOS ma' AndroidDan il-proġett intgħażel bħala l-ewwel wieħed f'sensiela ta' artikli ddedikati għall-ittestjar tal-klijenti RDP bl-użu tal-analizzatur statiku PVS-Studio.

Ftit storja

Proġett FreeRDP seħħ wara li Microsoft fetħet l-ispeċifikazzjonijiet għall-protokoll RDP proprjetarju tagħha. F'dak iż-żmien, kien hemm klijent rdesktop, li l-implimentazzjoni tiegħu kienet ibbażata fuq ir-riżultati tar-Reverse Engineering.

Hekk kif il-protokoll ġie implimentat, sar aktar diffiċli li tiżdied funzjonalità ġdida minħabba l-arkitettura tal-proġett li kienet eżistenti dak iż-żmien. Bidliet fiha taw lok għal kunflitt bejn l-iżviluppaturi, li wassal għall-ħolqien ta 'furketta ta' rdesktop - FreeRDP. Distribuzzjoni ulterjuri tal-prodott kienet limitata mil-liċenzja GPLv2, li b'riżultat ta' dan ittieħdet deċiżjoni biex jingħata liċenzja mill-ġdid lil Apache License v2. Madankollu, mhux kulħadd qabel li jibdel il-liċenzja tal-kodiċi tagħhom, għalhekk l-iżviluppaturi ddeċidew li jikteb mill-ġdid il-proġett, li jirriżulta f'kodiċi modern.

Tista 'taqra aktar dwar l-istorja tal-proġett fil-blog post uffiċjali: "L-istorja tal-proġett FreeRDP".

Użat bħala għodda biex jiġu identifikati żbalji u vulnerabbiltajiet potenzjali fil-kodiċi. PVS-IstudjoHuwa analizzatur tal-kodiċi statiku għal C, C++, C# u Java, disponibbli fuq pjattaformi Windows, Linux и macOS.

L-artiklu jippreżenta biss dawk l-iżbalji li dehru l-aktar interessanti għalija.

Tnixxija tal-memorja

V773 Il-funzjoni nħarġet mingħajr ma ħarġet il-pointer 'cwd'. Tnixxija tal-memorja hija possibbli. l-ambjent.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;
  ....
}

Dan il-framment ittieħed mis-subsistema winpr, li timplimenta wrapper WINAPI għal dawk mhuxWindows sistemi, jiġifieri, huwa analogu ħafif ta' Wine. Hawnhekk tista' tara tnixxija: memorja allokata mill-funzjoni getcwd, jiġi rilaxxat biss meta jiġu ttrattati każijiet speċjali. Biex tirranġa l-iżball trid iżżid sejħa ħielsa wara memcpy.

Array barra mill-limiti

V557 Array qbiż huwa possibbli. Il-valur tal-indiċi 'event->EventHandlerCount' jista' jilħaq 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++;
  }
  ....
}

Dan l-eżempju jżid element ġdid mal-lista anki jekk in-numru ta 'elementi jkun laħaq il-massimu. Hawnhekk huwa biżżejjed li jissostitwixxi l-operatur <= fuq <, sabiex ma tmurx lil hinn mill-konfini tal-firxa.

Instab żball ieħor ta' dan it-tip:

  • V557 Array overrun huwa possibbli. Il-valur tal-indiċi 'iBitmapFormat' jista' jilħaq 8. orders.c 2623

Typos

Framment 1

V547 L-espressjoni '!pipe->In' hija dejjem falza. 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;
  ....
}

Hawnhekk naraw typo komuni: it-tieni kundizzjoni tikkontrolla l-istess varjabbli bħall-ewwel. Probabbilment, l-iżball deher bħala riżultat ta 'ikkupjar tal-kodiċi mingħajr suċċess.

Framment 2

V760 Instabu żewġ blokki ta' test identiċi. It-tieni blokk jibda mil-linja 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);
  ....
}

Typo ieħor: il-kumment tal-kodiċi jimplika li l-ħajt għandu jiġi minorVersion, madankollu, il-qari jseħħ f'varjabbli msemmi majorVersion. Madankollu, jien mhux familjari mal-protokoll, għalhekk din hija biss raden.

Framment 3

V524 Huwa stramb li l-korp tal-funzjoni 'trio_index_last' huwa kompletament ekwivalenti għall-korp tal-funzjoni '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);
}

Ġġudikati mill-kumment, il-funzjoni trio_index isib l-ewwel karattru jaqblu fi string meta trio_index_last - l-aħħar ħaġa. Iżda l-korpi ta 'dawn il-funzjonijiet huma identiċi! Ħafna probabbli dan huwa typo, u fil-funzjoni trio_index_last jeħtieġ li tuża strhrchr minflok strchr. Imbagħad l-imġieba tkun mistennija.

Framment 4

V769 Il-pointer 'data' fl-espressjoni huwa ugwali għal nullptr. Il-valur li jirriżulta ta 'operazzjonijiet aritmetiċi fuq dan il-pointer huwa bla sens u m'għandux jintuża. 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;
  ....
}

Jidher li l-operatur tan-negazzjoni kien aċċidentalment mitluf hawn ! Qrib data. Hija stramba li dan għadda inosservat.

Framment 5

V517 Instab l-użu tal-mudell 'jekk (A) {…} inkella jekk (A) {…}'. Hemm probabbiltà ta 'preżenza ta' żball loġiku. Iċċekkja linji: 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);
  }
  ....
}

L-aħħar żewġ kundizzjonijiet huma l-istess: milli jidher xi ħadd nesa jiċċekkjahom wara li kkopja. Huwa notevoli mill-kodiċi li l-aħħar parti taħdem b'valuri ta 'erba' byte, għalhekk nistgħu nassumu li l-aħħar kundizzjoni għandha tkun valur <= 0x3FFFFFFFF.

Instab żball ieħor ta' dan it-tip:

  • V517 Instab l-użu tal-mudell 'jekk (A) {…} inkella jekk (A) {…}'. Hemm probabbiltà ta 'preżenza ta' żball loġiku. Iċċekkja linji: 169, 173. file.c 169

Validazzjoni tad-dejta tal-input

Framment 1

V547 L-espressjoni 'strcat(mira, sors) != NULL' hija dejjem vera. 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);
}

Il-verifika tar-riżultat tal-funzjoni f'dan l-eżempju mhix korretta. Funzjoni strcat jirritorna pointer għall-verżjoni finali tas-sekwenza, i.e. l-ewwel parametru għadda. F'dan il-każ huwa mira. Madankollu, jekk huwa ugwali NULL, allura jkun tard wisq biex tiċċekkjaha, peress li fil-funzjoni strcat se jkun dereferenced.

Framment 2

V547 L-espressjoni 'cache' hija dejjem vera. 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)
  {
    ....
  }
  ....
}

F'dan il-każ il-varjabbli cache l-indirizz ta 'array statiku huwa assenjat glyphCache->glyphCache. Għalhekk, iċċekkja jekk (cache) jistgħu jitħallew barra.

Żball fil-ġestjoni tar-riżorsi

V1005 Ir-riżors inkiseb bl-użu tal-funzjoni 'CreateFileA' iżda ġie rilaxxat bl-użu tal-funzjoni 'fclose' inkompatibbli. ċertifikat.ċ 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;
  }
  ....
}

Deskrittur tal-fajl fp, maħluqa minn sejħa ta 'funzjoni CreateFile magħluqa bi żball bil-funzjoni fclose mil-librerija standard, le CloseHandle.

L-istess kundizzjonijiet

V581 L-espressjonijiet kundizzjonali tad-dikjarazzjonijiet 'jekk' li jinsabu maġenb xulxin huma identiċi. Iċċekkja linji: 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);
  }
  ....
}

Dan l-eżempju jista 'ma jkunx bug. Madankollu, iż-żewġ kundizzjonijiet fihom l-istess messaġġi, li wieħed minnhom x'aktarx jista' jitneħħa.

Tindif null pointers

V575 Il-pointer null huwa mgħoddi fil-funzjoni 'ħielsa'. Spezzjona l-ewwel 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);
  ....
}

Fil-funzjoni ħielsa tista 'tgħaddi pointer null u l-analizzatur ikun jaf biha. Imma jekk tinstab sitwazzjoni li fiha l-pointer dejjem jgħaddi null, bħal f'dan is-snippet, tinħareġ twissija.

Pointer mszGroupsA inizjalment ugwali NULL u ma tiġix initialized imkien ieħor. L-unika fergħa tal-kodiċi fejn il-pointer jista 'jiġi inizjalizzat ma tistax tintlaħaq.

Kien hemm messaġġi oħra bħal:

  • V575 Il-pointer null huwa mgħoddi fil-funzjoni 'ħielsa'. Spezzjona l-ewwel argument. liċenzja.ċ 790
  • V575 Il-pointer null huwa mgħoddi fil-funzjoni 'ħielsa'. Spezzjona l-ewwel argument. rdpsnd_alsa.c 575

Ħafna probabbli, tali varjabbli minsija jinqalgħu matul il-proċess ta 'refactoring u jistgħu sempliċement jitneħħew.

Possibbli overflow

V1028 Possibbli overflow. Ikkunsidra l-ikkastjar tal-operandi, mhux ir-riżultat. 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));
  ....
}

Inġibu r-riżultat għal twil mhix protezzjoni overflow, peress li l-kalkolu innifsu jseħħ bl-użu tat-tip int.

Dereference pointer fl-inizjalizzazzjoni

V595 Il-pointer tal-'kuntest' ġie utilizzat qabel ma ġie vverifikat kontra nullptr. Iċċekkja linji: 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;
  ....
}

Hawn il-pointer kuntest huwa dereferenced waqt l-inizjalizzazzjoni - qabel ma jiġi ċċekkjat.

Instabu żbalji oħra ta’ dan it-tip:

  • V595 Il-pointer 'ntlm' ġie utilizzat qabel ma ġie vverifikat kontra nullptr. Iċċekkja linji: 236, 255. ntlm.c 236
  • V595 Il-pointer tal-'kuntest' ġie utilizzat qabel ma ġie vverifikat kontra nullptr. Iċċekkja linji: 1003, 1007. rfx.c 1003
  • V595 Il-pointer 'rdpei' ġie utilizzat qabel ma ġie vverifikat kontra nullptr. Iċċekkja linji: 176, 180. rdpei_main.c 176
  • V595 Il-pointer 'gdi' ġie utilizzat qabel ma ġie vverifikat kontra nullptr. Iċċekkja linji: 121, 123. xf_gfx.c 121

Kundizzjoni bla sens

V547 L-espressjoni 'rdp->state >= CONNECTION_STATE_ACTIVE' hija dejjem vera. konnessjoni.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;
      }
    ....
  }
  ....
}

Huwa faċli li wieħed jara li l-ewwel kundizzjoni hija bla sens minħabba li l-valur korrispondenti jiġi assenjat qabel.

Parsing tal-istring mhux korrett

V576 Format mhux korrett. Ikkunsidra li tiċċekkja t-tielet argument attwali tal-funzjoni 'sscanf'. Huwa mistenni pointer għat-tip int mhux iffirmat. prokura.c 220

V560 Parti mill-espressjoni kondizzjonali hija dejjem vera: (rc >= 0). prokura.c 222

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

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

L-analizzatur immedjatament joħroġ 2 twissijiet għal dan il-framment. Speċifikatur %u jistenna varjabbli tat-tip mhux iffirmat int, iżda varjabbli sotto għandha tip int. Sussegwentement naraw kontroll suspettuż: il-kundizzjoni fuq il-lemin ma tagħmilx sens, peress li fil-bidu hemm paragun ma 'wieħed. Ma nafx x'ried ifisser l-awtur ta 'dan il-kodiċi, iżda xi ħaġa hija ċara ħażina hawn.

Verifiki li ma humiex fl-ordni

V547 L-espressjoni 'status == 0x00090314' hija dejjem falza. 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;
  ....
}

Il-kundizzjonijiet iċċekkjati dejjem ikunu foloz, peress li l-eżekuzzjoni tilħaq biss it-tieni kundizzjoni jekk status == SEC_E_OK. Il-kodiċi korrett jista' jidher bħal dan:

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;
}

Konklużjoni

Għalhekk, il-verifika tal-proġett żvelat ħafna problemi, iżda l-aktar parti interessanti minnhom biss ġiet deskritta fl-artiklu. L-iżviluppaturi tal-proġett jistgħu jiċċekkjaw il-proġett huma stess billi jitolbu ċavetta tal-liċenzja temporanja fuq il-websajt PVS-Istudjo. Kien hemm ukoll pożittivi foloz, xogħol li se jgħin biex itejjeb l-analizzatur. Madankollu, l-analiżi statika hija importanti jekk trid mhux biss ittejjeb il-kwalità tal-kodiċi tiegħek, iżda wkoll tnaqqas il-ħin imqatta' biex issib żbalji, u PVS-Studio jista 'jgħin f'dan.

Iċċekkja FreeRDP billi tuża l-analizzatur PVS-Studio

Jekk trid taqsam dan l-artiklu ma’ udjenza li titkellem bl-Ingliż, jekk jogħġbok uża l-link tat-traduzzjoni: Sergey Larin. Iċċekkjar FreeRDP ma PVS-Studio

Sors: www.habr.com

Ixtri hosting affidabbli għal siti bi protezzjoni DDoS, servers VPS VDS 🔥 Ixtri hosting ta' websajts affidabbli bi protezzjoni DDoS, servers VPS VDS | ProHoster