Geekly articles weekly

Memeriksa FreeRDP dengan penganalisa PVS-Studio

Gambar 2

FreeRDP adalah implementasi open-source Remote Desktop Protocol (RDP), sebuah protokol yang mengimplementasikan kontrol komputer jarak jauh yang dikembangkan oleh Microsoft. Proyek ini mendukung banyak platform, termasuk Windows, Linux, macOS, dan bahkan iOS dengan Android. Proyek ini dipilih sebagai yang pertama dari serangkaian artikel yang ditujukan untuk memeriksa klien RDP menggunakan penganalisa statis PVS-Studio.

Sedikit sejarah


Proyek FreeRDP datang setelah Microsoft meluncurkan spesifikasi protokol RDP miliknya. Saat itu, ada klien rdesktop, yang implementasinya didasarkan pada hasil Reverse Engineering.

Dalam proses implementasi protokol, menjadi lebih sulit untuk menambahkan fungsionalitas baru karena arsitektur proyek yang ada. Perubahan di dalamnya menciptakan konflik antara pengembang, yang mengarah pada pembuatan garpu rdesktop - FreeRDP. Distribusi lebih lanjut dari produk dibatasi oleh lisensi GPLv2, sebagai akibatnya keputusan dibuat untuk lisensi ulang ke Lisensi Apache v2. Namun, tidak semua orang sepakat untuk mengubah lisensi kode mereka, sehingga pengembang memutuskan untuk menulis ulang proyek, sehingga kami memiliki tampilan modern dari basis kode.

Rincian lebih lanjut tentang sejarah proyek dapat ditemukan di catatan blog resmi: "Sejarah proyek FreeRDP".

PVS-Studio digunakan sebagai alat untuk mendeteksi kesalahan dan potensi kerentanan dalam kode. Ini adalah penganalisa kode statis untuk C, C ++, C #, dan Java, tersedia di Windows, Linux, dan macOS.

Artikel ini hanya menyajikan kesalahan-kesalahan yang menurut saya paling menarik.

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

Fragmen ini diambil dari subsistem winpr yang mengimplementasikan pembungkus WINAPI untuk sistem non-Windows, yaitu Ini adalah mitra ringan untuk Wine. Ada kebocoran di sini: memori yang dialokasikan oleh fungsi getcwd dibebaskan hanya selama kasus-kasus khusus. Untuk memperbaiki kesalahan, tambahkan panggilan gratis setelah memcpy .

Melampaui batas array


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, item baru ditambahkan ke daftar, bahkan jika jumlah item telah mencapai maksimum. Cukup untuk mengganti operator <= dengan < agar tidak melampaui batas array.

Kesalahan lain dari jenis ini ditemukan:

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

Salah ketik


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

Di sini kita melihat kesalahan ketik yang biasa: dalam kondisi kedua, variabel yang sama diperiksa seperti pada yang pertama. Kemungkinan besar, kesalahan muncul sebagai akibat dari penyalinan kode yang gagal.

Fragmen 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 pada kode menyiratkan bahwa minorVersion harus berasal dari aliran, namun, pembacaan terjadi dalam variabel bernama majorVersion . Namun, saya tidak terbiasa dengan protokol, jadi ini hanya asumsi.

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

Dilihat oleh komentar, fungsi trio_index menemukan kecocokan pertama dari sebuah karakter dalam string ketika trio_index_last adalah yang terakhir. Tetapi tubuh dari fungsi-fungsi ini identik! Kemungkinan besar, ini adalah salah ketik, dan dalam fungsi trio_index_last Anda perlu menggunakan strrchr alih-alih strchr . Maka perilaku akan diharapkan.

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

Sepertinya mereka tidak sengaja melewatkan operator negasi ! di sebelah data . Sungguh aneh bahwa ini tidak diperhatikan.

