Geekly articles weekly

Memeriksa rdesktop dan xrdp dengan PVS-Studio

Gambar3

Ini adalah posting kedua dalam seri artikel kami tentang hasil pemeriksaan perangkat lunak open-source yang bekerja dengan protokol RDP. Hari ini kita akan melihat rdesktop client dan xrdp server.

Analisis dilakukan oleh PVS-Studio . Ini adalah penganalisa statis untuk kode yang ditulis dalam C, C ++, C #, dan Java, dan ini berjalan pada Windows, Linux, dan macOS.

Saya hanya akan membahas bug yang terlihat paling menarik bagi saya. Di sisi lain, karena proyeknya cukup kecil, toh tidak ada banyak bug di dalamnya :).

Catatan Artikel sebelumnya tentang pemeriksaan FreeRDP tersedia di sini .

rdesktop


rdesktop adalah klien RDP gratis untuk sistem berbasis UNIX. Itu juga dapat berjalan di Windows jika dibangun di bawah Cygwin. rdesktop dirilis di bawah GPLv3.

Ini adalah klien yang sangat populer. Ini digunakan sebagai klien default pada ReactOS, dan Anda juga dapat menemukan front-end grafis pihak ketiga untuk digunakan. Proyek ini cukup tua: dirilis untuk pertama kali pada 4 April 2001, dan berusia 17 tahun, saat tulisan ini dibuat.

Seperti yang sudah saya katakan, proyek ini sangat kecil - sekitar 30 KLOC, yang agak aneh mengingat usianya. Bandingkan dengan FreeRDP dengan 320 KLOC-nya. Ini hasil keluaran Cloc:

Gambar1


Kode tidak dapat dijangkau


V779 Kode tidak terjangkau terdeteksi. Mungkin saja ada kesalahan. rdesktop.c 1502

int main(int argc, char *argv[]) { .... return handle_disconnect_reason(deactivated, ext_disc_reason); if (g_redirect_username) xfree(g_redirect_username); xfree(g_username); }

Kesalahan pertama ditemukan segera di fungsi utama : kode yang mengikuti pernyataan kembali dimaksudkan untuk membebaskan memori yang dialokasikan sebelumnya. Tetapi cacat ini tidak berbahaya karena semua memori yang dialokasikan sebelumnya akan dibebaskan oleh sistem operasi setelah program berakhir.

Tidak ada penanganan kesalahan


