FreeRDP je open-source implementace protokolu Remote Desktop Protocol (RDP), což je protokol vyvinutý společností Microsoft pro vzdálené ovládání počítačů. Projekt podporuje více platforem, včetně Windows, Linux, macOS a dokonce i iOS s AndroidTento projekt byl vybrán jako první ze série článků věnovaných testování RDP klientů pomocí statického analyzátoru PVS-Studio.
Trocha historie
projekt došlo poté, co Microsoft otevřel specifikace pro svůj proprietární protokol RDP. V té době existoval klient rdesktop, jehož implementace byla založena na výsledcích Reverse Engineering.
Jak byl protokol implementován, bylo obtížnější přidávat nové funkce kvůli tehdy existující architektuře projektu. Změny v něm vyvolaly konflikt mezi vývojáři, který vedl k vytvoření forku rdesktop - FreeRDP. Další distribuce produktu byla omezena licencí GPLv2, v důsledku čehož bylo rozhodnuto o jeho přelicencování na licenci Apache v2. Ne všichni však souhlasili se změnou licence svého kódu, a tak se vývojáři rozhodli projekt přepsat a výsledkem je moderní kódová základna.
Více o historii projektu si můžete přečíst v oficiálním příspěvku na blogu: “Historie projektu FreeRDP”.
Používá se jako nástroj k identifikaci chyb a potenciálních zranitelností v kódu. Jedná se o statický analyzátor kódu pro C, C++, C# a Javu, dostupný na platformách Windows, Linux и macOS.
Článek uvádí 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ý únik paměti. prostředí.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 ze subsystému winpr, který implementuje wrapper WINAPI pro ne-Windows systémy, tj. je to odlehčená obdoba Wine. Zde můžete vidět únik: paměť alokovaná funkcí getcwd, se uvolňuje pouze při vyřizování speciálních případů. Chcete-li chybu opravit, musíte přidat hovor uvolnit po memcpy.
Pole mimo hranice
Překročení pole je možné. 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++;
}
....
}Tento příklad přidá nový prvek do seznamu, i když počet prvků dosáhl maxima. Zde stačí vyměnit operátor <= na <, aby nepřekročila hranice pole.
Byla nalezena další chyba tohoto typu:
- V557 Přetečení pole je možné. Hodnota indexu 'iBitmapFormat' by mohla dosáhnout 8. orders.c 2623
Překlepy
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ěžný překlep: druhá podmínka kontroluje stejnou proměnnou jako první. S největší pravděpodobností se chyba objevila v důsledku neúspěšného zkopírování kódu.
Fragment 2
Byly nalezeny dva bloky identického textu. Druhý blok začíná od řá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 bytes) */
Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
/* MinorVersion (2 bytes) */
Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
....
}Další překlep: komentář ke kódu naznačuje, že vlákno by mělo přijít minorVerzeke čtení však dochází do proměnné s názvem hlavní verze. Nejsem však obeznámen s protokolem, takže je to jen odhad.
Fragment 3
Je zvláštní, ž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);
}Soudě podle komentáře, funkce trio_index najde první shodu znaků v řetězci, když trio_index_last - poslední věc. Ale těla těchto funkcí jsou totožná! S největší pravděpodobností se jedná o překlep a ve funkci trio_index_last potřeba použít strhrchr místo strchr. Potom bude chování očekáváno.
Fragment 4
Ukazatel 'data' ve výrazu se rovná nullptr. Výsledná hodnota aritmetických operací na tomto ukazateli je nesmyslná a neměla by se používat. 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;
....
}Vypadá to, že operátor negace zde náhodou chyběl ! U datum. Je zvláštní, že to zůstalo bez povšimnutí.
Fragment 5
Bylo zjištěno použití vzoru 'if (A) {…} else if (A) {…}'. Existuje pravděpodobnost výskytu logické chyby. Kontrolní řá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 <= 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í dvě podmínky jsou stejné: zřejmě je někdo zapomněl po zkopírování zkontrolovat. Z kódu je patrné, že poslední část pracuje se čtyřbajtovými hodnotami, takže můžeme předpokládat, že poslední podmínka by měla být hodnota <= 0x3FFFFFFFF.
Byla nalezena další chyba tohoto typu:
- V517 Bylo zjištěno použití vzoru 'if (A) {…} else if (A) {…}'. Existuje pravděpodobnost výskytu logické chyby. Kontrolní řádky: 169, 173. soubor.c 169
Validace vstupních dat
Fragment 1
Výraz 'strcat(cíl, 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 funkce v tomto příkladu je nesprávná. Funkce strcat vrací ukazatel na konečnou verzi řetězce, tj. první parametr prošel. V tomto případě je cíl. Pokud se však rovná NULL, pak už je na kontrolu pozdě, protože ve funkci strcat bude dereferencováno.
Fragment 2
Výraz 'mezipaměť' 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 případě proměnná Cache je přiřazena adresa statického pole glyphCache->glyphCache. Tak zkontrolujte if (mezipaměť) lze vynechat.
Chyba správy zdrojů
Zdroj byl získán pomocí funkce 'CreateFileA', ale byl uvolněn pomocí nekompatibilní funkce '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;
}
....
}Popisovač souboru fp, vytvořené voláním funkce CreateFile zavřeno omylem funkcí fzavřít ze standardní knihovny, nikoli CloseHandle.
Stejné podmínky
Podmíněné výrazy příkazů 'if' umístěné vedle sebe jsou identické. Kontrolní řá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: "
"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 příklad nemusí být chyba. Obě podmínky však obsahují stejné zprávy, z nichž jednu lze s největší pravděpodobností odstranit.
Čištění nulových ukazatelů
Nulové ukazatele jsou předány 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 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);
....
}Ve funkci uvolnit můžete předat nulový ukazatel a analyzátor o tom ví. Ale pokud je detekována situace, ve které je ukazatel vždy předán null, jako v tomto úryvku, bude vydáno varování.
Ukazatel mszGroupsA zpočátku rovné NULL a nikde jinde se neinicializuje. Jediná větev kódu, kde by mohl být ukazatel inicializován, je nedostupná.
Byly tam další zprávy jako:
- V575 Nulový ukazatel je předán do funkce 'free'. Zkontrolujte první argument. licence.c 790
- V575 Nulový ukazatel je předán do funkce 'free'. Zkontrolujte první argument. rdpsnd_alsa.c 575
S největší pravděpodobností takové zapomenuté proměnné vznikají během procesu refaktoringu a lze je jednoduše odstranit.
Možné přetečení
Možné přetečení. Zvažte přetypování operandů, nikoli výsledek. 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řivedení výsledku do dlouhý není ochrana proti přetečení, protože samotný výpočet probíhá pomocí typu int.
Dereference ukazatele při inicializaci
Ukazatel 'kontext' byl použit předtím, než byl ověřen proti nullptr. Kontrolní řá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;
....
}Tady je ukazatel kontext je dereferencována během inicializace - před kontrolou.
Byly nalezeny další chyby tohoto typu:
- V595 Ukazatel 'ntlm' byl použit předtím, než byl ověřen proti nullptr. Kontrolní řádky: 236, 255. ntlm.c 236
- V595 Ukazatel 'kontext' byl použit předtím, než byl ověřen proti nullptr. Kontrolní řádky: 1003, 1007. rfx.c 1003
- V595 Ukazatel 'rdpei' byl použit předtím, než byl ověřen proti nullptr. Kontrolní řádky: 176, 180. rdpei_main.c 176
- V595 Ukazatel 'gdi' byl použit předtím, než byl ověřen proti nullptr. Kontrolní řádky: 121, 123. xf_gfx.c 121
Bezvýznamný stav
Výraz 'rdp->state >= CONNECTION_STATE_ACTIVE' je vždy pravdivý. připojení.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 snadné vidět, že první podmínka nemá smysl, protože odpovídající hodnota byla přiřazena dříve.
Nesprávná analýza řetězce
Nesprávný formát. Zvažte kontrolu třetího aktuálního argumentu funkce 'sscanf'. Očekává se ukazatel na typ int unsigned. proxy.c 220
Část podmíněné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žitě vydá 2 varování pro tento fragment. Specifikátor %u očekává proměnnou typu neoznačené int, ale variabilní náhradník má typ int. Dále vidíme podezřelou kontrolu: podmínka vpravo nedává smysl, protože na začátku je srovnání s jednou. Nevím, co tím autor tohoto kódu myslel, ale něco je zde zjevně špatně.
Kontroly mimo provoz
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škrtnuté podmínky budou vždy nepravdivé, protože provedení dosáhne druhé podmínky pouze tehdy, když stav == SEC_E_OK. Správný kód může 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 tedy odhalila mnoho problémů, ale v článku byla popsána pouze ta nejzajímavější část z nich. Vývojáři projektu mohou sami zkontrolovat projekt tak, že si na webu vyžádají dočasný licenční klíč . Došlo také k falešným poplachům, práce na kterých pomůže zlepšit analyzátor. Statická analýza je však důležitá, pokud chcete nejen zlepš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 tento článek sdílet s anglicky mluvícím publikem, použijte odkaz na překlad: Sergey Larin.
Zdroj: www.habr.com
