ProHoster > Blogg > Administration > Kontrollera FreeRDP med PVS-Studio analysator

Kontrollera FreeRDP med PVS-Studio analysator

Kontroll av FreeRDP med analysatorn PVS-Studio
FreeRDP – en öppen implementation av Remote Desktop Protocol (RDP), ett protokoll som möjliggör fjĂ€rrstyrning av datorer, utvecklat av Microsoft. Projektet stöder flera plattformar, inklusive Windows, Linux, macOS och till och med iOS och Android. Detta projekt Ă€r valt som det första i en serie artiklar som handlar om att granska RDP-klienter med hjĂ€lp av den statiska analysatorn PVS-Studio.

Lite historia

Projektet FreeRDP uppstod efter att Microsoft öppnade specifikationerna för sitt proprietÀra RDP-protokoll. Vid den tiden fanns klienten rdesktop, vars implementation baserades pÄ resultat frÄn Reverse Engineering.

Under implementeringen av protokollet blev det svĂ„rare att lĂ€gga till ny funktionalitet pĂ„ grund av projektets existerande arkitektur. Ändringar i den gav upphov till konflikter mellan utvecklarna, vilket ledde till att forken rdesktop skapades – FreeRDP. Den fortsatta spridningen av produkten begrĂ€nsades av GPLv2-licensen, vilket ledde till beslutet om att omlicensiera den under Apache License v2. Men inte alla var överens om att Ă€ndra licensen för sin kod, sĂ„ utvecklarna beslutade att skriva om projektet, vilket ger oss den moderna versionen av kodbasen.

Mer detaljerad information om projektets historia kan lÀsas i notisen pÄ den officiella bloggen: "The history of the FreeRDP project."

Som ett verktyg för att identifiera fel och potentiella sÄrbarheter i koden anvÀndes PVS-Studio. Detta Àr en statisk kodanalysator för sprÄken C, C++, C# och Java, tillgÀnglig pÄ plattformarna Windows, Linux och macOS.

I artikeln presenteras endast de fel som verkade mest intressanta för mig.

MinneslÀcka

V773 Funktionen avslutades utan att frigöra pekaren 'cwd'. En minneslÀcka Àr möjlig. 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;
  ....
}

Denna fragment Àr hÀmtat frÄn winpr-underdelen, som implementerar en wrapper för WINAPI för icke-Windows-system, det vill sÀga en lÀtt analog av Wine. HÀr noteras en lÀcka: minnet som allokeras av funktionen getcwd, frigörs endast vid sÀrskilda fall. För att ÄtgÀrda felet behöver ett anrop till free efter memcpy.

ArraygrÀnsöverskridning

V557 Arrayöverskridning Àr möjligt. VÀrdet av 'event->EventHandlerCount' indexet kan nÄ 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++;
  }
  ....
}

I detta exempel lÀggs ett nytt element till i listan, Àven om antalet element har nÄtt maximalt. HÀr rÀcker det att byta ut operatorn <= till <, för att inte överskrida arraygrÀnserna.

En annan typ av fel har upptÀckts:

  • V557 Arrayöverskridning Ă€r möjlig. VĂ€rdet av 'iBitmapFormat'-indexet kan nĂ„ 8. orders.c 2623

Tryckfel

Fragment 1

V547 Uttrycket '!pipe->In' Àr alltid falskt. 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;
  ....
}

HÀr ser vi ett vanligt skrivfel: i det andra villkoret kontrolleras samma variabel som i det första. Troligtvis uppstod felet pÄ grund av ett misslyckat kodkopierande.

Fragment 2

V760 TvÄ identiska textblock har hittats. Det andra blocket börjar frÄn rad 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);
  ....
}

Ännu ett skrivfel: kommentaren till koden antyder att det frĂ„n strömmen borde komma minorVersion, men lĂ€sningen sker i en variabel med namnet majorVersion. Jag Ă€r dock inte bekant med protokollet, sĂ„ detta Ă€r endast en gissning.

Fragment 3

V524 Det Àr mÀrkligt att kroppen av 'trio_index_last'-funktionen Àr helt identisk med kroppen av 'trio_index'-funktionen. 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);
}

