This is the second review in a series of articles on checking open programs for working with the RDP protocol. In it, we will look at the rdesktop client and the xrdp server.
used as a tool for error detection. . It is a static code analyzer for C, C++, C# and Java languages, available on Windows, Linux and macOS platforms.
The article presents only those errors that seemed interesting to me. However, the projects are small, so there were few mistakes :).
Note. A previous article on checking the FreeRDP project can be found .
rdesktop
is a free implementation of the RDP client for UNIX-based systems. It can also be used under Windows if you build the project under Cygwin. Licensed under GPLv3.
This client is very popular - it is used by default in ReactOS, and you can also find third-party graphical front-ends for it. However, it is quite old: the first release took place on April 4, 2001 - at the time of writing, his age is 17 years.
As I mentioned earlier, the project is quite small. It contains approximately 30 thousand lines of code, which is a bit strange considering its age. For comparison, FreeRDP contains 320 thousand lines. Here is the output of the Cloc program:
Unreachable code
Unavailable 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 error meets us immediately in the function main: we see the code that comes after the operator return - this fragment cleans up memory. However, the error is not a threat: all allocated memory will be cleaned up by the operating system after the program terminates.
No error handling
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] = ' '; // <=
str_handle_lines(output, &rest, linehandler, data);
}
....
}The code fragment in this case reads from the file into the buffer until the file ends. However, there is no error handling here: if something goes wrong, then read will return -1, and then the array will be out of bounds output.
Using EOF on a char type
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++;
}
....
}Here we see incorrect end-of-file handling: if fgetc will return a character whose code is 0xFF, then it will be perceived as the end of the file (EOF).
EOF it is a constant, usually defined as -1. For example, in the CP1251 encoding, the last letter of the Russian alphabet has the code 0xFF, which corresponds to the number -1, if we are talking about a variable like tank. It turns out that the symbol 0xFF, like EOF (-1) is taken as the end of the file. To avoid such errors, the result of the function fgetc should be stored in a variable like int.
Typos
Fragment 1
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; // <=
....
}Perhaps the author of this code mixed up || и && in condition. Consider possible options for values write_time и change_time:
- Both variables are 0: in this case, we will get to the branch else: variable mod_time will always be 0 regardless of the subsequent condition.
- One of the variables is 0: mod_time will be equal to 0 (provided that the other variable has a non-negative value), since MIN will choose the smallest of the two options.
- Both variables are not equal to 0: choose the minimum value.
When replacing the condition with write_time && change_time the behavior will look correct:
- One or both variables are not equal to 0: choose a non-zero value.
- Both variables are not equal to 0: choose the minimum value.
Fragment 2
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;
....
}Apparently, the operators are also confused here || и &&or == и !=: Variable cannot be both 20 and 9 at the same time.
Unlimited line copy
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);
....
}By examining the function in its entirety, it will become clear that this code does not cause problems. However, they may arise in the future: one careless change and we will get a buffer overflow − sprintf is not limited by anything, so when concatenating paths, we can go beyond the boundaries of the array. It is recommended to notice this call on snprintf(fullpath, PATH_MAX, ....).
Redundant condition
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)
{
....
}
}inspection add > 0 there is no point here: the variable will always be greater than zero, because read %4 will return the remainder of the division, and it will never be equal to 4.
xrdp
is an open source RDP server implementation. The project is divided into 2 parts:
- xrdp is the implementation of the protocol. Distributed under the Apache 2.0 license.
- xorgxrdp is a set of Xorg drivers for use with xrdp. License - X11 (like MIT, but prohibits use in advertising)
The development of the project is based on the results of rdesktop and FreeRDP. Initially, to work with graphics, you had to use a separate VNC server, or a special X11 server with RDP support - X11rdp, but with the advent of xorgxrdp, the need for them disappeared.
In this article, we will not touch on xorgxrdp.
The xrdp project, like the previous one, is very small and contains about 80 thousand lines.
More typos
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 was taken from the librfxcodec library, which implements the jpeg2000 codec for RemoteFX. Here, apparently, the channels of graphic data are mixed up - instead of "blue" color, "red" is recorded. Such an error most likely appeared as a result of copy-paste.
The same problem got into a similar function rfx_encode_format_argb, which was also reported to us by the analyzer:
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++;
}Array declaration
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];
....
}
....
}The array declaration and definition in these two files is incompatible - the size differs by 1. However, no errors occur - the evdev-map.c file has the correct size, so there is no overflow. So it's just a bug that's easy to fix.
Incorrect comparison
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 function reads a variable of type unsigned shorts into a type variable int. The check is not needed here, because we are reading an unsigned type variable and assigning the result to a variable of a larger size, so the variable cannot take a negative value.
Unnecessary Checks
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;
}
....
}Inequality tests don't make sense here, because we already have a comparison at the beginning. It is likely that this is a typo and the developer wanted to use the operator || to filter out invalid arguments.
Conclusion
During the audit, no serious errors were identified, but many shortcomings were found. Nevertheless, these projects are used in many systems, albeit small in scope. A small project doesn't have to have a lot of bugs, so you shouldn't judge the analyzer performance only on small projects. You can read more about this in the article "«.
You can download a trial version of PVS-Studio here at .
If you want to share this article with an English-speaking audience, please use the translation link: Sergey Larin.
Source: habr.com