ProHoster > Blogi > Haldamine > FreeRDP kontrollimine PVS-Studio analĂŒsaatori abil

FreeRDP kontrollimine PVS-Studio analĂŒsaatori abil

PVS-Studio abil FreeRDP kontrollimine
FreeRDP – avatud Remote Desktop Protocol (RDP) teostus, mis vĂ”imaldab kaugjuhtimist arvutile ning on vĂ€lja töötatud Microsoft'i poolt. Projekt toetab mitmeid platvorme, sealhulgas Windows, Linux, macOS, ning isegi iOS ja Android. See projekt on valitud esimeseks artiklite seerias, kus uuritakse RDP-kliente staatilise analĂŒsaatori PVS-Studio abil.

Veidi ajalugu

Projekt FreeRDP sai alguse pÀrast seda, kui Microsoft avas oma patenteeritud protokolli RDP spetsifikatsioonid. Sel ajal eksisteeris klient rdesktop, mille teostus pÔhines reverse engineering'i tulemustel.

Protokolli rakendamise kĂ€igus muutus uue funktsionaalsuse lisamine keerulisemaks olemasoleva projekti arhitektuuri tĂ”ttu. Selle muudatused pĂ”hjustasid konfliktide tekkimise arendajate vahel, mis viis rdesktop'i fork'ini – FreeRDP. Toote edasine levik oli piiratud GPLv2 litsentsiga, mille tulemusena otsustati litsentsida projekt uuesti Apache License v2 alla. Siiski ei olnud kĂ”ik nĂ”us oma koodi litsentsi muutmisega, seega otsustasid arendajad projekti ĂŒmber kirjutada, mille tulemusena on meil tĂ€napĂ€evane koodibaas.

Projekt ajaloo kohta saab rohkem lugeda ametliku blogi postitusest: „FreeRDP projekti ajalugu”.

Veakontrollimiseks ja potentsiaalsete haavatavuste leidmiseks koodis kasutati PVS-Studio. See on staatiline koodianalĂŒsaator C, C++, C# ja Java keelte jaoks, mis on saadaval Windowsi, Linuxi ja macOS-i platvormidel.

Artiklis on toodud vaid need vead, mis tundusid mulle kÔige huvitavamad.

MĂ€lu leke

V773 Funktsioon lĂ”petati ilma ‘cwd’ punkti vabastamata. MĂ€lu leke on vĂ”imalik. 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;
  ....
}

See fragment on vĂ”etud winpr alamsĂŒsteemist, mis rakendab WINAPI vahetust mitte-Windowsi sĂŒsteemides, st see on kerge alternatiiv Wine'ile. Siin on mĂ€rgata leket: mĂ€lu, mille funktsioon getcwd, vabastatakse ainult erijuhtude töötlemisel. Veast vabanemiseks tuleb lisada kutsung free pĂ€rast memcpy.

Massiivi ĂŒletamine

V557 Massiivi ĂŒletamine on vĂ”imalik. ‘event->EventHandlerCount’ indeksi vÀÀrtus vĂ”ib ulatuda 32-ni. 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++;
  }
  ....
}

Selles nĂ€ites lisatakse uus element nimekirja, isegi kui elementide arv on saavutanud maksimumi. Siin piisab operaatori asendamisest <= jĂ€rgnevaga <, et mitte ĂŒletada massiivi piire.

Leiti ka teine viga sellise tĂŒĂŒbi kohta:

  • V557 Massiivi ĂŒletamine on vĂ”imalik. 'iBitmapFormat' indeksi vÀÀrtus vĂ”ib ulatuda 8-ni. orders.c 2623

TrĂŒkivead

Fragment 1

V547 VĂ€ljend '!pipe->In' on alati vale. 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;
  ....
}

Siin nĂ€eme tavalist trĂŒkiviga: teises tingimuses kontrollitakse sama muutujat, mis esmasses. TĂ”enĂ€oliselt tekkis viga halva koodi kopeerimise tulemusel.

Fragment 2

V760 Leiti kaks identset tekstiblokki. Teine blokk algab realt 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);
  ....
}

Veel ĂŒks trĂŒkiviga: koodi kommentaar vihjab, et voolust peaks tulema minorVersion, kuid lugemine toimub muutuja nimega majorVersion. Siiski, ma ei tunne protokolli, seega on see vaid oletus.

Fragment 3

V524 On imelik, et ‘trio_index_last’ funktsiooni keha on tĂ€iesti samavÀÀrne ‘trio_index’ funktsiooni kehaga. 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);
}

