ProHoster > Blog > Bestjoer > Kontrolearje FreeRDP mei de PVS-Studio analyzer

Kontrolearje FreeRDP mei de PVS-Studio analyzer

FreeRDP kontrolearje mei de PVS-Studio analyzer
FreeRDP is in iepen boarne-ymplemintaasje fan it Remote Desktop Protocol (RDP), in protokol ûntwikkele troch Microsoft foar kompjûterkontrôle op ôfstân. It projekt stipet meardere platfoarms, ynklusyf Windows, Linux, macOS en sels iOS mei AndroidDit projekt waard keazen as it earste yn in searje artikels wijd oan it testen fan RDP-kliïnten mei de PVS-Studio statyske analyzer.

In bytsje skiednis

It projekt FreeRDP kaam neidat Microsoft de spesifikaasjes iepene foar har proprietêre RDP-protokol. Op dat stuit wie der in rdesktop client, de útfiering fan dat wie basearre op de resultaten fan Reverse Engineering.

As it protokol ymplementearre waard, waard it dreger om nije funksjonaliteit ta te foegjen troch de doe besteande projektarsjitektuer. Feroarings yn it joech oanlieding ta in konflikt tusken ûntwikkelders, dy't late ta de oprjochting fan in foarke fan rdesktop - FreeRDP. Fierdere distribúsje fan it produkt waard beheind troch de GPLv2-lisinsje, as gefolch wêrfan in beslút waard makke om it opnij te lisinsjejen nei de Apache License v2. Net elkenien wie lykwols iens om de lisinsje fan har koade te feroarjen, sadat de ûntwikkelders besletten it projekt te herskriuwen, wat resultearre yn in moderne koadebase.

Jo kinne mear lêze oer de skiednis fan it projekt yn 'e offisjele blogpost: "De skiednis fan it FreeRDP-projekt".

Wurdt brûkt as ark om flaters en potinsjele kwetsberens yn 'e koade te identifisearjen. PVS-StudioIt is in statyske koade-analysator foar C, C++, C# en Java, beskikber op platfoarms Windows, Linux и macOS.

It artikel presintearret allinich de flaters dy't my it meast ynteressant wiene.

Unthâld lek

V773 De funksje is ôfsletten sûnder de 'cwd'-oanwizer los te litten. In ûnthâld lek is mooglik. omjouwing.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;
  ....
}

Dit fragmint is nommen út it winpr-subsysteem, dat in WINAPI-wrapper implementearret foar net-Windows systemen, d.w.s. it is in lichtgewicht analoog fan Wine. Hjir kinne jo in lek sjen: ûnthâld tawiisd troch de funksje getcwd, wurdt allinich frijlitten by it behanneljen fan spesjale gefallen. Om de flater te reparearjen moatte jo in oprop tafoegje frij после memcpy.

Array bûten grinzen

V557 Array oerrin is mooglik. De wearde fan 'event->EventHandlerCount' yndeks koe 32 berikke. 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++;
  }
  ....
}

Dit foarbyld foeget in nij elemint ta oan de list, sels as it oantal eleminten it maksimum hat berikt. Hjir is it genôch om de operator te ferfangen <= op <, om net oer de grinzen fan 'e array te gean.

In oare flater fan dit type waard fûn:

  • V557 Array oerrin is mooglik. De wearde fan 'iBitmapFormat' yndeks koe berikke 8. orders.c 2623

Typos

Fragmint 1

V547 Ekspresje '!pipe->In' is altyd falsk. 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;
  ....
}

Hjir sjogge wy in gewoane typflater: de twadde betingst kontrolearret deselde fariabele as de earste. Meast wierskynlik, de flater ferskynde as gefolch fan mislearre koade kopiearjen.

Fragmint 2

V760 Twa blokken identike tekst waarden fûn. It twadde blok begjint fan 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);
  ....
}

Noch in typflater: it koadekommentaar hâldt yn dat de tried komme moat minorVersion, lykwols, lêzen bart yn in fariabele neamd majorVersion. Ik bin lykwols net bekend mei it protokol, dus dit is mar in gissing.

Fragmint 3

V524 It is frjemd dat it lichem fan 'trio_index_last' funksje folslein lykweardich is oan it lichem fan 'trio_index' funksje. 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);
}

