FreeRDP kontrollimine PVS-Studio analysaatori abil

FreeRDP – avatud rakendus Remote Desktop Protokollile (RDP), mis võimaldab kaugtöötlemist, mille on välja töötanud Microsoft. Projekt toetab paljusid platvorme, sealhulgas Windows, Linux, macOS ja isegi iOS koos Androidiga. See projekt valiti esimeseks artiklite seerias, mis on pühendatud RDP-klientide kontrollimisele staatilise analüsaatori PVS-Studio abil.

Enne kui liigume edasi

Projekt FreeRDP sai alguse pärast seda, kui Microsoft avas oma patenteeritud RDP protokolli spetsifikatsioonid. Sel hetkel oli olemas klient rdesktop, mille rakendamine põhines tagurpidi inseneritöö tulemustel.

Protokolli rakendamisel muutus uue funktsionaalsuse lisamine keerulisemaks olemasoleva projekti arhitektuuri tõttu. Selle muutused tõid kaasa konflikti arendajate vahel, mis viis rdesktopi forkimiseni – FreeRDP. Toote edasine levik oli piiratud GPLv2 litsentsiga, mille tulemusena otsustati relitsentseerida Apache License v2-le. Siiski ei olnud kõik nõus oma koodi litsentsi muutma, seetõttu otsustasid arendajad projekti ümber kirjutada, mille tulemusena on meil tänapäevasel kujul koodibaas.

Projektist saab põhjalikumalt lugeda ametlikus blogipostituses: „FreeRDP projekti ajalugu“.

Vigade ja võimalike haavatavuste tuvastamise vahendina koodis kasutati PVS-Studio. See on staatiline analüsaator keelte C, C++, C# ja Java jaoks, mis on saadaval Windowsi, Linuxi ja macOSi platvormidel.

Artiklis on esitatud vaid need vead, mis tundusid mulle kõige huvitavamad.

Mälu leke

V773 Funktsioon lõpetati, vabastamata 'cwd' pointerit. 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;
  ....
}

Antud fragment on võetud winpri alam-süsteemist, mis rakendab WINAPI mähist mitte-Windowsi süsteemidele, st see on kerge analoog Wine'ist. Siit võime näha leket: mälu, mille funktsioon getcwd, vabastatakse ainult erijuhtumite töötlemisel. Veaga tegelemiseks tuleb lisada kutsumine free pärast memcpy.

Massiivi piirilähedus

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 loendisse, isegi kui elementide arv on maksimaalne. Siin piisab operaatori muutmisest <= . Tundub, et <, et mitte ületada massiivi piire.

Leiti veel üks sellise tüübi viga:

• V557 Massiivi ületamine on võimalik. ‘iBitmapFormat’ indeksi väärtus võib ulatuda 8. orders.c 2623

Trükivead

Fragment 1

V547 Avaldis ‘!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 esimeses. Tõenäoliselt tekkis viga ebaõnnestunud koodi kopeerimise tõttu.

Fragment 2

V760 Leiti kaks identset tekstilõiku. Teine lõik 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 viitab sellele, et voog peab saadetama minorVersion, kuid lugemine toimub muutuja nimega majorVersion. Siiski ei tunne ma protokolli, nii et see on lihtsalt oletus.

Fragment 3

V524 On kummaline, 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);
}

Kommenteerimise põhjal näitab funktsioon trio_index esimese vastavuse leidmist sümboli seas, kui trio_index_last — viimane. Kuid nende funktsioonide kehad on identsete! Tõenäoliselt on see trükiviga, ja funktsioonis trio_index_last tuleb kasutada strrchr asemel strchr. Siis on käitumine oodatud.

Fragment 4

V769 Avaldises on ‘data’ указатель равен NULL. Matemaatiliste operatsioonide tulemus selle pointeriga 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 siin jäi juhuslikult negatsioon operaator vahele ! lähedal data. Kummaline, et see jäi tähelepanuta.

Fragment 5

V517 Leiti ‘if (A) {…} else if (A) {…}’ mustri kasutamine. Loogilise vea tõenäosus on olemas. Kontrollige realt: 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 sama: ilmselt keegi unustas need pärast kopeerimist üle vaadata. Koode analüüsides on näha, et viimane osa töötab neljabaidiste väärtustega, seega võib oletada, et viimane tingimus peaks olema value <= 0x3FFFFFFF.

Leiti veel üks sellise tüübi viga:

• V517 Tuvastati 'if (A) {...} else if (A) {...}' kasutusmuster. On tõenäosus, et esineb loogiline viga. Kontrolli ridu: 169, 173. file.c 169

Sissetulevate andmete kontrollimine

Fragment 1

