Dit is de tweede review in de serie artikelen over de controle van open programma's voor het werken met het RDP-protocol. In dit artikel bekijken we de client rdesktop en de server xrdp.
Als hulpmiddel voor het identificeren van fouten werd gebruikt . Dit is een statische code-analyzer voor de talen C, C++, C# en Java, beschikbaar op de platforms Windows, Linux en macOS.
In dit artikel worden alleen de fouten gepresenteerd die mij interessant leken. Aangezien de projecten klein zijn, waren er echter ook niet veel fouten. :)
Opmerking. Het vorige artikel over de controle van het FreeRDP-project is te vinden .
rdesktop
— een vrije implementatie van de RDP-client voor UNIX-gebaseerde systemen. Het kan ook onder Windows worden gebruikt als het project onder Cygwin wordt samengesteld. Gecertificeerd onder GPLv3.
Deze client is erg populair — hij wordt standaard gebruikt in ReactOS, en er zijn ook externe grafische front-ends beschikbaar. Desalniettemin is hij behoorlijk oud: de eerste release vond plaats op 4 april 2001 — op het moment van schrijven is hij 17 jaar oud.
Zoals ik eerder al aangaf, is het project heel klein. Het bevat ongeveer 30.000 regels code, wat vreemd is gezien zijn leeftijd. Ter vergelijking: FreeRDP bevat 320.000 regels. Hier is de output van het Cloc-programma:
Onbereikbaar code
Onbereikbare code gedetecteerd. Het is mogelijk dat er een fout aanwezig is. 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);
}De fout begroet ons onmiddellijk in de functie main: we zien code die volgt na de operator terug — dit fragment voert geheugenreiniging uit. Desondanks vormt de fout geen bedreiging: al het toegewezen geheugen zal door het besturingssysteem worden opgeruimd na het beëindigen van het programma.
Afwezigheid van foutafhandeling
Array underrun is mogelijk. De waarde van de index ‘n’ kan -1 bereiken. 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);
}
....
}Het codefragment leest in dit geval uit een bestand naar de buffer totdat het bestand is afgelopen. Echter, hier ontbreekt foutafhandeling: als er iets misgaat, read zal het -1 retourneren, en dan zal er een overschrijding van de array plaatsvinden output.
Gebruik van EOF in het type char
EOF mag niet worden vergeleken met een waarde van het type 'char'. De '(c = fgetc(fp))' moet van het type 'int' zijn. 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++;
}
....
}Hier zien we een onjuiste verwerking van het bereiken van het einde van een bestand: als fgetc een teken retourneert waarvan de code gelijk is aan 0xFF, zal dit worden geïnterpreteerd als het einde van het bestand (EOF).
EOF dit is een constante die meestal wordt gedefinieerd als -1. Bijvoorbeeld, in de CP1251 codering heeft de laatste letter van het Russische alfabet de code 0xFF, wat overeenkomt met het getal -1, als we het hebben over een variabele van het type char. Dit betekent dat het teken 0xFF, net als EOF (-1) wordt gezien als het einde van het bestand. Om dergelijke fouten te voorkomen, moet het resultaat van de functie fgetc worden opgeslagen in een variabele van het type int.
Typfouten
Fragment 1
Expressie 'write_time' is altijd onwaar. 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; // <=
....
}Misschien heeft de auteur van deze code verwisseld || en && in de voorwaarde. Laten we de mogelijke waarden beschouwen write_time en change_time:
- Beide variabelen zijn gelijk aan 0: in dit geval komen we in tak else: de variabele mod_time zal altijd gelijk zijn aan 0, ongeacht de daaropvolgende voorwaarde.
- Een van de variabelen is gelijk aan 0: mod_time zal gelijk zijn aan 0 (op voorwaarde dat de andere variabele een niet-negatieve waarde heeft), omdat MIN de kleinste van de twee opties zal kiezen.
- Beide variabelen zijn niet gelijk aan 0: kies de minimumwaarde.
Door de voorwaarde te wijzigen in write_time && change_time zal het gedrag er correct uitzien:
- Een of beide variabelen zijn niet gelijk aan 0: kies een niet-nulwaarde.
- Beide variabelen zijn niet gelijk aan 0: kies de minimumwaarde.
Fragment 2
Expressie is altijd waar. Waarschijnlijk moet hier de ‘&&’ operator worden gebruikt. 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;
....
}Het lijkt erop dat hier ook de operators zijn verwisseld: || en &&, ofwel == en !=de variabele kan niet tegelijkertijd de waarde 20 en 9 aannemen.
Ongelimiteerd kopiëren van een tekenreeks
Een oproep naar de ‘sprintf’ functie zal leiden tot een buffer-overloop van de ‘fullpath’. disk.c 1257
RD_NTSTATUS
disk_query_directory(....)
{
....
char *dirname, fullpath[PATH_MAX];
....
/* Informatie verkrijgen voor directory-invoer */
sprintf(fullpath, "%s/%s", dirname, pdirent->d_name);
....
}Bij het volledig bekijken van de functie wordt duidelijk dat deze code geen problemen veroorzaakt. Echter, ze kunnen in de toekomst ontstaan: één ondoordachte wijziging en we krijgen buffer-overloop — sprintf geen beperking, dus bij het samenvoegen van paden kunnen we buiten de grenzen van de array komen. Het wordt aangeraden om deze aanroep te noteren op snprintf(fullpath, PATH_MAX, ….).
Overbodige voorwaarde
Een deel van de voorwaardelijke expressie is altijd waar: voeg > 0 toe. scard.c 507
static void
inRepos(STREAM in, unsigned int read)
{
SERVER_DWORD add = 4 - read % 4;
if (add 0)
{
....
}
}Controle add > 0 maakt hier niet uit: de variabele zal altijd groter dan nul zijn, omdat read % 4 de rest van de deling retourneert, en deze kan nooit gelijk zijn aan 4.
xrdp
— implementatie van een RDP-server met open source. Het project is verdeeld in 2 delen:
- xrdp — implementatie van het protocol. Het wordt verspreid onder de Apache 2.0-licentie.
- xorgxrdp — een set Xorg-stuurprogramma's voor gebruik met xrdp. Licentie — X11 (zoals MIT, maar verbiedt gebruik in reclame)
De ontwikkeling van het project is gebaseerd op de resultaten van rdesktop en FreeRDP. Aanvankelijk was het noodzakelijk om met grafiek te werken met een aparte VNC-server, of een speciale X11-server met RDP-ondersteuning — X11rdp, maar met de komst van xorgxrdp is die behoefte verdwenen.
In dit artikel zullen we xorgxrdp niet bespreken.
Het project xrdp, net als de vorige, is heel klein en bevat ongeveer 80.000 regels.
Nog typfouten
De code bevat een verzameling soortgelijke blokken. Controleer de items 'r', 'g', 'r' in regels 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++;
}
....
}
....
}Deze code is afkomstig uit de librfxcodec-bibliotheek, die de jpeg2000-codec implementeert voor gebruik met RemoteFX. Hier zijn waarschijnlijk de kanalen van de grafische gegevens verwisseld — in plaats van de kleur ‘blauw’ wordt ‘rood’ opgeslagen. Dergelijke fouten ontstaan waarschijnlijk door copy-paste.
Dit probleem is ook in een soortgelijke functie beland rfx_encode_format_argb, wat de analyzer ons ook meldde:
De code bevat een verzameling soortgelijke blokken. Controleer de items 'a', 'r', 'g', 'r' in regels 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++;
}Declaratie van de array
Array-overrun is mogelijk. De waarde van de index 'i - 8' zou 129 kunnen bereiken. 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];
....
}
....
}De declaratie en definitie van de array in deze twee bestanden zijn niet compatibel — de grootte verschilt met 1. Echter, er treden geen fouten op — in het bestand evdev-map.c is de juiste grootte opgegeven, dus er is geen overschrijding. Het is gewoon een slordigheid die gemakkelijk te verhelpen is.
Ongeldige vergelijking
Een deel van de voorwaardelijke expressie is altijd onwaar: (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))
{
....
}
....
}In de functie vindt het lezen van een variabele van het type unsigned short in een variabele van het type intHier is geen controle nodig, omdat we een unsigned variabele uitlezen en het resultaat toekennen aan een grotere variabele, dus kan de variabele geen negatieve waarde aannemen.
Onnodige controles
Een deel van de voorwaardelijke expressie is altijd waar: (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: fout");
return 1;
}
....
}Onveranderlijkheidscontroles zijn hier niet zinvol, aangezien we al een vergelijking aan het begin hebben. Het is heel goed mogelijk dat dit een typfout is en dat de ontwikkelaar de operator wilde gebruiken || om onjuiste argumenten te filteren.
Conclusie
Tijdens de controle zijn er geen ernstige fouten gevonden, maar er zijn veel tekortkomingen ontdekt. Deze projecten worden echter in veel systemen gebruikt, hoewel ze klein van omvang zijn. In een klein project hoeven niet veel fouten voor te komen, dus het is niet terecht om de werking van de analyzer alleen op kleine projecten te beoordelen. Meer hierover is te lezen in het artikel "«.
U kunt de proefversie van PVS-Studio bij ons downloaden op .
Als je dit artikel wilt delen met een Engelstalig publiek, gebruik dan alsjeblieft de link naar de vertaling: Sergey Larin.
Bron: habr.com
