🥇Kontrol af FreeRDP ved hjælp af analysatoren PVS-Studio

Kontrol af FreeRDP ved hjælp af analysatoren PVS-Studio
FreeRDP – en open-source implementering af Remote Desktop Protocol (RDP), en protokol, der muliggør fjernkontrol af computere, udviklet af Microsoft. Projektet understøtter mange platforme, herunder Windows, Linux, macOS og endda iOS og Android. Dette projekt er valgt som det første i en artikelserie, der fokuserer på test af RDP-klienter ved hjælp af den statiske analyser PVS-Studio.

Lidt historie

Projekt FreeRDP opstod efter at Microsoft offentliggjorde specifikationerne for sin proprietære RDP-protokol. På det tidspunkt eksisterede der en klient ved navn rdesktop, hvis implementering er baseret på resultaterne af reverse engineering.

I implementeringen af protokollen blev det vanskeligere at tilføje nye funktioner på grund af den daværende projektarkitektur. Ændringer i den gav anledning til konflikt mellem udviklerne, hvilket førte til dannelsen af en fork af rdesktop – FreeRDP. Den videre distribution af produktet var begrænset af GPLv2-licensen, hvilket resulterede i beslutningen om at relicensere til Apache License v2. Imidlertid var ikke alle enige i at ændre licensen for deres kode, så udviklerne besluttede at genoptage projektet, hvilket resulterede i den moderne version af kodebasen.

For mere detaljeret information om projektets historie kan man læse notatet på den officielle blog: "The history of the FreeRDP project".

Som et værktøj til at identificere fejl og potentielle sårbarheder i koden blev der anvendt PVS-Studio. Dette er en statisk kodeanalysator til C-, C++-, C#- og Java-sprog, tilgængelig på platforme som Windows, Linux og macOS.

I artiklen præsenteres kun de fejl, der syntes mig at være mest interessante.

Hukommelseslækage

V773 Funktionen blev afsluttet uden at frigive 'cwd'-pegepinden. En hukommelseslækage er mulig. 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;
  ....
}

Dette fragment blev taget fra winpr-under-systemet, som implementerer en wrapper til WINAPI for ikke-Windows-systemer, det vil sige, det er en letvægtsalternativ til Wine. Her kan man bemærke en lækage: hukommelsen, der er tildelt funktionen getcwd, frigives kun ved behandling af specielle tilfælde. For at løse fejlen skal der tilføjes et kald til free efter memcpy.

Array-grænseoverskridelse

V557 Det er muligt med array-grænseoverskridelse. Værdien af 'event->EventHandlerCount' indekset kunne 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 dette eksempel tilføjes et nyt element til listen, selvom antallet af elementer har nået det maksimale. Her er det nok at ændre operatoren <= og få en fungerende kommando. <, så det ikke overstiger arrayets grænser.

Der blev fundet en anden fejl af denne type:

V557 Array overrun is possible. The value of ‘iBitmapFormat’ index could reach 8. orders.c 2623

Slåfejl

Fragment 1

V547 Udtrykket ‘!pipe->In’ er altid 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;
  ....
}

Her ser vi en almindelig slåfejl: i den anden betingelse kontrolleres den samme variabel som i den første. Sandsynligvis opstod fejlen som følge af en mislykket kopiering af koden.

Fragment 2

V760 Der blev fundet to identiske tekstblokke. Den anden blok begynder fra linje 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->minorVersion);
  ....
}

Endnu en slåfejl: kommentaren til koden antyder, at der skal komme noget fra strømmen minorVersion, men læsningen sker i en variabel med navnet majorVersion. Jeg kender dog ikke protokollen, så dette er kun en antagelse.

Fragment 3

V524 Det er underligt, at kroppen af ‘trio_index_last’ funktionen er fuldstændig ækvivalent med kroppen af ‘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);
}

Ifølge kommentaren finder funktionen trio_index den første forekomst af et tegn i en streng, når trio_index_last — den sidste. Men kroppe af disse funktioner er identiske! Sandsynligvis er dette en slåfejl, og i funktionen trio_index_last skal der anvendes strrchr i stedet for strchr. Så vil adfærden være forventet.

Fragment 4

V769 Den ‘data’ pointer i udtrykket er lig med nullptr. Den resulterende værdi af aritmetiske operationer på denne pointer er meningsløs, og den bør ikke bruges. 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 ser ud til, at operatoren for negation blev tilfældigt udeladt ! tæt på data. Det er mærkeligt, at dette forblev uset.

Fragment 5

V517 Mønstret ‘if (A) {…} else if (A) {…}’ blev registreret. Der er en sandsynlighed for tilstedeværelsen af logisk fejl. Tjek linjer: 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 sidste to betingelser er de samme: det ser ud til, at nogen glemte at kontrollere dem efter kopiering. I koden er det tydeligt, at den sidste del arbejder med 4-bits værdier, så man kan antage, at den sidste betingelse skal være value <= 0x3FFFFFFF.

Der blev fundet en anden fejl af denne type:

V517 Mønsteret 'if (A) {…} else if (A) {…}' blev opdaget. Der er en sandsynlighed for, at der er til stede en logisk fejl. Kontroller linjerne: 169, 173. fil.c 169

Kontrol af inputdata

Fragment 1

V547 Udtrykket 'strcat(target, source) != NULL' er altid sandt. 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);
}

