rdesktop

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 :).. 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:

Unreachable code

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); }

No error handling

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); } .... }

Using EOF in char

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 != '

') { result[index] = c; index++; } .... }

Typos

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; // <= .... }

Both variables have 0. In this case, execution moves on to the else branch: the mod_time variable will always be evaluated to 0 no matter what the next condition is.

branch: the variable will always be evaluated to 0 no matter what the next condition is. One of the variables has 0. In this case, mod_time will be assigned 0 (given that the other variable has a non-negative value) since MIN will choose the least of the two.

will be assigned 0 (given that the other variable has a non-negative value) since will choose the least of the two. Neither variable has 0: the minimum value is chosen.

Only one or neither variable has 0: the non-zero value is chosen.

Neither variable has 0: the minimum value is chosen.

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; .... }

Unlimited string copying

RD_NTSTATUS disk_query_directory(....) { .... char *dirname, fullpath[PATH_MAX]; .... /* Get information for directory entry */ sprintf(fullpath, "%s/%s", dirname, pdirent->d_name); .... }

Redundant condition

static void inRepos(STREAM in, unsigned int read) { SERVER_DWORD add = 4 - read % 4; if (add < 4 && add > 0) { .... } }

xrdp

xrdp — the protocol implementation. It is released under Apache 2.0.

xorgxrdp — a collection of Xorg drivers to be used with xrdp. It is released under X11 (just like MIT, but use in advertising is prohibited)

V779 Unreachable code detected. It is possible that an error is present. rdesktop.c 1502The first error is found immediately in thefunction: the code following thestatement 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 1872The 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,will return -1 and execution will start reading beyond the bounds of thearray. 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 500This code implements incorrecthandling: ifreturns a character whose code is 0xFF, it will be interpreted as the end of file ().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. It means that the 0xFF character, just like(-1), will be interpreted as the end of file. To avoid errors like that, the result returned by thefunction should be stored in a variable of type V547 Expression 'write_time' is always false. disk.c 805The author of this code must have accidentally used theoperator instead ofin the condition. Let's see what values the variablesandcan have:Changing that line towill fix the behavior: V547 Expression is always true. Probably the '&&' operator should be used here. disk.c 1419Again, it looks like the problem of using the wrong operator — eitherinstead oforinstead ofbecause 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 1257If 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 sinceis not limited in any way, so concatenating the paths could take execution beyond the array bounds. We recommend replacing this call with V560 A part of conditional expression is always true: add > 0. scard.c 507Thecheck doesn't make any difference as the variable will always be greater than zero becausereturns 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.

More typos

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++; } .... } .... }

while (x < 64) { *la_buf++ = a; *lr_buf++ = r; *lg_buf++ = g; *lb_buf++ = r; x++; }

Array declaration

// 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]; .... } .... }

Incorrect comparison

// 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)) { .... } .... }

Redundant checks

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; } .... }

Conclusion