Security fixes from X.Org Advisory:

X Font Service Protocol & Font metadata file handling issues in libXfont
May 13, 2014

- CVE-2014-0209: integer overflow of allocations in font metadata file parsing

    When a local user who is already authenticated to the X server adds
    a new directory to the font path, the X server calls libXfont to open
    the fonts.dir and fonts.alias files in that directory and add entries
    to the font tables for every line in it.  A large file (~2-4 gb) could
    cause the allocations to overflow, and allow the remaining data read
    from the file to overwrite other memory in the heap.

    Affected functions: FontFileAddEntry(), lexAlias()

- CVE-2014-0210: unvalidated length fields when parsing xfs protocol replies

    When parsing replies received from the font server, these calls do not
    check that the lengths and/or indexes returned by the font server are
    within the size of the reply or the bounds of the memory allocated to
    store the data, so could write past the bounds of allocated memory when
    storing the returned data.

    Affected functions: _fs_recv_conn_setup(), fs_read_open_font(),
    fs_read_query_info(), fs_read_extent_info(), fs_read_glyphs(),
    fs_read_list(), fs_read_list_info()

- CVE-2014-0211: integer overflows calculating memory needs for xfs replies

    These calls do not check that their calculations for how much memory
    is needed to handle the returned data have not overflowed, so can
    result in allocating too little memory and then writing the returned
    data past the end of the allocated buffer.

    Affected functions: fs_get_reply(), fs_alloc_glyphs(),
    fs_read_extent_info()

Reported by Ilja van Sprundel of IOActive
Fixes by Alan Coopersmith of Oracle

2 files changed, 244 insertions, 11 deletions

