FreeRDP – otevřená implementace Remote Desktop Protocol (RDP), protokolu pro vzdálenou správu počítačů, vyvinutého společností Microsoft. Projekt podporuje různé platformy, včetně Windows, Linux, macOS a dokonce i iOS s Androidem. Tento projekt byl vybrán jako první v rámci série článků zaměřených na testování RDP klientů pomocí statického analyzátoru PVS-Studio.
Trocha historie
Projekt vznikl poté, co Microsoft zveřejnil specifikace svého proprietárního protokolu RDP. V té době existoval klient rdesktop, jehož implementace vychází z výsledků reverzního inženýrství.
V průběhu implementace protokolu se stávalo obtížnější přidávat nové funkce kvůli tehdejší architektuře projektu. Změny v ní vyvolaly konflikt mezi vývojáři, což vedlo k vytvoření forku rdesktop – FreeRDP. Další šíření produktu bylo omezeno licencí GPLv2, což vedlo k rozhodnutí o relicencování na Apache License v2. Nicméně ne všichni souhlasili se změnou licence svého kódu, a proto se vývojáři rozhodli přepsat projekt, což vedlo k moderní podobě kódové základny.
Podrobnější informace o historii projektu naleznete v příspěvku oficiálního blogu: „Historie projektu FreeRDP“.
Jako nástroj pro odhalování chyb a potenciálních zranitelností v kódu bylo použito . Je to statický analyzátor kódu pro jazyky C, C++, C# a Java, dostupný na platformách Windows, Linux a macOS.
V článku jsou prezentovány pouze ty chyby, které se mi zdály nejzajímavější.
Únik paměti
Funkce byla ukončena bez uvolnění ukazatele ‘cwd’. Je možné, že dojde k úniku paměti. 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;
....
}Tento fragment byl převzat z podsystemu winpr, který implementuje obal WINAPI pro systémy mimo Windows, tedy je to lehký ekvivalent Wine. Zde je vidět únik: paměť alokovaná funkcí getcwd, se uvolňuje pouze při zpracování speciálních případů. K odstranění chyby je třeba přidat volání free po memcpy.
Překročení hranic pole
Je možné překročení hranic pole. Hodnota indexu ‚event->EventHandlerCount‘ by mohla dosáhnout 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++;
}
....
}V tomto příkladu je nový prvek přidán do seznamu, i když počet prvků dosáhl maxima. Stačí nahradit operátor <= na <, abychom se vyhnuli překročení hranic pole.
Byla nalezena další chyba tohoto typu:
- V557 Může dojít k přeplnění pole. Hodnota indexu 'iBitmapFormat' může dosáhnout 8. orders.c 2623
Tiskové chyby
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;
....
}Zde vidíme běžnou chybu: ve druhé podmínce se kontroluje ta samá proměnná jako v první. Pravděpodobně k chybě došlo v důsledku neúspěšného kopírování kódu.
Fragment 2
Byly nalezeny dva identické bloky textu. Druhý blok začíná na řádku 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 byty) */
Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
/* MinorVersion (2 byty) */
Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
....
}Další chyba: komentář ke kódu naznačuje, že z proudu by měla přijít minorVersion, avšak čtení probíhá do proměnné s názvem majorVersion. Přestože s protokolem neznám, je to jen domněnka.
Fragment 3
Je divné, že tělo funkce 'trio_index_last' je plně ekvivalentní tělu funkce '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);
}Podle komentáře funkce trio_index nalezne první shodu znaku v řetězci, když trio_index_last — je poslední. Ale těla těchto funkcí jsou identická! Pravděpodobně se jedná o chybu a ve funkci trio_index_last je potřeba použít strrchr místo strchr. Pak bude chování očekávané.
Fragment 4
Ukazatel 'data' v výrazu je roven nullptr. Výsledná hodnota aritmetických operací na tomto ukazateli je bezvýznamná a neměla by být použita. 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á se, že zde byl náhodně vynechán operátor negace ! vedle data. Je divné, že si toho nikdo nevšiml.
Fragment 5
Byl detekován vzor 'if (A) {...} else if (A) {...}'. Existuje pravděpodobnost přítomnosti logické chyby. Zkontrolujte řádky: 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);
}
....
}Poslední dvě podmínky jsou shodné: zřejmě někdo zapomněl je zkontrolovat po kopírování. Z kódu je patrné, že poslední část pracuje se čtyřbytečnými hodnotami, takže lze předpokládat, že poslední podmínka měla být value <= 0x3FFFFFFF.
Byla nalezena další chyba tohoto typu:
- V517 Byla detekována konstrukce ‘if (A) {…} else if (A) {…}’. Existuje pravděpodobnost výskytu logické chyby. Zkontrolujte řádky: 169, 173. file.c 169
Kontrola vstupních dat
Fragment 1
Výraz ‘strcat(target, source) != NULL’ je vždy pravda. 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 provedení funkce v tomto příkladu není správná. Funkce strcat vrací ukazatel na konečnou variantu řetězce, tj. první předaný parametr. V tomto případě je to target. Pokud je však roven NULL, tak je pozdě ho kontrolovat, protože ve funkci strcat dojde k dereferenci.
Fragment 2
Výraz ‘cache’ je vždy pravda. 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 případě je proměnné cache přiřazen adres statického pole glyphCache->glyphCache. Tímto způsobem lze kontrolu if (cache) přeskočit.
Chyba správy prostředků
Prostředek byl získán pomocí funkce ‘CreateFileA’ ale byl uvolněn pomocí nekompatibilní funkce ‘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;
}
....
}Identifikátor souboru fp, vytvořený voláním funkce CreateFile, byl omylem uzavřen funkcí fclose standardní knihovny, a nikoli CloseHandle.
Shodné podmínky
Podmínkové výrazy ‘if’ se nacházející vedle sebe jsou shodné. Zkontrolujte řádky: 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 neimplementováno", 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 neimplementováno", array_type);
}
....
}Tento příklad nemusí být chybou. Nicméně, obě podmínky obsahují stejné zprávy, z nichž jedna může být pravděpodobně odstraněna.
Údržba nulových ukazatelů
Null pointer je předán do funkce 'free'. Zkontrolujte první argument. smartcard_pcsc.c 875
WINSCARDAPI LONG WINAPI PCSC_SCardListReadersW(
SCARDCONTEXT hContext,
LPCWSTR mszGroups,
LPWSTR mszReaders,
LPDWORD pcchReaders)
{
LPSTR mszGroupsA = NULL;
....
mszGroups = NULL; /* mszGroups není podporován 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);
....
}Do funkce free Nulový ukazatel může být předán a analyzátor o tom ví. Ale pokud nastane situace, kdy je ukazatel vždy předáván jako nulový, jako v tomto fragmentu, bude vydáno varování.
Ukazatel mszGroupsA původně se rovná NULL a nikde jinde není inicializován. Jediná větev kódu, kde by se ukazatel mohl inicializovat, je nedosažitelná.
Byly i další zprávy tohoto typu:
- V575 Null pointer je předán do funkce 'free'. Zkontrolujte první argument. license.c 790
- V575 Null pointer je předán do funkce 'free'. Zkontrolujte první argument. rdpsnd_alsa.c 575
Pravděpodobně vznikají podobné zapomenuté proměnné během refaktoringu a mohou být jednoduše odstraněny.
Možné přetečení
Možné přetečení. Zvažte převod operandů, nikoliv výsledku. 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));
....
}Převod výsledku na long není ochranou proti přetečení, protože samotné výpočty probíhají s použitím typu int.
Derek dereferencování ukazatele v inicializaci
Ukazatel 'context' byl použit předtím, než byl ověřen proti nullptr. Zkontrolujte řádky: 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;
....
}Zde je ukazatel context je dereferencován při inicializaci — dříve, než probíhá jeho kontrola.
Byly nalezeny i další podobné chyby:
- V595 Ukazatel 'ntlm' byl použit před jeho ověřením proti nullptr. Zkontrolujte řádky: 236, 255. ntlm.c 236
- V595 Ukazatel 'context' byl použit před jeho ověřením proti nullptr. Zkontrolujte řádky: 1003, 1007. rfx.c 1003
- V595 Ukazatel 'rdpei' byl použit před jeho ověřením proti nullptr. Zkontrolujte řádky: 176, 180. rdpei_main.c 176
- V595 Ukazatel 'gdi' byl použit před jeho ověřením proti nullptr. Zkontrolujte řádky: 121, 123. xf_gfx.c 121
Bezsmyslná podmínka
Výraz 'rdp->state >= CONNECTION_STATE_ACTIVE' je vždy pravdivý. 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;
}
....
}
....
}Je snadné si všimnout, že první podmínka nedává smysl kvůli přiřazení odpovídající hodnoty dříve.
Nesprávná analýza řetězce
Nesprávný formát. Zvažte kontrolu třetího skutečného argumentu funkce 'sscanf'. Očekává se ukazatel na typ unsigned int. proxy.c 220
Část podmínkové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))
{
....
}
....
}Analyzer pro tento fragment vykazuje hned 2 varování. Specifikátor %u očekává proměnnou typu unsigned int, ale proměnná sub má typ int. Dále vidíme podezřelou kontrolu: podmínka napravo nedává smysl, protože na začátku se porovnává s jedničkou. Nevím, co tímto kód měl autor na mysli, ale rozhodně zde něco není v pořádku.
Nepřehledné kontroly
Výraz 'status == 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;
....
}Označené podmínky budou vždy nepravdivé, protože vykonání dosáhne druhé podmínky pouze v případě, že status == SEC_E_OK. Správný kód by mohl vypadat 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ávěr
Kontrola projektu odhalila řadu problémů, ale pouze ta nejzajímavější část byla popsána v článku. Vývojáři projektu si mohou místo toho zkontrolovat projekt a požádat o dočasný licenční klíč na stránkách. . Byly také falešné poplachy, na kterých bude třeba pracovat, aby se zlepšil analyzátor. Nicméně, statická analýza je důležitá, pokud chcete nejen zvýšit kvalitu kódu, ale také zkrátit čas strávený hledáním chyb, a PVS-Studio vám s tím může pomoci.
Pokud chcete sdílet tento článek s anglicky mluvící veřejností, použijte prosím odkaz na překlad: Sergey Larin.
Zdroj: habr.com
