Ito ang pangalawang pagsusuri sa isang serye ng mga artikulo tungkol sa pagsubok sa mga open source na programa para sa pagtatrabaho sa RDP protocol. Sa loob nito ay titingnan natin ang rdesktop client at ang xrdp server.
Ginamit bilang isang tool upang matukoy ang mga error . Ito ay isang static na code analyzer para sa C, C++, C# at Java na mga wika, na magagamit sa Windows, Linux at macOS platform.
Ang artikulo ay nagpapakita lamang ng mga error na tila interesante sa akin. Gayunpaman, ang mga proyekto ay maliit, kaya may ilang mga pagkakamali :).
Nota. Ang isang nakaraang artikulo tungkol sa pag-verify ng proyekto ng FreeRDP ay matatagpuan .
rdesktop
β isang libreng pagpapatupad ng isang RDP client para sa UNIX-based system. Maaari rin itong gamitin sa ilalim ng Windows kung bubuo ka ng proyekto sa ilalim ng Cygwin. Lisensyado sa ilalim ng GPLv3.
Napakasikat ng kliyenteng ito - ginagamit ito bilang default sa ReactOS, at makakahanap ka rin ng mga third-party na graphical na front-end para dito. Gayunpaman, medyo matanda na siya: ang kanyang unang paglaya ay naganap noong Abril 4, 2001 - sa oras ng pagsulat, siya ay 17 taong gulang.
Tulad ng nabanggit ko kanina, ang proyekto ay medyo maliit. Naglalaman ito ng humigit-kumulang 30 libong linya ng code, na medyo kakaiba kung isasaalang-alang ang edad nito. Para sa paghahambing, ang FreeRDP ay naglalaman ng 320 libong linya. Narito ang output ng Cloc program:
Hindi maabot na code
Natukoy ang hindi magagamit na code. Posibleng may error. 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);
}Ang error ay nakatagpo kaagad sa amin sa function pangunahin: nakikita namin ang code na darating pagkatapos ng operator pagbabalik β ang fragment na ito ay nagsasagawa ng paglilinis ng memorya. Gayunpaman, ang error ay hindi nagbabanta: ang lahat ng inilalaan na memorya ay tatanggalin ng operating system pagkatapos na lumabas ang programa.
Walang error sa paghawak
Posible ang array underrun. Ang halaga ng 'n' index ay maaaring umabot sa -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] = ' '; // <=
str_handle_lines(output, &rest, linehandler, data);
}
....
}Ang code snippet sa kasong ito ay nagbabasa mula sa file patungo sa isang buffer hanggang sa matapos ang file. Gayunpaman, walang error sa paghawak dito: kung may mali, kung gayon basahin ay babalik -1, at pagkatapos ay ang array ay masasakop output.
Paggamit ng EOF sa uri ng char
Ang EOF ay hindi dapat ikumpara sa isang halaga ng uri ng 'char'. Ang '(c = fgetc(fp))' ay dapat nasa 'int' na uri. 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++;
}
....
}Dito nakikita natin ang maling paghawak sa pag-abot sa dulo ng file: if fgetc nagbabalik ng isang character na ang code ay 0xFF, ito ay bibigyang-kahulugan bilang dulo ng file (EOF).
EOF ito ay isang pare-pareho, karaniwang tinukoy bilang -1. Halimbawa, sa pag-encode ng CP1251, ang huling titik ng alpabetong Ruso ay may code na 0xFF, na tumutugma sa numero -1 kung pinag-uusapan natin ang isang variable tulad ng tangke. Ito ay lumiliko out na ang simbolo 0xFF, tulad ng EOF (-1) ay binibigyang kahulugan bilang dulo ng file. Upang maiwasan ang mga naturang error, ang resulta ng function ay fgetc ay dapat na naka-imbak sa isang variable tulad ng int.
Mga typo
Fragment 1
Palaging mali ang ekspresyong 'write_time'. 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; // <=
....
}Marahil ay nagkamali ang may-akda ng code na ito || ΠΈ && nasa kondisyon. Isaalang-alang natin ang mga posibleng opsyon para sa mga halaga write_time ΠΈ pagbabago_panahon:
- Ang parehong mga variable ay katumbas ng 0: sa kasong ito mapupunta tayo sa isang sangay iba: variable mod_time ay palaging magiging 0 anuman ang kasunod na kundisyon.
- Ang isa sa mga variable ay 0: mod_time ay magiging katumbas ng 0 (sa kondisyon na ang ibang variable ay may hindi negatibong halaga), dahil MIN pipiliin ang mas maliit sa dalawang opsyon.
- Ang parehong mga variable ay hindi katumbas ng 0: piliin ang pinakamababang halaga.
Kapag pinapalitan ang kundisyon ng write_time && change_time magiging tama ang pag-uugali:
- Ang isa o parehong mga variable ay hindi katumbas ng 0: pumili ng isang hindi-zero na halaga.
- Ang parehong mga variable ay hindi katumbas ng 0: piliin ang pinakamababang halaga.
Fragment 2
Ang pagpapahayag ay laging totoo. Marahil ang operator na '&&' ay dapat gamitin dito. 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;
....
}Halu-halo na rin yata ang mga operator dito || ΠΈ &&O == ΠΈ !=: Ang isang variable ay hindi maaaring magkaroon ng value na 20 at 9 sa parehong oras.
Walang limitasyong pagkopya ng linya
Ang isang tawag ng 'sprintf' function ay hahantong sa pag-apaw ng 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);
....
}Kapag tiningnan mo ang function nang buo, magiging malinaw na ang code na ito ay hindi nagdudulot ng mga problema. Gayunpaman, maaaring lumitaw ang mga ito sa hinaharap: isang walang ingat na pagbabago at magkakaroon tayo ng buffer overflow - sprint ay hindi limitado sa anumang bagay, kaya kapag pinagsama-sama ang mga landas maaari tayong lumampas sa mga hangganan ng array. Inirerekomenda na mapansin ang tawag na ito sa snprintf(fullpath, PATH_MAX, β¦.).
Kalabisan kondisyon
Ang isang bahagi ng conditional expression ay palaging totoo: magdagdag ng > 0. scard.c 507
static void
inRepos(STREAM in, unsigned int read)
{
SERVER_DWORD add = 4 - read % 4;
if (add < 4 && add > 0)
{
....
}
}ΠΡΠΎΠ²Π΅ΡΠΊΠ° magdagdag ng > 0 hindi na kailangan dito: ang variable ay palaging mas malaki kaysa sa zero, dahil basahin ang% 4 ibabalik ang natitirang bahagi ng dibisyon, ngunit hindi ito magiging katumbas ng 4.
xrdp
β pagpapatupad ng isang RDP server na may open source code. Ang proyekto ay nahahati sa 2 bahagi:
- xrdp - pagpapatupad ng protocol. Ibinahagi sa ilalim ng lisensya ng Apache 2.0.
- xorgxrdp - Isang set ng mga driver ng Xorg para gamitin sa xrdp. Lisensya - X11 (tulad ng MIT, ngunit ipinagbabawal ang paggamit sa advertising)
Ang pagbuo ng proyekto ay batay sa mga resulta ng rdesktop at FreeRDP. Sa una, upang gumana sa mga graphics, kailangan mong gumamit ng isang hiwalay na VNC server, o isang espesyal na X11 server na may suporta sa RDP - X11rdp, ngunit sa pagdating ng xorgxrdp, ang pangangailangan para sa kanila ay nawala.
Sa artikulong ito hindi namin sasaklawin ang xorgxrdp.
Ang proyektong xrdp, tulad ng nauna, ay napakaliit at naglalaman ng humigit-kumulang 80 libong linya.
Marami pang typo
Ang code ay naglalaman ng koleksyon ng mga katulad na bloke. Suriin ang mga item na 'r', 'g', 'r' sa mga linya 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++;
}
....
}
....
}Ang code na ito ay kinuha mula sa librfxcodec library, na nagpapatupad ng jpeg2000 codec para sa RemoteFX. Dito, tila, ang mga graphic na channel ng data ay pinaghalo - sa halip na ang "asul" na kulay, ang "pula" ay naitala. Malamang na lumitaw ang error na ito bilang resulta ng copy-paste.
Ang parehong problema ay nangyari sa isang katulad na function rfx_encode_format_argb, na sinabi rin sa amin ng analyzer:
Ang code ay naglalaman ng koleksyon ng mga katulad na bloke. Suriin ang mga item na 'a', 'r', 'g', 'r' sa mga linya 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++;
}Array Deklarasyon
Posible ang pag-overrun ng array. Ang halaga ng 'i β 8' index ay maaaring umabot sa 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];
....
}
....
}Ang deklarasyon at kahulugan ng array sa dalawang file na ito ay hindi magkatugma - ang laki ay nag-iiba ng 1. Gayunpaman, walang mga error na nangyari - ang tamang laki ay tinukoy sa evdev-map.c file, kaya walang out of bounds. Kaya ito ay isang bug lamang na madaling maayos.
Maling paghahambing
Ang isang bahagi ng conditional expression ay palaging mali: (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))
{
....
}
....
}Ang function ay nagbabasa ng isang uri ng variable unsigned maikling sa isang variable na tulad ng int. Hindi kailangan ang pagsuri dito dahil nagbabasa kami ng isang unsigned variable at itinatalaga ang resulta sa isang mas malaking variable, kaya hindi maaaring kumuha ng negatibong value ang variable.
Mga hindi kinakailangang pagsusuri
Ang isang bahagi ng conditional expression ay palaging totoo: (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;
}
....
}Walang saysay ang mga pagsusuri sa hindi pagkakapantay-pantay dito dahil mayroon na tayong paghahambing sa simula. Malamang na ito ay isang typo at gustong gamitin ng developer ang operator || upang i-filter ang mga di-wastong argumento.
Konklusyon
Sa panahon ng pag-audit, walang malubhang pagkakamali ang natukoy, ngunit maraming mga pagkukulang ang natagpuan. Gayunpaman, ang mga disenyong ito ay ginagamit sa maraming mga sistema, kahit na maliit ang saklaw. Ang isang maliit na proyekto ay hindi kinakailangang magkaroon ng maraming mga error, kaya hindi mo dapat husgahan ang pagganap ng analyzer lamang sa maliliit na proyekto. Maaari mong basahin ang higit pa tungkol dito sa artikulong "".
Maaari kang mag-download ng trial na bersyon ng PVS-Studio mula sa amin .
Kung gusto mong ibahagi ang artikulong ito sa isang madla na nagsasalita ng Ingles, mangyaring gamitin ang link ng pagsasalin: Sergey Larin.
Pinagmulan: www.habr.com