From 412777664a20dd3561b936c02c96571a756fe9b2 Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Tue, 20 Dec 2022 10:42:03 +1000 Subject: [PATCH] Disallow byte-swapped clients by default The X server swapping code is a huge attack surface, much of this code is untested and prone to security issues. The use-case of byte-swapped clients is very niche, so let's disable this by default and allow it only when the respective config option or commandline flag is given. For Xorg, this adds the ServerFlag "AllowByteSwappedClients" "on". For all DDX, this adds the commandline options +byteswappedclients and -byteswappedclients to enable or disable, respectively. Fixes #1201 https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1029 Signed-off-by: Peter Hutterer --- dix/dispatch.c | 4 +++- hw/xfree86/common/xf86Config.c | 8 ++++++++ hw/xfree86/man/xorg.conf.man | 2 ++ hw/xwayland/xwayland.pc.in | 1 + include/opaque.h | 2 ++ man/Xserver.man | 6 ++++++ os/utils.c | 9 +++++++++ 7 files changed, 31 insertions(+), 1 deletion(-) diff --git a/dix/dispatch.c b/dix/dispatch.c index 92be773e6c..9c26753a96 100644 --- a/dix/dispatch.c +++ b/dix/dispatch.c @@ -3772,7 +3772,9 @@ ProcEstablishConnection(ClientPtr client) prefix = (xConnClientPrefix *) ((char *) stuff + sz_xReq); - if ((client->req_len << 2) != sz_xReq + sz_xConnClientPrefix + + if (client->swapped && !AllowByteSwappedClients) { + reason = "Prohibited client endianess, see the Xserver man page "; + } else if ((client->req_len << 2) != sz_xReq + sz_xConnClientPrefix + pad_to_int32(prefix->nbytesAuthProto) + pad_to_int32(prefix->nbytesAuthString)) reason = "Bad length"; diff --git a/hw/xfree86/common/xf86Config.c b/hw/xfree86/common/xf86Config.c index 5d814c1485..41acb25aa2 100644 --- a/hw/xfree86/common/xf86Config.c +++ b/hw/xfree86/common/xf86Config.c @@ -646,6 +646,7 @@ typedef enum { FLAG_MAX_CLIENTS, FLAG_IGLX, FLAG_DEBUG, + FLAG_ALLOW_BYTE_SWAPPED_CLIENTS, } FlagValues; /** @@ -705,6 +706,8 @@ static OptionInfoRec FlagOptions[] = { {0}, FALSE}, {FLAG_DEBUG, "Debug", OPTV_STRING, {0}, FALSE}, + {FLAG_ALLOW_BYTE_SWAPPED_CLIENTS, "AllowByteSwappedClients", OPTV_BOOLEAN, + {0}, FALSE}, {-1, NULL, OPTV_NONE, {0}, FALSE}, }; @@ -746,6 +749,11 @@ configServerFlags(XF86ConfFlagsPtr flagsconf, XF86OptionPtr layoutopts) xf86Msg(X_CONFIG, "Ignoring ABI Version\n"); } + xf86GetOptValBool(FlagOptions, FLAG_ALLOW_BYTE_SWAPPED_CLIENTS, &AllowByteSwappedClients); + if (AllowByteSwappedClients) { + xf86Msg(X_CONFIG, "Allowing byte-swapped clients\n"); + } + if (xf86IsOptionSet(FlagOptions, FLAG_AUTO_ADD_DEVICES)) { xf86GetOptValBool(FlagOptions, FLAG_AUTO_ADD_DEVICES, &xf86Info.autoAddDevices); diff --git a/hw/xfree86/man/xorg.conf.man b/hw/xfree86/man/xorg.conf.man index 01b47247ee..d057f26ecd 100644 --- a/hw/xfree86/man/xorg.conf.man +++ b/hw/xfree86/man/xorg.conf.man @@ -677,6 +677,8 @@ Possible values are or .BR sync . Unset by default. +.BI "Option \*qAllowByteSwappedClients\*q \*q" boolean \*q +Allow clients with a different byte-order than the server. Disabled by default. .SH "MODULE SECTION" The .B Module diff --git a/hw/xwayland/xwayland.pc.in b/hw/xwayland/xwayland.pc.in index c62a95a023..d65d52e51a 100644 --- a/hw/xwayland/xwayland.pc.in +++ b/hw/xwayland/xwayland.pc.in @@ -17,3 +17,4 @@ have_geometry=true have_fullscreen=true have_host_grab=true have_decorate=@have_libdecor@ +have_byteswappedclients=true diff --git a/include/opaque.h b/include/opaque.h index 256261c2ad..398d4b4e51 100644 --- a/include/opaque.h +++ b/include/opaque.h @@ -74,4 +74,6 @@ extern _X_EXPORT Bool bgNoneRoot; extern _X_EXPORT Bool CoreDump; extern _X_EXPORT Bool NoListenAll; +extern _X_EXPORT Bool AllowByteSwappedClients; + #endif /* OPAQUE_H */ diff --git a/man/Xserver.man b/man/Xserver.man index 764bd1d907..e7adf9eb35 100644 --- a/man/Xserver.man +++ b/man/Xserver.man @@ -114,6 +114,12 @@ pattern. This is the default unless -retro or -wr is specified. .B \-bs disables backing store support on all screens. .TP 8 +.B \+byteswappedclients +Allow connections from clients with an endianess different to that of the server. +.TP 8 +.B \-byteswappedclients +Prohibit connections from clients with an endianess different to that of the server. +.TP 8 .B \-c turns off key-click. .TP 8 diff --git a/os/utils.c b/os/utils.c index fe94912f34..405bf7d8b4 100644 --- a/os/utils.c +++ b/os/utils.c @@ -189,6 +189,8 @@ Bool CoreDump; Bool enableIndirectGLX = FALSE; +Bool AllowByteSwappedClients = FALSE; + #ifdef PANORAMIX Bool PanoramiXExtensionDisabledHack = FALSE; #endif @@ -523,6 +525,8 @@ UseMsg(void) ErrorF("-br create root window with black background\n"); ErrorF("+bs enable any backing store support\n"); ErrorF("-bs disable any backing store support\n"); + ErrorF("+byteswappedclients Allow clients with endianess different to that of the server\n"); + ErrorF("-byteswappedclients Prohibit clients with endianess different to that of the server\n"); ErrorF("-c turns off key-click\n"); ErrorF("c # key-click volume (0-100)\n"); ErrorF("-cc int default color visual class\n"); @@ -720,6 +724,11 @@ ProcessCommandLine(int argc, char *argv[]) else UseMsg(); } + else if (strcmp(argv[i], "-byteswappedclients") == 0) { + AllowByteSwappedClients = FALSE; + } else if (strcmp(argv[i], "+byteswappedclients") == 0) { + AllowByteSwappedClients = TRUE; + } else if (strcmp(argv[i], "-br") == 0); /* default */ else if (strcmp(argv[i], "+bs") == 0) enableBackingStore = TRUE;