Nei it kommentaar te oardieljen, de funksje trio_index fynt de earste karakter wedstriid yn in tekenrige wannear trio_index_last - lêste ding. Mar de lichems fan dizze funksjes binne identyk! Meast wierskynlik is dit in typflater, en yn 'e funksje trio_index_last moatte brûke strhrchr вместо strchr. Dan wurdt it gedrach ferwachte.

Fragmint 4

V769 De 'gegevens'-oanwizer yn 'e útdrukking is lyk oan nullptr. De resultearjende wearde fan arithmetyske operaasjes op dizze oanwizer is sûnder sin en it moat net brûkt wurde. 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;
  ....
}

It liket derop dat de negaasjeoperator hjir per ongeluk mist waard ! Tichtby data. It is nuver dat dit ûngemurken gie.

Fragmint 5

V517 It gebrûk fan 'as (A) {...} oars as (A) {...}' patroan waard ûntdutsen. Der is in kâns fan logyske flater oanwêzigens. Kontrolearje rigels: 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);
  }
  ....
}

De lêste twa betingsten binne itselde: blykber hat immen fergetten om se nei it kopiearjen te kontrolearjen. It is opmurken út de koade dat it lêste diel wurket mei fjouwer-byte wearden, dus wy kinne oannimme dat de lêste betingst moat wêze wearde <= 0x3FFFFFFFF.

In oare flater fan dit type waard fûn:

  • V517 It gebrûk fan 'as (A) {...} oars as (A) {...}' patroan waard ûntdutsen. Der is in kâns fan logyske flater oanwêzigens. Kontrolearje rigels: 169, 173. file.c 169

Validaasje fan ynfiergegevens

Fragmint 1

V547 Ekspresje 'strcat(target, source) != NULL' is altyd wier. 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);
}

It kontrolearjen fan it resultaat fan 'e funksje yn dit foarbyld is ferkeard. Funksje strcat jout in oanwizer werom nei de definitive ferzje fan de tekenrige, d.w.s. de earste parameter trochjûn. Yn dit gefal is it doel. Lykwols, as it is gelyk NULL, dan is it te let om it te kontrolearjen, sûnt yn 'e funksje strcat it sil ôfwiisd wurde.

Fragmint 2

V547 Ekspresje 'cache' is altyd wier. 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)
  {
    ....
  }
  ....
}

Yn dit gefal de fariabele lytsûnthâld it adres fan in statyske array wurdt tawiisd glyphCache->glyphCache. Dus, kontrolearje as (cache) kin wurde weilitten.

Boarne behear flater

V1005 De boarne waard oankocht mei 'CreateFileA'-funksje, mar waard frijjûn mei in ynkompatibele 'fclose'-funksje. 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;
  }
  ....
}

Triem descriptor fp, makke troch in funksje oprop CreateFile sletten by fersin troch funksje fslute út de standert bibleteek, net CloseHandle.

Deselde betingsten

V581 De betingstlike útdrukkingen fan 'e' if'-útspraken dy't neist elkoar lizze binne identyk. Kontrolearje rigels: 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);
  }
  ....
}

Dit foarbyld kin gjin bug wêze. Beide betingsten befetsje lykwols deselde berjochten, wêrfan ien wierskynlik fuorthelle wurde kin.

Opromjen fan nul pointers

V575 De nul-oanwizer wurdt trochjûn yn 'frije' funksje. Kontrolearje it earste argumint. 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);
  ....
}

Funksjonearje frij jo kinne in nul-oanwizer trochjaan en de analysator wit derfan. Mar as in situaasje wurdt ûntdutsen wêryn de oanwizer altyd nul wurdt trochjûn, lykas yn dit snippet, sil in warskôging wurde útjûn.

Pointer mszGroupsA ynearsten gelyk NULL en wurdt net inisjalisearre earne oars. De ienige tûke fan koade wêr't de oanwizer kin wurde inisjalisearre is net te berikken.

D'r wiene oare berjochten lykas:

  • V575 De nuloanwizer wurdt trochjûn yn 'frije' funksje. Kontrolearje it earste argumint. lisinsje.c 790
  • V575 De nuloanwizer wurdt trochjûn yn 'frije' funksje. Kontrolearje it earste argumint. rdpsnd_alsa.c 575

