[xorg-bugzilla-noise] [Bug 1459] New: Stack overflow in libX11

[bugzilla-daemon at freedesktop.org]

Please do not reply to this email: if you want to comment on the bug, go to          
the URL shown below and enter yourcomments there.   
 
https://freedesktop.org/bugzilla/show_bug.cgi?id=1459        
   
           Summary: Stack overflow in libX11
           Product: xorg
           Version: unspecified
          Platform: PC
        OS/Version: Linux
            Status: NEW
          Severity: normal
          Priority: P2
         Component: Lib/Xlib
        AssignedTo: xorg-bugzilla-noise at freedesktop.org
        ReportedBy: mharris at www.linux.org.uk


Xlib contains a stack overflow which was discovered by Arjan van de Ven when
recompiling X.Org X11 6.8.1 with gcc4 modified with new security features that
are under development which can autodetect many common buffer overflows at
compiletime and runtime.

The stack overflow was detected at compiletime and occurs in the file
xc/lib/X11/XKBBind.c in function XkbRefreshKeyboardMapping() on line #366:


    if (event->xkb_type==XkbMapNotify) {
        XkbMapChangesRec        changes;
        Status                  rtrn;

        if (xkbi->flags&XkbMapPending)
             changes= xkbi->changes;
----->  else bzero(&changes,sizeof(XkbChangesRec));
        XkbNoteMapChanges(&changes,event,XKB_XLIB_MAP_MASK);


I investigated this issue and found the following:

The "changes" structure is of type "XkbMapChangesRec", however bzero is
being called to zero it out with a size of "sizeof(XkbChangesRec)" which
is incorrect.  Both of these structures are defined in the header
xc/include/extensions/XKBstr.h as follows:

typedef struct _XkbMapChanges {
        unsigned short           changed;
        KeyCode                  min_key_code;
        KeyCode                  max_key_code;
        unsigned char            first_type;
        unsigned char            num_types;
        KeyCode                  first_key_sym;
        unsigned char            num_key_syms;
        KeyCode                  first_key_act;
        unsigned char            num_key_acts;
        KeyCode                  first_key_behavior;
        unsigned char            num_key_behaviors;
        KeyCode                  first_key_explicit;
        unsigned char            num_key_explicit;
        KeyCode                  first_modmap_key;
        unsigned char            num_modmap_keys;
        KeyCode                  first_vmodmap_key;
        unsigned char            num_vmodmap_keys;
        unsigned char            pad;
        unsigned short           vmods;
} XkbMapChangesRec,*XkbMapChangesPtr;

typedef struct _XkbChanges {
        unsigned short           device_spec;
        unsigned short           state_changes;
        XkbMapChangesRec         map;
        XkbControlsChangesRec    ctrls;
        XkbIndicatorChangesRec   indicators;
        XkbNameChangesRec        names;
        XkbCompatChangesRec      compat;
} XkbChangesRec, *XkbChangesPtr;


Observe that the XkbChangesRec struct is much larger than the XkbMapChangesRec
struct, and actually contains a member of type XkbMapChangesRec.  As such, the
call to bzero to zero out the "changes" struct overflows, and data on the
stack is corrupted.

A preliminary examination of the effects of the overflow seem to show no
security issues would be caused by this other than application failure.

I have written a patch to fix this problem which replaces the faulty
bzero call in a manner similar to other locations in the code, as
follows:

-        bzero(&changes,sizeof(XkbChangesRec));
+        bzero(&changes,sizeof(changes));


I have not yet examined the 6.7.0 release to see if the same issue is
present, but I assume it is.  The patch I've written should be applied
to CVS HEAD, as well as the 6.8 branch (and 6.7 stable branch if the
problem exists there also), as it is a clean and obviously correct fix.

Attaching patch to fix this issue.        
   
   
--         
Configure bugmail: https://freedesktop.org/bugzilla/userprefs.cgi?tab=email       
   
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.