FreeRDP je odprtokodna implementacija protokola Remote Desktop Protocol (RDP), protokola za oddaljeno upravljanje računalnikov, ki ga je razvil Microsoft. Projekt podpira več platform, vključno z Windows, Linux, macOS in celo iOS z AndroidTa projekt je bil izbran kot prvi v seriji člankov, posvečenih testiranju RDP odjemalcev z uporabo statičnega analizatorja PVS-Studio.
Malo zgodovine
Projekt se je pojavil, potem ko je Microsoft odprl specifikacije za svoj lastniški protokol RDP. Takrat je obstajal odjemalec rdesktop, katerega implementacija je temeljila na rezultatih obratnega inženiringa.
Ko je bil protokol implementiran, je bilo zaradi takrat obstoječe arhitekture projekta težje dodati nove funkcionalnosti. Spremembe v njem so povzročile konflikt med razvijalci, kar je privedlo do nastanka razcepa rdesktop - FreeRDP. Nadaljnja distribucija izdelka je bila omejena z licenco GPLv2, zaradi česar je bila sprejeta odločitev o prelicenciranju na licenco Apache v2. Vendar se vsi niso strinjali s spremembo licence svoje kode, zato so se razvijalci odločili, da bodo projekt ponovno napisali, kar je povzročilo sodobno kodno zbirko.
Več o zgodovini projekta si lahko preberete v uradni objavi na blogu: “Zgodovina projekta FreeRDP”.
Uporablja se kot orodje za prepoznavanje napak in potencialnih ranljivosti v kodi. Je statični analizator kode za C, C++, C# in Java, ki je na voljo na platformah Windows, Linux и macOS.
V članku so predstavljene samo tiste napake, ki so se mi zdele najbolj zanimive.
Utechka pamyati
Funkcija je bila zapuščena, ne da bi spustili kazalec 'cwd'. Možno je puščanje spomina. okolje.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;
....
}Ta fragment je bil vzet iz podsistema winpr, ki implementira ovojnico WINAPI za ne-Windows sistemi, tj. gre za lahek analog programa Wine. Tukaj lahko vidite puščanje: pomnilnik, ki ga dodeli funkcija getcwd, se sprosti samo pri obravnavi posebnih primerov. Če želite odpraviti napako, morate dodati klic brezplačno po memcpy.
Niz izven meja
Možna je prekoračitev polja. Vrednost indeksa 'event->EventHandlerCount' lahko doseže 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++;
}
....
}Ta primer doda nov element na seznam, tudi če je število elementov doseglo največje število. Tukaj zadostuje zamenjava operaterja <= o <, da ne bi presegli meja polja.
Najdena je bila še ena napaka te vrste:
- Možna je prekoračitev polja V557. Vrednost indeksa 'iBitmapFormat' lahko doseže 8. orders.c 2623
Pravopisne napake
Fragment 1
Izraz '!pipe->In' je vedno napačen. 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;
....
}Tukaj vidimo običajno tipkarsko napako: drugi pogoj preverja isto spremenljivko kot prvi. Najverjetneje se je napaka pojavila kot posledica neuspešnega kopiranja kode.
Fragment 2
Najdena sta bila dva bloka enakega besedila. Drugi blok se začne z vrstico 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);
....
}Še ena tipkarska napaka: komentar kode nakazuje, da bi nit morala priti minorVersionvendar pride do branja spremenljivke z imenom majorVersion. Vendar pa nisem seznanjen s protokolom, zato je to le ugibanje.
Fragment 3
Nenavadno je, da je telo funkcije 'trio_index_last' popolnoma enakovredno telesu funkcije '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);
}Po komentarju sodeč funkcija trio_index najde prvo ujemanje znakov v nizu, ko trio_index_last - zadnja stvar. Toda telesa teh funkcij so enaka! Najverjetneje je to tipkarska napaka in v funkciji trio_index_last potrebo po uporabi strhrchr namesto strchr. Potem bo vedenje pričakovano.
Fragment 4
Kazalec 'data' v izrazu je enak nullptr. Dobljena vrednost aritmetičnih operacij na tem kazalcu je nesmiselna in je ne bi smeli uporabljati. 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;
....
}Videti je, da je bil tukaj pomotoma zgrešen operator zanikanja ! Blizu datum. Čudno je, da je to ostalo neopaženo.
Fragment 5
Zaznana je bila uporaba vzorca »if (A) {…} else if (A) {…}«. Obstaja verjetnost prisotnosti logične napake. Preverite vrstice: 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);
}
....
}Zadnja dva pogoja sta enaka: očitno ju je nekdo po kopiranju pozabil preveriti. Iz kode je razvidno, da zadnji del deluje s štiribajtnimi vrednostmi, zato lahko domnevamo, da mora biti zadnji pogoj vrednost <= 0x3FFFFFFFF.
Najdena je bila še ena napaka te vrste:
- V517 Zaznana je bila uporaba vzorca 'if (A) {…} else if (A) {…}'. Obstaja verjetnost prisotnosti logične napake. Preverite vrstice: 169, 173. file.c 169
Validacija vhodnih podatkov
Fragment 1
Izraz 'strcat(target, source) != NULL' je vedno resničen. 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);
}Preverjanje rezultata funkcije v tem primeru ni pravilno. funkcija strcat vrne kazalec na končno različico niza, tj. prvi parameter prešel. V tem primeru je ciljna. Vendar, če je enako NULL, potem je prepozno za preverjanje, saj v funkciji strcat dereferenciran bo.
Fragment 2
Izraz 'cache' je vedno resničen. glif.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 tem primeru spremenljivka predpomnilnik je dodeljen naslov statične matrike glyphCache->glyphCache. Torej preverite če (predpomnilnik) lahko izpustimo.
Napaka pri upravljanju virov
Vir je bil pridobljen s funkcijo »CreateFileA«, vendar je bil sproščen z nekompatibilno funkcijo »fclose«. potrdilo.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 datoteke fp, ustvarjen s klicem funkcije CreateFile zaprt po pomoti po funkciji fclose iz standardne knjižnice, ne CloseHandle.
Enaki pogoji
Pogojni izrazi stavkov 'if', ki se nahajajo drug poleg drugega, so enaki. Preverite vrstice: 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);
}
....
}Ta primer morda ni napaka. Vendar oba pogoja vsebujeta ista sporočila, od katerih je eno najverjetneje mogoče odstraniti.
Čiščenje ničelnih kazalcev
Ničelni kazalec se posreduje funkciji 'free'. Preglejte prvi 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);
....
}V funkciji brezplačno lahko posredujete ničelni kazalec in analizator ve za to. Toda če je zaznana situacija, v kateri je kazalec vedno posredovan na ničelno vrednost, kot v tem izrezku, bo izdano opozorilo.
Kazalec mszGroupsA sprva enaka NULL in ni inicializiran nikjer drugje. Edina veja kode, kjer je bilo mogoče inicializirati kazalec, je nedosegljiva.
Bila so še druga sporočila, kot so:
- V575 Ničelni kazalec se posreduje funkciji 'free'. Preglejte prvi argument. dovoljenje.c 790
- V575 Ničelni kazalec se posreduje funkciji 'free'. Preglejte prvi argument. rdpsnd_alsa.c 575
Najverjetneje se takšne pozabljene spremenljivke pojavijo med procesom refaktoriranja in jih je mogoče preprosto odstraniti.
Možno prelivanje
Možno prelivanje. Razmislite o pretvorbi operandov, ne o rezultatu. 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));
....
}Prinašanje rezultata do dolgi ni zaščita pred prelivom, saj se sam izračun izvede z uporabo vrste int.
Odstranitev kazalca pri inicializaciji
Kazalec 'konteksta' je bil uporabljen, preden je bil preverjen glede na nullptr. Preverite vrstice: 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;
....
}Tukaj je kazalec ozadje se dereferencira med inicializacijo – preden se preveri.
Najdene so bile druge napake te vrste:
- V595 Kazalec 'ntlm' je bil uporabljen, preden je bil preverjen glede na nullptr. Preverite vrstice: 236, 255. ntlm.c 236
- V595 Kazalec 'konteksta' je bil uporabljen, preden je bil preverjen glede na nullptr. Preverite vrstice: 1003, 1007. rfx.c 1003
- V595 Kazalec 'rdpei' je bil uporabljen, preden je bil preverjen glede na nullptr. Preverite vrstice: 176, 180. rdpei_main.c 176
- V595 Kazalec 'gdi' je bil uporabljen, preden je bil preverjen glede na nullptr. Preverite vrstice: 121, 123. xf_gfx.c 121
Nesmiselno stanje
Izraz 'rdp->state >= CONNECTION_STATE_ACTIVE' je vedno resničen. povezava.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;
}
....
}
....
}Preprosto je videti, da je prvi pogoj brez pomena, ker je bila ustrezna vrednost dodeljena prej.
Nepravilno razčlenjevanje niza
Nepravilna oblika. Razmislite o preverjanju tretjega dejanskega argumenta funkcije 'sscanf'. Pričakovan je kazalec na tip unsigned int. proxy.c 220
Del pogojnega izraza je vedno resničen: (rc >= 0). proxy.c 222
static BOOL check_no_proxy(....)
{
....
int sub;
int rc = sscanf(range, "%u", &sub);
if ((rc == 1) && (rc >= 0))
{
....
}
....
}Analizator takoj izda 2 opozorili za ta fragment. Specifikator %u pričakuje spremenljivko tipa nepodpisan int, vendar spremenljivo spodaj prijazen fant int. Nato vidimo sumljivo preverjanje: pogoj na desni ni smiseln, saj je na začetku primerjava z enim. Ne vem, kaj je avtor te kode mislil, ampak tukaj očitno nekaj ni v redu.
Čeki izven reda
Izraz 'status == 0x00090314' je vedno napačen. 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;
....
}Preverjeni pogoji bodo vedno napačni, saj bo izvedba dosegla le drugi pogoj, če status == SEC_E_OK. Pravilna koda bi lahko izgledala takole:
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;
}Zaključek
Tako je preverjanje projekta razkrilo številne težave, vendar je bil v članku opisan le najbolj zanimiv del. Razvijalci projekta lahko sami preverijo projekt tako, da na spletnem mestu zahtevajo začasni licenčni ključ . Obstajajo tudi lažni pozitivni rezultati, delo na katerih bo pomagalo izboljšati analizator. Vendar pa je statična analiza pomembna, če želite ne samo izboljšati kakovost vaše kode, ampak tudi zmanjšati čas, porabljen za iskanje napak, in PVS-Studio vam lahko pri tem pomaga.
Če želite ta članek deliti z angleško govorečim občinstvom, uporabite povezavo za prevod: Sergey Larin.
Vir: www.habr.com
