Dit is het tweede overzicht in een reeks artikelen over het controleren van opensourceprogramma's op geschiktheid voor het RDP-protocol. Hierin kijken we naar de rdesktop-client en de xrdp-server.
Het hulpmiddel dat werd gebruikt om fouten te identificeren was Het is een statische code-analysator voor C, C++, C# en Java, beschikbaar op verschillende platformen. Windows, Linux и macOS.
Het artikel vermeldt alleen de fouten die ik interessant vind. Het zijn echter kleine projecten, dus er zijn weinig fouten gemaakt :).
Noot. Een eerder artikel over de FreeRDP-projectbeoordeling is te vinden .
desktop
— свободная реализация клиента RDP для UNIX-based систем. Его также можно использовать и под Windows, если собирать проект под Cygwin. Лицензирован под GPLv3.
Deze client is erg populair: deze wordt standaard gebruikt in ReactOS en er zijn ook grafische front-ends van derden voor beschikbaar. Het is echter al behoorlijk oud: de eerste release was op 4 april 2001, dus op het moment van schrijven is het 17 jaar oud.
Zoals ik al eerder zei, het project is erg klein. Het bevat ongeveer 30 regels code, wat vreemd is gezien de leeftijd. Ter vergelijking: FreeRDP bevat 320 duizend regels. Dit is de uitvoer van het Cloc-programma:
Onbereikbare code
Onbereikbare code gedetecteerd. Het is mogelijk dat er een fout is opgetreden. 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 direct in de functie tegen hoofd-: we zien de code die na de operator komt terugkeer - dit fragment voert geheugenwissing uit. De fout vormt echter geen bedreiging: nadat het programma is beëindigd, wordt het gehele toegewezen geheugen door het besturingssysteem gewist.
Geen foutverwerking
Array-underrun is mogelijk. De waarde van 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);
}
....
}In dit geval leest het codefragment het bestand in de buffer totdat het bestand is afgelopen. Er is hier echter geen sprake van foutafhandeling: als er iets fout gaat, dan dit artikel lezen zal -1 retourneren, en dan zal de array buiten de grenzen zijn uitvoer.
EOF gebruiken in char-type
EOF mag niet worden vergeleken met een waarde van het type 'char'. '(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: als fgetc retourneert een teken waarvan de code 0xFF is, dan wordt dit gezien als het einde van het bestand (EOF).
EOF Dit 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 van het type verkolen. Het blijkt dat het symbool 0xFF, zoals EOF (-1) wordt beschouwd als het einde van het bestand. Om dergelijke fouten te voorkomen, moet het resultaat van de functie fgetc moet worden opgeslagen in een variabele van het type int.
Typefouten
Fragment 1
Expressie 'write_time' is altijd false. 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 verkeerd begrepen || и && in de staat. Laten we mogelijke waarden overwegen schrijftijd и veranderingstijd:
- Beide variabelen zijn 0: in dit geval komen we bij de tak anders:variabele mod_tijd zal altijd gelijk zijn aan 0, ongeacht de daaropvolgende voorwaarde.
- Eén van de variabelen is gelijk aan 0: mod_tijd zal gelijk zijn aan 0 (op voorwaarde dat de andere variabele een niet-negatieve waarde heeft), omdat MIN kiest de kleinste van de twee opties.
- Beide variabelen zijn niet gelijk aan 0: kies de minimumwaarde.
Bij het vervangen van de voorwaarde 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 operatoren hier ook verwisseld. || и &&Of == и !=: een variabele kan niet tegelijkertijd de waarde 20 en 9 hebben.
Onbeperkt kopiëren van strings
Een aanroep van de functie 'sprintf' leidt 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);
....
}Wanneer je de functie in zijn geheel bekijkt, wordt het duidelijk dat deze code geen problemen oplevert. Ze kunnen zich echter in de toekomst voordoen: één onzorgvuldige wijziging en we krijgen een bufferoverloop - sprint wordt door niets beperkt, dus bij het samenvoegen van paden kunnen we buiten de grenzen van de array gaan. Het is aan te raden om 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)
{
....
}
}Проверка toevoegen > 0 hier is geen punt: de variabele zal altijd groter zijn dan nul, omdat lees % 4 retourneert de restwaarde van de deling en zal nooit gelijk zijn aan 4.
xrdp
— open source RDP-serverimplementatie. Het project is verdeeld in twee delen:
- xrdp - protocol implementatie. Gedistribueerd onder de Apache 2.0-licentie.
- xorgxrdp - een set Xorg-stuurprogramma's voor gebruik met xrdp. Licentie - X11 (zoals MIT, maar gebruik in advertenties is verboden)
De ontwikkeling van het project is gebaseerd op de resultaten van rdesktop en FreeRDP. In eerste instantie was het voor het werken met graphics nodig om een aparte VNC-server te gebruiken, of een speciale X11-server met RDP-ondersteuning (X11rdp). Met de komst van xorgxrdp verdween echter de behoefte hieraan.
In dit artikel gaan we niet in op xorgxrdp.
Het xrdp-project is net als het vorige vrij klein en bevat ongeveer 80 duizend regels.
Meer typefouten
De code bevat de verzameling van 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 de grafische gegevenskanalen blijkbaar verwisseld: in plaats van de kleur 'blauw' wordt 'rood' opgenomen. Deze fout is waarschijnlijk ontstaan door kopiëren en plakken.
Hetzelfde probleem is ook in een soortgelijke functie opgetreden rfx_encode_format_argb, waar de analysator ons ook over vertelde:
De code bevat de verzameling van 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 index 'i — 8' kan 129 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. Er doen zich echter geen fouten voor: de juiste grootte is opgegeven in het bestand evdev-map.c, dus er treedt geen out-of-bounds-fout op. Het is dus gewoon een fout die gemakkelijk opgelost kan worden.
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 variabele van het type ongetekend kort in een variabele van het type int. Hier is geen controle nodig, omdat we een ongetekende variabele lezen en het resultaat aan een grotere variabele toewijzen. De variabele kan dus 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: error");
return 1;
}
....
}Ongelijkheidstesten hebben hier geen zin, omdat we aan het begin al een vergelijking hebben. Het is goed mogelijk dat dit een typefout is en dat de ontwikkelaar de operator wilde gebruiken || om ongeldige argumenten eruit te filteren.
Conclusie
Tijdens de audit werden geen ernstige fouten geconstateerd, maar er werden wel veel tekortkomingen vastgesteld. Toch worden deze projecten in veel systemen gebruikt, zij het in kleinere omvang. Een klein project bevat niet noodzakelijkerwijs veel fouten. Daarom moet u de prestaties van de analyzer niet alleen op kleine projecten beoordelen. Meer hierover kunt u lezen in het artikel “".
U kunt een proefversie van PVS-Studio bij ons downloaden op .
Als u dit artikel met een Engelssprekend publiek wilt delen, gebruik dan de vertaallink: Sergey Larin.
Bron: www.habr.com
