Bug 706087 - libvncserver-0.9.7 unallocated memory access at tightBeforeBufSize and tightAfterBufSize
Summary: libvncserver-0.9.7 unallocated memory access at tightBeforeBufSize and tightA...
Keywords:
Status: CLOSED WONTFIX
Alias: None
Product: Security Response
Classification: Other
Component: vulnerability
Version: unspecified
Hardware: All
OS: Linux
low
low
Target Milestone: ---
Assignee: Red Hat Product Security
QA Contact:
URL:
Whiteboard:
Depends On:
Blocks: 1247623
TreeView+ depends on / blocked
 
Reported: 2011-05-19 13:14 UTC by Petr Pisar
Modified: 2019-09-29 12:45 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-09-03 06:41:01 UTC
Embargoed:


Attachments (Terms of Use)

Description Petr Pisar 2011-05-19 13:14:58 UTC
libvncserver/tight.c:rfbTightCleanup() frees a buffer without zeroing freed pointer:

void rfbTightCleanup(rfbScreenInfoPtr screen)
{
  if(tightBeforeBufSize) {
    free(tightBeforeBuf);
    tightBeforeBufSize=0;
  }
  if(tightAfterBufSize) {
    free(tightAfterBuf);
    tightAfterBufSize=0;
  }
}

The tightBeforeBufSize is global variable used on other places in similar manner:

    if (tightBeforeBufSize < 4) {
        tightBeforeBufSize = 4;
        if (tightBeforeBuf == NULL)
            tightBeforeBuf = (char *)malloc(tightBeforeBufSize);
        else
            tightBeforeBuf = (char *)realloc(tightBeforeBuf,
                                              tightBeforeBufSize);
    }

which can try to reallocate unallocated/other memory region.

This problem is explained in upstream commit:

commit c1363fa9583ed41b94fbc79b3ff410b7d5189407
Author: George Kiagiadakis <kiagiadakis.george>
Date:   Wed Nov 10 18:57:23 2010 +0000

    Fix memory corruption bug.
    
    This bug occured when a second telepathy tubes client was connected after
    the first one had disconnected and the channel (thus, the screen too)
    had been destroyed.
    
    Signed-off-by: Johannes Schindelin <johannes.schindelin>


The commit itself is useless as the bug has been fixed already in:

commit 804335f9d296440bb708ca844f5d89b58b50b0c6
Author: runge <runge>
Date:   Thu May 21 10:32:18 2009 -0400

    Thread safety for zrle, zlib, tight.
    Proposed tight security type fix for debian bug 517422.


The huge commit fixes a lot of other bugs by utilizing thread local storage. Unfortunately it adds new members to public data structure which breaks ABI, thus I do not recommend back-porting it.

However the small fix for nullifying freed pointers is good candidate to apply to current libvncserver packages:

 void rfbTightCleanup(rfbScreenInfoPtr screen)
 {
   if(tightBeforeBufSize) {
     free(tightBeforeBuf);
     tightBeforeBufSize=0;
+    tightBeforeBuf = NULL;
   }
   if(tightAfterBufSize) {
     free(tightAfterBuf);
     tightAfterBufSize=0;
+    tightAfterBuf = NULL;
   }
 }

Comment 1 Huzaifa S. Sidhpurwala 2015-09-03 06:31:29 UTC
Analysis:

It seems this issue exists because libvncserver < 0.9.8 is not thread-safe. "tightAfterBuf" etc are declared as a global variables. rfbTightCleanup() which is responsible for freeing these variables, do not reset them to NULL.

Consequently when other parts of the code use these variables. (For example, realloc() after checking if they are NULL etc). It can cause memory corruption.

This is specially possible, when multiple connections are made using libvncserver library.

This issue has been fixed via:

https://github.com/LibVNC/libvncserver/commit/804335f9d296440bb708ca844f5d89b58b50b0c6

As mentioned in comment #0, this breaks ABI.

Comment 2 Huzaifa S. Sidhpurwala 2015-09-03 06:39:34 UTC
CVE Request:

http://openwall.com/lists/oss-security/2015/09/03/8


Note You need to log in before you can comment on or make changes to this bug.