diff --git a/lib/libXfont/src/fc/fsconvert.c b/lib/libXfont/src/fc/fsconvert.c
index faf2e5d25..7a6033119 100644
--- a/lib/libXfont/src/fc/fsconvert.c
+++ b/lib/libXfont/src/fc/fsconvert.c
@@ -118,6 +118,10 @@ _fs_convert_props(fsPropInfo *pi, fsPropOffset *po, pointer pd,
     for (i = 0; i < nprops; i++, dprop++, is_str++)
     {
 	memcpy(&local_off, off_adr, SIZEOF(fsPropOffset));
+	if ((local_off.name.position >= pi->data_len) ||
+		(local_off.name.length >
+		 (pi->data_len - local_off.name.position)))
+	    goto bail;
 	dprop->name = MakeAtom(&pdc[local_off.name.position],
 			       local_off.name.length, 1);
 	if (local_off.type != PropTypeString) {
@@ -125,10 +129,15 @@ _fs_convert_props(fsPropInfo *pi, fsPropOffset *po, pointer pd,
 	    dprop->value = local_off.value.position;
 	} else {
 	    *is_str = TRUE;
+	    if ((local_off.value.position >= pi->data_len) ||
+		(local_off.value.length >
+		 (pi->data_len - local_off.value.position)))
+		goto bail;
 	    dprop->value = (INT32) MakeAtom(&pdc[local_off.value.position],
 					    local_off.value.length, 1);
 	    if (dprop->value == BAD_RESOURCE)
 	    {
+	      bail:
 		free (pfi->props);
 		pfi->nprops = 0;
 		pfi->props = 0;
@@ -712,7 +721,12 @@ fs_alloc_glyphs (FontPtr pFont, int size)
     FSGlyphPtr	glyphs;
     FSFontPtr	fsfont = (FSFontPtr) pFont->fontPrivate;
 
-    glyphs = malloc (sizeof (FSGlyphRec) + size);
+    if (size < (INT_MAX - sizeof (FSGlyphRec)))
+	glyphs = malloc (sizeof (FSGlyphRec) + size);
+    else
+	glyphs = NULL;
+    if (glyphs == NULL)
+	return NULL;
     glyphs->next = fsfont->glyphs;
     fsfont->glyphs = glyphs;
     return (pointer) (glyphs + 1);
diff --git a/lib/libXfont/src/fc/fserve.c b/lib/libXfont/src/fc/fserve.c
index a4451448f..6c2bd3a81 100644
--- a/lib/libXfont/src/fc/fserve.c
+++ b/lib/libXfont/src/fc/fserve.c
@@ -70,6 +70,7 @@ in this Software without prior written authorization from The Open Group.
 #include	"fservestr.h"
 #include	<X11/fonts/fontutil.h>
 #include	<errno.h>
+#include	<limits.h>
 
 #include	<time.h>
 #define Time_t time_t
@@ -91,6 +92,15 @@ in this Software without prior written authorization from The Open Group.
 			     (pci)->descent || \
 			     (pci)->characterWidth)
 
+/*
+ * SIZEOF(r) is in bytes, length fields in the protocol are in 32-bit words,
+ * so this converts for doing size comparisons.
+ */
+#define LENGTHOF(r)	(SIZEOF(r) >> 2)
+
+/* Somewhat arbitrary limit on maximum reply size we'll try to read. */
+#define MAX_REPLY_LENGTH	((64 * 1024 * 1024) >> 2)
+
 extern void ErrorF(const char *f, ...);
 
 static int fs_read_glyphs ( FontPathElementPtr fpe, FSBlockDataPtr blockrec );
@@ -206,9 +216,22 @@ _fs_add_rep_log (FSFpePtr conn, fsGenericReply *rep)
 		 rep->sequenceNumber,
 		 conn->reqbuffer[i].opcode);
 }
+
+#define _fs_reply_failed(rep, name, op) do {                            \
+    if (rep) {                                                          \
+        if (rep->type == FS_Error)                                      \
+            fprintf (stderr, "Error: %d Request: %s\n",                 \
+                     ((fsError *)rep)->request, #name);                 \
+        else                                                            \
+            fprintf (stderr, "Bad Length for %s Reply: %d %s %d\n",     \
+                     #name, rep->length, op, LENGTHOF(name));           \
+    }                                                                   \
+} while (0)
+
 #else
 #define _fs_add_req_log(conn,op)    ((conn)->current_seq++)
 #define _fs_add_rep_log(conn,rep)
+#define _fs_reply_failed(rep,name,op)
 #endif
 
 static Bool
@@ -600,6 +623,21 @@ fs_get_reply (FSFpePtr conn, int *error)
 
     rep = (fsGenericReply *) buf;
 
+    /*
+     * Refuse to accept replies longer than a maximum reasonable length,
+     * before we pass to _fs_start_read, since it will try to resize the
+     * incoming connection buffer to this size.  Also avoids integer overflow
+     * on 32-bit systems.
+     */
+    if (rep->length > MAX_REPLY_LENGTH)
+    {
+	ErrorF("fserve: reply length %d > MAX_REPLY_LENGTH, disconnecting"
+	       " from font server\n", rep->length);
+	_fs_connection_died (conn);
+	*error = FSIO_ERROR;
+	return 0;
+    }
+
     ret = _fs_start_read (conn, rep->length << 2, &buf);
     if (ret != FSIO_READY)
     {
@@ -682,13 +720,15 @@ fs_read_open_font(FontPathElementPtr fpe, FSBlockDataPtr blockrec)
     int			    ret;
 
     rep = (fsOpenBitmapFontReply *) fs_get_reply (conn, &ret);
-    if (!rep || rep->type == FS_Error)
+    if (!rep || rep->type == FS_Error ||
+	(rep->length != LENGTHOF(fsOpenBitmapFontReply)))
     {
 	if (ret == FSIO_BLOCK)
 	    return StillWorking;
 	if (rep)
 	    _fs_done_read (conn, rep->length << 2);
 	fs_cleanup_bfont (bfont);
+	_fs_reply_failed (rep, fsOpenBitmapFontReply, "!=");
 	return BadFontName;
     }
 
@@ -815,6 +855,7 @@ fs_read_query_info(FontPathElementPtr fpe, FSBlockDataPtr blockrec)
     FSFpePtr		conn = (FSFpePtr) fpe->private;
     fsQueryXInfoReply	*rep;
     char		*buf;
+    long		bufleft; /* length of reply left to use */
     fsPropInfo		*pi;
     fsPropOffset	*po;
     pointer		pd;
@@ -824,13 +865,15 @@ fs_read_query_info(FontPathElementPtr fpe, FSBlockDataPtr blockrec)
     int			ret;
 
     rep = (fsQueryXInfoReply *) fs_get_reply (conn, &ret);
-    if (!rep || rep->type == FS_Error)
+    if (!rep || rep->type == FS_Error ||
+	(rep->length < LENGTHOF(fsQueryXInfoReply)))
     {
 	if (ret == FSIO_BLOCK)
 	    return StillWorking;
 	if (rep)
 	    _fs_done_read (conn, rep->length << 2);
 	fs_cleanup_bfont (bfont);
+	_fs_reply_failed (rep, fsQueryXInfoReply, "<");
 	return BadFontName;
     }
 
@@ -844,6 +887,9 @@ fs_read_query_info(FontPathElementPtr fpe, FSBlockDataPtr blockrec)
     buf = (char *) rep;
     buf += SIZEOF(fsQueryXInfoReply);
 
+    bufleft = rep->length << 2;
+    bufleft -= SIZEOF(fsQueryXInfoReply);
+
     /* move the data over */
     fsUnpack_XFontInfoHeader(rep, pInfo);
 
@@ -851,17 +897,50 @@ fs_read_query_info(FontPathElementPtr fpe, FSBlockDataPtr blockrec)
     _fs_init_fontinfo(conn, pInfo);
 
     /* Compute offsets into the reply */
+    if (bufleft < SIZEOF(fsPropInfo))
+    {
+	ret = -1;
+#ifdef DEBUG
+	fprintf(stderr, "fsQueryXInfo: bufleft (%ld) < SIZEOF(fsPropInfo)\n",
+		bufleft);
+#endif
+	goto bail;
+    }
     pi = (fsPropInfo *) buf;
     buf += SIZEOF (fsPropInfo);
+    bufleft -= SIZEOF(fsPropInfo);
 
+    if ((bufleft / SIZEOF(fsPropOffset)) < pi->num_offsets)
+    {
+	ret = -1;
+#ifdef DEBUG
+	fprintf(stderr,
+		"fsQueryXInfo: bufleft (%ld) / SIZEOF(fsPropOffset) < %d\n",
+		bufleft, pi->num_offsets);
+#endif
+	goto bail;
+    }
     po = (fsPropOffset *) buf;
     buf += pi->num_offsets * SIZEOF(fsPropOffset);
+    bufleft -= pi->num_offsets * SIZEOF(fsPropOffset);
 
+    if (bufleft < pi->data_len)
+    {
+	ret = -1;
+#ifdef DEBUG
+	fprintf(stderr,
+		"fsQueryXInfo: bufleft (%ld) < data_len (%d)\n",
+		bufleft, pi->data_len);
+#endif
+	goto bail;
+    }
     pd = (pointer) buf;
     buf += pi->data_len;
+    bufleft -= pi->data_len;
 
     /* convert the properties and step over the reply */
     ret = _fs_convert_props(pi, po, pd, pInfo);
+  bail:
     _fs_done_read (conn, rep->length << 2);
 
     if (ret == -1)
@@ -951,13 +1030,15 @@ fs_read_extent_info(FontPathElementPtr fpe, FSBlockDataPtr blockrec)
     FontInfoRec		    *fi = &bfont->pfont->info;
 
     rep = (fsQueryXExtents16Reply *) fs_get_reply (conn, &ret);
-    if (!rep || rep->type == FS_Error)
+    if (!rep || rep->type == FS_Error ||
+	(rep->length < LENGTHOF(fsQueryXExtents16Reply)))
     {
 	if (ret == FSIO_BLOCK)
 	    return StillWorking;
 	if (rep)
 	    _fs_done_read (conn, rep->length << 2);
 	fs_cleanup_bfont (bfont);
+	_fs_reply_failed (rep, fsQueryXExtents16Reply, "<");
 	return BadFontName;
     }
 
@@ -970,7 +1051,26 @@ fs_read_extent_info(FontPathElementPtr fpe, FSBlockDataPtr blockrec)
 	numInfos *= 2;
 	haveInk = TRUE;
     }
-    ci = pCI = malloc(sizeof(CharInfoRec) * numInfos);
+    if (numInfos >= (INT_MAX / sizeof(CharInfoRec))) {
+#ifdef DEBUG
+	fprintf(stderr,
+		"fsQueryXExtents16: numInfos (%d) >= %ld\n",
+		numInfos, (INT_MAX / sizeof(CharInfoRec)));
+#endif
+	pCI = NULL;
+    }
+    else if (numExtents > ((rep->length - LENGTHOF(fsQueryXExtents16Reply))
+			    / LENGTHOF(fsXCharInfo))) {
+#ifdef DEBUG
+	fprintf(stderr,
+		"fsQueryXExtents16: numExtents (%d) > (%d - %d) / %d\n",
+		numExtents, rep->length,
+		LENGTHOF(fsQueryXExtents16Reply), LENGTHOF(fsXCharInfo));
+#endif
+	pCI = NULL;
+    }
+    else
+	pCI = malloc(sizeof(CharInfoRec) * numInfos);
 
     if (!pCI)
     {
@@ -1809,6 +1909,7 @@ fs_read_glyphs(FontPathElementPtr fpe, FSBlockDataPtr blockrec)
     FontInfoPtr		    pfi = &pfont->info;
     fsQueryXBitmaps16Reply  *rep;
     char		    *buf;
+    long		    bufleft; /* length of reply left to use */
     fsOffset32		    *ppbits;
     fsOffset32		    local_off;
     char		    *off_adr;
@@ -1825,22 +1926,48 @@ fs_read_glyphs(FontPathElementPtr fpe, FSBlockDataPtr blockrec)
     unsigned long	    minchar, maxchar;
 
     rep = (fsQueryXBitmaps16Reply *) fs_get_reply (conn, &ret);
-    if (!rep || rep->type == FS_Error)
+    if (!rep || rep->type == FS_Error ||
+	(rep->length < LENGTHOF(fsQueryXBitmaps16Reply)))
     {
 	if (ret == FSIO_BLOCK)
 	    return StillWorking;
 	if (rep)
 	    _fs_done_read (conn, rep->length << 2);
 	err = AllocError;
+	_fs_reply_failed (rep, fsQueryXBitmaps16Reply, "<");
 	goto bail;
     }
 
     buf = (char *) rep;
     buf += SIZEOF (fsQueryXBitmaps16Reply);
 
+    bufleft = rep->length << 2;
+    bufleft -= SIZEOF (fsQueryXBitmaps16Reply);
+
+    if ((bufleft / SIZEOF (fsOffset32)) < rep->num_chars)
+    {
+#ifdef DEBUG
+	fprintf(stderr,
+		"fsQueryXBitmaps16: num_chars (%d) > bufleft (%ld) / %d\n",
+		rep->num_chars, bufleft, SIZEOF (fsOffset32));
+#endif
+	err = AllocError;
+	goto bail;
+    }
     ppbits = (fsOffset32 *) buf;
     buf += SIZEOF (fsOffset32) * (rep->num_chars);
+    bufleft -= SIZEOF (fsOffset32) * (rep->num_chars);
 
+    if (bufleft < rep->nbytes)
+    {
+#ifdef DEBUG
+	fprintf(stderr,
+		"fsQueryXBitmaps16: nbytes (%d) > bufleft (%ld)\n",
+		rep->nbytes, bufleft);
+#endif
+	err = AllocError;
+	goto bail;
+    }
     pbitmaps = (pointer ) buf;
 
     if (blockrec->type == FS_LOAD_GLYPHS)
@@ -1898,7 +2025,9 @@ fs_read_glyphs(FontPathElementPtr fpe, FSBlockDataPtr blockrec)
 	     */
 	    if (NONZEROMETRICS(&fsdata->encoding[minchar].metrics))
 	    {
-		if (local_off.length)
+		if (local_off.length &&
+		    (local_off.position < rep->nbytes) &&
+		    (local_off.length <= (rep->nbytes - local_off.position)))
 		{
 		    bits = allbits;
 		    allbits += local_off.length;
@@ -2228,31 +2357,48 @@ fs_read_list(FontPathElementPtr fpe, FSBlockDataPtr blockrec)
     FSBlockedListPtr	blist = (FSBlockedListPtr) blockrec->data;
     fsListFontsReply	*rep;
     char		*data;
+    long		dataleft; /* length of reply left to use */
     int			length,
 			i,
 			ret;
     int			err;
 
     rep = (fsListFontsReply *) fs_get_reply (conn, &ret);
-    if (!rep || rep->type == FS_Error)
+    if (!rep || rep->type == FS_Error ||
+	(rep->length < LENGTHOF(fsListFontsReply)))
     {
 	if (ret == FSIO_BLOCK)
 	    return StillWorking;
 	if (rep)
 	    _fs_done_read (conn, rep->length << 2);
+	_fs_reply_failed (rep, fsListFontsReply, "<");
 	return AllocError;
     }
     data = (char *) rep + SIZEOF (fsListFontsReply);
+    dataleft = (rep->length << 2) - SIZEOF (fsListFontsReply);
 
     err = Successful;
     /* copy data into FontPathRecord */
     for (i = 0; i < rep->nFonts; i++)
     {
+	if (dataleft < 1)
+	    break;
 	length = *(unsigned char *)data++;
+	dataleft--; /* used length byte */
+	if (length > dataleft) {
+#ifdef DEBUG
+	    fprintf(stderr,
+		    "fsListFonts: name length (%d) > dataleft (%ld)\n",
+		    length, dataleft);
+#endif
+	    err = BadFontName;
+	    break;
+	}
 	err = AddFontNamesName(blist->names, data, length);
 	if (err != Successful)
 	    break;
 	data += length;
+	dataleft -= length;
     }
     _fs_done_read (conn, rep->length << 2);
     return err;
@@ -2347,6 +2493,7 @@ fs_read_list_info(FontPathElementPtr fpe, FSBlockDataPtr blockrec)
     FSBlockedListInfoPtr	binfo = (FSBlockedListInfoPtr) blockrec->data;
     fsListFontsWithXInfoReply	*rep;
     char			*buf;
+    long			bufleft;
     FSFpePtr			conn = (FSFpePtr) fpe->private;
     fsPropInfo			*pi;
     fsPropOffset		*po;
@@ -2358,12 +2505,15 @@ fs_read_list_info(FontPathElementPtr fpe, FSBlockDataPtr blockrec)
     _fs_free_props (&binfo->info);
 
     rep = (fsListFontsWithXInfoReply *) fs_get_reply (conn, &ret);
-    if (!rep || rep->type == FS_Error)
+    if (!rep || rep->type == FS_Error ||
+	((rep->nameLength != 0) &&
+	 (rep->length < LENGTHOF(fsListFontsWithXInfoReply))))
     {
 	if (ret == FSIO_BLOCK)
 	    return StillWorking;
 	binfo->status = FS_LFWI_FINISHED;
 	err = AllocError;
+	_fs_reply_failed (rep, fsListFontsWithXInfoReply, "<");
 	goto done;
     }
     /*
@@ -2380,6 +2530,7 @@ fs_read_list_info(FontPathElementPtr fpe, FSBlockDataPtr blockrec)
     }
 
     buf = (char *) rep + SIZEOF (fsListFontsWithXInfoReply);
+    bufleft = (rep->length << 2) - SIZEOF (fsListFontsWithXInfoReply);
 
     /*
      * The original FS implementation didn't match
@@ -2388,19 +2539,71 @@ fs_read_list_info(FontPathElementPtr fpe, FSBlockDataPtr blockrec)
      */
     if (conn->fsMajorVersion <= 1)
     {
+	if (rep->nameLength > bufleft) {
+#ifdef DEBUG
+	    fprintf(stderr,
+		    "fsListFontsWithXInfo: name length (%d) > bufleft (%ld)\n",
+		    (int) rep->nameLength, bufleft);
+#endif
+	    err = AllocError;
+	    goto done;
+	}
+	/* binfo->name is a 256 char array, rep->nameLength is a CARD8 */
 	memcpy (binfo->name, buf, rep->nameLength);
 	buf += _fs_pad_length (rep->nameLength);
+	bufleft -= _fs_pad_length (rep->nameLength);
     }
     pi = (fsPropInfo *) buf;
+    if (SIZEOF (fsPropInfo) > bufleft) {
+#ifdef DEBUG
+	fprintf(stderr,
+		"fsListFontsWithXInfo: PropInfo length (%d) > bufleft (%ld)\n",
+		(int) SIZEOF (fsPropInfo), bufleft);
+#endif
+	err = AllocError;
+	goto done;
+    }
+    bufleft -= SIZEOF (fsPropInfo);
     buf += SIZEOF (fsPropInfo);
     po = (fsPropOffset *) buf;
+    if (pi->num_offsets > (bufleft / SIZEOF (fsPropOffset))) {
+#ifdef DEBUG
+	fprintf(stderr,
+		"fsListFontsWithXInfo: offset length (%d * %d) > bufleft (%ld)\n",
+		pi->num_offsets, (int) SIZEOF (fsPropOffset), bufleft);
+#endif
+	err = AllocError;
+	goto done;
+    }
+    bufleft -= pi->num_offsets * SIZEOF (fsPropOffset);
     buf += pi->num_offsets * SIZEOF (fsPropOffset);
     pd = (pointer) buf;
+    if (pi->data_len > bufleft) {
+#ifdef DEBUG
+	fprintf(stderr,
+		"fsListFontsWithXInfo: data length (%d) > bufleft (%ld)\n",
+		pi->data_len, bufleft);
+#endif
+	err = AllocError;
+	goto done;
+    }
+    bufleft -= pi->data_len;
     buf += pi->data_len;
     if (conn->fsMajorVersion > 1)
     {
+	if (rep->nameLength > bufleft) {
+#ifdef DEBUG
+	    fprintf(stderr,
+		    "fsListFontsWithXInfo: name length (%d) > bufleft (%ld)\n",
+		    (int) rep->nameLength, bufleft);
+#endif
+	    err = AllocError;
+	    goto done;
+	}
+	/* binfo->name is a 256 char array, rep->nameLength is a CARD8 */
 	memcpy (binfo->name, buf, rep->nameLength);
 	buf += _fs_pad_length (rep->nameLength);
+	bufleft -= _fs_pad_length (rep->nameLength);
     }
 
 #ifdef DEBUG
@@ -2786,7 +2989,7 @@ _fs_recv_conn_setup (FSFpePtr conn)
     int			ret = FSIO_ERROR;
     fsConnSetup		*setup;
     FSFpeAltPtr		alts;
-    int			i, alt_len;
+    unsigned int	i, alt_len;
     int			setup_len;
     char		*alt_save, *alt_names;
 
@@ -2813,8 +3016,9 @@ _fs_recv_conn_setup (FSFpePtr conn)
 	}
 	if (setup->num_alternates)
 	{
+	    size_t alt_name_len = setup->alternate_len << 2;
 	    alts = malloc (setup->num_alternates * sizeof (FSFpeAltRec) +
-			   (setup->alternate_len << 2));
+			   alt_name_len);
 	    if (alts)
 	    {
 		alt_names = (char *) (setup + 1);
@@ -2823,10 +3027,25 @@ _fs_recv_conn_setup (FSFpePtr conn)
 		{
 		    alts[i].subset = alt_names[0];
 		    alt_len = alt_names[1];
+		    if (alt_len >= alt_name_len) {
+			/*
+			 * Length is longer than setup->alternate_len
+			 * told us to allocate room for, assume entire
+			 * alternate list is corrupted.
+			 */
+#ifdef DEBUG
+			fprintf (stderr,
+				 "invalid alt list (length %lx >= %lx)\n",
+				 (long) alt_len, (long) alt_name_len);
+#endif
+			free(alts);
+			return FSIO_ERROR;
+		    }
 		    alts[i].name = alt_save;
 		    memcpy (alt_save, alt_names + 2, alt_len);
 		    alt_save[alt_len] = '\0';
 		    alt_save += alt_len + 1;
+		    alt_name_len -= alt_len + 1;
 		    alt_names += _fs_pad_length (alt_len + 2);
 		}
 		conn->numAlts = setup->num_alternates;