FreeRDP je open-source implementácia protokolu Remote Desktop Protocol (RDP), protokolu vyvinutého spoločnosťou Microsoft pre vzdialené ovládanie počítačov. Projekt podporuje viacero platforiem vrátane Windows, Linux, macOS a dokonca aj iOS s AndroidTento projekt bol vybraný ako prvý zo série článkov venovaných testovaniu RDP klientov pomocou statického analyzátora PVS-Studio.
Trocha histórie
Projekt došlo po tom, čo spoločnosť Microsoft otvorila špecifikácie svojho proprietárneho protokolu RDP. V tom čase existoval klient rdesktop, ktorého implementácia bola založená na výsledkoch Reverse Engineering.
Keď bol protokol implementovaný, bolo ťažšie pridať novú funkcionalitu kvôli vtedy existujúcej architektúre projektu. Zmeny v ňom vyvolali konflikt medzi vývojármi, ktorý viedol k vytvoreniu forku rdesktop - FreeRDP. Ďalšia distribúcia produktu bola obmedzená licenciou GPLv2, v dôsledku čoho bolo prijaté rozhodnutie o jeho prelicencovaní na licenciu Apache v2. Nie všetci však súhlasili so zmenou licencie svojho kódu, a tak sa vývojári rozhodli projekt prepísať, výsledkom čoho je moderná kódová základňa.
Viac o histórii projektu si môžete prečítať v oficiálnom blogovom príspevku: “História projektu FreeRDP”.
Používa sa ako nástroj na identifikáciu chýb a potenciálnych zraniteľností v kóde. Je to statický analyzátor kódu pre C, C++, C# a Javu, dostupný na platformách Windows, Linux и macOS.
Článok uvádza len tie chyby, ktoré sa mi zdali najzaujímavejšie.
Únik pamäte
Funkcia bola ukončená bez uvoľnenia ukazovateľa 'cwd'. Je možný únik pamäte. prostredie.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;
....
}Tento fragment bol prevzatý zo subsystému winpr, ktorý implementuje obal WINAPI pre ne-Windows systémy, t. j. je to odľahčený analóg Wine. Tu vidíte únik: pamäť pridelená funkciou getcwd, sa uvoľňuje len pri vybavovaní špeciálnych prípadov. Ak chcete chybu opraviť, musíte pridať hovor zadarmo po memcpy.
Pole mimo hraníc
Prekročenie poľa je možné. Hodnota indexu 'event->EventHandlerCount' by mohla dosiahnuť 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++;
}
....
}Tento príklad pridá nový prvok do zoznamu, aj keď počet prvkov dosiahol maximum. Tu stačí vymeniť operátor <= na <, aby neprekročili hranice poľa.
Bola zistená ďalšia chyba tohto typu:
- V557 Prekročenie poľa je možné. Hodnota indexu 'iBitmapFormat' by mohla dosiahnuť 8. orders.c 2623
Preklepy
Fragment 1
Výraz '!pipe->In' je vždy nepravdivý. 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;
....
}Tu vidíme bežný preklep: druhá podmienka kontroluje rovnakú premennú ako prvá. S najväčšou pravdepodobnosťou sa chyba objavila v dôsledku neúspešného kopírovania kódu.
Fragment 2
Našli sa dva bloky rovnakého textu. Druhý blok začína od riadku 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);
....
}Ďalší preklep: komentár kódu naznačuje, že vlákno by malo prísť minorVerzia, čítanie však prebieha do premennej s názvom hlavná verzia. Nie som však oboznámený s protokolom, takže je to len odhad.
Fragment 3
Je zvláštne, že telo funkcie 'trio_index_last' je plne ekvivalentné telu funkcie '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);
}Súdiac podľa komentára, funkcie trio_index nájde prvý zhodný znak v reťazci, keď trio_index_last - posledná vec. Ale telá týchto funkcií sú identické! S najväčšou pravdepodobnosťou ide o preklep a vo funkcii trio_index_last treba použiť strhrchr namiesto strchr. Potom bude správanie očakávané.
Fragment 4
Ukazovateľ „údaje“ vo výraze sa rovná nullptr. Výsledná hodnota aritmetických operácií na tomto ukazovateli je nezmyselná a nemala by sa používať. 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;
....
}Zdá sa, že operátor negácie sa tu náhodou minul ! Blízko data. Je zvláštne, že to zostalo nepovšimnuté.
Fragment 5
Bolo zistené použitie vzoru „ak (A) {…} else if (A) {…}“. Existuje pravdepodobnosť výskytu logickej chyby. Kontrolné riadky: 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);
}
....
}Posledné dve podmienky sú rovnaké: zrejme ich niekto zabudol po skopírovaní skontrolovať. Z kódu je zrejmé, že posledná časť pracuje so štvorbajtovými hodnotami, takže môžeme predpokladať, že posledná podmienka by mala byť hodnota <= 0x3FFFFFFFF.
Bola zistená ďalšia chyba tohto typu:
- V517 Bolo zistené použitie vzoru „ak (A) {…} else if (A) {…}“. Existuje pravdepodobnosť výskytu logickej chyby. Kontrolné riadky: 169, 173. súbor.c 169
Validácia vstupných údajov
Fragment 1
Výraz 'strcat(cieľ, zdroj) != NULL' je vždy pravdivý. 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);
}Kontrola výsledku funkcie v tomto príklade je nesprávna. Funkcia strcat vráti ukazovateľ na konečnú verziu reťazca, t.j. prvý parameter prešiel. V tomto prípade je to tak terč. Ak je však rovný NULOVÝ, potom je príliš neskoro to skontrolovať, pretože vo funkcii strcat bude dereferencovaná.
Fragment 2
Výraz 'cache' je vždy pravdivý. 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)
{
....
}
....
}V tomto prípade premenná medzipamäte je priradená adresa statického poľa glyphCache->glyphCache. Preto skontrolujte if (cache) možno vynechať.
Chyba správy zdrojov
Zdroj bol získaný pomocou funkcie „CreateFileA“, ale bol uvoľnený pomocou nekompatibilnej funkcie „fclose“. certifikát.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 súboru fp, vytvorený volaním funkcie CreateFile zatvorené omylom funkciou fclose zo štandardnej knižnice, nie CloseHandle.
Rovnaké podmienky
Podmienené výrazy príkazov „ak“ umiestnené vedľa seba sú identické. Kontrolné riadky: 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);
}
....
}Tento príklad nemusí byť chyba. Obe podmienky však obsahujú rovnaké správy, z ktorých jedna môže byť s najväčšou pravdepodobnosťou odstránená.
Čistenie nulových ukazovateľov
Nulový ukazovateľ je odovzdaný do funkcie „free“. Skontrolujte prvý 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);
....
}Vo funkcii zadarmo môžete prejsť nulovým ukazovateľom a analyzátor o tom vie. Ak sa však zistí situácia, v ktorej je ukazovateľ vždy nulový, ako v tomto úryvku, vydá sa varovanie.
Index mszGroupsA spočiatku rovnaké NULOVÝ a nie je inicializovaný nikde inde. Jediná vetva kódu, kde by sa dal inicializovať ukazovateľ, je nedostupná.
Boli tam aj ďalšie správy ako:
- V575 Nulový ukazovateľ je odovzdaný do funkcie 'free'. Skontrolujte prvý argument. licencia.c 790
- V575 Nulový ukazovateľ je odovzdaný do funkcie 'free'. Skontrolujte prvý argument. rdpsnd_alsa.c 575
S najväčšou pravdepodobnosťou takéto zabudnuté premenné vznikajú počas procesu refaktorizácie a možno ich jednoducho odstrániť.
Možný prepad
Možný prepad. Zvážte pretypovanie operandov, nie výsledok. 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));
....
}Prinesenie výsledku do dlho nie je ochrana proti pretečeniu, keďže samotný výpočet prebieha pomocou typu int.
Dereferencia ukazovateľa pri inicializácii
Ukazovateľ 'kontext' bol použitý predtým, ako bol overený proti nullptr. Kontrolné riadky: 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;
....
}Tu je ukazovateľ kontext je dereferencovaná počas inicializácie - pred jej kontrolou.
Boli nájdené ďalšie chyby tohto typu:
- V595 Ukazovateľ 'ntlm' bol použitý pred overením voči nullptr. Skontrolujte riadky: 236, 255. ntlm.c 236
- V595 Ukazovateľ 'kontext' bol použitý predtým, ako bol overený voči nullptr. Kontrolné riadky: 1003, 1007. rfx.c 1003
- V595 Ukazovateľ 'rdpei' bol použitý pred overením voči nullptr. Kontrolné riadky: 176, 180. rdpei_main.c 176
- V595 Ukazovateľ 'gdi' bol použitý pred overením voči nullptr. Skontrolujte riadky: 121, 123. xf_gfx.c 121
Nezmyselný stav
Výraz 'rdp->state >= CONNECTION_STATE_ACTIVE' je vždy pravdivý. spojenie.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;
}
....
}
....
}Je ľahké vidieť, že prvá podmienka nemá zmysel, pretože zodpovedajúca hodnota bola priradená skôr.
Nesprávna analýza reťazca
Nesprávny formát. Zvážte kontrolu tretieho aktuálneho argumentu funkcie 'sscanf'. Očakáva sa ukazovateľ na typ int unsigned. proxy.c 220
Časť podmieneného výrazu je vždy pravdivá: (rc >= 0). proxy.c 222
static BOOL check_no_proxy(....)
{
....
int sub;
int rc = sscanf(range, "%u", &sub);
if ((rc == 1) && (rc >= 0))
{
....
}
....
}Analyzátor okamžite vydá 2 varovania pre tento fragment. Špecifikátor %u očakáva premennú typu nepodpísané int, ale variabilné nižšie má typ int. Ďalej vidíme podozrivú kontrolu: podmienka vpravo nedáva zmysel, keďže na začiatku je porovnanie s jedným. Neviem, čo tým autor tohto kódu myslel, ale niečo tu zjavne nie je v poriadku.
Kontroly mimo poradia
Výraz 'stav == 0x00090314' je vždy nepravdivý. 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;
....
}Začiarknuté podmienky budú vždy nepravdivé, pretože vykonanie dosiahne druhú podmienku iba vtedy, ak stav == SEC_E_OK. Správny kód môže vyzerať takto:
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;
}Záver
Kontrola projektu teda odhalila veľa problémov, no v článku bola popísaná len najzaujímavejšia časť z nich. Vývojári projektu môžu sami skontrolovať projekt vyžiadaním dočasného licenčného kľúča na webovej stránke . Vyskytli sa aj falošné pozitíva, práca na ktorých pomôže zlepšiť analyzátor. Statická analýza je však dôležitá, ak chcete nielen zlepšiť kvalitu svojho kódu, ale aj skrátiť čas strávený hľadaním chýb a PVS-Studio vám s tým môže pomôcť.
Ak chcete tento článok zdieľať s anglicky hovoriacim publikom, použite odkaz na preklad: Sergey Larin.
Zdroj: hab.com
