Possible SEGV in x_error_handler()

[GJDuck]

There is a possible libX11 API error that leads to a SEGV (null-pointer dereference).

The problem appears to be that the x_error_handler() function in lib/tty/x11conn.c calls XCloseDisplay(), but calling "functions ... on the display that will generate protocol requests" seems to be generally disallowed (see manpage for XSetErrorHandler).

If an X11 error occurs during XOpenDisplay(), then x_error_handler() will be called, which then in turn calls XCloseDisplay(). The call to XCloseDisplay() will crash (see stack trace below) since it attempts to free resources that have not yet been created. This appears to be a NULL pointer dereference.

Reproducing the bug is difficult as it requires an X11 error to occur during XOpenDisplay(). But let me know if you need a PoC.

Tested on the latest Github head.

Here is the relevant source snippet (annotated) from lib/tty/x11conn.c:

    static int x_error_handler (Display * dpy, XErrorEvent * ee)
    {
        ...
        /* Handler closes the display on error. */
        (void) func_XCloseDisplay (dpy);
        ...
    }

    /* Handler is installed *before* XOpenDisplay() */
    static void install_error_handlers (void)
    {
        ...
        (void) func_XSetErrorHandler (x_error_handler);
        ...
    }
    static gboolean x11_available (void)
    {
        ...
        install_error_handlers ();
        return TRUE;
    }
    Display *mc_XOpenDisplay (const char *displayname)
    {
        if (x11_available ())   // <--- handler installed here.
        {
            ...
            /* If an X11 error occurs during XOpenDisplay(), the
             * x_error_handler() and XCloseDisplay may be called
             * *before* the display is fully opened.  This leads to
             * a SIGSEGV, null-pointer dereference in XFreeGC(),
             * where libx11 attempts to free a resource (gc) not yet
             * created. */
            retval = func_XOpenDisplay (displayname);
            ...
        }
    }

Here is the relevant stack trace:

Program received signal SIGSEGV, Segmentation fault.
0x00007ffde7edf8d4 in XFreeGC (dpy=dpy@entry=0x555555700bb0, gc=0x0)
    at ../../src/FreeGC.c:43
(gdb) bt
#0  0x00007ffde7edf8d4 in XFreeGC (dpy=dpy@entry=0x555555700bb0, gc=0x0)
    at ../../src/FreeGC.c:43
#1  0x00007ffde7ee3bce in XCloseDisplay (dpy=0x555555700bb0)
    at ../../src/ClDisplay.c:56
#2  0x000055555563895a in x_error_handler (dpy=0x555555700bb0,
    ee=0x7fffffffdaa0) at x11conn.c:109
#3  0x00007ffde7f048bb in _XError (dpy=dpy@entry=0x555555700bb0,
    rep=rep@entry=0x5555557028e0) at ../../src/XlibInt.c:1503
#4  0x00007ffde7f049b7 in handle_error (dpy=0x555555700bb0,
    err=0x5555557028e0, in_XReply=<optimized out>) at ../../src/xcb_io.c:211
#5  0x00007ffde7f04a55 in handle_response (dpy=dpy@entry=0x555555700bb0,
    response=0x5555557028e0, in_XReply=in_XReply@entry=0)
    at ../../src/xcb_io.c:403
#6  0x00007ffde7f04b0a in _XEventsQueued (dpy=0x555555700bb0,
    mode=mode@entry=1) at ../../src/xcb_io.c:442
#7  0x00007ffde7f04bcc in _XFlush (dpy=<optimized out>)
    at ../../src/xcb_io.c:611
#8  0x00007ffde7f04ebd in _XGetRequest (dpy=0x555555700bb0,
    type=<optimized out>, len=16) at ../../src/XlibInt.c:1787
#9  0x00007ffde7ee0c9a in XCreateGC (dpy=0x555555700bb0, d=1330,
    valuemask=12, values=0x7fffffffdca0) at ../../src/CrGC.c:89
#10 0x00007ffde7ef2621 in XOpenDisplay (display=<optimized out>)
    at ../../src/OpenDis.c:514
#11 0x0000555555638bd1 in mc_XOpenDisplay (displayname=0x0) at x11conn.c:195
#12 0x0000555555631b47 in init_key_x11 () at key.c:709
#13 0x000055555563288d in init_key () at key.c:1367
#14 0x00005555555701ed in main (argc=1, argv=0x7fffffffdf68) at main.c:358

Other info:

mc -V

    GNU Midnight Commander 4.8.29-146-g299d9a2fb
    Built with GLib 2.76.1
    Built with S-Lang 2.3.3 with terminfo database
    With builtin Editor
    With subshell support as default
    With support for background operations
    With mouse support on xterm and Linux console
    With support for X11 events
    With internationalization support
    With multiple codepages support
    Virtual File Systems:
     cpiofs, tarfs, sfs, extfs, ftpfs, fish
    Data types:
     char: 8; int: 32; long: 64; void *: 64; size_t: 64; off_t: 64;

mc -F

    Home directory: /home/gjd
    Profile root directory: /home/gjd

    [System data]
        Config directory: /usr/local/etc/mc/
        Data directory:   /usr/local/share/mc/
        File extension handlers: /usr/local/libexec/mc/ext.d/
        VFS plugins and scripts: /usr/local/libexec/mc/
        extfs.d:        /usr/local/libexec/mc/extfs.d/
        fish:           /usr/local/libexec/mc/fish/

    [User data]
        Config directory: /home/gjd/.config/mc/
        Data directory:   /home/gjd/.local/share/mc/
        skins:          /home/gjd/.local/share/mc/skins/
        extfs.d:        /home/gjd/.local/share/mc/extfs.d/
        fish:           /home/gjd/.local/share/mc/fish/
        mcedit macros:  /home/gjd/.local/share/mc/mc.macros
        mcedit external macros: /home/gjd/.local/share/mc/mcedit/macros.d/macro.*
        Cache directory:  /home/gjd/.cache/mc/

[andrew_b]

Should we install error handlers only when XOpenDisplay() finished successfully and deinstall after XCloseDisplay()?

[ossi]

the error handler should not close the display, as these errors are non-fatal. consequently, it should also not chain to the i/o error handler. however, it should print something (or otherwise save a messages somewhere the user can find it), as silent failures aren't all that nice.

[zaytsev]

What the heck, now there is even a CVE for that: ​https://www.cve.org/CVERecord?id=CVE-2023-45925 . Feels like some security "researchers" are after CVE entries.

[andrew_b]

I'm afraid I don't have enough skills to fix that.

[zaytsev]

Sorry, I didn't mean to try to pressure you in fixing this problem, I was just expressing my frustration with unhelpful CVE-grabbing, as I stumbled upon it, while looking for a different CVE.

Regarding the error itself, I think our error handling strategy stinks and ossi is generally right, but without redoing everything properly, probably the best way to fix the immediate problem would be to install error handlers after the display has been properly opened as you suggested.

I guess this would be better than leaving it for another 10 years...