Questa è la seconda recensione di una serie di articoli sui test dei programmi open source per lavorare con il protocollo RDP. In esso esamineremo il client rdesktop e il server xrdp.
Utilizzato come strumento per identificare gli errori . È un analizzatore di codice statico per i linguaggi C, C++, C# e Java, disponibile su piattaforme Windows, Linux e macOS.
L'articolo presenta solo quegli errori che mi sono sembrati interessanti. Tuttavia, i progetti sono piccoli, quindi ci sono stati pochi errori :).
Nota. È possibile trovare un articolo precedente sulla verifica del progetto FreeRDP .
desktop
— un'implementazione gratuita di un client RDP per sistemi basati su UNIX. Può essere utilizzato anche in Windows se si crea il progetto in Cygwin. Concesso in licenza con GPLv3.
Questo client è molto popolare: viene utilizzato per impostazione predefinita in ReactOS e puoi anche trovare front-end grafici di terze parti per esso. Tuttavia, è piuttosto vecchio: la sua prima liberazione è avvenuta il 4 aprile 2001 - al momento in cui scrivo ha 17 anni.
Come ho notato prima, il progetto è piuttosto piccolo. Contiene circa 30mila righe di codice, il che è un po' strano considerando la sua età. Per fare un confronto, FreeRDP contiene 320mila righe. Ecco l'output del programma Cloc:
Codice irraggiungibile
Rilevato codice non disponibile. È possibile che sia presente un errore. 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'errore si riscontra immediatamente nella funzione principale: vediamo il codice che segue l'operatore ritorno — questo frammento esegue la pulizia della memoria. Tuttavia, l'errore non rappresenta una minaccia: tutta la memoria allocata verrà cancellata dal sistema operativo dopo la chiusura del programma.
Nessuna gestione degli errori
È possibile un underrun dell'array. Il valore dell'indice 'n' potrebbe raggiungere -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);
}
....
}Lo snippet di codice in questo caso legge dal file in un buffer finché il file non termina. Qui però non c'è alcuna gestione degli errori: se qualcosa va storto, allora read restituirà -1 e quindi l'array verrà sovraccaricato produzione.
Utilizzo di EOF nel tipo char
EOF non deve essere confrontato con un valore del tipo 'char'. Il '(c = fgetc(fp))' dovrebbe essere del tipo '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++;
}
....
}Qui vediamo una gestione errata del raggiungimento della fine del file: if fgetc restituisce un carattere il cui codice è 0xFF, verrà interpretato come la fine del file (EOF).
EOF è una costante, solitamente definita come -1. Ad esempio, nella codifica CP1251, l'ultima lettera dell'alfabeto russo ha il codice 0xFF, che corrisponde al numero -1 se parliamo di una variabile come serbatoio. Si scopre che il simbolo 0xFF, come EOF (-1) viene interpretato come la fine del file. Per evitare tali errori, il risultato della funzione è fgetc dovrebbe essere memorizzato in una variabile come int.
Errori di battitura
Frammento 1
L'espressione 'write_time' è sempre falsa. disco.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; // <=
....
}Forse l'autore di questo codice ha sbagliato || и && in condizione. Consideriamo le possibili opzioni per i valori write_time и cambia_ora:
- Entrambe le variabili sono pari a 0: in questo caso ci ritroveremo in un ramo altro: variabile mod_time sarà sempre 0 indipendentemente dalla condizione successiva.
- Una delle variabili è 0: mod_time sarà uguale a 0 (a condizione che l'altra variabile abbia un valore non negativo), perché MIN sceglierà la più piccola delle due opzioni.
- Entrambe le variabili non sono uguali a 0: scegliere il valore minimo.
Quando si sostituisce la condizione con write_time && change_time il comportamento apparirà corretto:
- Una o entrambe le variabili non sono uguali a 0: scegli un valore diverso da zero.
- Entrambe le variabili non sono uguali a 0: scegliere il valore minimo.
Frammento 2
L'espressione è sempre vera. Probabilmente qui dovrebbe essere utilizzato l'operatore '&&'. disco.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;
....
}A quanto pare anche qui gli operatori sono confusi || и &&O == и !=: Una variabile non può avere il valore 20 e 9 contemporaneamente.
Copia di righe illimitata
Una chiamata alla funzione 'sprintf' porterà all'overflow del buffer 'fullpath'. disco.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);
....
}Quando guardi la funzione per intero, diventerà chiaro che questo codice non causa problemi. Tuttavia, potrebbero verificarsi in futuro: una modifica negligente e otterremo un buffer overflow - sprintf non è limitato da nulla, quindi quando concatenamo i percorsi possiamo andare oltre i confini dell'array. Si consiglia di notare questa chiamata snprintf(percorso completo, PATH_MAX, ….).
Condizione ridondante
Una parte dell'espressione condizionale è sempre vera: add > 0. scard.c 507
static void
inRepos(STREAM in, unsigned int read)
{
SERVER_DWORD add = 4 - read % 4;
if (add < 4 && add > 0)
{
....
}
}Проверка aggiungi > 0 non ce n'è bisogno qui: la variabile sarà sempre maggiore di zero, perché leggi % 4 restituirà il resto della divisione, ma non sarà mai uguale a 4.
xrdp
— implementazione di un server RDP con codice open source. Il progetto è diviso in 2 parti:
- xrdp: implementazione del protocollo. Distribuito sotto la licenza Apache 2.0.
- xorgxrdp - Un set di driver Xorg da utilizzare con xrdp. Licenza - X11 (come MIT, ma ne vieta l'uso nella pubblicità)
Lo sviluppo del progetto si basa sui risultati di rdesktop e FreeRDP. Inizialmente, per lavorare con la grafica, dovevi utilizzare un server VNC separato o uno speciale server X11 con supporto RDP - X11rdp, ma con l'avvento di xorgxrdp la loro necessità è scomparsa.
In questo articolo non tratteremo xorgxrdp.
Il progetto xrdp, come il precedente, è molto piccolo e contiene circa 80mila righe.
Altri errori di battitura
Il codice contiene la raccolta di blocchi simili. Controlla gli elementi 'r', 'g', 'r' nelle righe 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++;
}
....
}
....
}Questo codice è stato preso dalla libreria librfxcodec, che implementa il codec jpeg2000 per RemoteFX. Qui, a quanto pare, i canali dei dati grafici sono confusi: invece del colore "blu", viene registrato il "rosso". Questo errore molto probabilmente è apparso come risultato del copia-incolla.
Lo stesso problema si è verificato in una funzione simile rfx_encode_format_argb, che l'analizzatore ci ha anche detto:
Il codice contiene la raccolta di blocchi simili. Controlla gli elementi 'a', 'r', 'g', 'r' nelle righe 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++;
}Dichiarazione di matrice
È possibile il sovraccarico dell'array. Il valore dell'indice 'i — 8' potrebbe raggiungere 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 dichiarazione e la definizione dell'array in questi due file sono incompatibili - la dimensione differisce di 1. Tuttavia, non si verificano errori - la dimensione corretta è specificata nel file evdev-map.c, quindi non ci sono limiti. Quindi questo è solo un bug che può essere facilmente risolto.
Confronto errato
Una parte dell'espressione condizionale è 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 funzione legge una variabile di tipo corto non firmato in una variabile come int. Il controllo non è necessario qui perché stiamo leggendo una variabile senza segno e assegnando il risultato a una variabile più grande, quindi la variabile non può assumere un valore negativo.
Controlli inutili
Una parte dell'espressione condizionale è sempre vera: (bpp != 16). libxrdp.c704
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;
}
....
}I controlli della disuguaglianza non hanno senso in questo caso poiché abbiamo già un confronto all'inizio. È probabile che si tratti di un errore di battitura e che lo sviluppatore abbia voluto utilizzare l'operatore || per filtrare gli argomenti non validi.
conclusione
Durante l'audit non sono stati individuati errori gravi, ma sono state riscontrate numerose carenze. Tuttavia, questi progetti sono utilizzati in molti sistemi, anche se di piccola portata. Un piccolo progetto non contiene necessariamente molti errori, quindi non dovresti giudicare le prestazioni dell'analizzatore solo su piccoli progetti. Puoi leggere di più a riguardo nell’articolo “«.
Puoi scaricare da noi una versione di prova di PVS-Studio .
Se desideri condividere questo articolo con un pubblico di lingua inglese, utilizza il link di traduzione: Sergey Larin.
Fonte: habr.com