FreeRDP ir attālās darbvirsmas protokola (RDP) atvērtā koda ieviešana, kas ir Microsoft izstrādāts protokols datoru attālinātai vadībai. Projekts atbalsta vairākas platformas, tostarp Windows, Linux, macOS un pat iOS ar AndroidŠis projekts tika izvēlēts kā pirmais rakstu sērijā, kas veltīta RDP klientu testēšanai, izmantojot statisko analizatoru PVS-Studio.
Nedaudz vēstures
Projekts radās pēc tam, kad Microsoft atklāja sava patentētā RDP protokola specifikācijas. Tolaik bija rdesktop klients, kura ieviešana balstījās uz Reverse Engineering rezultātiem.
Ieviešot protokolu, tobrīd esošās projekta arhitektūras dēļ kļuva grūtāk pievienot jaunu funkcionalitāti. Izmaiņas tajā izraisīja konfliktu starp izstrādātājiem, kā rezultātā tika izveidota rdesktop dakša - FreeRDP. Produkta tālāku izplatīšanu ierobežoja GPLv2 licence, kā rezultātā tika pieņemts lēmums to atkārtoti licencēt Apache License v2. Tomēr ne visi piekrita mainīt sava koda licenci, tāpēc izstrādātāji nolēma projektu pārrakstīt, kā rezultātā tika izveidota moderna kodu bāze.
Vairāk par projekta vēsturi var lasīt oficiālajā bloga ierakstā: “FreeRDP projekta vēsture”.
Izmanto kā rīku, lai identificētu kļūdas un iespējamās koda ievainojamības. Tas ir statisks koda analizators C, C++, C# un Java valodām, kas pieejams platformās Windows, Linux и macOS.
Rakstā ir parādītas tikai tās kļūdas, kuras man šķita visinteresantākās.
Atmiņas noplūde
Funkcija tika aizvērta, neatlaižot “cwd” rādītāju. Iespējama atmiņas noplūde. vide.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;
....
}Šis fragments tika ņemts no WinPR apakšsistēmas, kas ievieš WINAPI apvalku ne-Windows sistēmas, t. i., tas ir viegls Wine analogs. Šeit var redzēt noplūdi: funkcijas piešķirtā atmiņa getcwd, tiek atbrīvots tikai tad, ja tiek apstrādāti īpaši gadījumi. Lai labotu kļūdu, jāpievieno zvans bezmaksas pēc memcpy.
Masīvs ārpus robežām
Ir iespējama masīva pārtēriņa. Indeksa 'event->EventHandlerCount' vērtība varētu sasniegt 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++;
}
....
}Šis piemērs sarakstam pievieno jaunu elementu pat tad, ja elementu skaits ir sasniedzis maksimālo. Šeit pietiek ar operatora nomaiņu <= par <, lai nepārsniegtu masīva robežas.
Tika atrasta vēl viena šāda veida kļūda:
- V557 Ir iespējama masīva pārtēriņa. 'iBitmapFormat' indeksa vērtība varētu sasniegt 8. orders.c 2623
Drukas kļūdas
1. fragments
Izteiksme "!pipe->In" vienmēr ir nepatiesa. 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;
....
}Šeit mēs redzam izplatītu drukas kļūdu: otrais nosacījums pārbauda to pašu mainīgo, ko pirmais. Visticamāk, kļūda parādījās neveiksmīgas koda kopēšanas rezultātā.
2. fragments
Tika atrasti divi identiska teksta bloki. Otrais bloks sākas no rindas 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);
....
}Vēl viena drukas kļūda: koda komentārs nozīmē, ka pavedienam ir jānāk minorVersija, tomēr lasīšana notiek mainīgajā ar nosaukumu galvenā versija. Tomēr es neesmu pazīstams ar protokolu, tāpēc tas ir tikai minējums.
3. fragments
Ir dīvaini, ka funkcijas "trio_index_last" pamatteksts ir pilnībā līdzvērtīgs funkcijas "trio_index" pamattekstam. 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);
}Spriežot pēc komentāra, funkcija trio_indekss atrod pirmo rakstzīmju atbilstību virknē, kad trio_index_last - pēdējā lieta. Bet šo funkciju ķermeņi ir identiski! Visticamāk, tā ir drukas kļūda un funkcijā trio_index_last nepieciešams lietot strhrchr nevis strchr. Tad uzvedība būs sagaidāma.
4. fragments
"Datu" rādītājs izteiksmē ir vienāds ar nullptr. Šī rādītāja iegūtā aritmētisko darbību vērtība ir bezjēdzīga, un to nevajadzētu izmantot. 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;
....
}Šķiet, ka šeit nejauši palaida garām negācijas operatoru ! Netālu dati. Dīvaini, ka tas palika nepamanīts.
5. fragments
Tika atklāts modelis “ja (A) {…} else if (A) {…}”. Pastāv loģiskās kļūdas iespējamība. Pārbaudes rindiņas: 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);
}
....
}Pēdējie divi nosacījumi ir vienādi: acīmredzot kāds aizmirsa tos pārbaudīt pēc kopēšanas. No koda ir pamanāms, ka pēdējā daļa darbojas ar četru baitu vērtībām, tāpēc varam pieņemt, ka pēdējam nosacījumam jābūt vērtība <= 0x3FFFFFFFF.
Tika atrasta vēl viena šāda veida kļūda:
- V517 Tika atklāts modelis “ja (A) {…} else if (A) {…}”. Pastāv loģiskās kļūdas iespējamība. Pārbaudes rindiņas: 169, 173. file.c 169
Ievaddatu validācija
1. fragments
Izteiksme 'strcat(mērķis, avots) != NULL' vienmēr ir patiesa. 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);
}Funkcijas rezultāta pārbaude šajā piemērā ir nepareiza. Funkcija strcat atgriež rādītāju uz virknes galīgo versiju, t.i. pirmais parametrs pagājis. Šajā gadījumā tā ir mērķis. Tomēr, ja tas ir vienāds NULL, tad ir par vēlu to pārbaudīt, jo funkcijā strcat tas tiks noņemts.
2. fragments
Izteiciens "kešatmiņa" vienmēr ir patiess. glifs.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)
{
....
}
....
}Šajā gadījumā mainīgais kešatmiņa tiek piešķirta statiskā masīva adrese glyphCache->glyphCache. Tādējādi pārbaudiet ja (kešatmiņa) var izlaist.
Resursu pārvaldības kļūda
Resurss tika iegūts, izmantojot funkciju “CreateFileA”, bet tika atbrīvots, izmantojot nesaderīgu funkciju “fclose”. sertifikāts.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;
}
....
}Faila deskriptors fp, kas izveidots ar funkcijas izsaukumu Izveidot failu aizvērts kļūdas pēc funkcijas fclose no standarta bibliotēkas, nevis CloseHandle.
Tie paši nosacījumi
“Ja” priekšrakstu nosacījuma izteiksmes, kas atrodas blakus viena otrai, ir identiskas. Pārbaudes rindiņas: 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);
}
....
}Šis piemērs var nebūt kļūda. Tomēr abi nosacījumi satur vienus un tos pašus ziņojumus, no kuriem vienu, visticamāk, var noņemt.
Nulles rādītāju tīrīšana
Nulles rādītājs tiek nodots 'free' funkcijai. Pārbaudiet pirmo argumentu. viedkarte_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);
....
}Funkcijā bezmaksas jūs varat nodot nulles rādītāju, un analizators par to zina. Bet, ja tiek atklāta situācija, kurā rādītājs vienmēr tiek nodots nullei, kā tas ir šajā fragmentā, tiks parādīts brīdinājums.
Rādītājs mszGroupsA sākotnēji vienādi NULL un nav inicializēts nekur citur. Vienīgā koda daļa, kurā var inicializēt rādītāju, nav sasniedzama.
Bija arī citi ziņojumi, piemēram:
- V575 Nulles rādītājs tiek nodots 'free' funkcijai. Pārbaudiet pirmo argumentu. licence.c 790
- V575 Nulles rādītājs tiek nodots 'free' funkcijai. Pārbaudiet pirmo argumentu. rdpsnd_alsa.c 575
Visticamāk, šādi aizmirsti mainīgie rodas pārveidošanas procesā, un tos var vienkārši noņemt.
Iespējama pārplūde
Iespējama pārplūde. Apsveriet operandu apraidi, nevis rezultātu. 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));
....
}Novedot rezultātu uz garš nav aizsardzība pret pārplūdi, jo pats aprēķins notiek, izmantojot veidu int.
Rādītāja atsauce inicializācijā
"Konteksta" rādītājs tika izmantots, pirms tas tika pārbaudīts pret nullptr. Pārbaudes rindiņas: 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;
....
}Šeit ir rādītājs konteksts tiek atcelta atsauce inicializācijas laikā — pirms tā tiek pārbaudīta.
Tika atrastas citas šāda veida kļūdas:
- V595 'ntlm' rādītājs tika izmantots, pirms tas tika pārbaudīts pret nullptr. Pārbaudes rindiņas: 236, 255. ntlm.c 236
- V595 “Konteksta” rādītājs tika izmantots, pirms tas tika pārbaudīts pret nullptr. Pārbaudes rindas: 1003, 1007. rfx.c 1003
- V595 Rdpei rādītājs tika izmantots, pirms tas tika pārbaudīts pret nullptr. Pārbaudes rindas: 176, 180. rdpei_main.c 176
- V595 “gdi” rādītājs tika izmantots, pirms tas tika pārbaudīts pret nullptr. Pārbaudes rindiņas: 121., 123. xf_gfx.c 121
Bezjēdzīgs stāvoklis
Izteiksme "rdp->state >= CONNECTION_STATE_ACTIVE" vienmēr ir patiesa. savienojums.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;
}
....
}
....
}Ir viegli saprast, ka pirmais nosacījums ir bezjēdzīgs, jo atbilstošā vērtība tika piešķirta agrāk.
Nepareiza virknes parsēšana
Nepareizs formāts. Apsveriet iespēju pārbaudīt funkcijas "sscanf" trešo faktisko argumentu. Paredzams rādītājs uz neparakstīto int veidu. proxy.c 220
Nosacītā izteiksmes daļa vienmēr ir patiesa: (rc >= 0). starpniekserveris.c 222
static BOOL check_no_proxy(....)
{
....
int sub;
int rc = sscanf(range, "%u", &sub);
if ((rc == 1) && (rc >= 0))
{
....
}
....
}Analizators nekavējoties izdod 2 brīdinājumus par šo fragmentu. Precizētājs %u sagaida tipa mainīgo neparakstīts int, bet mainīgs zemāk ir tips int. Tālāk mēs redzam aizdomīgu pārbaudi: nosacījumam labajā pusē nav jēgas, jo sākumā ir salīdzinājums ar vienu. Es nezinu, ko domāja šī koda autors, bet šeit kaut kas acīmredzami nav kārtībā.
Pārbaudes ārpus kārtas
Izteiksme "statuss == 0x00090314" vienmēr ir nepatiesa. 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;
....
}Pārbaudītie nosacījumi vienmēr būs nepatiesi, jo izpilde sasniegs tikai otro nosacījumu, ja statuss == SEC_E_OK. Pareizais kods varētu izskatīties šādi:
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;
}Secinājums
Tādējādi, pārbaudot projektu, atklājās daudzas problēmas, taču rakstā tika aprakstīta tikai interesantākā daļa no tām. Projekta izstrādātāji paši var pārbaudīt projektu, vietnē pieprasot pagaidu licences atslēgu . Bija arī kļūdaini pozitīvi rezultāti, kuru darbs palīdzēs uzlabot analizatoru. Tomēr statiskā analīze ir svarīga, ja vēlaties ne tikai uzlabot sava koda kvalitāti, bet arī samazināt laiku, kas pavadīts kļūdu atrašanai, un PVS-Studio var palīdzēt šajā jautājumā.
Ja vēlaties dalīties ar šo rakstu ar angliski runājošu auditoriju, lūdzu, izmantojiet tulkošanas saiti: Sergejs Larins.
Avots: www.habr.com
