Hi, On 03/11/2014 07:16 AM, Peter Hutterer wrote: > On Mon, Mar 10, 2014 at 10:21:39PM +0100, Hans de Goede wrote: >> With the recent systemd-logind changes it is possible to install the Xorg >> binary without suid root rights and still have everything working as it >> should *if* the user only has cards which are supported by kms. >> >> This commit adds a little suid root wrapper, which is a bit weird, first we >> strip the suid-root bit of the Xorg binary, and then we add a wrapper ? >> >> The function of this wrapper is to see if a system still needs root-rights, >> if it does not (it supports kms and the kms drivers are properly loaded), >> then it will immediately drop all elevated rights before executing the real >> Xorg binary. If it finds (some) cards which don't support kms, or no cards >> at all, then it will execute the Xorg server with elevated rights so that >> ie the nvidia binary driver and the vesa driver can keep working normally. >> >> To make it possible for security concious users who don't need the root >> rights to completely remove the wrapper, Xorg is started in a 3 step process >> when the wrapper is enabled during build time: >> >> 1) A simple shell script which checks if the wrapper is there, if it is >> it executes the wrapper, if not it directly executes the real Xorg binary >> >> 2) The wrapper gets executed, does its checks, normally drops all elevated >> rights and then executes the real Xorg binary >> >> 3) The real Xorg binary does its thing >> >> This allows distributions to put the wrapper binary in a separate package, >> and >> will allow users to remove this package. IE the plan with Fedora is to make >> "legacy" drivers depend on the wrapper pkg, and since our default install >> contains some legacy drivers it will be part of the default install, but >> users can later yum remove it (which will also automatically remove the >> legacy driver packages as those won't work without it anyways). >> >> The wrapper is loosely modelled after the existing Debian Xwrapper, it >> uses the same config-file + config-file format, and also allows restricting >> Xserver execution (through the wrapper) to console users only. >> >> There also is a new needs_root_rights config file directive, which can >> be used to override the auto-detection the wrapper does. >> >> Hopefully this will allow Debian to replace their own wrapper with this >> upstream one. >> >> Signed-off-by: Hans de Goede <[email protected]> >> --- >> configure.ac | 14 ++- >> hw/xfree86/Makefile.am | 15 ++- >> hw/xfree86/Xorg.sh.in | 12 ++ >> hw/xfree86/man/Makefile.am | 5 + >> hw/xfree86/man/Xorg.wrap.man | 51 ++++++++ >> hw/xfree86/man/Xwrapper.config.man | 66 +++++++++++ >> hw/xfree86/xorg-wrapper.c | 231 >> +++++++++++++++++++++++++++++++++++++ >> include/dix-config.h.in | 6 + >> include/xwin-config.h.in | 3 - >> manpages.am | 1 + >> 10 files changed, 398 insertions(+), 6 deletions(-) >> create mode 100644 hw/xfree86/Xorg.sh.in >> create mode 100644 hw/xfree86/man/Xorg.wrap.man >> create mode 100644 hw/xfree86/man/Xwrapper.config.man >> create mode 100644 hw/xfree86/xorg-wrapper.c >> >> diff --git a/configure.ac b/configure.ac >> index 162c0cf..749fa84 100644 >> --- a/configure.ac >> +++ b/configure.ac >> @@ -627,6 +627,7 @@ AC_ARG_ENABLE(pciaccess, >> AS_HELP_STRING([--enable-pciaccess], [Build Xorg with p >> AC_ARG_ENABLE(linux_acpi, AS_HELP_STRING([--disable-linux-acpi], [Disable >> building ACPI support on Linux (if available).]), >> [enable_linux_acpi=$enableval], [enable_linux_acpi=yes]) >> AC_ARG_ENABLE(linux_apm, AS_HELP_STRING([--disable-linux-apm], [Disable >> building APM support on Linux (if available).]), >> [enable_linux_apm=$enableval], [enable_linux_apm=yes]) >> AC_ARG_ENABLE(systemd-logind, AC_HELP_STRING([--enable-systemd-logind], >> [Build systemd-logind support (default: auto)]), >> [SYSTEMD_LOGIND=$enableval], [SYSTEMD_LOGIND=auto]) >> +AC_ARG_ENABLE(suid-wrapper, AC_HELP_STRING([--enable-suid-wrapper], [Build >> suid-root wrapper for legacy driver support on rootless xserver systems >> (default: no)]), [SUID_WRAPPER=$enableval], [SUID_WRAPPER=no]) >> >> dnl DDXes. >> AC_ARG_ENABLE(xorg, AS_HELP_STRING([--enable-xorg], [Build >> Xorg server (default: auto)]), [XORG=$enableval], [XORG=auto]) >> @@ -924,6 +925,16 @@ if test "x$SYSTEMD_LOGIND" = xyes; then >> fi >> AM_CONDITIONAL(SYSTEMD_LOGIND, [test "x$SYSTEMD_LOGIND" = xyes]) >> >> +if test "x$SUID_WRAPPER" = xyes; then >> + dnl The wrapper uses libdrm headers, so ensure they are available >> + PKG_CHECK_MODULES([LIBDRM], $LIBDRM) >> + dnl This is a define so that if some platforms want to put the >> wrapper >> + dnl somewhere else this can be easily changed >> + AC_DEFINE_DIR(SUID_WRAPPER_DIR, libexecdir, [Where to install >> Xorg.bin and Xorg.wrap]) >> + SETUID="no" >> +fi >> +AM_CONDITIONAL(SUID_WRAPPER, [test "x$SUID_WRAPPER" = xyes]) >> + >> if test "x$NEED_DBUS" = xyes; then >> AC_DEFINE(NEED_DBUS, 1, [Enable D-Bus core]) >> fi >> @@ -2116,7 +2127,6 @@ fi >> AC_MSG_RESULT([$XWIN]) >> >> if test "x$XWIN" = xyes; then >> - AC_DEFINE_DIR(SYSCONFDIR, sysconfdir, [Location of system.XWinrc]) >> AC_DEFINE_DIR(DEFAULT_LOGDIR, logdir, [Default log location]) >> AC_DEFINE_UNQUOTED(XORG_VERSION_CURRENT, [$VENDOR_RELEASE], [Current >> Xorg version]) >> AC_DEFINE_UNQUOTED(__VENDORDWEBSUPPORT__, ["$VENDOR_WEB"], [Vendor web >> address for support]) >> @@ -2440,6 +2450,7 @@ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[ >> [ Enable GNU and other extensions to the C environment for glibc])]) >> >> AC_DEFINE_DIR(PROJECTROOT, prefix, [Overall prefix]) >> +AC_DEFINE_DIR(SYSCONFDIR, sysconfdir, [sysconfdir]) >> >> AC_SUBST([RELEASE_DATE]) >> BUILD_DATE="`date +'%Y%m%d'`" >> @@ -2499,6 +2510,7 @@ dri3/Makefile >> present/Makefile >> hw/Makefile >> hw/xfree86/Makefile >> +hw/xfree86/Xorg.sh >> hw/xfree86/common/Makefile >> hw/xfree86/common/xf86Build.h >> hw/xfree86/ddc/Makefile >> diff --git a/hw/xfree86/Makefile.am b/hw/xfree86/Makefile.am >> index 73e1b4c..a315bbc 100644 >> --- a/hw/xfree86/Makefile.am >> +++ b/hw/xfree86/Makefile.am >> @@ -81,9 +81,15 @@ Xorg_DEPENDENCIES = $(LOCAL_LIBS) >> >> Xorg_LDFLAGS = $(LD_EXPORT_SYMBOLS_FLAG) >> >> +if SUID_WRAPPER >> +wrapdir = $(SUID_WRAPPER_DIR) >> +wrap_PROGRAMS = Xorg.wrap >> +Xorg_wrap_SOURCES = xorg-wrapper.c >> +endif >> + >> BUILT_SOURCES = xorg.conf.example >> DISTCLEANFILES = xorg.conf.example >> -EXTRA_DIST = xorgconf.cpp >> +EXTRA_DIST = xorgconf.cpp Xorg.sh.in >> >> # Without logdir, X will post an error on the terminal and will not start >> install-data-local: >> @@ -98,6 +104,11 @@ if INSTALL_SETUID >> chown root $(DESTDIR)$(bindir)/Xorg >> chmod u+s $(DESTDIR)$(bindir)/Xorg >> endif >> +if SUID_WRAPPER >> + mv $(DESTDIR)$(bindir)/Xorg $(DESTDIR)$(SUID_WRAPPER_DIR)/Xorg.bin >> + ${INSTALL} -m 755 Xorg.sh $(DESTDIR)$(bindir)/Xorg >> + -chown root $(DESTDIR)$(SUID_WRAPPER_DIR)/Xorg.wrap && chmod u+s >> $(DESTDIR)$(SUID_WRAPPER_DIR)/Xorg.wrap >> +endif >> >> uninstall-local: >> if CYGWIN >> @@ -119,7 +130,7 @@ xorg.conf.example: xorgconf.cpp >> relink: >> $(AM_V_at)rm -f Xorg$(EXEEXT) && $(MAKE) Xorg$(EXEEXT) >> >> -CLEANFILES = sdksyms.c sdksyms.dep >> +CLEANFILES = sdksyms.c sdksyms.dep Xorg.sh >> EXTRA_DIST += sdksyms.sh >> >> sdksyms.dep sdksyms.c: sdksyms.sh >> diff --git a/hw/xfree86/Xorg.sh.in b/hw/xfree86/Xorg.sh.in >> new file mode 100644 >> index 0000000..1356e11 >> --- /dev/null >> +++ b/hw/xfree86/Xorg.sh.in >> @@ -0,0 +1,12 @@ >> +#!/bin/sh >> +# >> +# See if Xorg.wrap is installed and if it is execute it, otherwise execute >> +# Xorg.bin directly. This allows distros to put the suid wrapper in a >> +# separate package. > > fwiw, this may be better rewritten as "execute Xorg.wrap if it exists, > otherwise ..." > >> + >> +basedir=@SUID_WRAPPER_DIR@ >> +if [ -x "$basedir"/Xorg.wrap ]; then >> + exec "$basedir"/Xorg.wrap "$@" >> +else >> + exec "$basedir"/Xorg.bin "$@" >> +fi >> diff --git a/hw/xfree86/man/Makefile.am b/hw/xfree86/man/Makefile.am >> index 80e22cb..f41d26c 100644 >> --- a/hw/xfree86/man/Makefile.am >> +++ b/hw/xfree86/man/Makefile.am >> @@ -1,3 +1,8 @@ >> include $(top_srcdir)/manpages.am >> appman_PRE = Xorg.man >> fileman_PRE = xorg.conf.man xorg.conf.d.man >> + >> +if SUID_WRAPPER >> +appman_PRE += Xorg.wrap.man >> +fileman_PRE += Xwrapper.config.man >> +endif >> diff --git a/hw/xfree86/man/Xorg.wrap.man b/hw/xfree86/man/Xorg.wrap.man >> new file mode 100644 >> index 0000000..cc62b5b >> --- /dev/null >> +++ b/hw/xfree86/man/Xorg.wrap.man >> @@ -0,0 +1,51 @@ >> +.\" Xwrapper.wrap.1 >> +.\" >> +.\" Copyright 2014 Red Hat, Inc. >> +.\" >> +.\" Permission to use, copy, modify, distribute, and sell this software and >> its >> +.\" documentation for any purpose is hereby granted without fee, provided >> that >> +.\" the above copyright notice appear in all copies and that both that >> +.\" copyright notice and this permission notice appear in supporting >> +.\" documentation. >> +.\" >> +.\" The above copyright notice and this permission notice shall be included >> +.\" in all copies or substantial portions of the Software. >> +.\" >> +.\" THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS >> +.\" OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF >> +.\" MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. >> +.\" IN NO EVENT SHALL THE OPEN GROUP BE LIABLE FOR ANY CLAIM, DAMAGES OR >> +.\" OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, >> +.\" ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR >> +.\" OTHER DEALINGS IN THE SOFTWARE. >> +.\" >> +.\" Except as contained in this notice, the name of The Open Group shall >> +.\" not be used in advertising or otherwise to promote the sale, use or >> +.\" other dealings in this Software without prior written authorization >> +.\" from The Open Group. >> +.\" >> +.\" shorthand for double quote that works everywhere. >> +.ds q \N'34' >> +.TH Xorg.wrap 1 __xorgversion__ >> +.SH NAME >> +Xorg.wrap \- Xorg xserver binary wrapper > > run a s/xserver/X server/ on the two man pages please, for consistency with > Xorg(1) > >> +.SH DESCRIPTION >> +The Xorg xserver may need root rights to function properly. To start the >> +Xorg xserver with these rights your system is using a suid root wrapper >> +installed as __suid_wrapper_dir__/Xorg.wrap which will execute the real >> +xserver which is installed as __suid_wrapper_dir__/Xorg.bin . >> +.PP >> +By default Xorg.wrap will autodetect if root rights are necessary, and >> +if not it will drop its elevated rights before starting the real xserver. >> +By default Xorg.wrap will only allow executing the real xserver from login >> +sessions on a physical console. This behavior can be overridden from the >> +\fI__sysconfdir__/X11/Xwrapper.config\fP config file. >> +.PP >> +When auto-detecting the wrapper will drop rights if kms graphics are >> available >> +and not drop them if no kms graphics are detected. If a system has multiple >> +graphics cards and some are not kms capable auto-detection may fail, >> +in this case manual configuration should be used. >> + >> +.SH "SEE ALSO" >> +Xorg.wrap configuration file: \fIXwrapper.config\fP(5) >> +Xorg xserver information: \fIXorg\fP(1) >> diff --git a/hw/xfree86/man/Xwrapper.config.man >> b/hw/xfree86/man/Xwrapper.config.man >> new file mode 100644 >> index 0000000..07ecbc7 >> --- /dev/null >> +++ b/hw/xfree86/man/Xwrapper.config.man >> @@ -0,0 +1,66 @@ >> +.\" Xwrapper.config.5 >> +.\" >> +.\" Copyright 2014 Red Hat, Inc. >> +.\" >> +.\" Permission to use, copy, modify, distribute, and sell this software and >> its >> +.\" documentation for any purpose is hereby granted without fee, provided >> that >> +.\" the above copyright notice appear in all copies and that both that >> +.\" copyright notice and this permission notice appear in supporting >> +.\" documentation. >> +.\" >> +.\" The above copyright notice and this permission notice shall be included >> +.\" in all copies or substantial portions of the Software. >> +.\" >> +.\" THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS >> +.\" OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF >> +.\" MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. >> +.\" IN NO EVENT SHALL THE OPEN GROUP BE LIABLE FOR ANY CLAIM, DAMAGES OR >> +.\" OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, >> +.\" ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR >> +.\" OTHER DEALINGS IN THE SOFTWARE. >> +.\" >> +.\" Except as contained in this notice, the name of The Open Group shall >> +.\" not be used in advertising or otherwise to promote the sale, use or >> +.\" other dealings in this Software without prior written authorization >> +.\" from The Open Group. >> +.\" >> +.\" shorthand for double quote that works everywhere. >> +.ds q \N'34' >> +.TH Xwrapper.config 5 __xorgversion__ >> +.SH NAME >> +Xwrapper.config \- Xorg xserver binary wrapper config file >> +.SH DESCRIPTION >> +The Xorg xserver may need root rights to function properly. To start the >> +Xorg xserver with these rights your system is using a suid root wrapper >> +installed as __suid_wrapper_dir__/Xorg.wrap which will execute the real >> +xserver which is installed as __suid_wrapper_dir__/Xorg.bin . >> +.PP >> +By default Xorg.wrap will autodetect if root rights are necessary, and >> +if not it will drop its elevated rights before starting the real xserver. >> +By default Xorg.wrap will only allow executing the real xserver from login >> +sessions on a physical console. This behavior can be overridden from the >> +\fI__sysconfdir__/X11/Xwrapper.config\fP config file. > > Given how bad we have been in the past in keeping man pages up-to-date I > strongly suggest to merge the two pages into one and simply alias it for > Xwrapper.config(5). The description is already the same anyway. > >> + >> +.SH OPTIONS >> +Lines starting with a \fB#\fP in Xwrapper.config are considered comments >> +and will be ignored any other non empty lines must take the form of >> \fBkey\fP = > > missing "." after "ignored". > >> +\fIvalue\fP. >> +.TP 8 >> +\fBallowed_users\fP = \fIrootonly\fP|\fIconsole\fP|\fIanybody\fP >> +Specify which users may start the xserver through the wrapper. Use >> +\fIrootonly\fP to only allow root, use \fIconsole\fP to only allow users >> +logged into a physical console, and use \fIanybody\fP to allow anybody. > > "the default is ..." > >> +.TP 8 >> +\fBneeds_root_rights\fP = \fIyes\fP|\fIno\fP|\fIauto\fP >> +Configure if the wrapper should drop its elevated (root) rights before >> starting >> +the xserver. Use \fIyes\fP to force execution as root, \fIno\fP to force >> +execution with all suid rights dropped, and \fIauto\fP to letter the wrapper >> +auto-detect. >> +.PP >> +When auto-detecting the wrapper will drop rights if kms graphics are >> available >> +and not drop them if no kms graphics are detected. If a system has multiple >> +graphics cards and some are not kms capable auto-detection may fail, >> +in this case manual configuration should be used. >> + >> +.SH "SEE ALSO" >> +Xorg xserver information: \fIXorg\fP(1) >> diff --git a/hw/xfree86/xorg-wrapper.c b/hw/xfree86/xorg-wrapper.c >> new file mode 100644 >> index 0000000..221f1b6 >> --- /dev/null >> +++ b/hw/xfree86/xorg-wrapper.c >> @@ -0,0 +1,231 @@ >> +/* >> + * Copyright © 2014 Red Hat, Inc. >> + * >> + * Permission is hereby granted, free of charge, to any person obtaining a >> + * copy of this software and associated documentation files (the >> "Software"), >> + * to deal in the Software without restriction, including without limitation >> + * the rights to use, copy, modify, merge, publish, distribute, sublicense, >> + * and/or sell copies of the Software, and to permit persons to whom the >> + * Software is furnished to do so, subject to the following conditions: >> + * >> + * The above copyright notice and this permission notice (including the next >> + * paragraph) shall be included in all copies or substantial portions of the >> + * Software. >> + * >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS >> OR >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR >> OTHER >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING >> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER >> + * DEALINGS IN THE SOFTWARE. >> + * >> + * Author: Hans de Goede <[email protected]> >> + */ >> + >> +#include "dix-config.h" >> + >> +#include <fcntl.h> >> +#include <limits.h> >> +#include <stdint.h> >> +#include <stdio.h> >> +#include <stdlib.h> >> +#include <string.h> >> +#include <sys/ioctl.h> >> +#include <sys/stat.h> >> +#include <sys/types.h> >> +#include <unistd.h> >> +#include <drm/drm.h> >> +#include <xf86drm.h> /* For DRM_DEV_NAME */ >> + >> +#define CONFIG_FILE SYSCONFDIR "/X11/Xwrapper.config" >> + >> +enum { ROOT_ONLY, CONSOLE_ONLY, ANYBODY }; >> + >> +/* KISS non locale / LANG parsing isspace version */ >> +static int is_space(char c) >> +{ >> + return c == ' ' || c == '\t' || c == '\n'; >> +} >> + >> +static char *strip(char *s) >> +{ >> + int i; >> + >> + /* Strip leading whitespace */ >> + while (s[0] && is_space(s[0])) >> + s++; >> + >> + /* Strip trailing whitespace */ >> + i = strlen(s) - 1; >> + while (i >= 0 && is_space(s[i])) { >> + s[i] = 0; >> + i--; >> + } >> + >> + return s; >> +} >> + >> +static void parse_config(int *allowed, int *needs_root_rights) >> +{ >> + FILE *f; >> + char buf[1024]; >> + char *stripped, *equals, *key, *value; >> + int line = 0; >> + >> + f = fopen(CONFIG_FILE, "r"); >> + if (!f) >> + return; >> + >> + while (fgets(buf, sizeof(buf), f)) { >> + line++; >> + >> + /* Skip comments and empty lines */ >> + stripped = strip(buf); >> + if (stripped[0] == '#' || stripped[0] == 0) >> + continue; >> + >> + /* Split in a key + value pair */ >> + equals = strchr(stripped, '='); >> + if (!equals) { >> + fprintf(stderr, "Syntax error at %s line %d\n", CONFIG_FILE, >> line); >> + exit(1); >> + } >> + *equals = 0; >> + key = strip(stripped); /* To remove trailing whitespace from >> key */ >> + value = strip(equals + 1); /* To remove leading whitespace from val >> */ >> + if (!key[0]) { >> + fprintf(stderr, "Missing key at %s line %d\n", CONFIG_FILE, >> line); >> + exit(1); >> + } >> + if (!value[0]) { >> + fprintf(stderr, "Missing value at %s line %d\n", CONFIG_FILE, >> line); >> + exit(1); >> + } >> + >> + /* And finally process */ >> + if (strcmp(key, "allowed_users") == 0) { >> + if (strcmp(value, "rootonly") == 0) >> + *allowed = ROOT_ONLY; >> + else if (strcmp(value, "console") == 0) >> + *allowed = CONSOLE_ONLY; >> + else if (strcmp(value, "anybody") == 0) >> + *allowed = ANYBODY; >> + else { >> + fprintf(stderr, >> + "Unknown value '%s' for 'allowed_users' at %s line >> %d\n", > > maybe "Invalid" instead of Unknown, since you use that a few lines down. > >> + value, CONFIG_FILE, line); >> + exit(1); >> + } >> + } >> + else if (strcmp(key, "needs_root_rights") == 0) { >> + if (strcmp(value, "yes") == 0) >> + *needs_root_rights = 1; >> + else if (strcmp(value, "no") == 0) >> + *needs_root_rights = 0; >> + else if (strcmp(value, "auto") == 0) >> + *needs_root_rights = -1; >> + else { >> + fprintf(stderr, >> + "Invalid value '%s' for 'needs_root_rights' at %s line >> %d\n", >> + value, CONFIG_FILE, line); >> + exit(1); >> + } >> + } >> + else if (strcmp(key, "nice_value") == 0) { >> + /* Backward compatibility with older Debian Xwrapper, ignore */ >> + } >> + else { >> + fprintf(stderr, "Unknown key '%s' at %s line %d\n", key, > > same here, I'd prefer "invalid" over unknown. > > for the rest: > Reviewed-by: Peter Hutterer <[email protected]> >
Thanks for the review, I've made all suggested changes and added your Reviewed-by to the commit message. Regards, Hans _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