Meast wierskynlik, sokke fergetten fariabelen ûntsteane tidens it refactoring proses en kinne gewoan fuortsmiten wurde.

Mooglike oerstreaming

V1028 Mooglike oerstreaming. Tink oan it casten fan operanden, net it resultaat. 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));
  ....
}

It resultaat bringe nei lang is gjin overflow beskerming, sûnt de berekkening sels bart mei help fan it type int.

Pointer dereference yn inisjalisaasje

V595 De 'kontekst'-oanwizer waard brûkt foardat it waard ferifiearre tsjin nullptr. Kontrolearje rigels: 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;
  ....
}

Hjir is de oanwizer kontekst wurdt derreferenced by inisjalisaasje - foardat it wurdt kontrolearre.

Oare flaters fan dit type waarden fûn:

  • V595 De 'ntlm'-oanwizer waard brûkt foardat it waard ferifiearre tsjin nullptr. Kontrolearje rigels: 236, 255. ntlm.c 236
  • V595 De 'kontekst'-oanwizer waard brûkt foardat it waard ferifiearre tsjin nullptr. Kontrolearje rigels: 1003, 1007. rfx.c 1003
  • V595 De 'rdpei'-oanwizer waard brûkt foardat it waard ferifiearre tsjin nullptr. Kontrolearje rigels: 176, 180. rdpei_main.c 176
  • V595 De 'gdi'-oanwizer waard brûkt foardat it waard ferifiearre tsjin nullptr. Kontrolearje rigels: 121, 123. xf_gfx.c 121

Sinleaze betingst

V547 Ekspresje 'rdp->state>= CONNECTION_STATE_ACTIVE' is altyd wier. ferbining.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;
      }
    ....
  }
  ....
}

It is maklik om te sjen dat de earste betingst sûnder betsjutting is fanwege de oerienkommende wearde dy't earder wurdt tawiisd.

Ferkearde tekenrige parsing

V576 Ferkearde opmaak. Besykje it tredde eigentlike argumint fan 'e funksje 'sscanf' te kontrolearjen. In oanwizer nei it net-ûndertekene int-type wurdt ferwachte. proxy.c 220

V560 In diel fan betingsten útdrukking is altyd wier: (rc >= 0). proxy.c 222

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

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

De analysator jout fuortendaliks 2 warskôgings foar dit fragmint. Specifier %u ferwachtet in fariabele fan type net ûndertekene int, mar fariabele sub hat type int. Dêrnei sjogge wy in fertochte kontrôle: de betingst oan 'e rjochter hat gjin sin, om't yn it begjin in ferliking is mei ien. Ik wit net wat de skriuwer fan dizze koade bedoelde, mar wat is hjir dúdlik mis.

Out-of-oarder sjeks

V547 Ekspresje 'status == 0x00090314' is altyd falsk. 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;
  ....
}

De kontrolearre betingsten sille altyd wêze falsk, sûnt útfiering sil allinne berikke de twadde betingst as status == SEC_E_OK. De juste koade kin der sa útsjen:

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

konklúzje

Sa, it kontrolearjen fan it projekt die bliken in protte problemen, mar allinnich it meast nijsgjirrige diel fan harren waard beskreaun yn it artikel. Projektûntwikkelders kinne it projekt sels kontrolearje troch in tydlike lisinsjekaai oan te freegjen op 'e webside PVS-Studio. D'r wiene ek falske posityfen, wurk oan dat sil helpe om de analysator te ferbetterjen. Lykwols, statyske analyze is wichtich as jo wolle net allinnich ferbetterje de kwaliteit fan jo koade, mar ek ferminderje de tiid bestege oan it finen fan flaters, en PVS-Studio kin helpe mei dit.

FreeRDP kontrolearje mei de PVS-Studio analyzer

As jo ​​​​dit artikel wolle diele mei in Ingelsktalig publyk, brûk dan de oersettingskeppeling: Sergey Larin. Kontrolearje FreeRDP mei PVS-Studio

Boarne: www.habr.com

Keapje betroubere hosting foar siden mei DDoS-beskerming, VPS VDS-tsjinners 🔥 Keapje betroubere websidehosting mei DDoS-beskerming, VPS VDS-tsjinners | ProHoster