Sergey Larin

Apr 12 2019

Tags:

#Cpp

Checking rdesktop and xrdp with PVS-Studio

Apr 12 2019

Author: Sergey Larin

rdesktop
xrdp
Conclusion

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

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

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.

No error handling

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.

Using EOF in char

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.

Typos

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:

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.
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.
Neither variable has 0: the minimum value is chosen.

Changing that line to write_time && change_time will fix the behavior:

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

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.

Unlimited string copying

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

Redundant condition

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

xrdp is an open-source RDP server. The project consists of two parts:

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)

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

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

Array declaration

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.

Incorrect comparison

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.

Redundant checks

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.

Conclusion

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.

#Cpp