On Sun, Feb 15, 2015 at 3:08 PM, Mihai Moldovan <[email protected]> wrote: > On 14.02.2015 05:47 PM, [email protected] 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 <[email protected]> >> 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/cf1e18c13a5ba2054e36a6f0b044c3ed10c1b358/SOURCES!0006-CVE-2014-0210-unvalidated-length-fields-in-fs_read_q.patch;jsessionid=12hi4vv3qj9yo2ercznrbzevc > > 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? _______________________________________________ x2go-dev mailing list [email protected] http://lists.x2go.org/listinfo/x2go-dev
