To jest druga recenzja w serii artykułów dotyczących testowania otwartych programów do pracy z protokołem RDP. W tym artykule przyjrzymy się klientowi rdesktop oraz serwerowi xrdp.
Jako narzędzie do wykrywania błędów użyto . To statyczny analizator kodu dla języków C, C++, C# i Java, dostępny na platformach Windows, Linux i macOS.
W artykule przedstawione zostały tylko te błędy, które uznałem za interesujące. Chociaż projekty są niewielkie, błędów było niewiele :).
Uwaga. Poprzedni artykuł dotyczący testowania projektu FreeRDP można znaleźć .
rdesktop
— wolna implementacja klienta RDP dla systemów UNIX-based. Można go również używać pod systemem Windows, jeśli skompilujesz projekt pod Cygwin. Licencjonowany na GPLv3.
Ten klient cieszy się dużą popularnością — jest domyślnie używany w ReactOS, a także można znaleźć alternatywne graficzne front-endy. Niemniej jednak jest dość stary: pierwszy wydanie miało miejsce 4 kwietnia 2001 roku — w momencie pisania artykułu ma on 17 lat.
Jak już wcześniej wspomniałem, projekt jest naprawdę mały. Zawiera około 30 tysięcy linii kodu, co jest trochę dziwne, biorąc pod uwagę jego wiek. Dla porównania, FreeRDP zawiera 320 tysięcy linii. Oto wynik programu Cloc:
Nieosiągalny kod
Wykryty nieosiągalny kod. Może występować błąd. 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);
}Błąd napotykamy od razu w funkcji main: widzimy kod, który występuje po operatorze return — ten fragment zajmuje się czyszczeniem pamięci. Niemniej jednak błąd nie stanowi zagrożenia: cała przydzielona pamięć zostanie wyczyszczona przez system operacyjny po zakończeniu pracy programu.
Brak obsługi błędów
Możliwe wystąpienie underrun tablicy. Wartość indeksu 'n' może osiągnąć -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);
}
....
}Fragment kodu w tym przypadku czyta z pliku do bufora, aż plik się skończy. Jednak brak obsługi błędów oznacza, że jeśli coś pójdzie nie tak, to read zwróci -1, co spowoduje przekroczenie granic tablicy output.
Użycie EOF w typie char
EOF nie powinno być porównywane z wartością typu 'char'. '(c = fgetc(fp))' powinno być typu '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++;
}
....
}Tutaj widzimy niewłaściwe przetwarzanie osiągnięcia końca pliku: jeśli fgetc zwróci bajt, którego kod wynosi 0xFF, to zostanie on odczytany jako koniec pliku (EOF).
EOF to stała, zazwyczaj definiowana jako -1. Na przykład w kodowaniu CP1251 ostatnia litera alfabetu rosyjskiego ma kod 0xFF, co odpowiada liczbie -1, jeśli mówimy o zmiennej typu char. Wychodzi na to, że symbol 0xFF, podobnie jak EOF (-1) jest postrzegany jako koniec pliku. Aby uniknąć takich błędów, wynik działania funkcji fgetc należy przechowywać w zmiennej typu int.
Błędy pisowni
Fragment 1
Wyrażenie 'write_time' jest zawsze fałszywe. 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; // <=
....
}Możliwe, że autor tego kodu pomylił || i && w warunku. Rozważmy możliwe wartości write_time i change_time:
- Obie zmienne są równe 0: w tym przypadku wejdziemy w gałąź inaczej: zmienna mod_time zawsze będzie równa 0, niezależnie od następnego warunku.
- Jedna z zmiennych równa 0: mod_time będzie równa 0 (zakładając, że druga zmienna ma wartość nieujemną), ponieważ MIN wybierze najmniejsze z dwóch rozwiązań.
- Obie zmienne nie są równe 0: wybieramy minimalną wartość.
Przy zmianie warunku na write_time && change_time zachowanie będzie wyglądało poprawnie:
- Jedna lub obie zmienne są różne od 0: wybieramy wartość różną od zera.
- Obie zmienne nie są równe 0: wybieramy minimalną wartość.
Fragment 2
Wyrażenie jest zawsze prawdziwe. Prawdopodobnie operator '&&' powinien być użyty tutaj. 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;
....
}Wygląda na to, że tutaj również pomylono operatory || i &&, albo == i !=: zmienna nie może jednocześnie przyjmować wartości 20 i 9.
Nieograniczone kopiowanie ciągu
Wywołanie funkcji 'sprintf' doprowadzi do przepełnienia bufora 'fullpath'. disk.c 1257
RD_NTSTATUS
disk_query_directory(....)
{
....
char *dirname, fullpath[PATH_MAX];
....
/* Pobierz informacje o wpisie katalogu */
sprintf(fullpath, "%s/%s", dirname, pdirent->d_name);
....
}Rozważając funkcję w całości, stanie się jasne, że ten kod nie wywołuje problemów. Mogą one się jednak pojawić w przyszłości: jedna nieostrożna zmiana i możemy otrzymać przepełnienie bufora — sprintf nie ma żadnych ograniczeń, więc podczas łączenia ścieżek możemy przekroczyć granicę tablicy. Zaleca się zauważyć to wywołanie na snprintf(fullpath, PATH_MAX, ….).
Nadmierny warunek
Część wyrażenia warunkowego jest zawsze prawdziwa: dodaj > 0. scard.c 507
static void
inRepos(STREAM in, unsigned int read)
{
SERVER_DWORD add = 4 - read % 4;
if (add 0)
{
....
}
}Weryfikacja add > 0 tutaj nie ma sensu: zmienna zawsze będzie większa od zera, ponieważ read % 4 zwróci resztę z dzielenia, a ona nigdy nie będzie równa 4.
xrdp
— implementacja serwera RDP z otwartym kodem źródłowym. Projekt dzieli się na 2 części:
- xrdp — implementacja protokołu. Dystrybuowany na podstawie licencji Apache 2.0.
- xorgxrdp — zestaw sterowników Xorg do użycia z xrdp. Licencja — X11 (jak MIT, ale zabrania użycia w reklamie)
Rozwój projektu oparty jest na wynikach rdesktop i FreeRDP. Początkowo do pracy z grafiką konieczne było użycie osobnego serwera VNC lub specjalnego serwera X11 z obsługą RDP — X11rdp, jednak z pojawieniem się xorgxrdp potrzebna była już pomoc tych systemów.
W tym artykule nie będziemy dotykać xorgxrdp.
Projekt xrdp, tak jak poprzedni, jest stosunkowo mały i zawiera około 80 tysięcy linii.
Jeszcze błędy drukarskie
Kod zawiera zbiór podobnych bloków. Sprawdź elementy 'r', 'g', 'r' w liniach 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++;
}
....
}
....
}Ten kod pochodzi z biblioteki librfxcodec, realizującej kodek jpeg2000 do pracy z RemoteFX. Tutaj najwyraźniej pomylono kanały danych graficznych — zamiast koloru „niebieskiego” zapisywany jest „czerwony”. Taki błąd najprawdopodobniej powstał w wyniku skopiowania i wklejenia.
Ten sam problem wystąpił również w podobnej funkcji rfx_encode_format_argb, o czym również poinformował nas analizator:
Kod zawiera zbiór podobnych bloków. Sprawdź elementy 'a', 'r', 'g', 'r' w liniach 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++;
}Deklaracja tablicy
Przepełnienie tablicy jest możliwe. Wartość indeksu 'i - 8' może osiągnąć 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];
....
}
....
}Deklaracja i definicja tablicy w tych dwóch plikach są niezgodne — rozmiar różni się o 1. Jednak nie występują żadne błędy — w pliku evdev-map.c podany jest prawidłowy rozmiar, więc nie ma przekroczenia granic. Tak więc jest to tylko niedociągnięcie, które można łatwo naprawić.
Niepoprawne porównanie
Część wyrażenia warunkowego jest zawsze fałszywa: (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))
{
....
}
....
}W funkcji następuje odczyt zmiennej typu unsigned short do zmiennej typu intW tej części nie ma potrzeby sprawdzania, ponieważ odczytujemy zmienną typu unsigned i przypisujemy wynik większej zmiennej, więc zmienna nie może przyjąć wartości ujemnej.
Niepotrzebne sprawdzenia
Część wyrażenia warunkowego jest zawsze prawdziwa: (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: błąd");
return 1;
}
....
}Sprawdzanie nierówności nie ma sensu, ponieważ mamy już porównanie na początku. Możliwe, że to literówka i programista chciał użyć operatora || aby odfiltrować błędne argumenty.
Podsumowanie
Podczas sprawdzania nie stwierdzono poważnych błędów, ale znaleziono wiele niedociągnięć. Niemniej jednak projekty te są używane w wielu systemach, choć są małe pod względem skali. W małym projekcie niekoniecznie musi być wiele błędów, dlatego nie należy oceniać pracy analizatora tylko na podstawie małych projektów. Więcej o tym można przeczytać w artykule „«.
Możesz pobrać wersję próbną PVS-Studio u nas na .
Jeśli chcesz podzielić się tym artykułem z anglojęzyczną publicznością, proszę użyć linku do tłumaczenia: Sergey Larin.
Źródło: habr.com