Kontrol af resultatet af funktionen i dette eksempel er ukorrekt. Funktionen strcat returnerer en peger til den endelige version af strengen, dvs. den første overførte parameter. I dette tilfælde er det target. Men hvis den er lig med NULL, så er det for sent at kontrollere den, da funktionen strcat vil dereferere det.

Fragment 2

V547 Udtrykket 'cache' er altid sandt. 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)
  {
    ....
  }
  ....
}

I dette tilfælde tildeles variablen cache en adresse til det statiske array glyphCache->glyphCache. Således kan kontrollen if (cache) springes over.

Fejl i håndtering af ressourcer

V1005 Ressourcen blev erhvervet ved hjælp af 'CreateFileA'-funktionen, men blev frigivet med den inkompatible 'fclose'-funktion. 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;
  }
  ....
}

Fildeskriptoren fp, oprettet ved kaldet til funktionen CreateFile, blev fejlagtigt lukket af funktionen fclose fra standardbiblioteket, og ikke CloseHandle.

Identiske betingelser

V581 De betingede udtryk for de 'if'-betingelser, der ligger ved siden af hinanden, er identiske. Kontroller linjerne: 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, "advarsel: NdrComplexStructBufferSize array_type: "
 "0xX ikke implementeret", 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, "advarsel: NdrComplexStructBufferSize array_type: "
 "0xX ikke implementeret", array_type);
 }
 ....
}

Måske er dette eksempel ikke en fejl. Men begge betingelser indeholder de samme meddelelser, hvoraf den ene sandsynligvis kan fjernes.

Rydning af null-pointere

V575 Den null-pointer er sendt til 'free' funktionen. Undersøg den første argument. smartcard_pcsc.c 875

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

skal du videregive filterfunktionen. Grundlæggende er et filter blot en funktion, der modtager en beskedinstans og returnerer free Det er muligt at sende en null-pointer, og analyzeren ved dette. Men hvis der identificeres en situation, hvor pointeren altid sendes null, som i dette fragment, vil der blive givet en advarsel.

Pointer mszGroupsA er oprindeligt lig med NULL og initialiseres ikke andetsteds. Den eneste kodegren, hvor pointeren kunne være blevet initialiseret, er utilgængelig.

Der var også andre meddelelser af denne type:

V575 Den null-pointer er sendt til 'free' funktionen. Undersøg den første argument. license.c 790
V575 Den null-pointer er sendt til 'free' funktionen. Undersøg den første argument. rdpsnd_alsa.c 575

Sandsynligvis opstår sådanne glemte variable under refaktorering, og de kan blot fjernes.

Mulig overfyldning

V1028 Mulig overfyldning. Overvej at kaste operander, ikke 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));
  ....
}

Casting resultatet til long er ikke en beskyttelse mod overfyldning, da beregningen i sig selv udføres ved hjælp af typen int.

Dereferencering af pointer i initialisering

V595 Pointeren 'context' blev brugt, før den blev verificeret mod nullptr. Tjek linjer: 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;
  ....
}

Her er en peger context dereferences in initialiseringen — før der sker en kontrol af det.

Der blev fundet andre fejl af denne type:

V595 Pointeren 'ntlm' blev brugt, før den blev verificeret mod nullptr. Tjek linjer: 236, 255. ntlm.c 236
V595 Pointeren 'context' blev brugt, før den blev verificeret mod nullptr. Tjek linjer: 1003, 1007. rfx.c 1003
V595 Pointeren 'rdpei' blev brugt, før den blev verificeret mod nullptr. Tjek linjer: 176, 180. rdpei_main.c 176
V595 Pointeren 'gdi' blev brugt, før den blev verificeret mod nullptr. Tjek linjer: 121, 123. xf_gfx.c 121

Meningsløs betingelse

V547 Udtrykket 'rdp->state >= CONNECTION_STATE_ACTIVE' er altid sandt. 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 er let at se, at den første betingelse er meningsløs på grund af tildeling af den tilsvarende værdi tidligere.

Ukorrekt parsing af strengen

V576 Forkert format. Overvej at tjekke det tredje faktiske argument af 'sscanf' funktionen. En peger til unsigned int type forventes. proxy.c 220

V560 En del af den betingede udtryk er altid sand: (rc >= 0). proxy.c 222

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

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

Analysatoren for dette fragment giver straks 2 advarsler. Specifikatoren %u forventer en variabel af typen unsigned int, men variablen sub har typen int. Videre ser vi en mistænkelig kontrol: betingelsen til højre giver ikke mening, da der sammenlignes med én i starten. Jeg ved ikke, hvad forfatteren af denne kode mente, men der er helt klart noget galt.

Usorterede kontroller

V547 Udtrykket 'status == 0x00090314' er altid 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 markerede betingelser vil altid være falske, da udførelsen kun når den anden betingelse, når status == SEC_E_OK. Korrekt kode kan se sådan ud:

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

Konklusion

Således afslørede projektkontrollen mange problemer, men kun den mest interessante del blev beskrevet i artiklen. Projektudviklerne kan selv kontrollere projektet ved at anmode om en midlertidig licensnøgle på hjemmesiden. PVS-Studio. Der var også falske positiver, arbejdet med hvilke vil hjælpe med at forbedre analysatoren. Ikke desto mindre er statisk analyse vigtig, hvis du vil forbedre kodekvaliteten og forkorte søgningen efter fejl, og PVS-Studio kan hjælpe med dette.

Hvis du ønsker at dele denne artikel med et engelsktalende publikum, bedes du bruge linket til oversættelsen: Sergey Larin. Kontrol af FreeRDP med PVS-Studio

Kilde: habr.com