Aquesta és la segona revisió d'una sèrie d'articles sobre proves de programes de codi obert per treballar amb el protocol RDP. En ella mirarem el client rdesktop i el servidor xrdp.
S'utilitza com a eina per identificar errors . És un analitzador de codi estàtic per a llenguatges C, C++, C# i Java, disponible a les plataformes Windows, Linux i macOS.
L'article només presenta aquells errors que em van semblar interessants. Tanmateix, els projectes són petits, així que hi va haver pocs errors :).
Nota. Es pot trobar un article anterior sobre la verificació del projecte FreeRDP .
rdesktop
— una implementació gratuïta d'un client RDP per a sistemes basats en UNIX. També es pot utilitzar a Windows si creeu el projecte amb Cygwin. Amb llicència sota GPLv3.
Aquest client és molt popular: s'utilitza de manera predeterminada a ReactOS i també podeu trobar-hi front-end gràfics de tercers. No obstant això, és bastant vell: el seu primer llançament va tenir lloc el 4 d'abril de 2001; en el moment d'escriure aquest escrit, té 17 anys.
Com he comentat abans, el projecte és molt petit. Conté aproximadament 30 mil línies de codi, la qual cosa és una mica estrany tenint en compte la seva antiguitat. Per comparar, FreeRDP conté 320 mil línies. Aquí teniu la sortida del programa Cloc:
Codi inabastable
S'ha detectat un codi no disponible. És possible que hi hagi un 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);
}L'error ens troba immediatament a la funció principal: veiem el codi que ve després de l'operador return — aquest fragment realitza la neteja de memòria. Tanmateix, l'error no suposa cap amenaça: tota la memòria assignada serà esborrada pel sistema operatiu després de sortir del programa.
Sense tractament d'errors
És possible que la matriu sigui inferior. El valor de l'índex 'n' podria arribar a -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);
}
....
}En aquest cas, el fragment de codi es llegeix del fitxer a una memòria intermèdia fins que finalitza el fitxer. Tanmateix, aquí no hi ha cap tractament d'errors: si alguna cosa va malament, aleshores llegir retornarà -1 i, aleshores, la matriu serà desbordada sortida.
Utilitzant EOF en tipus de caràcters
L'EOF no s'ha de comparar amb un valor del tipus 'char'. El '(c = fgetc(fp))' hauria de ser del tipus '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++;
}
....
}Aquí veiem un maneig incorrecte d'arribar al final del fitxer: si fgetc retorna un caràcter el codi del qual és 0xFF, s'interpretarà com el final del fitxer (EOF).
EOF és una constant, normalment definida com -1. Per exemple, a la codificació CP1251, l'última lletra de l'alfabet rus té el codi 0xFF, que correspon al número -1 si estem parlant d'una variable com char. Resulta que el símbol 0xFF, com EOF (-1) s'interpreta com el final del fitxer. Per evitar aquests errors, el resultat de la funció és fgetc s'ha d'emmagatzemar en una variable com int.
Errors tipogràfics
Fragment 1
L'expressió 'write_time' sempre és falsa. disc.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; // <=
....
}Potser l'autor d'aquest codi s'ha equivocat || и && en condicions. Considerem possibles opcions per als valors temps_escriptura и canvi_hora:
- Ambdues variables són iguals a 0: en aquest cas acabarem en una branca else: variable temps_mod sempre serà 0 independentment de la condició posterior.
- Una de les variables és 0: temps_mod serà igual a 0 (sempre que l'altra variable tingui un valor no negatiu), perquè MIN triarà la més petita de les dues opcions.
- Les dues variables no són iguals a 0: trieu el valor mínim.
En substituir la condició per write_time && change_time el comportament semblarà correcte:
- Una o les dues variables no són iguals a 0: trieu un valor diferent de zero.
- Les dues variables no són iguals a 0: trieu el valor mínim.
Fragment 2
L'expressió sempre és certa. Probablement l'operador '&&' s'hauria d'utilitzar aquí. disc.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;
....
}Sembla que els operadors també estan barrejats aquí || и &&O == и !=: Una variable no pot tenir el valor 20 i 9 alhora.
Còpia de línies il·limitades
Una crida a la funció 'sprintf' provocarà un desbordament de la memòria intermèdia 'fullpath'. disc.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);
....
}Quan mireu la funció al complet, quedarà clar que aquest codi no causa problemes. No obstant això, poden sorgir en el futur: un canvi descuidat i tindrem un desbordament de buffer - sprint no està limitat per res, de manera que en concatenar camins podem anar més enllà dels límits de la matriu. Es recomana notar aquesta convocatòria snprintf(camí complet, PATH_MAX, ….).
Condició redundant
Una part de l'expressió condicional sempre és certa: afegeix > 0. scard.c 507
static void
inRepos(STREAM in, unsigned int read)
{
SERVER_DWORD add = 4 - read % 4;
if (add < 4 && add > 0)
{
....
}
}inspecció sumar > 0 aquí no cal: la variable sempre serà major que zero, perquè llegir % 4 retornarà la resta de la divisió, però mai serà igual a 4.
xrdp
— implementació d'un servidor RDP amb codi font obert. El projecte es divideix en 2 parts:
- xrdp - implementació del protocol. Distribuït sota la llicència Apache 2.0.
- xorgxrdp - Un conjunt de controladors Xorg per utilitzar amb xrdp. Llicència: X11 (com el MIT, però prohibeix l'ús en publicitat)
El desenvolupament del projecte es basa en els resultats de rdesktop i FreeRDP. Inicialment, per treballar amb gràfics, calia utilitzar un servidor VNC separat o un servidor X11 especial amb suport RDP: X11rdp, però amb l'arribada de xorgxrdp, la necessitat d'ells va desaparèixer.
En aquest article no tractarem xorgxrdp.
El projecte xrdp, com l'anterior, és molt petit i conté aproximadament 80 mil línies.
Més faltes d'ortografia
El codi conté la col·lecció de blocs similars. Comproveu els elements "r", "g", "r" a les línies 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++;
}
....
}
....
}Aquest codi es va extreure de la biblioteca librfxcodec, que implementa el còdec jpeg2000 per a RemoteFX. Aquí, aparentment, els canals de dades gràfics es barregen: en comptes del color "blau", es grava el "vermell". Aquest error probablement va aparèixer com a resultat de copiar i enganxar.
El mateix problema es va produir en una funció similar rfx_encode_format_argb, que l'analitzador també ens va dir:
El codi conté la col·lecció de blocs similars. Comproveu els elements "a", "r", "g", "r" a les línies 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++;
}Declaració de matriu
És possible l'excés de matriu. El valor de l'índex 'i — 8' podria arribar a 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];
....
}
....
}La declaració i la definició de la matriu en aquests dos fitxers són incompatibles: la mida difereix en 1. Tanmateix, no es produeixen errors; la mida correcta s'especifica al fitxer evdev-map.c, de manera que no hi ha límits. Per tant, aquest és només un error que es pot solucionar fàcilment.
Comparació incorrecta
Una part de l'expressió condicional és sempre falsa: (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))
{
....
}
....
}La funció llegeix una variable de tipus curt sense signar en una variable com int. No cal comprovar aquí perquè estem llegint una variable sense signe i assignant el resultat a una variable més gran, de manera que la variable no pot prendre un valor negatiu.
Comprovacions innecessàries
Una part de l'expressió condicional és sempre certa: (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;
}
....
}Els controls de desigualtat no tenen sentit aquí, ja que ja tenim una comparació al principi. És probable que es tracti d'una errada ortogràfica i el desenvolupador volgués utilitzar l'operador || per filtrar arguments no vàlids.
Conclusió
Durant l'auditoria, no es van identificar errors greus, però sí moltes mancances. Tanmateix, aquests dissenys s'utilitzen en molts sistemes, encara que d'abast petit. Un projecte petit no té necessàriament molts errors, de manera que no hauríeu de jutjar el rendiment de l'analitzador només en projectes petits. Podeu llegir més sobre això a l'article "".
Podeu descarregar-nos una versió de prova de PVS-Studio .
Si voleu compartir aquest article amb un públic de parla anglesa, utilitzeu l'enllaç de traducció: Sergey Larin.
Font: www.habr.com