[X2Go-Dev] [X2Go-Commits] [nx-libs] 23/52: CVE-2014-0210: unvalidated length fields in fs_read_query_info() from xorg/lib/libXfont commit 491291cabf78efdeec8f18b09e14726a9030cc8f

[Michael DePaulo]

On Sun, Feb 15, 2015 at 3:08 PM, Mihai Moldovan <ionic at ionic.de> wrote:
> On 14.02.2015 05:47 PM, git-admin at 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 at 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/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?