Enligt kommentaren hittar funktionen trio_index det första matchande tecknet i en strĂ€ng, medan trio_index_last — det sista. Men kropparna av dessa funktioner Ă€r identiska! Troligtvis Ă€r det ett skrivfel, och i funktionen trio_index_last mĂ„ste du anvĂ€nda strrchr istĂ€llet för strchr. DĂ„ skulle beteendet vara förvĂ€ntat.

Fragment 4

V769 Pekaren 'data' i uttrycket Àr lika med nullptr. Det resulterande vÀrdet av aritmetiska operationer pÄ denna pekare Àr meningslöst och bör inte anvÀndas. 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;
  ....
}

Det verkar som om man rÄkade missa negationstecknet ! nÀra data. Det Àr konstigt att detta förblev obemÀrkt.

Fragment 5

V517 AnvÀndning av mönstret 'if (A) {...} else if (A) {...}' har upptÀckts. Det finns en sannolikhet för att en logisk felaktighet Àr nÀrvarande. Kontrollera rader: 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);
  }
  ....
}

De senaste tvÄ villkoren Àr identiska: uppenbarligen har nÄgon glömt att kontrollera dem efter kopieringen. I koden Àr det tydligt att den sista delen arbetar med fyrabyte-vÀrden, sÄ man kan anta att det sista villkoret bör vara value <= 0x3FFFFFFF.

En annan typ av fel har upptÀckts:

  • V517 Mönstret ‘if (A) {
} else if (A) {
}’ har upptĂ€ckts. Det finns en sannolikhet för att en logisk felaktighet föreligger. Kontrollera rader: 169, 173. file.c 169

Kontroll av indata

Fragment 1

V547 Uttrycket ‘strcat(target, source) != NULL’ Ă€r alltid sant. 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);
}

Kontrollen av funktionsresultatet i detta exempel Ă€r felaktig. Funktionen strcat returnerar en pekare till den slutliga versionen av strĂ€ngen, dvs. den första överförda parametern. I detta fall Ă€r det target. Men om den Ă€r lika med NULL, Ă€r det för sent att kontrollera den, eftersom den kommer att derefereras i funktionen. strcat Uttrycket ‘cache’ Ă€r alltid sant. glyph.c 730

Fragment 2

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

I det hÀr fallet tilldelas variabeln

adressen till den statiska arrayen cache glyphCache->glyphCache . SĂ„ledes kan kontrollenif (cache) uteslutas. Felhantering av resurser

V1005

Resursen förvĂ€rvades med funktionen ‘CreateFileA’ men slĂ€pptes med den oförenliga funktionen ‘fclose’. 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; } .... }

Filens deskriptor

fp , som skapades genom att anropa funktionenCreateFile, stÀngdes av misstag med funktionen fclose frÄn standardbiblioteket, istÀllet för CloseHandle Identiska villkor.

V581

De betingade uttrycken i ‘if’-uttalandena som ligger bredvid varandra Ă€r identiska. Kontrollera rader: 269, 283. ndr_structure.c 283 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: "
 "0xX 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: "
 "0xX unimplemented", array_type);
 }
 ....
}

Detta exempel Àr kanske inte ett fel. Men bÄda villkoren innehÄller samma meddelanden, varav en troligen kan tas bort.

Rensa nollpekare

V575 Nollpekaren skickas till 'free'-funktionen. Kontrollera den första parametern. smartcard_pcsc.c 875

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

I funktionen free Det gÄr att skicka en nollpekare och analysatorn Àr medveten om detta. Men om en situation upptÀcks dÀr pekaren alltid skickas som noll, som i detta fragment, kommer en varning att utfÀrdas.

Pekare mszGroupsA Àr frÄn början NULL och initieras inte nÄgon annanstans. Den enda kodgrenen dÀr pekaren kunde ha initierats Àr otillgÀnglig.

