FreeRDP – o implementare deschisă a Remote Desktop Protocol (RDP), un protocol care permite gestionarea de la distanță a computerului, dezvoltat de Microsoft. Proiectul suportă numeroase platforme, inclusiv Windows, Linux, macOS și chiar iOS cu Android. Acest proiect a fost ales primul în cadrul ciclului de articole dedicate testării clienților RDP cu ajutorul analizorului static PVS-Studio.
Puțin istorie
Proiect a apărut după ce Microsoft a deschis specificațiile protocolului său proprietar RDP. La acea vreme, exista clientul rdesktop, a cărui implementare se bazează pe rezultatele ingineriei inversate.
În procesul de implementare a protocolului, era din ce în ce mai greu să se adauge noi funcționalități din cauza arhitecturii existente a proiectului. Modificările aduse au generat conflicte între dezvoltatori, ducând la crearea fork-ului rdesktop – FreeRDP. Diseminarea ulterioară a produsului a fost limitată de licența GPLv2, ceea ce a dus la decizia de reelicențiere sub Apache License v2. Cu toate acestea, nu toți au fost de acord să schimbe licența codului lor, motiv pentru care dezvoltatorii au decis să rescrie proiectul, rezultatul fiind versiunea modernă a bazei de cod.
Pentru mai multe detalii despre istoria proiectului, puteți citi nota de pe blogul oficial: „Istoria proiectului FreeRDP”.
Ca instrument pentru identificarea erorilor și a posibilelor vulnerabilități din cod, s-a folosit . Este un analizator static de cod pentru limbajele C, C++, C# și Java, disponibil pe platformele Windows, Linux și macOS.
În articol sunt prezentate doar acele erori care mi s-au părut cele mai interesante.
Scurgere de memorie
Funcția a fost părăsită fără a elibera pointerul ‘cwd’. Este posibilă o scurgere de memorie. 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;
....
}Acest fragment a fost preluat din subsistemul winpr, care implementează un wrapper WINAPI pentru sisteme non-Windows, adică este un analog simplificat al Wine. Aici putem observa o scurgere: memoria alocată de funcția getcwd, se eliberează doar în cazul procesării cazurilor speciale. Pentru a remedia eroarea, este necesar să adăugăm un apel free după memcpy.
Depășirea limitelor array-ului
Este posibilă o depășire a array-ului. Valoarea indexului ‘event->EventHandlerCount’ ar putea ajunge la 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 acest exemplu, un nou element este adăugat în listă, chiar dacă numărul de elemente a atins maximul. Aici este suficient să înlocuiți operatorul <= pe <, pentru a nu depăși limitele array-ului.
A fost găsită și o altă eroare de acest tip:
- V557 Este posibil un depășire a tabloului. Valoarea indexului ‘iBitmapFormat’ ar putea ajunge la 8. orders.c 2623
Greșeli de tipar
Fragment 1
Expresia ‘!pipe->In’ este întotdeauna falsă. 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;
....
}Aici vedem o greșeală obișnuită de tip tipar: în a doua condiție se verifică aceeași variabilă ca în prima. Probabil, eroarea a apărut ca urmare a unei copieri nereușite a codului.
Fragment 2
Au fost găsite două blocuri identice de text. Al doilea bloc începe de la linia 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);
....
}Încă o greșeală de tip tipar: comentariul din cod indică faptul că ar trebui să vină din flux minorVersion, totuși citirea se face în variabila cu numele majorVersion. Cu toate acestea, nu sunt familiarizat cu protocolul, așa că acestea sunt doar presupuneri.
Fragment 3
Este ciudat că corpul funcției ‘trio_index_last’ este complet echivalent cu corpul funcției ‘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);
}Judecând după comentariu, funcția trio_index găsește prima apariție a unui simbol în șir, în timp ce trio_index_last — este ultima. Dar corpurile acestor funcții sunt identice! Probabil, aceasta este o greșeală, iar funcția trio_index_last trebuie utilizat strrchr în loc de strchr. Atunci comportamentul va fi așteptat.
Fragment 4
Pointeurul ‘data’ din expresie este egal cu nullptr. Valoarea rezultantă a operațiunilor aritmetice asupra acestui pointer nu are sens și nu ar trebui utilizată. 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;
....
}Se pare că aici s-a omis din întâmplare operatorul de negare ! lângă data. Ciudat că acest lucru a rămas neobservat.
Fragment 5
A fost detectat patternul ‘if (A) {…} else if (A) {…}’. Există probabilitatea unei erori logice. Verificați liniile: 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);
}
....
}Ultimele două condiții sunt identice: se pare că cineva a uitat să le verifice după copiere. Din cod reiese că ultima parte lucrează cu valori de patru bytes, așadar se poate presupune că ultima condiție ar trebui să fie value <= 0x3FFFFFFF.
A fost găsită și o altă eroare de acest tip:
- V517 S-a detectat utilizarea modelului ‘if (A) {…} else if (A) {…}’. Există o probabilitate de prezență a unei erori logice. Verificați liniile: 169, 173. file.c 169
Verificarea datelor de intrare
Fragment 1
Expresia ‘strcat(target, source) != NULL’ este întotdeauna adevărată. 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);
}Verificarea rezultatului execuției funcției în acest exemplu este incorectă. Funcția strcat returnează un pointer la varianta finală a șirului, adică primul parametru transmis. În acest caz este destinația. Totuși, dacă acesta este egal cu NULL, atunci verificarea sa este tardivă, deoarece în funcție strcat va avea loc dereferirea sa.
Fragment 2
Expresia ‘cache’ este întotdeauna adevărată. 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 acest caz variabilei cache i se atribuie adresa unui array static glyphCache->glyphCache. Astfel, verificarea if (cache) poate fi omisă.
Eroare de gestionare a resurselor
Resursa a fost obținută folosind funcția ‘CreateFileA’ dar a fost eliberată folosind funcția incompatibilă ‘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;
}
....
}Descriptorul de fișier fp, creat prin apelul funcției CreateFile, a fost închis din greșeală prin funcția fclose din biblioteca standard, nu prin CloseHandle.
Condiții identice
Expresiile condiționale ale instrucțiunilor ‘if’ situate una lângă alta sunt identice. Verificați liniile: 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 neimplementat", 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 neimplementat", array_type);
}
....
}Este exemplu poate să nu fie o eroare. Totuși, ambele condiții conțin mesaje identice, dintre care unul este, cel mai probabil, redundant.
Eliminarea pointerilor nuli
Pointerul nul este transmis în funcția ‘free’. Inspectați primul argument. smartcard_pcsc.c 875
WINSCARDAPI LONG WINAPI PCSC_SCardListReadersW(
SCARDCONTEXT hContext,
LPCWSTR mszGroups,
LPWSTR mszReaders,
LPDWORD pcchReaders)
{
LPSTR mszGroupsA = NULL;
....
mszGroups = NULL; /* mszGroups nu este suportat de 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);
....
}În funcția free Se poate transmite un pointer nul și analizorul știe despre aceasta. Dar dacă apare o situație în care pointerul este întotdeauna transmis ca nul, ca în acest fragment, se va emite un avertisment.
Pointer mszGroupsA este inițial NULL și nu este inițializat nicăieri altundeva. Singura ramură de cod unde pointerul ar fi putut fi inițializat este inaccesibilă.
Au fost și alte mesaje de acest tip:
- V575 Pointerul nul este transmis în funcția ‘free’. Inspectați primul argument. license.c 790
- V575 Pointerul nul este transmis în funcția ‘free’. Inspectați primul argument. rdpsnd_alsa.c 575
Cel mai probabil, astfel de variabile uitate apar în procesul de refactoring și pot fi pur și simplu eliminate.
Posibilă suprasarcină
Posibilă suprasarcină. Luați în considerare conversia operanzilor, nu a rezultatului. 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));
....
}Conversia rezultatului în long nu este o protecție împotriva suprasarcinii, deoarece calculul în sine se realizează folosind tipul int.
Dereferințierea pointerului în inițializare
Pointerul ‘context’ a fost utilizat înainte de a fi verificat față de nullptr. Verificați liniile: 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;
....
}Aici este un pointer context de-referențiat în inițializare — înainte de a fi verificat.
Au fost găsite și alte erori de acest tip:
- V595 Pointerul ‘ntlm’ a fost utilizat înainte de a fi verificat cu nullptr. Verificați liniile: 236, 255. ntlm.c 236
- V595 Pointerul ‘context’ a fost utilizat înainte de a fi verificat cu nullptr. Verificați liniile: 1003, 1007. rfx.c 1003
- V595 Pointerul ‘rdpei’ a fost utilizat înainte de a fi verificat cu nullptr. Verificați liniile: 176, 180. rdpei_main.c 176
- V595 Pointerul ‘gdi’ a fost utilizat înainte de a fi verificat cu nullptr. Verificați liniile: 121, 123. xf_gfx.c 121
Condiție lipsită de sens
Expresia ‘rdp->state >= CONNECTION_STATE_ACTIVE’ este întotdeauna adevărată. 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;
}
....
}
....
}Este evident că prima condiție nu are sens din cauza atribuirii valorii corespunzătoare anterior.
Analiza incorectă a șirului
Format incorect. Verificați al treilea argument efectiv al funcției ‘sscanf’. Se așteaptă un pointer la tipul unsigned int. proxy.c 220
O parte a expresiei condiționale este întotdeauna adevărată: (rc >= 0). proxy.c 222
static BOOL check_no_proxy(....)
{
....
int sub;
int rc = sscanf(range, "%u", &sub);
if ((rc == 1) && (rc >= 0))
{
....
}
....
}Analizatorul pentru acest fragment emiterea imediat 2 avertismente. Specifierul %u așteaptă o variabilă de tipul unsigned int, dar variabila sub are tipul int. Apoi vedem o verificare suspectă: condiția din dreapta nu are sens, deoarece la început se compară cu unu. Nu știu ce a vrut să spună autorul acestui cod, dar aici ceva nu este în ordine.
Verificări neordonate
Expresia ‘status == 0x00090314’ este întotdeauna falsă. 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;
....
}Condițiile marcate vor fi întotdeauna false, deoarece execuția va ajunge la a doua condiție doar atunci când status == SEC_E_OK. Codul corect poate arăta astfel:
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;
}Concluzie
Astfel, verificarea proiectului a relevat numeroase probleme, dar doar cea mai interesantă parte a fost descrisă în articol. Dezvoltatorii proiectului pot verifica singuri proiectul, solicitând o cheie temporară de licență pe site. . Au existat și alerte false, lucrul la care va ajuta la îmbunătățirea analizei. Cu toate acestea, analiza statică este importantă dacă doriți nu doar să îmbunătățiți calitatea codului, ci și să reduceți timpul de căutare a erorilor, iar PVS-Studio vă poate ajuta în acest sens.
Dacă doriți să împărtășiți acest articol cu o audiență anglofonă, vă rog să folosiți linkul pentru traducere: Sergey Larin.
Sursa: habr.com