Fragmen 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 syarat terakhir sama: rupanya, seseorang lupa memeriksanya setelah menyalin. Menurut kode, terlihat bahwa bagian terakhir bekerja dengan nilai empat-byte, sehingga kita dapat mengasumsikan bahwa kondisi terakhir harus bernilai <= 0x3FFFFFFF .

Kesalahan lain dari jenis ini ditemukan:

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

Input Validasi


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

Memeriksa hasil fungsi dalam contoh ini salah. Fungsi strcat mengembalikan pointer ke versi terakhir dari string, yaitu parameter pertama terlewati. Dalam hal ini, itu adalah target . Namun, jika NULL , maka sudah terlambat untuk memeriksa, karena dalam fungsi strcat akan dereferensi.

Fragmen 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 hal ini, variabel cache diberi alamat larik statis glyphCache-> glyphCache . Dengan demikian, pemeriksaan if (cache) dapat dihilangkan.

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

Deskriptor file fp yang dibuat dengan memanggil fungsi CreateFile secara keliru ditutup oleh fungsi fclose dari pustaka standar, bukan CloseHandle .

Kondisi yang sama


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

Mungkin contoh ini bukan kesalahan. Namun, kedua kondisi tersebut mengandung pesan yang sama, salah satunya, kemungkinan besar, dapat dihapus.

Membersihkan Pointer Null


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

Anda dapat melewatkan pointer nol ke fungsi bebas , dan penganalisa tahu tentang itu. Tetapi jika situasi terungkap di mana pointer selalu melewati nol, seperti dalam fragmen ini, peringatan akan dikeluarkan.

Penunjuk mszGroupsA awalnya NULL dan tidak diinisialisasi di tempat lain. Satu-satunya cabang kode tempat pointer dapat diinisialisasi tidak dapat dijangkau.

Ada posting lain seperti 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

Kemungkinan besar, variabel yang terlupakan tersebut muncul selama refactoring dan dapat dengan mudah dihapus.

Kemungkinan 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)); .... }

Membawa hasil terlalu lama bukanlah perlindungan terhadap overflow, karena perhitungan itu sendiri dilakukan dengan menggunakan tipe int .

Pointer dereferencing dalam 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; .... }

Di sini, penunjuk konteks dereferensi dalam inisialisasi - sebelum diperiksa.

Kesalahan lain dari jenis ini ditemukan:

  • 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 penetapan nilai yang sesuai sebelumnya.

Penguraian baris 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)) { .... } .... }

Penganalisa untuk fragmen ini segera memberikan 2 peringatan. Penentu % u mengharapkan variabel bertipe unsigned int , tetapi sub variabel bertipe int . Selanjutnya, kita melihat pemeriksaan yang mencurigakan: kondisi di sebelah kanan tidak masuk akal, karena pada awalnya ada perbandingan dengan persatuan. Saya tidak tahu apa yang ada dalam pikiran pembuat kode ini, tetapi jelas ada sesuatu yang salah di sini.

Cek Tidak Berurutan


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 eksekusi akan mencapai kondisi kedua hanya jika status == SEC_E_OK . Kode yang benar mungkin terlihat seperti ini:

 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


Dengan demikian, verifikasi proyek mengungkapkan banyak masalah, tetapi hanya bagian yang paling menarik yang dijelaskan dalam artikel tersebut. Pengembang proyek dapat memverifikasi sendiri proyek tersebut dengan meminta kunci lisensi sementara di situs web PVS-Studio . Ada juga positif palsu, pekerjaan yang akan membantu meningkatkan penganalisa. Namun demikian, analisis statis penting jika Anda ingin tidak hanya meningkatkan kualitas kode, tetapi juga mengurangi waktu untuk mencari kesalahan, dan PVS-Studio dapat membantu dengan ini.



Jika Anda ingin berbagi artikel ini dengan audiens yang berbahasa Inggris, silakan gunakan tautan ke terjemahan: Sergey Larin. Memeriksa FreeRDP dengan PVS-Studio

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

More articles:


All Articles