CVE-2023-42822

- font_items in struct xrdp_font renamed to chars to catch all
  accesses to it. This name is consistent with the type of
  the array elements (struct xrdp_font_char).
- Additional fields added to struct xrdp_font to allow for range
  checking and for a default character to be provided
- Additional checks and logic added to xrdp_font_create()
- New macro XRDP_FONT_GET_CHAR() added to perform checked access
  to chars field in struct xrdp_font

xrdp/xrdp.h

@@ -381,6 +381,15 @@ xrdp_font_delete(struct xrdp_font *self);
 int
 xrdp_font_item_compare(struct xrdp_font_char *font1,
                        struct xrdp_font_char *font2);
+/**
+ * Gets a checked xrdp_font_char from a font
+ * @param f Font
+ * @param c32 Unicode codepoint
+ */
+#define XRDP_FONT_GET_CHAR(f, c32) \
+    (((unsigned int)(c32) >= ' ') && ((unsigned int)(c32) < (f)->char_count) \
+     ? ((f)->chars + (unsigned int)(c32)) \
+     : (f)->default_char)
 
 /* funcs.c */
 int

xrdp/xrdp_font.c

@@ -52,6 +52,12 @@ static char w_char[] =
 };
 #endif
 
+// Unicode definitions
+#define UNICODE_WHITE_SQUARE 0x25a1
+
+// First character allocated in the 'struct xrdp_font.chars' array
+#define FIRST_CHAR ' '
+
 /*****************************************************************************/
 /**
  * Parses the fv1_select configuration value to get the font to use,
@@ -145,8 +151,8 @@ xrdp_font_create(struct xrdp_wm *wm, unsigned int dpi)
     int fd;
     int b;
     int i;
-    int index;
+    unsigned int char_count;
-    int datasize;
+    unsigned int datasize; // Size of glyph data on disk
     int file_size;
     struct xrdp_font_char *f;
     const char *file_path;
@@ -207,17 +213,39 @@ xrdp_font_create(struct xrdp_wm *wm, unsigned int dpi)
     }
 
     self = (struct xrdp_font *)g_malloc(sizeof(struct xrdp_font), 1);
+    if (self == NULL)
+    {
+        LOG(LOG_LEVEL_ERROR, "xrdp_font_create: "
+            "Can't allocate memory for font");
+        return self;
+    }
     self->wm = wm;
     make_stream(s);
     init_stream(s, file_size + 1024);
     fd = g_file_open_ro(file_path);
 
-    if (fd != -1)
+    if (fd < 0)
+    {
+        LOG(LOG_LEVEL_ERROR,
+            "xrdp_font_create: Can't open %s - %s", file_path,
+            g_get_strerror());
+        g_free(self);
+        self = NULL;
+    }
+    else
     {
         b = g_file_read(fd, s->data, file_size + 1024);
         g_file_close(fd);
 
-        if (b > 0)
+        // Got at least a header?
+        if (b < (4 + 32 + 2 + 2 + 2 + 2 + 4))
+        {
+            LOG(LOG_LEVEL_ERROR,
+                "xrdp_font_create: Font %s is truncated", file_path);
+            g_free(self);
+            self = NULL;
+        }
+        else
         {
             s->end = s->data + b;
             in_uint8s(s, 4);
@@ -227,11 +255,27 @@ xrdp_font_create(struct xrdp_wm *wm, unsigned int dpi)
             in_uint16_le(s, self->body_height);
             in_sint16_le(s, min_descender);
             in_uint8s(s, 4);
-            index = 32;
+            char_count = FIRST_CHAR;
 
-            while (s_check_rem(s, 16))
+            while (!s_check_end(s))
             {
-                f = self->font_items + index;
+                if (!s_check_rem(s, 16))
+                {
+                    LOG(LOG_LEVEL_WARNING,
+                        "xrdp_font_create: "
+                        "Can't parse header for character U+%X", char_count);
+                    break;
+                }
+
+                if (char_count >= MAX_FONT_CHARS)
+                {
+                    LOG(LOG_LEVEL_WARNING,
+                        "xrdp_font_create: "
+                        "Ignoring characters >= U+%x", MAX_FONT_CHARS);
+                    break;
+                }
+
+                f = self->chars + char_count;
                 in_sint16_le(s, i);
                 f->width = i;
                 in_sint16_le(s, i);
@@ -249,9 +293,19 @@ xrdp_font_create(struct xrdp_wm *wm, unsigned int dpi)
                 if (datasize < 0 || datasize > 512)
                 {
                     /* shouldn't happen */
-                    LOG(LOG_LEVEL_ERROR, "error in xrdp_font_create, datasize wrong "
+                    LOG(LOG_LEVEL_ERROR,
-                        "width %d, height %d, datasize %d, index %d",
+                        "xrdp_font_create: "
-                        f->width, f->height, datasize, index);
+                        "datasize for U+%x wrong "
+                        "width %d, height %d, datasize %d",
+                        char_count, f->width, f->height, datasize);
+                    break;
+                }
+
+                if (!s_check_rem(s, datasize))
+                {
+                    LOG(LOG_LEVEL_ERROR,
+                        "xrdp_font_create: "
+                        "Not enough data for character U+%X", char_count);
                     break;
                 }
 
