From 7ef01f7b0cec6e75631a22906d67d8ff44cb1a8c Mon Sep 17 00:00:00 2001
From: matt335672 <30179339+matt335672@users.noreply.github.com>
Date: Sat, 18 Apr 2020 16:56:53 +0100
Subject: [PATCH] Address memory allocation overflow security issues

---
 sesman/chansrv/chansrv.c        | 12 ++++++++-
 sesman/chansrv/smartcard_pcsc.c | 22 ++++++++++++++---
 xrdp/xrdp_bitmap.c              | 44 +++++++++++++++++++++++++++++++--
 3 files changed, 71 insertions(+), 7 deletions(-)

diff --git a/sesman/chansrv/chansrv.c b/sesman/chansrv/chansrv.c
index 6b1b9326..d21d5b70 100644
--- a/sesman/chansrv/chansrv.c
+++ b/sesman/chansrv/chansrv.c
@@ -39,6 +39,9 @@
 #include "xrdp_sockets.h"
 #include "audin.h"
 
+#define CHANNEL_NAME_BYTES 7
+#define MAX_PATH 260
+
 static struct trans *g_lis_trans = 0;
 static struct trans *g_con_trans = 0;
 static struct trans *g_api_lis_trans = 0;
@@ -1042,7 +1045,7 @@ my_api_trans_data_in(struct trans *trans)
     int rv;
     int bytes;
     int ver;
-    int channel_name_bytes;
+    unsigned int channel_name_bytes;
     struct chansrv_drdynvc_procs procs;
     char *chan_name;
 
@@ -1067,6 +1070,13 @@ my_api_trans_data_in(struct trans *trans)
         rv = 1;
         in_uint32_le(s, channel_name_bytes);
         //g_writeln("my_api_trans_data_in: channel_name_bytes %d", channel_name_bytes);
+        /*
+         * Name is limited to CHANNEL_NAME_BYTES for an SVC, or MAX_PATH
+         * bytes for a DVC */
+        if (channel_name_bytes > MAX(CHANNEL_NAME_BYTES, MAX_PATH))
+        {
+            return 1;
+        }
         chan_name = g_new0(char, channel_name_bytes + 1);
         if (chan_name == NULL)
         {
diff --git a/sesman/chansrv/smartcard_pcsc.c b/sesman/chansrv/smartcard_pcsc.c
index 51409567..5f4516db 100644
--- a/sesman/chansrv/smartcard_pcsc.c
+++ b/sesman/chansrv/smartcard_pcsc.c
@@ -586,9 +586,18 @@ int
 scard_process_list_readers(struct trans *con, struct stream *in_s)
 {
     int hContext;
-    int bytes_groups;
+    unsigned int bytes_groups;
     int cchReaders;
-    char *groups;
+    /*
+     * At the time of writing, the groups strings which can be sent
+     * over this interface are all small:-
+     *
+     * "SCard$AllReaders", "SCard$DefaultReaders", "SCard$LocalReaders" and
+     * "SCard$SystemReaders"
+     *
+     * We'll allow a bit extra in case the interface changes
+     */
+    char groups[256];
     struct pcsc_uds_client *uds_client;
     struct pcsc_context *lcontext;
     struct pcsc_list_readers *pcscListReaders;
@@ -597,8 +606,14 @@ scard_process_list_readers(struct trans *con, struct stream *in_s)
     uds_client = (struct pcsc_uds_client *) (con->callback_data);
     in_uint32_le(in_s, hContext);
     in_uint32_le(in_s, bytes_groups);
-    groups = (char *) g_malloc(bytes_groups + 1, 1);
+    if (bytes_groups > (sizeof(groups) - 1))
+    {
+        LLOGLN(0, ("scard_process_list_readers: Unreasonable string length %u",
+                   bytes_groups));
+        return 1;
+    }
     in_uint8a(in_s, groups, bytes_groups);
+    groups[bytes_groups] = '\0';
     in_uint32_le(in_s, cchReaders);
     LLOGLN(10, ("scard_process_list_readers: hContext 0x%8.8x cchReaders %d",
            hContext, cchReaders));
@@ -615,7 +630,6 @@ scard_process_list_readers(struct trans *con, struct stream *in_s)
     pcscListReaders->cchReaders = cchReaders;
     scard_send_list_readers(pcscListReaders, lcontext->context,
                             lcontext->context_bytes, groups, cchReaders, 1);
-    g_free(groups);
     return 0;
 }
 
diff --git a/xrdp/xrdp_bitmap.c b/xrdp/xrdp_bitmap.c
index 1f5d010f..627f73c7 100644
--- a/xrdp/xrdp_bitmap.c
+++ b/xrdp/xrdp_bitmap.c
@@ -25,6 +25,8 @@
 #include <config_ac.h>
 #endif
 
+#include <limits.h>
+
 #include "xrdp.h"
 #include "log.h"
 
@@ -93,6 +95,30 @@ static const unsigned int g_crc_table[256] =
     (in_crc) = g_crc_table[((in_crc) ^ (in_pixel)) & 0xff] ^ ((in_crc) >> 8)
 #define CRC_END(in_crc) (in_crc) = ((in_crc) ^ 0xFFFFFFFF)
 
+/*****************************************************************************/
+/* Allocate bitmap for specified dimensions, checking for int overflow */
+static char *
+alloc_bitmap_data(int width, int height, int Bpp)
+{
+    char *result = NULL;
+    if (width > 0 && height > 0 && Bpp > 0)
+    {
+        int len = width;
+        /* g_malloc() currently takes an 'int' size */
+        if (len < INT_MAX / height)
+        {
+            len *= height;
+            if (len < INT_MAX / Bpp)
+            {
+                len *= Bpp;
+                result = (char *)malloc(len);
+            }
+        }
+    }
+
+    return result;
+}
+
 /*****************************************************************************/
 struct xrdp_bitmap *
 xrdp_bitmap_create(int width, int height, int bpp,
@@ -123,14 +149,28 @@ xrdp_bitmap_create(int width, int height, int bpp,
 
     if (self->type == WND_TYPE_BITMAP || self->type == WND_TYPE_IMAGE)
     {
-        self->data = (char *)g_malloc(width * height * Bpp, 0);
+        self->data = alloc_bitmap_data(width, height, Bpp);
+        if (self->data == NULL)
+        {
+            LLOGLN(0, ("xrdp_bitmap_create: size overflow %dx%dx%d",
+                       width, height, Bpp));
+            g_free(self);
+            return NULL;
+        }
     }
 
 #if defined(XRDP_PAINTER)
     if (self->type == WND_TYPE_SCREEN) /* noorders */
     {
         LLOGLN(0, ("xrdp_bitmap_create: noorders"));
-        self->data = (char *) g_malloc(width * height * Bpp, 0);
+        self->data = alloc_bitmap_data(width, height, Bpp);
+        if (self->data == NULL)
+        {
+            LLOGLN(0, ("xrdp_bitmap_create: size overflow %dx%dx%d",
+                       width, height, Bpp));
+            g_free(self);
+            return NULL;
+        }
     }
 #endif