FreeRDP je otvorena implementacija Remote Desktop Protocol (RDP), protokola koji omogućava daljinsko upravljanje računalom, razvijenog od strane Microsofta. Projekt podržava brojne platforme, uključujući Windows, Linux, macOS, pa čak i iOS i Android. Ovaj projekt je odabran prvi u okviru ciklusa članaka posvećenih provjeri RDP klijenata pomoću statičkog analizatora PVS-Studio.
Malo povijesti
Projekt pojavila se nakon što je Microsoft otvorio specifikacije svog proprietarnog RDP protokola. U to vrijeme postojao je klijent rdesktop, čija se implementacija temelji na rezultatima obrnjenog inženjeringa.
Tijekom implementacije protokola postajalo je sve teže dodavati nove funkcionalnosti zbog tadašnje arhitekture projekta. Promjene u njoj izazvale su sukob među programerima, što je dovelo do stvaranja forka rdesktop — FreeRDP. Daljnje širenje proizvoda bilo je ograničeno GPLv2 licencom, zbog čega je donesena odluka o promjeni licencije na Apache License v2. Međutim, nisu svi bili saglasni promijeniti licencu svog koda, pa su programeri odlučili prepisati projekt, rezultirajući time u današnjem izgledu kodne baze.
Više informacija o povijesti projekta možete pročitati u napomeni službenog bloga: "Povijest FreeRDP projekta".
Kao alat za otkrivanje grešaka i potencijalnih ranjivosti u kodu korišten je . To je statički analizator koda za jezike C, C++, C# i Java, dostupan na platformama Windows, Linux i macOS.
U članku su prikazane samo one greške koje su mi se činile najzanimljivijima.
Cur-memory leak
Funkcija je napustila bez oslobađanja pokazivača 'cwd'. Moguća je cur-memory leak. 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;
....
}Ovaj dio koda preuzet je iz winpr podsustava koja implementira omotu WINAPI za non-Windows sustave, tj. to je lagani analog Wine. Ovdje možemo primijetiti cur-memory leak: memorija koju dodjeljuje funkcija getcwd, oslobađa se samo u obradi posebnih slučajeva. Da bi se ispravila greška, potrebno je dodati poziv free nakon memcpy.
Prekoračenje niza
Moguće je prekoračenje niza. Vrijednost indeksa 'event->EventHandlerCount' mogla bi doseći 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++;
}
....
}U ovom primjeru novi element se dodaje u popis, čak i kada je dostignut maksimalni broj elemenata. Dovoljno je zamijeniti operator <= na <, kako bi se izbjeglo izlazak iz granica niza.
Nađena je i druga greška ove vrste:
- V557 Moguće je prekoračenje niza. Vrijednost indeksa 'iBitmapFormat' mogla bi doseći 8. orders.c 2623
Typo
Fragment 1
Izraz '!pipe->In' uvijek je lažan. 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;
....
}Ovde vidimo uobičajenu grešku: u drugom uslovu provodi se provjera iste varijable kao i u prvom. Vjerojatno je greška nastala prilikom neuspješnog kopiranja koda.
Fragment 2
Nađena su dva identična bloka teksta. Drugi blok počinje od linije 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);
....
}Još jedna greška: komentar u kodu sugerira da bi iz toka trebala doći minorVersion, međutim, čitanje se događa u varijablu s imenom majorVersion. Ipak, nisam upoznat s protokolom, tako da je to samo pretpostavka.
Fragment 3
Čudno je što je tijelo funkcije ‘trio_index_last’ potpuno ekvivalentno tijelu funkcije ‘trio_index’. 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);
}Sudeći prema komentaru, funkcija trio_index nalazi prvo podudaranje znaka u stringu, dok trio_index_last — posljednje. No tijela ovih funkcija su identična! Vjerojatno se radi o grešci, i u funkciji trio_index_last treba koristiti strrchr umjesto strchr. Tada će ponašanje biti očekivano.
Fragment 4
Pokazivač ‘data’ u izrazu jednako je nullptr. Rezultantna vrijednost aritmetičkih operacija na ovom pokazivaču je besmislena i ne bi trebala biti korištena. 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;
....
}Čini se da je ovde slučajno propušten operator negacije ! pored data. Čudno je da to nije primjećeno.
Fragment 5
Otkriven je uzorak ‘if (A) {…} else if (A) {…}’. Postoji vjerojatnost prisutnosti logičke greške. Provjerite linije: 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);
}
....
}Posljednja dva uvjeta su identična: izgleda da je netko zaboravio provjeriti ih nakon kopiranja. Iz koda se može primijetiti da posljednji dio radi s četverobajtni vrijednostima, stoga se može pretpostaviti da bi posljednji uvjet trebao biti value <= 0x3FFFFFFF.
Nađena je i druga greška ove vrste:
- V517 Uočena je upotreba obrasca 'if (A) {…} else if (A) {…}'. Postoji vjerojatnost prisutnosti logičke pogreške. Provjerite redove: 169, 173. file.c 169
Provjera ulaznih podataka
Fragment 1
Izraz 'strcat(target, source) != NULL' je uvijek točan. 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);
}Provjera rezultata izvedbe funkcije u ovom primjeru nije ispravna. Funkcija strcat vraća pokazivač na konačnu verziju stringa, tj. prvi proslijeđeni parametar. U ovom slučaju to je cilj. Međutim, ako je on jednak NULL, provjeriti ga je prekasno, jer će u funkciji strcat doći do njegovog dereferenciranja.
Fragment 2
Izraz 'cache' je uvijek točan. 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)
{
....
}
....
}U ovom slučaju varijabli cache dodijeljuje se adresa statičkog niza glyphCache->glyphCache. Tako da se provjera if (cache) može izostaviti.
Pogreška u upravljanju resursima
Resurs je stečen korištenjem funkcije 'CreateFileA', ali je pušten koristeći nespojivu funkciju '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;
}
....
}Deskriptor datoteke fp, stečen pozivom funkcije CreateFile, greškom je zatvoren funkcijom fclose iz standardne biblioteke, a ne CloseHandle.
Identični uvjeti
Uvjeti izraza 'if' koji su postavljeni jedan pored drugog su identični. Provjerite redove: 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);
}
....
}Ovo možda nije greška. Međutim, oba uslova sadrže iste poruke, od kojih jednu najvjerojatnije treba ukloniti.
Čišćenje nultih pokazivača
Nulti pokazivač je prosleđen u funkciju 'free'. Proverite prvi argument. smartcard_pcsc.c 875
WINSCARDAPI LONG WINAPI PCSC_SCardListReadersW(
SCARDCONTEXT hContext,
LPCWSTR mszGroups,
LPWSTR mszReaders,
LPDWORD pcchReaders)
{
LPSTR mszGroupsA = NULL;
....
mszGroups = NULL;
/* mszGroups nije podržan od strane 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);
....
}U funkciju free Može se proslediti nulti pokazivač i analizator o tome zna. Ali ako se otkrije situacija u kojoj se pokazivač uvek prosleđuje kao nulti, kao u ovom fragmentu, biće prikazano upozorenje.
Pokazivač mszGroupsA prvobitno jednak NULL i nigde više ne inicijalizuje. Jedina grana koda gde je pokazivač mogao biti inicijalizovan je nedostupna.
Postojale su i druge poruke ovaj tip:
- V575 Nulti pokazivač je prosleđen u funkciju 'free'. Proverite prvi argument. license.c 790
- V575 Nulti pokazivač je prosleđen u funkciju 'free'. Proverite prvi argument. rdpsnd_alsa.c 575
Verovatno slične zaboravljene promenljive nastaju tokom refaktorisanja i mogu se jednostavno ukloniti.
Moguć preliv
Moguće prelivanje. Razmotrite kastovanje operanada, a ne rezultata. 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));
....
}Kastovanje rezultata u long nije zaštita od prelivanja, jer se samo izračunavanje vrši koristeći tip int.
Dereferenciranje pokazivača u inicijalizaciji
Pokazivač 'context' je korišćen pre nego što je verifikovan protiv nullptr. Proverite linije: 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;
....
}Ovdje je pokazivač kontekst dereferenciran prilikom inicijalizacije — prije nego što je provjeren.
Pronađene su i druge greške ovog tipa:
- V595 Pokazivač 'ntlm' korišten je prije nego što je provjeren protiv nullptr. Provjerite redove: 236, 255. ntlm.c 236
- V595 Pokazivač 'context' korišten je prije nego što je provjeren protiv nullptr. Provjerite redove: 1003, 1007. rfx.c 1003
- V595 Pokazivač 'rdpei' korišten je prije nego što je provjeren protiv nullptr. Provjerite redove: 176, 180. rdpei_main.c 176
- V595 Pokazivač 'gdi' korišten je prije nego što je provjeren protiv nullptr. Provjerite redove: 121, 123. xf_gfx.c 121
Besmisleni uvjet
Izraz 'rdp->state >= CONNECTION_STATE_ACTIVE' je uvijek istinit. 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;
}
....
}
....
}Lako je primijetiti da prvi uvjet nema smisla zbog dodjeljivanja odgovarajuće vrijednosti ranije.
Nekorektan razlomak
Nekorektan format. Provjerite treći stvarni argument funkcije ‘sscanf’. Očekuje se pokazivač tipa unsigned int. proxy.c 220
Dio uvjetnog izraza je uvijek istinit: (rc >= 0). proxy.c 222
static BOOL check_no_proxy(....)
{
....
int sub;
int rc = sscanf(range, "%u", &sub);
if ((rc == 1) && (rc >= 0))
{
....
}
....
}Analizator za ovaj fragment generira odmah 2 upozorenja. Specifikator %u očekuje varijablu tipa unsigned int, ali varijabla sub ima tip int. Dalje, vidimo sumnjivu provjeru: uvjet s desne strane nema smisla, jer na početku dolazi do usporedbe s jedinicom. Ne znam što je autor ovog koda mislio, ali očigledno nešto nije u redu.
Nepoređene provjere
Izraz 'status == 0x00090314' je uvijek lažan. 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;
....
}Označeni uvjeti će uvijek biti lažni, jer će izvođenje doći do drugog uvjeta samo ako status == SEC_E_OK. Ispravan kod može izgledati ovako:
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;
}Zaključak
Tako je, provjera projekta otkrila brojne probleme, no samo je najzanimljiviji dio njih opisan u članku. Razvijači projekta mogu sami provjeriti projekt zatraživanjem privremenog ključa licence na web stranici . Bilo je i lažnih alarma, rad na kojima će pomoći poboljšati analitičar. Ipak, statička analiza je važna ako želite ne samo poboljšati kvalitetu koda, već i smanjiti vrijeme provedeno u traženju grešaka, a PVS-Studio vam može pomoći u tome.
Ako želite podijeliti ovaj članak s anglofonim publikom, molim vas koristite vezu na prijevod: Sergey Larin.
Izvor: habr.com
