Geekly articles weekly

Memeriksa FreeRDP dengan PVS-Studio

Gambar 2

FreeRDP adalah implementasi open-source Remote Desktop Protocol (RDP), protokol milik Microsoft. Proyek ini mendukung banyak platform, termasuk Windows, Linux, macOS, dan bahkan iOS dan Android. Kami memilih itu untuk menjadi proyek pertama yang dianalisis dengan alat analisis kode statis PVS-Studio untuk serangkaian artikel tentang pemeriksaan RDP-klien.

Beberapa sejarah


Proyek FreeRDP dimulai setelah Microsoft membuka spesifikasi untuk RDP protokol milik mereka. Pada saat itu, klien bernama rdesktop sudah digunakan, sebagian besar didasarkan pada pekerjaan rekayasa terbalik.

Ketika mereka menerapkan protokol, para pengembang merasa kesulitan untuk menambahkan fungsionalitas baru karena masalah arsitektur. Perubahan pada arsitektur memerlukan konflik antara pengembang dan menyebabkan pembuatan garpu rdesktop yang dikenal sebagai FreeRDP. Distribusi lebih lanjut dibatasi oleh lisensi GPLv2, dan penulis memutuskan untuk beralih ke Lisensi Apache v2. Namun, beberapa tidak mau mengubah lisensi, sehingga pengembang memutuskan untuk menulis ulang basis kode dari awal dan itulah bagaimana proyek seperti yang kita kenal sekarang muncul.

Sejarah lengkap proyek tersedia di blog resmi: " Sejarah proyek FreeRDP ".

Saya menggunakan PVS-Studio untuk memindai proyek dari bug dan kerentanan potensial. PVS-Studio adalah penganalisa statis untuk kode yang ditulis dalam C, C ++, C #, dan Java dan berjalan pada Windows, Linux, dan macOS.

Perhatikan bahwa saya hanya akan membahas bug yang terlihat paling menarik bagi saya.

Kebocoran memori


V773 Fungsi itu keluar tanpa melepaskan pointer 'cwd'. Kebocoran memori dimungkinkan. lingkungan.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; .... }

Cuplikan ini berasal dari subsistem winpr, yang mengimplementasikan pembungkus WINAPI untuk sistem non-Windows, yaitu bertindak sebagai setara dengan Wine yang lebih ringan. Kode di atas berisi kebocoran memori: memori yang dialokasikan oleh fungsi getcwd dirilis hanya dalam cabang-cabang kasus khusus. Untuk memperbaiki ini, penulis harus menambahkan panggilan gratis setelah panggilan ke memcpy .

Array indeks di luar batas


V557 Array overrun dimungkinkan. Nilai indeks 'event-> EventHandlerCount' bisa mencapai 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++; } .... }

Dalam contoh ini, elemen baru akan ditambahkan ke daftar bahkan ketika yang terakhir telah mencapai jumlah maksimum elemen. Bug ini dapat diperbaiki dengan hanya mengganti operator <= dengan < .

Penganalisa menemukan bug lain dari jenis ini:

  • V557 Array overrun dimungkinkan. Nilai indeks 'iBitmapFormat' dapat mencapai 8. orders.c 2623

Salah ketik


Cuplikan 1


Ekspresi V547 '! Pipa-> Masuk' selalu salah. 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; ....

}

Apa yang kita lihat di sini adalah kesalahan ketik biasa: kondisi pertama dan kedua memeriksa variabel yang sama. Itu mirip sekali dengan produk salin-tempel yang buruk.

Cuplikan 2


V760 Dua blok teks yang identik ditemukan. Blok kedua dimulai dari baris 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); .... }

Kesalahan ketik lain: komentar mengatakan kita harus mengharapkan variabel minorVersion untuk dibaca dari aliran, sementara nilainya dibaca ke dalam variabel majorVersion . Saya tidak akrab dengan proyek dengan cukup baik untuk mengatakan dengan pasti.

