This is the second post in our series of articles about the results of checking open-source software working with the RDP protocol. Today we are going to take a look at the rdesktop client and xrdp server.
The analysis was performed by PVS-Studio. This is a static analyzer for code written in C, C++, C#, and Java, and it runs on Windows, Linux, and macOS.
I will be discussing only those bugs that looked most interesting to me. On the other hand, since the projects are pretty small, there aren't many bugs in them anyway :).
Note. The previous article about the check of FreeRDP is available here.
rdesktop is a free RDP client for UNIX-based systems. It can also run on Windows if built under Cygwin. rdesktop is released under GPLv3.
This is a very popular client. It is used as a default client on ReactOS, and you can also find third-party graphical front-ends to go with it. The project is pretty old, though: it was released for the first time on April 4, 2001, and is 17 years old, as of this writing.
As I already said, the project is very small - about 30 KLOC, which is a bit strange considering its age. Compare that with FreeRDP with its 320 KLOC. Here's Cloc's output:
V779 Unreachable code detected. It is possible that an error is present. 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);
}The first error is found immediately in the main function: the code following the return statement was meant to free the memory allocated earlier. But this defect isn't dangerous because all previously allocated memory will be freed by the operating system once the program terminates.
V557 Array underrun is possible. The value of 'n' index could reach -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] = '\0'; // <=
str_handle_lines(output, &rest, linehandler, data);
}
....
}The file contents are read into the buffer until EOF is reached. At the same time, this code lacks an error handling mechanism, and if something goes wrong, read will return -1 and execution will start reading beyond the bounds of the output array.
V739 EOF should not be compared with a value of the 'char' type. The '(c = fgetc(fp))' should be of the 'int' type. 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++;
}
....
}This code implements incorrect EOF handling: if fgetc returns a character whose code is 0xFF, it will be interpreted as the end of file (EOF).
EOF is a constant typically defined as -1. For example, in the CP1251 encoding, the last letter of the Russian alphabet is encoded as 0xFF, which corresponds to the number -1 in type char. It means that the 0xFF character, just like EOF (-1), will be interpreted as the end of file. To avoid errors like that, the result returned by the fgetc function should be stored in a variable of type int.
Snippet 1
V547 Expression 'write_time' is always false. 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; // <=
....
}The author of this code must have accidentally used the || operator instead of && in the condition. Let's see what values the variables write_time and change_time can have:
Changing that line to write_time && change_time will fix the behavior:
Snippet 2
V547 Expression is always true. Probably the '&&' operator should be used here. 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;
....
}Again, it looks like the problem of using the wrong operator - either || instead of && or == instead of != because the variable can't store the values 20 and 9 at the same time.
V512 A call of the 'sprintf' function will lead to overflow of the buffer 'fullpath'. disk.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);
....
}If you could follow the function to the end, you'd see that the code is OK, but it may get broken one day: just one careless change will end up with a buffer overflow since sprintf is not limited in any way, so concatenating the paths could take execution beyond the array bounds. We recommend replacing this call with snprintf(fullpath, PATH_MAX, ....).
V560 A part of conditional expression is always true: add > 0. scard.c 507
static void
inRepos(STREAM in, unsigned int read)
{
SERVER_DWORD add = 4 - read % 4;
if (add < 4 && add > 0)
{
....
}
}The add > 0 check doesn't make any difference as the variable will always be greater than zero because read % 4 returns the remainder, which will never be equal to 4.
xrdp is an open-source RDP server. The project consists of two parts:
The development is based on rdesktop and FreeRDP. Originally, in order to be able to work with graphics, you would have to use a separate VNC server or a special X11 server with RDP support, X11rdp, but those became unnecessary with the release of xorgxrdp.
We won't be talking about xorgxrdp in this article.
Just like the previous project, xrdp is a tiny one, consisting of about 80 KLOC.
V525 The code contains the collection of similar blocks. Check items 'r', 'g', 'r' in lines 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++;
}
....
}
....
}This code comes from the librfxcodec library, which implements the jpeg2000 codec to work with RemoteFX. The "red" color channel is read twice, while the "blue" channel is not read at all. Defects like this typically result from the use of copy-paste.
The same bug was found in the similar function rfx_encode_format_argb:
V525 The code contains the collection of similar blocks. Check items 'a', 'r', 'g', 'r' in lines 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++;
}V557 Array overrun is possible. The value of 'i - 8' index could reach 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];
....
}
....
}In the genkeymap.c file, the array is declared 1 element shorter than implied by the implementation. No bug will occur, though, because the evdev-map.c file stores the correct size, so there'll be no array overrun, which makes it a minor defect rather than a true error.
V560 A part of conditional expression is always false: (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))
{
....
}
....
}The value of a variable of type unsigned short is read into a variable of type int and then checked for being negative, which is not necessary because a value read from an unsigned type into a larger type can never become negative.
V560 A part of conditional expression is always true: (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;
}
....
}The not-equal checks aren't necessary because the first check does the job. The programmer was probably going to use the || operator to filter off incorrect arguments.
Today's check didn't reveal any critical bugs, but it did reveal a bunch of minor defects. That said, these projects, tiny as they are, are still used in many systems and, therefore, need some polishing. A small project shouldn't necessarily have tons of bugs in it, so testing the analyzer only on small projects isn't enough to reliably evaluate its effectiveness. This subject is discussed in more detail in the article "Feelings confirmed by numbers".
The demo version of PVS-Studio is available on our website.