FreeRDP është një implementim i hapur i Protokollit të Desktopit të Largët (RDP), një protokoll që realizon menaxhimin e largët të kompjuterit e zhvilluar nga Microsoft. Projekti mbështet shumë platforma, përfshirë Windows, Linux, macOS dhe madje edhe iOS me Android. Ky projekt u zgjodh i pari në kuadër të ciklit të artikujve që i kushtohen kontrollit të klientëve RDP duke përdorur analizuesin statik PVS-Studio.
Pak histori
Projekti ka lindur pas hapjes nga Microsoft e specifikimeve të protokollit të saj pronësor RDP. Në atë kohë, ekzistonte klienti rdesktop, implementimi i të cilit bazohej në rezultatet e Inxhinierisë së Reverse.
Gjatë procesit të implementimit të protokollit, bëhej më e vështirë të shtohej funksionalitet i ri për shkak të arkitekturës ekzistuese të projektit. Ndryshimet në të krijuan një konflikt mes zhvilluesve, çka shkaktoi krijimin e një forku nga rdesktop – FreeRDP. Përhapja më tej e produktit ishte e kufizuar nga licenca GPLv2, duke rezultuar në vendimin për ralicencimin në Licencën Apache v2. Megjithatë, jo të gjithë ishin dakord të ndryshonin licencën e kodit të tyre, prandaj zhvilluesit vendosën ta shkruanin përsëri projektin, duke rezultuar në pamjen moderne të bazës së kodit.
Për më shumë detaje mbi historinë e projektit, mund të lexoni në shënimin e blogut zyrtar: "Historia e projektit FreeRDP".
Si një mjet për identifikimin e gabimeve dhe potencialeve të vulnerabilitetit në kod është përdorur . Ky është një analizues statik i kodit për gjuhët C, C++, C# dhe Java, i disponueshëm në platformat Windows, Linux dhe macOS.
Në këtë artikull janë paraqitur vetëm ato gabime që më duken më interesante.
Leak memorjeje
Funksioni u mbyll pa çliruar treguesin 'cwd'. Një rrjedhje memorjeje është e mundur. 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;
....
}Ky fragment u mor nga nënsistemi winpr, duke realizuar një mbështetje për WINAPI për sistemet që nuk janë Windows, pra një analog të lehtë të Wine. Këtu mund të vëreni një rrjedhje: memoria e alokuar nga funksioni getcwd, çlirohet vetëm gjatë përpunimit të rasteve speciale. Për të eliminuar gabimin, duhet të shtohet thirrja free pas memcpy.
Shkak për anashkalimin e array
Anashkalimi i array mund të ndodhë. Vlera e indeksit 'event->EventHandlerCount' mund të arrijë 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++;
}
....
}Në këtë shembull, një element i ri shtohet në listë, madje nëse numri i elementeve ka arritur maksimumin. Këtu mjafton të zëvendësohet operatori <= në <, për të mos dalë jashtë kufijve të vargut.
Iu zbulua një gabim tjetër i këtij lloji:
- V557 Mund të ndodhë teprica e vargut. Vlera e indeksit ‘iBitmapFormat’ mund të arrijë 8. orders.c 2623
Gabimet tipografike
Framenti 1
Shprehja ‘!pipe->In’ është gjithmonë e pavërtetë. MessagePipe.c 63
wMessagePipe* MessagePipe_New()
{
....
pipe->In = MessageQueue_New(NULL);
nëse (!pipe->In)
shko te error_in;
pipe->Out = MessageQueue_New(NULL);
nëse (!pipe->In) // <=
shko te error_out;
....
}Këtu shohim një gabim tipik: në kushtin e dytë, po kontrollohet të njëjtën variabël si në të parin. Nga duket, gabimi ka ndodhur për shkak të një kopjimi të dështuar të kodit.
Framenti 2
U gjetën dy blloqe identike teksti. Blloku i dytë fillon nga rreshti 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;
....
/* Versioni i Madh (2 bytes) */
Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
/* Versioni i Vogël (2 bytes) */
Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
....
}Një tjetër gabim tipografik: komenti në kod nënkupton se nga rrjedha duhet të mbërrijë minorVersion, megjithatë, leximi ndodh në një variabël me emrin majorVersion. Megjithatë, nuk jam i njohur me protokollin, kështu që kjo është vetëm një supozim.
Framenti 3
Është çuditshme që trupi i funksionit ‘trio_index_last’ është plotësisht i barabartë me trupin e funksionit ‘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);
}Sipas komentit, funksioni trio_index gjen përputhjen e parë të karakterit në varg, kurse trio_index_last — është e fundit. Por trupat e këtyre funksioneve janë identike! Probabiliteti është se ky është një gabim tipografik, dhe në këtë funksion trio_index_last duhet të përdoret strrchr në vend të strchr. Atëherë sjellja do të jetë e pritshme.
Framenti 4
Pika ‘data’ në shprehjen është e barabartë me nullptr. Vlera rezultuese e operacioneve aritmetike mbi këtë tregues është e pakuptimtë dhe nuk duhet të përdoret. nsc_encode.c 124
static BOOL nsc_encode_argb_to_aycocg(NSC_CONTEXT* context,
const BYTE* data,
UINT32 scanline)
{
....
nëse (!context || data || (scanline == 0))
kthehu FALSE;
....
src = data + (context->height - 1 - y) * scanline;
....
}Duket se këtu është humbur rastësisht operatori i mohimit ! në afërsi të data. Çuditërisht, kjo ka mbetur e pa vërejtur.
Framenti 5
U zbulua modeli ‘nëse (A) {…} përndryshe nëse (A) {…}’. Ekziston probabiliteti i pranishëm të një gabimi logjik. Kontrolloni rreshtat: 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);
}
....
}Dyshimet e fundit janë të njëjta: duket se dikush harroi t'i kontrollojë ato pas kopjimit. Nga kodi është e dukshme se pjesa e fundit punon me vlera katër byte, prandaj mund të supozojmë se kushti i fundit duhet të jetë value <= 0x3FFFFFFF.
Iu zbulua një gabim tjetër i këtij lloji:
- V517 U shkelja e modelit 'if (A) {…} else if (A) {…}' u identifikua. Ka një probabilitet të pranishëm të gabimit logjik. Kontrolloni linjat: 169, 173. file.c 169
Kontrolli i të dhënave hyrëse
Framenti 1
Shprehja 'strcat(target, source) != NULL' gjithmonë është e vërtetë. 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);
}Kontrolli i rezultatit të ekzekutimit të funksionit në këtë shembull është i pavend. Funksioni strcat kthen një tregues në variantin përfundimtar të vargut, pra parametrin e parë të kaluar. Në këtë rast është target. Megjithatë, nëse ai është NULL, atëherë kontrollimi i tij është vonë, sepse në funksion strcat do të ndodhi dereferenca e tij.
Framenti 2
Shprehja 'cache' gjithmonë është e vërtetë. 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)
{
....
}
....
}Në këtë rast, variablës cache i jepet adresa e një array statik glyphCache->glyphCache. Kështu, kontrollimi if (cache) mund të anulohet.
Gabim në menaxhimin e burimeve
Burimi u mor me funksionin ‘CreateFileA’ por u lirua me funksionin e papajtueshëm ‘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;
}
....
}Treguesi i dosjes fp, i krijuar nga thirrja e funksionit CreateFile, gabimisht u mbyll nga funksioni fclose nga biblioteka standarde, e jo CloseHandle.
Kushtet e njëjta
Shprehjet kushtore të deklaratave 'if' të vendosura ngjitur njëra-tjetrës janë identike. Kontrolloni linjat: 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);
}
....
}Ky kjo shembull nuk është një gabim. Megjithatë, të dy kushtet përmbajnë mesazhe të njëjta, një nga të cilat, me siguri, mund të hiqet.
Pastrimi i treguesve të null
Treguese null kalon në funksionin 'free'. Kontrolloni argumentin e parë. smartcard_pcsc.c 875
WINSCARDAPI LONG WINAPI PCSC_SCardListReadersW(
SCARDCONTEXT hContext,
LPCWSTR mszGroups,
LPWSTR mszReaders,
LPDWORD pcchReaders)
{
LPSTR mszGroupsA = NULL;
....
mszGroups = NULL; /* mszGroups nuk mbështetet nga pcsc-lite */
nëse (mszGroups)
ConvertFromUnicode(CP_UTF8,0, mszGroups, -1,
(char**) &mszGroupsA, 0,
NULL, NULL);
status = PCSC_SCardListReaders_Internal(hContext, mszGroupsA,
(LPSTR) &mszReadersA,
pcchReaders);
nëse (status == SCARD_S_SUCCESS)
{
....
}
free(mszGroupsA);
....
}Në funksion free është e mundur të kalosh një tregues null dhe analizuesi e di këtë. Por nëse zbulohet një situatë në të cilën treguesi gjithmonë kalon si null, si në këtë fragment, do të lëshojë një paralajmërim.
Treguesi mszGroupsA fillimisht është baraz NULL dhe nuk initializohet askund tjetër. Degëza e vetme e kodit, ku treguesi mund të inicializohet, është e pakalueshme.
Ka pasur edhe mesazhe të tjera të këtij lloji:
- V575 Treguese null kalon në funksionin 'free'. Kontrolloni argumentin e parë. license.c 790
- V575 Treguese null kalon në funksionin 'free'. Kontrolloni argumentin e parë. rdpsnd_alsa.c 575
Me siguri, variablat e tilla të harruar ndodhin gjatë procesit të rifaktorizimit dhe mund të hiqen thjesht.
E mundshme mbingarkesa
E mundshme mbingarkesë. Merrni parasysh kalimin e operandëve, jo të rezultatit. 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));
....
}Kalimi i rezultatit në long nuk është një mbrojtje ndaj mbingarkesës, sepse vetë llogaritja bëhet me përdorimin e tipit int.
Dezimit të treguesit në inicializim
Treguesi ‘context’ u përdor para se të verifikohej kundër nullptr. Kontrolloni rreshtat: 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;
....
}Këtu treguesi context shkëputet në inicializim — më herët sesa bëhet kontrolli i tij.
U gjetën edhe gabime të tjera të këtij lloji:
- V595 Treguesi 'ntlm' u përdor para se të verifikohej kundër nullptr. Kontrolloni linjat: 236, 255. ntlm.c 236
- V595 Treguesi 'context' u përdor para se të verifikohej kundër nullptr. Kontrolloni linjat: 1003, 1007. rfx.c 1003
- V595 Treguesi 'rdpei' u përdor para se të verifikohej kundër nullptr. Kontrolloni linjat: 176, 180. rdpei_main.c 176
- V595 Treguesi 'gdi' u përdor para se të verifikohej kundër nullptr. Kontrolloni linjat: 121, 123. xf_gfx.c 121
Kusht i paarsyeshëm
Shprehja 'rdp->state >= CONNECTION_STATE_ACTIVE' gjithmonë është e vërtetë. 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;
}
....
}
....
}Është e lehtë të vëresh se kushti i parë nuk ka sens për shkak të caktimit të vlerës përkatëse më parë.
Analiza jo korrekte e vargut
Format jo i saktë. Merrni parasysh kontrollimin e argumentit të tretë efektiv të funksionit 'sscanf'. Një tregues ndaj tipit unsigned int pritet. proxy.c 220
Një pjesë e shprehjes kushtore gjithmonë është e vërtetë: (rc >= 0). proxy.c 222
static BOOL check_no_proxy(....)
{
....
int sub;
int rc = sscanf(range, "%u", &sub);
if ((rc == 1) && (rc >= 0))
{
....
}
....
}Analizatori për këtë fragment jep menjëherë 2 tërheqje. Specifikuesi %u pritet një ndryshore e tipit unsigned int, por ndryshorja sub ka tip int. Më pas shohim një kontroll të dyshimtë: kushti në të djathtë nuk ka sens, pasi fillimisht bëhet krahasimi me një.
Kontrollime të paorganizuar
Shprehja 'status == 0x00090314' gjithmonë është e falsifikuar. 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;
....
}Kushtet e shënuara gjithmonë do të jenë të gabuara, pasi ekzekutimi do të arrijë deri te kushti i dytë vetëm në rastin kur status == SEC_E_OK. Kodi i saktë mund të duket kështu:
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;
}Përfundim
Kështu, kontrolli i projektit zbuloi shumë probleme, por vetëm pjesa më interesante e tyre u përshkrua në artikull. Zhvilluesit e projektit mund ta kontrollojnë vetë projektin duke kërkuar një çelës temporar licence në web. . Kishte gjithashtu false alarm, puna mbi të cilat do të ndihmojë në përmirësimin e analizatorit. Megjithatë, analiza statike është e rëndësishme nëse dëshironi jo vetëm të rritni cilësinë e kodit, por edhe të reduktoni kohën për të gjetur gabime, dhe PVS-Studio mund të ndihmojë në këtë.
Nëse dëshironi të ndani këtë artikull me një audiencë anglishtfolëse, ju lutem përdorni lidhjen në përkthimin: Sergey Larin.
Burimi: habr.com