Cuplikan 3


V524 Aneh bahwa tubuh fungsi 'trio_index_last' sepenuhnya setara dengan tubuh fungsi '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); }

Seperti komentar menyarankan, fungsi trio_index menemukan kemunculan karakter pertama dalam string, sedangkan fungsi trio_index_last menemukan kejadian terakhir. Namun tubuh kedua fungsi ini persis sama! Ini harus salah ketik dan fungsi trio_index_last mungkin harus mengembalikan strrchr daripada strchr - dalam hal ini, program akan berperilaku seperti yang diharapkan.

Cuplikan 4


V769 Pointer 'data' dalam ekspresi sama dengan nullptr. Nilai operasi aritmatika yang dihasilkan pada pointer ini tidak masuk akal dan tidak boleh digunakan. 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; .... }

Pengembang pasti tidak sengaja meninggalkan operator negasi ! sebelum data . Saya bertanya-tanya mengapa tidak ada yang menyadarinya sebelumnya.

Cuplikan 5


V517 Penggunaan pola 'jika (A) {...} else jika (A) {...}' terdeteksi. Ada kemungkinan kehadiran kesalahan logis. Periksa baris: 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); } .... }

Dua kondisi terakhir adalah sama: programmer pasti lupa untuk mengubah salinannya. Dilihat oleh logika kode, bagian terakhir menangani nilai empat byte, sehingga kita dapat mengasumsikan bahwa kondisi terakhir harus memeriksa apakah nilai <= 0x3FFFFFFF .

Satu lagi bug jenis ini:

  • V517 Penggunaan pola 'jika (A) {...} else jika (A) {...}' terdeteksi. Ada kemungkinan kehadiran kesalahan logis. Periksa baris: 169, 173. file.c 169

Memeriksa data input


Cuplikan 1


V547 Ekspresi 'strcat (target, sumber)! = NULL' selalu benar. 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); }

Pemeriksaan nilai pengembalian fungsi salah. Fungsi strcat mengembalikan pointer ke string target, yaitu parameter pertama, yang dalam hal ini adalah target . Tetapi jika itu sama dengan NULL, sudah terlambat untuk memeriksanya karena itu sudah dereferenced dalam fungsi strcat .

Cuplikan 2


V547 Ekspresi 'cache' selalu benar. 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) { .... } .... }

Dalam cuplikan ini, variabel cache diberikan alamat larik statis glyphCache-> glyphCache . Cek jika (cache) dapat dihapus.

Kesalahan manajemen sumber daya


V1005 Sumber daya diperoleh dengan menggunakan fungsi 'CreateFileA' tetapi dirilis menggunakan fungsi 'fclose' yang tidak kompatibel. sertifikat.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; } .... }

Pegangan fp ke file yang dibuat oleh fungsi CreateFile ditutup secara tidak sengaja dengan memanggil fungsi fclose dari pustaka standar daripada fungsi CloseHandle .

Kondisi identik


V581 Ekspresi kondisional dari pernyataan 'jika' yang terletak berdampingan adalah identik. Periksa baris: 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); } .... }

Cuplikan ini mungkin benar, tetapi mencurigakan bahwa kedua kondisi tersebut berisi pesan yang identik - salah satunya mungkin tidak perlu.

Membebaskan pointer nol


V575 Penunjuk nol dilewatkan ke fungsi 'bebas'. Periksa argumen pertama. 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); .... }

Fungsi bebas dapat dipanggil pada pointer nol, dan PVS-Studio tahu itu. Tetapi jika pointer ditemukan selalu nol, seperti dalam cuplikan ini, penganalisa akan mengeluarkan peringatan.

Penunjuk mszGroupsA awalnya diatur ke NULL dan tidak diinisialisasi di tempat lain. Satu-satunya cabang yang dapat diinisialisasi tidak dapat dijangkau.

