[X2Go-Dev] [X2Go-Commits] [nx-libs] 19/52: CVE-2014-0210: unvalidated length in _fs_recv_conn_setup() from xorg/lib/libXfont commit 891e084b26837162b12f841060086a105edde86d

[Michael DePaulo]

On Sun, Feb 15, 2015 at 2:18 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 94c6de0649cd295044b1e4ff7265949c9c787519
>> Author: Mike DePaulo <mikedep333 at gmail.com>
>> Date:   Sun Feb 8 21:03:33 2015 -0500
>>
>>     CVE-2014-0210: unvalidated length in _fs_recv_conn_setup() from xorg/lib/libXfont commit 891e084b26837162b12f841060086a105edde86d
>>
>>     The connection setup reply from the font server can include a list
>>     of alternate servers to contact if this font server stops working.
>>
>>     The reply specifies a total size of all the font server names, and
>>     then provides a list of names. _fs_recv_conn_setup() allocated the
>>     specified total size for copying the names to, but didn't check to
>>     make sure it wasn't copying more data to that buffer than the size
>>     it had allocated.
>> ---
>>  nx-X11/lib/font/fc/fserve.c |   21 ++++++++++++++++++---
>>  1 file changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/nx-X11/lib/font/fc/fserve.c b/nx-X11/lib/font/fc/fserve.c
>> index bac0b8e..0fdcc1d 100644
>> --- a/nx-X11/lib/font/fc/fserve.c
>> +++ b/nx-X11/lib/font/fc/fserve.c
>> @@ -2782,7 +2782,7 @@ _fs_recv_conn_setup (FSFpePtr conn)
>>      int                      ret;
>>      fsConnSetup              *setup;
>>      FSFpeAltPtr              alts;
>> -    int                      i, alt_len;
>> +    unsigned int     i, alt_len;
>>      int                      setup_len;
>>      char             *alt_save, *alt_names;
>>
>> @@ -2809,9 +2809,9 @@ _fs_recv_conn_setup (FSFpePtr conn)
>>       }
>>       if (setup->num_alternates)
>>       {
>> +         size_t alt_name_len = setup->alternate_len << 2;
>>           alts = (FSFpeAltPtr) xalloc (setup->num_alternates *
>> -                                      sizeof (FSFpeAltRec) +
>> -                                      (setup->alternate_len << 2));
>> +                                      sizeof (FSFpeAltRec) + alt_name_len);
>>           if (alts)
>>           {
>>               alt_names = (char *) (setup + 1);
>> @@ -2820,10 +2820,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);
>
> Shouldn't this be xfree(alts) if using xalloc previously?

I am actually not sure, I need to learn memory management better.

The upstream commit uses and free(alts), and malloc is used before it:
http://cgit.freedesktop.org/xorg/lib/libXfont/commit/?id=891e084b26837162b12f841060086a105edde86d
And that is what I based my commit/patch on.

However, the RHEL5 patch also uses free(alts), and xalloc is used before it:
ftp://ftp.redhat.com/redhat/linux/enterprise/5Server/en/os/SRPMS/libXfont-1.2.2-1.0.6.el5_11.src.rpm
(0003-CVE-2014-0210-unvalidated-length-in-_fs_recv_conn_se.patch)
The patch doesn't specify who resolved the RHEL5 conflict, but it was
probably Adam Jackson. (ajax)

-Mike