Kommentaari pĂ”hjal, funktsioon trio_index leiab esimese vastavuse mĂ€rki stringis, kui trio_index_last on viimane. Kuid nende funktsioonide kehadega on identne! TĂ”enĂ€oliselt on see trĂŒkiviga ja funktsioonis trio_index_last peaks kasutama strrchr asetatakse strchr. Siis on kĂ€itumine ootuspĂ€rane.

Fragment 4

V769 ‘data’ pointer vĂ€ljendis on nullptr. Selle pointeriga tehtud aritmeetikategevuste tulemus on mĂ”ttetu ja seda ei tohiks kasutada. 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;
  ....
}

Tundub, et siit on kogemata vahele jÀetud eitamisoperaator ! lÀhedal data. Veider, et seda ei mÀrgatud.

Fragment 5

V517 Tuvastati ‘if (A) {
} else if (A) {
}’ mustrit. On tĂ”enĂ€osus, et esineb loogiline viga. Kontrollige ridasid: 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 > 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 > 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);
  }
  ....
}

Viimased kaks tingimust on identsed: tÔenÀoliselt unustas keegi nende kontrollimise kopeerimise jÀrel. Koodist on nÀha, et viimane osa töötab nelja baitiga vÀÀrtustega, seega vÔib eeldada, et viimane tingimus peaks olema value <= 0x3FFFFFFF.

Leiti ka teine viga sellise tĂŒĂŒbi kohta:

  • V517 Avati 'if (A) {
} else if (A) {
}' mustri kasutamine. On tĂ”enĂ€osus, et loogiline viga esineb. Kontrollige ridu: 169, 173. file.c 169

Sisendandmete kontroll

Fragment 1

V547 Avaldis 'strcat(target, source) != NULL' on alati tÔene. 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);
}

Funktsiooni tÀitmise tulemuse kontroll seda nÀites ei ole korrektne. Funktsioon strcat tagastab viite lÔppversioonile stringist, st esimesele edastatud parameetrile. Antud juhul see on target. Kuid kui see on vÔrdne NULL, siis on hilja seda kontrollida, kuna funktsioonis strcat toimub selle dereferents.

Fragment 2

V547 VÀide 'cache' on alati tÔene. 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)
  {
    ....
  }
  ....
}

Selles juhul muutujale cache omistatakse staatilise massiivi aadress glyphCache->glyphCache. Seega saab kontrolli if (cache) jÀtma.

Ressursihalduse viga

V1005 Ressurss saadi kasutades 'CreateFileA' funktsiooni, kuid vabastati kasutades ĂŒhilduvat 'fclose' funktsiooni. 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;
  }
  ....
}

Faili deskriptor fp, loodud funktsiooni CreateFile, vale tÔttu suleti funktsiooniga fclose standardsest raamatukogust, mitte CloseHandle.

Identsed tingimused

V581 Tehtud 'if' lause tingimuslikud vÀljendid, mis asuvad kÔrvuti, on identsed. Kontrollige ridu: 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);
  }
  ....
}

See, et nĂ€ide ei pruugi olla viga. Kuid mĂ”lemad tingimused sisaldavad samu teateid, millest ĂŒks on tĂ”enĂ€oliselt eemaldatav.

Nullviidete puhastamine

V575 Nullviidatud antakse 'free' funktsioonile. Kontrolli esimest argumenti. smartcard_pcsc.c 875