V557 Array underrun dimungkinkan. Nilai indeks 'n' bisa mencapai -1. rdesktop.c 1872

 RD_BOOL subprocess(char *const argv[], str_handle_lines_t linehandler, void *data) { int n = 1; char output[256]; .... while (n > 0) { n = read(fd[0], output, 255); output[n] = '\0'; // <= str_handle_lines(output, &rest, linehandler, data); } .... }

Isi file dibaca ke dalam buffer sampai EOF tercapai. Pada saat yang sama, kode ini tidak memiliki mekanisme penanganan kesalahan, dan jika terjadi kesalahan, baca akan kembali -1 dan eksekusi akan mulai membaca di luar batas array output .

Menggunakan EOF di char


V739 EOF tidak boleh dibandingkan dengan nilai tipe 'char'. '(C = fgetc (fp))' harus dari jenis 'int'. ctrl.c 500

 int ctrl_send_command(const char *cmd, const char *arg) { char result[CTRL_RESULT_SIZE], c, *escaped; .... while ((c = fgetc(fp)) != EOF && index < CTRL_RESULT_SIZE && c != '\n') { result[index] = c; index++; } .... }

Kode ini mengimplementasikan penanganan EOF yang salah: jika fgetc mengembalikan karakter yang kodenya 0xFF, itu akan ditafsirkan sebagai akhir file ( EOF ).

EOF adalah konstanta yang biasanya didefinisikan sebagai -1. Misalnya, dalam pengkodean CP1251, huruf terakhir dari alfabet Rusia dikodekan sebagai 0xFF, yang sesuai dengan angka -1 dalam tipe char . Ini berarti bahwa karakter 0xFF, seperti EOF (-1), akan ditafsirkan sebagai akhir file. Untuk menghindari kesalahan seperti itu, hasil yang dikembalikan oleh fungsi fgetc harus disimpan dalam variabel tipe int .

Salah ketik


Cuplikan 1

Ekspresi V547 'write_time' selalu salah. disk.c 805

 RD_NTSTATUS disk_set_information(....) { time_t write_time, change_time, access_time, mod_time; .... if (write_time || change_time) mod_time = MIN(write_time, change_time); else mod_time = write_time ? write_time : change_time; // <= .... }

Penulis kode ini harus secara tidak sengaja menggunakan || Operator bukannya && dalam kondisi. Mari kita lihat apa nilai yang bisa dimiliki variabel write_time dan change_time :
  • Kedua variabel memiliki 0. Dalam hal ini, eksekusi pindah ke cabang lain : variabel mod_time akan selalu dievaluasi ke 0 tidak peduli apa kondisi selanjutnya.
  • Salah satu variabel memiliki 0. Dalam hal ini, mod_time akan ditetapkan 0 (mengingat bahwa variabel lain memiliki nilai non-negatif) karena MIN akan memilih yang paling sedikit dari keduanya.
  • Variabel tidak memiliki 0: nilai minimum dipilih.

Mengubah baris itu menjadi write_time && change_time akan memperbaiki perilaku:
  • Hanya satu atau tidak satu pun variabel yang memiliki 0: nilai bukan nol dipilih.
  • Variabel tidak memiliki 0: nilai minimum dipilih.

Cuplikan 2

Ekspresi V547 selalu benar. Mungkin operator '&&' harus digunakan di sini. disk.c 1419

 static RD_NTSTATUS disk_device_control(RD_NTHANDLE handle, uint32 request, STREAM in, STREAM out) { .... if (((request >> 16) != 20) || ((request >> 16) != 9)) return RD_STATUS_INVALID_PARAMETER; .... }

Sekali lagi, sepertinya masalah menggunakan operator yang salah - baik || bukannya && atau == bukannya ! = karena variabel tidak dapat menyimpan nilai 20 dan 9 pada saat yang sama.

Penyalinan string tidak terbatas


V512 Panggilan fungsi 'sprintf' akan menyebabkan overflow buffer 'fullpath'. disk.c 1257

 RD_NTSTATUS disk_query_directory(....) { .... char *dirname, fullpath[PATH_MAX]; .... /* Get information for directory entry */ sprintf(fullpath, "%s/%s", dirname, pdirent->d_name); .... }

Jika Anda dapat mengikuti fungsi sampai akhir, Anda akan melihat bahwa kodenya OK, tetapi mungkin rusak suatu hari: hanya satu perubahan yang ceroboh akan berakhir dengan buffer overflow karena sprintf tidak dibatasi dengan cara apa pun, jadi menyatukan jalan bisa mengambil eksekusi di luar batas array. Kami merekomendasikan untuk mengganti panggilan ini dengan snprintf (fullpath, PATH_MAX, ....) .

Kondisi redundan


V560 Bagian dari ekspresi kondisional selalu benar: add> 0. scard.c 507

 static void inRepos(STREAM in, unsigned int read) { SERVER_DWORD add = 4 - read % 4; if (add < 4 && add > 0) { .... } }

Cek tambah> 0 tidak membuat perbedaan karena variabel akan selalu lebih besar dari nol karena baca% 4 mengembalikan sisanya, yang tidak akan pernah sama dengan 4.

xrdp


xrdp adalah server RDP open-source. Proyek ini terdiri dari dua bagian:

  • xrdp - implementasi protokol. Ini dirilis di bawah Apache 2.0.
  • xorgxrdp - koleksi driver Xorg untuk digunakan dengan xrdp. Ini dirilis di bawah X11 (seperti MIT, tetapi penggunaan dalam iklan dilarang)

Pengembangannya didasarkan pada rdesktop dan FreeRDP. Pada awalnya, agar dapat bekerja dengan grafik, Anda harus menggunakan server VNC yang terpisah atau server X11 khusus dengan dukungan RDP, X11rdp, tetapi itu menjadi tidak perlu dengan rilis xorgxrdp.

Kami tidak akan berbicara tentang xorgxrdp di artikel ini.

Sama seperti proyek sebelumnya, xrdp adalah yang kecil, terdiri dari sekitar 80 KLOC.

Gambar2


Kesalahan ketik lainnya


V525 Kode ini berisi kumpulan blok serupa. Periksa item 'r', 'g', 'r' pada baris 87, 88, 89. rfxencode_rgb_to_yuv.c 87

 static int rfx_encode_format_rgb(const char *rgb_data, int width, int height, int stride_bytes, int pixel_format, uint8 *r_buf, uint8 *g_buf, uint8 *b_buf) { .... switch (pixel_format) { case RFX_FORMAT_BGRA: .... while (x < 64) { *lr_buf++ = r; *lg_buf++ = g; *lb_buf++ = r; // <= x++; } .... } .... }

Kode ini berasal dari pustaka librfxcodec, yang mengimplementasikan codec jpeg2000 untuk bekerja dengan RemoteFX. Saluran warna "merah" dibaca dua kali, sedangkan saluran "biru" tidak dibaca sama sekali. Cacat seperti ini biasanya hasil dari penggunaan copy-paste.

Bug yang sama ditemukan di fungsi yang serupa rfx_encode_format_argb :

V525 Kode ini berisi kumpulan blok serupa. Periksa item 'a', 'r', 'g', 'r' pada baris 260, 261, 262, 263. rfxencode_rgb_to_yuv.c 260

 while (x < 64) { *la_buf++ = a; *lr_buf++ = r; *lg_buf++ = g; *lb_buf++ = r; x++; }

Deklarasi array


V557 Array overrun dimungkinkan. Nilai indeks 'i - 8' bisa mencapai 129. genkeymap.c 142

 // evdev-map.c int xfree86_to_evdev[137-8+1] = { .... }; // genkeymap.c extern int xfree86_to_evdev[137-8]; int main(int argc, char **argv) { .... for (i = 8; i <= 137; i++) /* Keycodes */ { if (is_evdev) e.keycode = xfree86_to_evdev[i-8]; .... } .... }

Dalam file genkeymap.c, array dinyatakan 1 elemen lebih pendek dari yang ditunjukkan oleh implementasi. Tidak ada bug yang terjadi, karena file evdev-map.c menyimpan ukuran yang benar, sehingga tidak akan ada array overrun, yang membuatnya cacat kecil daripada kesalahan sejati.

Perbandingan yang salah


V560 Bagian dari ekspresi kondisional selalu salah: (cap_len <0). xrdp_caps.c 616

 // common/parse.h #if defined(B_ENDIAN) || defined(NEED_ALIGN) #define in_uint16_le(s, v) do \ .... #else #define in_uint16_le(s, v) do \ { \ (v) = *((unsigned short*)((s)->p)); \ (s)->p += 2; \ } while (0) #endif int xrdp_caps_process_confirm_active(struct xrdp_rdp *self, struct stream *s) { int cap_len; .... in_uint16_le(s, cap_len); .... if ((cap_len < 0) || (cap_len > 1024 * 1024)) { .... } .... }

Nilai variabel bertipe unsigned short dibaca ke dalam variabel bertipe int dan kemudian diperiksa negatif, yang tidak perlu karena nilai yang dibaca dari tipe unsigned menjadi tipe yang lebih besar tidak pernah bisa menjadi negatif.

Cek berlebihan


V560 Bagian dari ekspresi kondisional selalu benar: (bpp! = 16). libxrdp.c 704

 int EXPORT_CC libxrdp_send_pointer(struct xrdp_session *session, int cache_idx, char *data, char *mask, int x, int y, int bpp) { .... if ((bpp == 15) && (bpp != 16) && (bpp != 24) && (bpp != 32)) { g_writeln("libxrdp_send_pointer: error"); return 1; } .... }

Pemeriksaan tidak-sama tidak diperlukan karena pemeriksaan pertama berfungsi. Si programmer mungkin akan menggunakan || operator untuk menyaring argumen yang salah.

Kesimpulan


Cek hari ini tidak mengungkapkan bug penting, tapi itu mengungkapkan banyak cacat kecil. Karena itu, proyek-proyek ini, sekecil apa pun, masih digunakan dalam banyak sistem dan, oleh karena itu, perlu dipoles. Sebuah proyek kecil seharusnya tidak memiliki banyak bug di dalamnya, jadi menguji penganalisa hanya pada proyek-proyek kecil tidak cukup untuk mengevaluasi efektivitasnya. Subjek ini dibahas lebih rinci dalam artikel " Perasaan dikonfirmasi oleh angka ".

Versi demo PVS-Studio tersedia di situs web kami.

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

More articles:


All Articles