Questo è il secondo articolo di una serie dedicata all'analisi di programmi open-source per il protocollo RDP. In questo articolo esamineremo il client rdesktop e il server xrdp.
Come strumento per l'individuazione degli errori è stato utilizzato . Si tratta di un analizzatore statico del codice per i linguaggi C, C++, C# e Java, disponibile su Windows, Linux e macOS.
In questo articolo sono presentati solo gli errori che mi sono sembrati interessanti. Tuttavia, i progetti sono piccoli, quindi ci sono stati pochi errori :).
Nota. Il precedente articolo sull'analisi del progetto FreeRDP può essere trovato .
rdesktop
— implementazione open-source del client RDP per sistemi UNIX-based. Può essere utilizzato anche su Windows, se il progetto viene compilato per Cygwin. Licenziato sotto GPLv3.
Questo client è molto popolare — è utilizzato di default in ReactOS e ci sono anche interfacce grafiche front-end di terze parti disponibili. Tuttavia, è piuttosto vecchio: il primo rilascio risale al 4 aprile 2001 — al momento della scrittura di quest’articolo ha 17 anni.
Come ho già menzionato, il progetto è davvero piccolo. Contiene circa 30.000 righe di codice, il che è un po' strano considerando la sua età. A titolo di confronto, FreeRDP ne contiene 320.000. Ecco l'output del programma Cloc:
Codice irraggiungibile
Codice inaccessibile rilevato. È possibile che ci sia 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 verifica immediatamente nella funzione main: vediamo il codice che segue l'operatore return — questo frammento esegue la pulizia della memoria. Tuttavia, l'errore non rappresenta una minaccia: tutta la memoria allocata sarà liberata dal sistema operativo dopo la chiusura del programma.
Mancanza di gestione degli errori
È possibile un underflow dell'array. Il valore dell'indice ‘n’ potrebbe arrivare 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);
}
....
}Il frammento di codice in questo caso legge da un file in un buffer fino a quando il file non è finito. Tuttavia, la gestione degli errori qui è assente: se qualcosa va storto, read ritornerà -1, e si verificherà un'uscita oltre i limiti dell'array output.
Uso di EOF nel tipo char
EOF non dovrebbe essere confrontato con un valore del tipo 'char'. Il '(c = fgetc(fp))' dovrebbe essere di 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 incorretta del raggiungimento della fine del file: se fgetc restituisce un carattere il cui codice è 0xFF, verrà interpretato come fine del file (EOF).
EOF questa è una costante di solito definita come -1. Ad esempio, nella codifica CP1251, l'ultima lettera dell'alfabeto russo ha il codice 0xFF, il quale corrisponde al numero -1 se parliamo di una variabile di tipo char. Ne consegue che il carattere 0xFF, come anche EOF (-1) è percepito come fine del file. Per evitare tali errori, il risultato del lavoro della funzione fgetc dovrebbe essere conservato in una variabile di tipo -int.
Refusi
Fragmento 1
L'espressione ‘write_time’ è sempre falsa. 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; // <=
....
}Forse l'autore di questo codice ha confuso || e && nelle condizioni. Consideriamo le possibili varianti dei valori write_time e change_time:
- Entrambe le variabili sono uguali a 0: in questo caso entreremo nel ramo else: la variabile mod_time sarà sempre uguale a 0 indipendentemente dalla condizione successiva.
- Una delle variabili è uguale a 0: mod_time sarà uguale a 0 (a condizione che l'altra variabile abbia un valore non negativo), poiché MIN sceglierà il minore dei due valori.
- Entrambe le variabili non sono uguali a 0: scegliamo il valore minimo.
Sostituendo la condizione con write_time && change_time il comportamento apparirà corretto:
- Una o entrambe le variabili non sono uguali a 0: scegliamo un valore diverso da zero.
- Entrambe le variabili non sono uguali a 0: scegliamo il valore minimo.
Fragmento 2
L'espressione è sempre vera. Probabilmente qui dovrebbe essere usato l'operatore ‘&&’. 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;
....
}Probabilmente anche qui sono stati confusi gli operatori || e &&, oppure == e !=: la variabile non può assumere contemporaneamente il valore 20 e 9.
Copia della stringa illimitata
Una chiamata alla funzione ‘sprintf’ porterà a un overflow del buffer ‘fullpath’. disk.c 1257
RD_NTSTATUS
disk_query_directory(....)
{
....
char *dirname, fullpath[PATH_MAX];
....
/* Ottieni informazioni per l'elemento della directory */
sprintf(fullpath, "%s/%s", dirname, pdirent->d_name);
....
}Considerando la funzione nel suo insieme, diventa chiaro che questo codice non causa problemi. Tuttavia, potrebbero sorgere in futuro: una modifica imprudente e otterremo un overflow del buffer — sprintf non ha alcuna limitazione, quindi nella concatenazione dei percorsi possiamo superare i confini dell'array. Si consiglia di notare questa chiamata in snprintf(fullpath, PATH_MAX, ….).
Condizione eccessiva
Una parte dell'espressione condizionale è sempre vera: aggiungi > 0. scard.c 507
static void
inRepos(STREAM in, unsigned int read)
{
SERVER_DWORD add = 4 - read % 4;
if (add 0)
{
....
}
}Verifica add > 0 qui è inutile: la variabile sarà sempre maggiore di zero, poiché read % 4 restituirà il resto della divisione, e non sarà mai uguale a 4.
xrdp
— implementazione di un server RDP open source. Il progetto è diviso in 2 parti:
- xrdp — implementazione del protocollo. Distribuito sotto la licenza Apache 2.0.
- xorgxrdp — un insieme di driver Xorg da utilizzare con xrdp. Licenza — X11 (simile a MIT, ma vieta l'uso in pubblicità)
Lo sviluppo del progetto si basa sui risultati di rdesktop e FreeRDP. Inizialmente, per lavorare con la grafica era necessario utilizzare un server VNC separato, oppure un server X11 speciale con supporto RDP — X11rdp, ma con l'arrivo di xorgxrdp non c'è più bisogno di questi.
In questo articolo non tratteremo xorgxrdp.
Il progetto xrdp, come il precedente, è piuttosto piccolo e contiene circa 80.000 righe.
Ancora errori di battitura
Il codice contiene una 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 lavorare con RemoteFX. Qui, sembra che i canali dei dati grafici siano stati confusi: al posto del colore "blu" viene scritto "rosso". Un errore di questo tipo è probabilmente avvenuto a causa di un'operazione di copia-incolla.
Anche questo stesso problema è presente in una funzione simile rfx_encode_format_argb, come ci ha anche segnalato l'analizzatore:
Il codice contiene una 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 dell'array
È possibile un 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 non sono compatibili: la dimensione differisce di 1. Tuttavia, non si verificano errori: nel file evdev-map.c è specificata la dimensione corretta, quindi non ci sono eccedenze. Si tratta semplicemente di una svista facilmente risolvibile.
Confronto non corretto
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))
{
....
}
....
}Nella funzione viene letta una variabile di tipo unsigned short in una variabile di tipo -int. Non è necessaria qui alcuna verifica, poiché leggiamo una variabile senza segno e assegniamo il risultato a una variabile di dimensioni maggiori; quindi la variabile non può assumere un valore negativo.
Controlli superflui
Una parte dell'espressione condizionale è sempre vera: (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: errore");
return 1;
}
....
}I confronti di disuguaglianza qui non hanno senso, poiché abbiamo già un confronto all'inizio. È probabile che si tratti di un errore di battitura e che lo sviluppatore volesse usare l'operatore || per filtrare gli argomenti non validi.
Conclusione
Durante la verifica non sono emersi errori gravi, ma sono stati riscontrati molti difetti. Tuttavia, questi progetti vengono utilizzati in molti sistemi, anche se sono di piccole dimensioni. In un progetto ridotto non è necessario che ci siano molti errori, quindi non bisognerebbe giudicare l'efficacia dell'analizzatore solo sulla base di piccoli progetti. Maggiori informazioni possono essere trovate nell'articolo ««.
Puoi scaricare la versione di prova di PVS-Studio da noi su .
Se desideri condividere questo articolo con un pubblico anglofono, ti prego di utilizzare il link alla traduzione: Sergey Larin.
Fonte: habr.com
