Dit is de tweede recensie in een reeks artikelen over het testen van open source-programma's voor het werken met het RDP-protocol. Daarin zullen we kijken naar de rdesktop-client en de xrdp-server.
Gebruikt als hulpmiddel om fouten te identificeren . Het is een statische code-analysator voor de talen C, C++, C# en Java, beschikbaar op Windows-, Linux- en macOS-platforms.
Het artikel bevat alleen die fouten die mij interessant leken. De projecten zijn echter klein, dus er waren weinig fouten :).
Noot. Een eerder artikel over FreeRDP-projectverificatie kunt u vinden .
desktop
— een gratis implementatie van een RDP-client voor op UNIX gebaseerde systemen. Het kan ook onder Windows worden gebruikt als u het project onder Cygwin bouwt. Gelicentieerd onder GPLv3.
Deze client is erg populair - hij wordt standaard gebruikt in ReactOS, en je kunt er ook grafische front-ends van derden voor vinden. Hij is echter vrij oud: zijn eerste release vond plaats op 4 april 2001 - op het moment van schrijven is hij 17 jaar oud.
Zoals ik eerder opmerkte, is het project vrij klein. Het bevat ongeveer 30 regels code, wat een beetje vreemd is gezien de leeftijd. Ter vergelijking: FreeRDP bevat 320 duizend regels. Hier is de uitvoer van het Cloc-programma:
Onbereikbare code
Niet-beschikbare 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 komt ons onmiddellijk tegen in de functie hoofd-: we zien de code achter de operator aan komen terugkeer — dit fragment voert een geheugenopschoning uit. De fout vormt echter geen bedreiging: al het toegewezen geheugen wordt door het besturingssysteem gewist nadat het programma is afgesloten.
Geen foutafhandeling
Array-underrun is mogelijk. De waarde van de 'n'-index zou -1 kunnen 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 wordt in dit geval vanuit het bestand in een buffer gelezen totdat het bestand eindigt. Er is hier echter geen sprake van foutafhandeling: als er iets misgaat, dan dit artikel lezen retourneert -1, waarna de array wordt overschreden uitgang.
EOF gebruiken in char-type
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 afhandeling van het bereiken van het einde van het bestand: if fgetc retourneert een teken waarvan de code 0xFF is, dit zal worden geïnterpreteerd als het einde van het bestand (EOF).
EOF het is een constante, meestal gedefinieerd als -1. In de CP1251-codering heeft de laatste letter van het Russische alfabet bijvoorbeeld de code 0xFF, wat overeenkomt met het getal -1 als we het hebben over een variabele als verkolen. Het blijkt dat het symbool 0xFF, zoals EOF (-1) wordt geïnterpreteerd als het einde van het bestand. Om dergelijke fouten te voorkomen, is het resultaat van de functie fgetc moet worden opgeslagen in een variabele zoals int.
Typefouten
Fragment 1
Expressie 'write_time' is altijd onwaar. schijf.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 het mis || и && in conditie. Laten we mogelijke opties voor waarden bekijken schrijf_tijd и verander tijd:
- Beide variabelen zijn gelijk aan 0: in dit geval komen we in een vertakking terecht anders: variabel mod_tijd zal altijd 0 zijn, ongeacht de volgende voorwaarde.
- Eén van de variabelen is 0: mod_tijd zal gelijk zijn aan 0 (op voorwaarde dat de andere variabele een niet-negatieve waarde heeft), omdat MIN zal de kleinste van de twee opties kiezen.
- Beide variabelen zijn niet gelijk aan 0: kies de minimumwaarde.
Bij het vervangen van de toestand door schrijftijd && verandertijd het gedrag zal er correct uitzien:
- Eén of beide variabelen zijn niet gelijk aan 0: kies een waarde die niet nul is.
- Beide variabelen zijn niet gelijk aan 0: kies de minimumwaarde.
Fragment 2
Expressie is altijd waar. Waarschijnlijk moet hier de operator '&&' worden gebruikt. schijf.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;
....
}Blijkbaar zijn de operators hier ook in de war || и &&Of == и !=: Een variabele kan niet tegelijkertijd de waarde 20 en 9 hebben.
Onbeperkt kopiëren van regels
Een aanroep van de functie 'sprintf' zal leiden tot een overloop van de buffer 'fullpath'. schijf.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);
....
}Als je de functie volledig bekijkt, wordt het duidelijk dat deze code geen problemen veroorzaakt. Ze kunnen zich echter in de toekomst voordoen: één onzorgvuldige verandering en we krijgen een bufferoverloop - sprint wordt door niets beperkt, dus bij het aaneenschakelen van paden kunnen we voorbij de grenzen van de array gaan. Het verdient aanbeveling deze oproep op te merken snprintf(volledig pad, PATH_MAX, ….).
Overbodige toestand
Een deel van de voorwaardelijke expressie is altijd waar: add > 0. scard.c 507
static void
inRepos(STREAM in, unsigned int read)
{
SERVER_DWORD add = 4 - read % 4;
if (add < 4 && add > 0)
{
....
}
}Проверка optellen > 0 dat is hier niet nodig: de variabele zal altijd groter zijn dan nul, omdat lees % 4 zal de rest van de deling retourneren, maar deze zal nooit gelijk zijn aan 4.
xrdp
— implementatie van een RDP-server met open source-code. Het project is opgedeeld in 2 delen:
- xrdp - protocolimplementatie. Gedistribueerd onder de Apache 2.0-licentie.
- xorgxrdp - Een set Xorg-stuurprogramma's voor gebruik met xrdp. Licentie - X11 (zoals MIT, maar verbiedt gebruik in advertenties)
De ontwikkeling van het project is gebaseerd op de resultaten van rdesktop en FreeRDP. Om met grafische afbeeldingen te werken, moest je aanvankelijk een aparte VNC-server gebruiken, of een speciale X11-server met RDP-ondersteuning - X11rdp, maar met de komst van xorgxrdp verdween de behoefte eraan.
In dit artikel behandelen we xorgxrdp niet.
Het xrdp-project is, net als het vorige, erg klein en bevat ongeveer 80 regels.
Nog meer typefouten
De code bevat de verzameling vergelijkbare blokken. Controleer 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 voor RemoteFX implementeert. Hier zijn blijkbaar de grafische gegevenskanalen verwisseld - in plaats van de "blauwe" kleur wordt "rood" opgenomen. Deze fout is hoogstwaarschijnlijk ontstaan als gevolg van kopiëren en plakken.
Hetzelfde probleem deed zich voor in een vergelijkbare functie rfx_encode_format_argb, wat de analysator ons ook vertelde:
De code bevat de verzameling vergelijkbare blokken. Controleer 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++;
}Array-declaratie
Array-overrun is mogelijk. De waarde van de 'i — 8'-index 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 incompatibel - de grootte verschilt met 1. Er treden echter geen fouten op - de juiste grootte is gespecificeerd in het bestand evdev-map.c, dus er is geen buitengrens. Dit is dus slechts een bug die eenvoudig kan worden opgelost.
Onjuiste 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))
{
....
}
....
}De functie leest een typevariabele ongetekend kort in een variabele als int. Controle is hier niet nodig omdat we een niet-ondertekende variabele lezen en het resultaat aan een grotere variabele toewijzen, zodat de variabele geen negatieve waarde kan 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: error");
return 1;
}
....
}Controles op de ongelijkheid hebben hier geen zin, omdat we in het begin al een vergelijking hebben. Het is waarschijnlijk dat dit een typefout is en dat de ontwikkelaar de operator wilde gebruiken || om ongeldige argumenten eruit te filteren.
Conclusie
Tijdens de audit zijn geen ernstige fouten geconstateerd, maar er zijn wel veel tekortkomingen geconstateerd. Deze ontwerpen worden echter in veel systemen gebruikt, zij het met een kleine omvang. Een klein project heeft niet noodzakelijkerwijs veel fouten, dus u moet de prestaties van de analysator niet alleen bij kleine projecten beoordelen. Meer hierover leest u in het artikel “".
U kunt bij ons een proefversie van PVS-Studio downloaden .
Als u dit artikel met een Engelssprekend publiek wilt delen, gebruik dan de vertaallink: Sergey Larin.
Bron: www.habr.com