Det har varit andra meddelanden av denna typ:

  • V575 Nollpekaren skickas till 'free'-funktionen. Kontrollera den första parametern. license.c 790
  • V575 Nollpekaren skickas till 'free'-funktionen. Kontrollera den första parametern. rdpsnd_alsa.c 575

Troligtvis uppstÄr sÄdana glömda variabler under refactoring och kan helt enkelt tas bort.

Möjlig överflödning

V1028 Möjligt överflöd. ÖvervĂ€g att typkonvertera operander, inte resultatet. 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));
  ....
}

Typkonvertering av resultatet till long Àr inte ett skydd mot överflödning, eftersom sjÀlva berÀkningen görs med hjÀlp av typ int.

Dereferering av pekare vid initiering

V595 Pekaren 'context' anvÀndes innan den verifierades mot nullptr. Kontrollera rader: 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;
  ....
}

HĂ€r Ă€r pekaren context avrefereras vid initialiseringen – tidigare Ă€n kontrollen sker.

Det har ocksÄ hittats andra sÄdana hÀr fel:

  • V595 Pekaren ‘ntlm’ anvĂ€ndes innan den verifierades mot nullptr. Kontrollera rader: 236, 255. ntlm.c 236
  • V595 Pekaren ‘context’ anvĂ€ndes innan den verifierades mot nullptr. Kontrollera rader: 1003, 1007. rfx.c 1003
  • V595 Pekaren ‘rdpei’ anvĂ€ndes innan den verifierades mot nullptr. Kontrollera rader: 176, 180. rdpei_main.c 176
  • V595 Pekaren ‘gdi’ anvĂ€ndes innan den verifierades mot nullptr. Kontrollera rader: 121, 123. xf_gfx.c 121

Meningslös villkor

V547 Uttrycket ‘rdp->state >= CONNECTION_STATE_ACTIVE’ Ă€r alltid sant. 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;
      }
    ....
  }
  ....
}

Det Àr lÀtt att se att det första villkoret Àr meningslöst pÄ grund av att motsvarande vÀrde tilldelades tidigare.

Felaktig analys av strÀngen

V576 Felaktigt format. Kontrollera det tredje aktuella argumentet i funktionen ‘sscanf’. En pekare till typen unsigned int förvĂ€ntas. proxy.c 220

V560 En del av det villkorliga uttrycket Àr alltid sant: (rc >= 0). proxy.c 222

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

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

Analysatorn för den hÀr biten ger omedelbart 2 varningar. Specifikatortypen %u förvÀntar en variabel av typen unsigned int, men variabeln sub har typen int. Vi ser sedan en misstÀnksam kontroll: villkoret till höger Àr meningslöst, eftersom det inledande jÀmförelsen med ett.

Oordnade kontroller

V547 Uttrycket ‘status == 0x00090314’ Ă€r alltid falskt. 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 markerade villkoren kommer alltid att vara falska, eftersom utförandet endast nÄr det andra villkoret nÀr status == SEC_E_OK. RÀtt kod kan se ut sÄ hÀr:

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

Slutsats

DĂ€rmed avslöjade projektkontrollen flera problem, men endast den mest intressanta delen av dem beskrevs i artikeln. Projektutvecklarna kan sjĂ€lva kontrollera projektet genom att begĂ€ra en tillfĂ€llig licensnyckel pĂ„ webbplatsen. PVS-Studio. Det fanns ocksĂ„ falska positiva resultat, och arbetet med dem kommer att förbĂ€ttra analysatorn. ÄndĂ„ Ă€r statisk analys viktig om du vill förbĂ€ttra kodkvaliteten och minska tiden för att hitta fel, och PVS-Studio kan hjĂ€lpa till med detta.

Kontroll av FreeRDP med analysatorn PVS-Studio

Om du vill dela denna artikel med en engelsktalande publik, vÀnligen anvÀnd lÀnken till översÀttningen: Sergey Larin. Kontrollerar FreeRDP med PVS-Studio

KĂ€lla: habr.com

Köp pĂ„litlig webbhotell med DDoS-skydd, VPS VDS-servrar đŸ”„ Köp pĂ„litlig webbhotell med DDoS-skydd, VPS VDS-servrar | ProHoster