On Sun, Feb 15, 2015 at 3:08 PM, Mihai Moldovan <ionic@ionic.de> wrote:

On 14.02.2015 05:47 PM, git-admin@x2go.org wrote:

...

This is an automated email from the git hooks/post-receive script.

x2go pushed a commit to branch 3.6.x in repository nx-libs.

commit c6aebf9284855a0e24ad9c5ffdd36aa65e16bec7 Author: Mike DePaulo <mikedep333@gmail.com> Date: Sun Feb 8 22:08:09 2015 -0500

CVE-2014-0210: unvalidated length fields in fs_read_query_info() from xorg/lib/libXfont commit 491291cabf78efdeec8f18b09e14726a9030cc8f

fs_read_query_info() parses a reply from the font server.  The reply
contains embedded length fields, none of which are validated.  This
can cause out of bound reads in either fs_read_query_info() or in
_fs_convert_props() which it calls to parse the fsPropInfo in the reply.

---

nx-X11/lib/font/fc/fsconvert.c | 19 ++++++++++++++----- nx-X11/lib/font/fc/fserve.c | 40 ++++++++++++++++++++++++++++++++++++++-- 2 files changed, 52 insertions(+), 7 deletions(-)

[...] diff --git a/nx-X11/lib/font/fc/fserve.c b/nx-X11/lib/font/fc/fserve.c index 7762653..2a6f6c9 100644 --- a/nx-X11/lib/font/fc/fserve.c +++ b/nx-X11/lib/font/fc/fserve.c @@ -865,6 +865,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 */

Please initialize variables. Even if they are re-set later on.

long bufleft = 0;

...

 fsPropInfo               *pi;
 fsPropOffset     *po;
 pointer          pd;

@@ -895,7 +896,10 @@ 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);

@@ -903,19 +907,51 @@ 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 -= pi->num_offsets * SIZEOF(fsPropOffset);

This looks wrong. You probably meant to use:

bufleft -= SIZEOF (fsPropInfo);

...

• if (bufleft < pi->data_len)

This looks wrong, too. Check:

https://git.centos.org/blob/rpms!libXfont.git/cf1e18c13a5ba2054e36a6f0b044c3...

You probably meant to use:

if ((bufleft / SIZEOF(fsPropOffset)) < pi->num_offsets)

or

if (bufleft < (SIZEOF(fsPropOffset) * pi->num_offsets))

and

...

• {

• ret = -1;

  +#ifdef DEBUG

• fprintf(stderr,

•         "fsQueryXInfo: bufleft (%ld) < data_len (%d)\n",

•         bufleft, pi->data_len);

here either:

"fsQueryXInfo: (bufleft / SIZEOF(fsPropOffset)) (%ld) < pi->num_offsets (%d)\n", bufleft / SIZEOF(fsPropOffset), pi->num_offsets);

or

"fsQueryXInfo: bufleft (%ld) < (SIZEOF(fsPropOffset) * pi->num_offsets) (%d)\n", bufleft, SIZEOF(fsPropOffset) * pi->num_offsets);

depending on what you used before.

...

+#endif

• goto bail;

• } po = (fsPropOffset *) buf; buf += pi->num_offsets * SIZEOF(fsPropOffset);
• bufleft -= pi->data_len;

Same problem here. Should be:

bufleft -= pi->num_offsets * SIZEOF(fsPropOffset);

...

• {

This block will ALWAYS be executed and ALWAYS *FAIL*!

You're missing (*before* the curly brace):

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) { fs_cleanup_bfont (bfont);

This looks OK again.

Would you mind making a follow-up commit?