V547 Avaldus '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 tagastamise tulemuse kontrollimine sellel näitel on vale. Funktsioon strcat tagastab näitaja stringi lõppvariandile, st esimesele edastatud parameetrile. Antud juhul on see target. Siiski, kui see on NULL, siis on selle kontrollimine hilja, kuna funktsioonis strcat toimub selle de-referentsimine.

Fragment 2

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

Sellisel juhul saab muutuja cache aadressi staatilise massiivi glyphCache->glyphCache. Seega võib kontrolli if (cache) õigustada.

Ressursi haldamise viga

V1005 Ressurss sai kätte 'CreateFileA' funktsiooni kaudu, kuid vabastati mittevastava 'fclose' funktsiooni abil. 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 abil, suletakse vale pärast funktsiooni fclose kasutades standardset teeki, mitte CloseHandle.

Sarnased tingimused

V581 'if' lauseid, mis asuvad külg-külje kõrval, tingimused on identsed. Kontrolli 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: "
 "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);
 }
 ....
}

See possibly this example is not an error. However, both conditions contain the same message, one of which can likely be removed.

Null Pointer Cleanup

V575 The null pointer is passed into ‘free’ function. Inspect the first 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);
  ....
}

To the function free you can pass a null pointer, and the parser knows about it. But if a situation arises where the pointer is always passed as null, as in this fragment, a warning will be issued.

Pointer mszGroupsA initially equals NULL and is not initialized anywhere else. The only branch of code where the pointer could be initialized is unreachable.

There have been other messages of this type:

• V575 The null pointer is passed into ‘free’ function. Inspect the first argument. license.c 790
• V575 The null pointer is passed into ‘free’ function. Inspect the first argument. rdpsnd_alsa.c 575

Such forgotten variables likely appear during refactoring and can simply be removed.

Possible Overflow

V1028 Possible overflow. Consider casting operands, not the result. 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 the result to long is not protection against overflow, as the calculation itself is done using the type int.

Dereferencing the pointer during initialization

V595 The ‘context’ pointer was utilized before it was verified against nullptr. Check lines: 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 on viitaja context de-referencitakse algväärtustamisel – enne, kui seda kontrollitakse.

Leiti ka muid sellise tüüpi vigu:

• V595 Viitaja 'ntlm' kasutati enne, kui see oli kontrollitud nullptri vastu. Kontrollige ridu: 236, 255. ntlm.c 236
• V595 Viitaja 'context' kasutati enne, kui see oli kontrollitud nullptri vastu. Kontrollige ridu: 1003, 1007. rfx.c 1003
• V595 Viitaja 'rdpei' kasutati enne, kui see oli kontrollitud nullptri vastu. Kontrollige ridu: 176, 180. rdpei_main.c 176
• V595 Viitaja 'gdi' kasutati enne, kui see oli kontrollitud nullptri vastu. Kontrollige ridu: 121, 123. xf_gfx.c 121

Tühine tingimus

V547 Väide '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;
      }
    ....
  }
  ....
}

Märkab, et esimene tingimus ei oma mõtet, kuna vastav väärtus on antud varem.

Vale stringi parsimine

V576 Vale formaat. Vaadake 'sscanf' funktsiooni kolmandat tegelikku argumenti. Oodatakse suurt numbrityypi viitajat. proxy.c 220

V560 A part of conditional expression is always true: (rc >= 0). proxy.c 222

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

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

Analüsaator annab sellele fragmendile kohe 2 hoiatust. Spetsifikaator %u ootab muutuja tüüpi unsigned int, kuid muutuja sub on tüübiga int. Edasi näeme kahtlast kontrolli: parempoolne tingimus ei oma mõtet, kuna alguses on võrdlus ühele. Ei tea, mida selle koodi autor silmas pidas, kuid siin on kindlasti midagi valesti.

Korraldamata kontrollid

V547 Väide '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ärgitud tingimused on alati vale, kuna teostamine jõuab teise tingimuseni ainult siis, kui status == SEC_E_OK. Õige kood võiks välja näha nii:

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, projekti kontroll näitas mitmeid probleeme, kuid ainult kõige huvitavam osa neist oli artiklis kirjeldatud. Projekti arendajad saavad projekti ise kontrollida, taotledes ajutist litsentsivõtit saidilt. PVS-StudioTekkisid ka valehäired, mille kallal töötamine aitab analüsaatorit parandada. Sellegipoolest on staatiline analüüs oluline, kui soovite mitte ainult parandada koodi kvaliteeti, vaid ka vähendada vigade otsimiseks kuluvat aega, ja PVS-Studio saab selles aidata.

Kui soovite seda artiklit jagada ingliskeelsele publikule, siis palun kasutage tõlke linki: Sergey Larin. FreeRDP kontrollimine PVS-Studioga

Allikas: habr.com