Beberapa peringatan lain dari jenis ini:

  • V575 Penunjuk nol dilewatkan ke fungsi 'bebas'. Periksa argumen pertama. lisensi.c 790
  • V575 Penunjuk nol dilewatkan ke fungsi 'bebas'. Periksa argumen pertama. rdpsnd_alsa.c 575

Variabel yang ditinggalkan seperti itu tampaknya merupakan residu yang tersisa setelah refactoring dan dapat dihilangkan.

Potensi meluap


V1028 Kemungkinan meluap. Pertimbangkan casting operan, bukan hasilnya. 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)); .... }

Menempatkan hasil ekspresi terlalu lama tidak akan mencegah overflow karena evaluasi dilakukan pada nilai saat masih bertipe int .

Penunjuk referensi saat inisialisasi


V595 Pointer 'konteks' digunakan sebelum diverifikasi terhadap nullptr. Periksa baris: 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; .... }

Pointer konteks ditereferensi selama inisialisasi, yaitu sebelum pemeriksaan.

Bug lain dari jenis ini:

  • V595 Pointer 'ntlm' digunakan sebelum diverifikasi terhadap nullptr. Periksa baris: 236, 255. ntlm.c 236
  • V595 Pointer 'konteks' digunakan sebelum diverifikasi terhadap nullptr. Periksa baris: 1003, 1007. rfx.c 1003
  • V595 Pointer 'rdpei' digunakan sebelum diverifikasi terhadap nullptr. Periksa baris: 176, 180. rdpei_main.c 176
  • V595 Pointer 'gdi' digunakan sebelum diverifikasi terhadap nullptr. Periksa baris: 121, 123. xf_gfx.c 121

Kondisi tidak berarti


V547 Ekspresi 'rdp-> state> = CONNECTION_STATE_ACTIVE' selalu benar. connection.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; } .... } .... }

Sangat mudah untuk melihat bahwa kondisi pertama tidak masuk akal karena nilai yang dimaksud sudah ditetapkan sebelumnya.

Penanganan string salah


V576 Format salah. Pertimbangkan untuk memeriksa argumen aktual ketiga dari fungsi 'sscanf'. Pointer ke tipe int unsigned diharapkan. proxy.c 220

V560 Bagian dari ekspresi kondisional selalu benar: (rc> = 0). proxy.c 222

 static BOOL check_no_proxy(....) { .... int sub; int rc = sscanf(range, "%u", &sub); if ((rc == 1) && (rc >= 0)) { .... } .... }

Kode ini memicu dua peringatan sekaligus. % U placeholder digunakan untuk variabel bertipe unsigned int , sedangkan sub variabel bertipe int . Peringatan kedua menunjukkan pemeriksaan yang mencurigakan: bagian kanan dari ekspresi kondisional tidak masuk akal karena variabel sudah diperiksa untuk 1 di bagian kiri. Saya tidak yakin dengan niat penulis, tetapi ada sesuatu yang salah dengan kode ini.

Memeriksa dengan urutan yang salah


Ekspresi V547 'status == 0x00090314' selalu salah. 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; .... }

Kondisi yang ditandai akan selalu salah karena kondisi kedua hanya dapat dijalankan jika status == SEC_E_OK . Seperti inilah versi yang benar:

 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; }

Kesimpulan


Cek mengungkapkan banyak bug, dan yang dibahas di atas adalah yang paling menarik. Pengembang proyek dipersilakan untuk menyerahkan formulir untuk kunci lisensi sementara di situs web PVS-Studio untuk melakukan pemeriksaan sendiri. Penganalisa juga menghasilkan sejumlah positif palsu, yang akan kami perbaiki untuk meningkatkan kinerjanya. Perhatikan bahwa analisis statis sangat diperlukan jika tujuan Anda tidak hanya untuk meningkatkan kualitas kode tetapi juga membuat pencarian bug lebih sedikit memakan waktu - dan di situlah berguna PVS-Studio.

Source: https://habr.com/ru/post/id444246/

More articles:


All Articles