WINSCARDAPI LONG WINAPI PCSC_SCardListReadersW(
  SCARDCONTEXT hContext,
  LPCWSTR mszGroups,
  LPWSTR mszReaders,
  LPDWORD pcchReaders)
{
  LPSTR mszGroupsA = NULL;
  ....
  mszGroups = NULL; /* mszGroups ei toeta 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);
  ....
}

Funktsioonisse free vĂ”ite edastada null viit ja analĂŒsaator on sellest teadlik. Kuid kui ilmneb olukord, kus viit edastatakse alati nullina, nagu antud fragment, antakse hoiatus.

Viit mszGroupsA alguses on seade NULL ja ei inizialiseerita kuskil mujal. Ainus koodiharu, kus viit vÔidi initsialiseerida, on saavutatav.

Oli ka teisi sĂ”numeid sellist tĂŒĂŒpi:

  • V575 Null viit edastatakse 'free' funktsioonile. Kontrollige esimest argumenti. license.c 790
  • V575 Null viit edastatakse 'free' funktsioonile. Kontrollige esimest argumenti. rdpsnd_alsa.c 575

TÔenÀoliselt tekivad sellised unustatud muutujad refaktoreerimise kÀigus ja need on lihtsalt eemaldatavad.

VĂ”imalik ĂŒletĂ€itumine

V1028 VĂ”imalik ĂŒletĂ€itumine. Kaaluge operandide tĂŒĂŒbimuudatust, mitte tulemust. 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));
  ....
}

Tulemuse tĂŒĂŒbimuutmine long ei ole ĂŒleujutuste eest kaitse, kuna ise arvutamine toimub tĂŒĂŒbi abil int.

Viidatud indeksi lahutamine initsialiseerimisel

V595 ‘context’ viidatud nĂ€idikut kasutati enne selle kontrollimist nullviidiku vastu. Kontrollige ridu: 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;
  ....
}

Siin viitator context lahutatakse initsialiseerimisel — varem, kui selle kontrollimine toimub.

Leiti ka muid selliseid vigu:

  • V595 ‘ntlm’ viidatud nĂ€idikut kasutati enne selle kontrollimist nullviidiku vastu. Kontrollige ridu: 236, 255. ntlm.c 236
  • V595 ‘context’ viidatud nĂ€idikut kasutati enne selle kontrollimist nullviidiku vastu. Kontrollige ridu: 1003, 1007. rfx.c 1003
  • V595 ‘rdpei’ viidatud nĂ€idikut kasutati enne selle kontrollimist nullviidiku vastu. Kontrollige ridu: 176, 180. rdpei_main.c 176
  • V595 ‘gdi’ viidatud nĂ€idikut kasutati enne selle kontrollimist nullviidiku vastu. Kontrollige ridu: 121, 123. xf_gfx.c 121

MÔttetu tingimus

V547 Avaldus ‘rdp->state >= CONNECTION_STATE_ACTIVE’ on alati tĂ”ene. connection.c 1489

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

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

On lihtne mÀrgata, et esimene tingimus pole mÔistlik, kuna vastavasisulist vÀÀrtust on antud varem.

Vale stringi analĂŒĂŒs

V576 Vale vorming. Kontrollige kolmandat tegelikku argumenti 'sscanf' funktsiooni puhul. Oodatakse pöid, mis on tĂŒĂŒbi unsigned int. proxy.c 220

V560 MÔne tingimuslause osa on alati tÔene: (rc >= 0). proxy.c 222

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

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

Selle fragmendi analĂŒsaator genereerib kohe 2 hoiatusi. Spetsifikaator %u ootab muutuja tĂŒĂŒpi unsigned int, kuid muutuja sub on tĂŒĂŒpi int. Edasi nĂ€eme kahtlast kontrolli: parempoolne tingimus pole mĂ”istlik, kuna alguses vĂ”rreldakse ĂŒhte. Ma ei tea, mida selle koodi autor mĂ”tles, aga siin on kindlasti midagi valesti.

KrĂŒpteeritud kontrollid

V547 VĂ€ljend 'status == 0x00090314' on alati vale. 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;
  ....
}

MĂ€rgatud tingimused on alati valed, kuna tĂ€itmine jĂ”uab teise tingimuse juurde ainult juhul, kui status == SEC_E_OK. Õige kood vĂ”iks vĂ€lja nĂ€ha jĂ€rgmiselt:

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

KokkuvÔte

Seega tuvastas projekti kontrollimine hulgaliselt probleeme, kuid ainult kĂ”ige huvitavam osa neist kirjeldati artiklis. Projekti arendajad saavad ise projekti kontrollida, kĂŒsides ajutist litsentsivĂ”tit veebisaidilt. PVS-Studio. Esines ka valehĂ€ired, mille kallal töötamine aitab analĂŒsaatorit parandada. Sellegipoolest on staatiline analĂŒĂŒs oluline, kui soovite mitte ainult koodi kvaliteeti parandada, vaid ka veaotsingu aega lĂŒhendada, ning PVS-Studio aitab sellega.

PVS-Studio abil FreeRDP kontrollimine

Kui soovite seda artiklit jagada ingliskeelsele publikule, palun kasutage tÔlke linki: Sergey Larin. Checking FreeRDP with PVS-Studio

Allikas: habr.com

Osta usaldusvÀÀrne veebihosting DDoS kaitsega, VPS VDS serverid đŸ”„ Osta usaldusvÀÀrne veebihosting DDoS kaitsega, VPS VDS serverid | ProHoster