@@ -261,25 +315,57 @@ xrdp_font_create(struct xrdp_wm *wm, unsigned int dpi)
                      * that it can be added to the glyph cache if required */
                     f->width = 1;
                     f->height = 1;
+
+                    /* GOTCHA - we need to allocate more than one byte in
+                     * memory for this glyph */
                     f->data = (char *)g_malloc(FONT_DATASIZE(f), 1);
                 }
-                else if (s_check_rem(s, datasize))
-                {
-                    f->data = (char *)g_malloc(datasize, 0);
-                    in_uint8a(s, f->data, datasize);
-                }
                 else
                 {
-                    LOG(LOG_LEVEL_ERROR, "error in xrdp_font_create");
+                    f->data = (char *)g_malloc(datasize, 0);
-                }
-                index++;
                 }
 
-            if (self->body_height == 0 && index > 32)
+                if (f->data == NULL)
+                {
+                    LOG(LOG_LEVEL_ERROR,
+                        "xrdp_font_create: "
+                        "Allocation error for character U+%X", char_count);
+                    break;
+                }
+                in_uint8a(s, f->data, datasize);
+
+                ++char_count;
+            }
+
+            self->char_count = char_count;
+            if (char_count <= FIRST_CHAR)
+            {
+                /* We read no characters from the font */
+                xrdp_font_delete(self);
+                self = NULL;
+            }
+            else
+            {
+                if (self->body_height == 0)
                 {
                     /* Older font made for xrdp v0.9.x. Synthesise this
                      * value from the first glyph */
-                self->body_height = -self->font_items[32].baseline + 1;
+                    self->body_height = -self->chars[FIRST_CHAR].baseline + 1;
+                }
+
+                // Find a default glyph
+                if (char_count > UNICODE_WHITE_SQUARE)
+                {
+                    self->default_char = &self->chars[UNICODE_WHITE_SQUARE];
+                }
+                else if (char_count > '?')
+                {
+                    self->default_char = &self->chars['?'];
+                }
+                else
+                {
+                    self->default_char = &self->chars[FIRST_CHAR];
+                }
             }
         }
     }
@@ -302,16 +388,16 @@ xrdp_font_create(struct xrdp_wm *wm, unsigned int dpi)
 void
 xrdp_font_delete(struct xrdp_font *self)
 {
-    int i;
+    unsigned int i;
 
     if (self == 0)
     {
         return;
     }
 
-    for (i = 0; i < NUM_FONTS; i++)
+    for (i = FIRST_CHAR; i < self->char_count; i++)
     {
-        g_free(self->font_items[i].data);
+        g_free(self->chars[i].data);
     }
 
     g_free(self);

xrdp/xrdp_painter.c

@@ -454,7 +454,7 @@ xrdp_painter_text_width(struct xrdp_painter *self, const char *text)
 
     for (index = 0; index < len; index++)
     {
-        font_item = self->font->font_items + wstr[index];
+        font_item = XRDP_FONT_GET_CHAR(self->font, wstr[index]);
         rv = rv + font_item->incby;
     }
 
@@ -837,7 +837,7 @@ xrdp_painter_draw_text(struct xrdp_painter *self,
             total_height = 0;
             for (index = 0; index < len; index++)
             {
-                font_item = font->font_items + wstr[index];
+                font_item = XRDP_FONT_GET_CHAR(font, wstr[index]);
                 k = font_item->incby;
                 total_width += k;
                 /* Use the nominal height of the font to work out the
@@ -875,7 +875,7 @@ xrdp_painter_draw_text(struct xrdp_painter *self,
                                      draw_rect.bottom - draw_rect.top);
                     for (index = 0; index < len; index++)
                     {
-                        font_item = font->font_items + wstr[index];
+                        font_item = XRDP_FONT_GET_CHAR(font, wstr[index]);
                         g_memset(&pat, 0, sizeof(pat));
                         pat.format = PT_FORMAT_c1;
                         pat.width = font_item->width;
@@ -917,7 +917,7 @@ xrdp_painter_draw_text(struct xrdp_painter *self,
 
     for (index = 0; index < len; index++)
     {
-        font_item = font->font_items + wstr[index];
+        font_item = XRDP_FONT_GET_CHAR(font, wstr[index]);
         i = xrdp_cache_add_char(self->wm->cache, font_item);
         f = HIWORD(i);
         c = LOWORD(i);

xrdp/xrdp_types.h

@@ -648,7 +648,7 @@ struct xrdp_bitmap
     int crc16;
 };
 
-#define NUM_FONTS 0x4e00
+#define MAX_FONT_CHARS 0x4e00
 #define DEFAULT_FONT_NAME "sans-10.fv1"
 #define DEFAULT_FONT_PIXEL_SIZE 16
 #define DEFAULT_FV1_SELECT "130:sans-18.fv1,0:" DEFAULT_FONT_NAME
@@ -669,7 +669,11 @@ struct xrdp_bitmap
 struct xrdp_font
 {
     struct xrdp_wm *wm;
-    struct xrdp_font_char font_items[NUM_FONTS];
+    // Font characters, accessed by Unicode codepoint. The first 32
+    // entries are unused.
+    struct xrdp_font_char chars[MAX_FONT_CHARS];
+    unsigned int char_count; // # elements in above array
+    struct xrdp_font_char *default_char; // Pointer into above array
     char name[32];
     int size;
     /** Body height in pixels */