[PATCH] Xorg: Add a suid root wrapper
Hans de Goede
hdegoede at redhat.com
Mon Mar 10 14:20:36 PDT 2014
Hi,
On 03/10/2014 07:03 AM, Peter Hutterer wrote:
> On Fri, Mar 07, 2014 at 11:38:52AM +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
>
> just thinking aloud here:
> I wonder if we need a three-step process here, specifically if we need the
> first shell script. Would a solution be useful where the wrapper simply
> checks for its own permissions and skips most of the code if it doesn't have
> root privs?
>
> I guess this may be bad from a packaging POV because we'd have two packages
> providing the same file, once with suid root set, once without.
> or it could be an empty package that just triggers chmod.
Doing a chmod from an rpm scriptlet will break rpm --verify (and similar problems
will happen with other package managers). Basically doing a chmod from a post
install script is a big no no for any package.
Likewise having 2 packages providing the same binary and conflicting with each
other is also quite ugly and far from ideal.
The only alternative to the shell wrapper would be using alternatives. But that
is a bit heavy weight for this. I really don't see the shell wrapper as a
big issue. If the xserver were something which got started multiple times in
a second, sure then it would be an issue. But the xserver gets (re)started
quite seldomly, so the performance impact is neglible. And it does what we
want in a very KISS manner.
>> 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 <hdegoede at redhat.com>
>> ---
>> configure.ac | 7 ++
>> hw/xfree86/Makefile.am | 14 ++-
>> hw/xfree86/Xorg.sh.in | 12 +++
>> hw/xfree86/xorg-wrapper.c | 211 ++++++++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 242 insertions(+), 2 deletions(-)
>> create mode 100644 hw/xfree86/Xorg.sh.in
>> create mode 100644 hw/xfree86/xorg-wrapper.c
>>
>> diff --git a/configure.ac b/configure.ac
>> index 588ae59..6028ab1 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])
>> @@ -922,6 +923,11 @@ if test "x$SYSTEMD_LOGIND" = xyes; then
>> fi
>> AM_CONDITIONAL(SYSTEMD_LOGIND, [test "x$SYSTEMD_LOGIND" = xyes])
>>
>> +if test "x$SUID_WRAPPER" = xyes; then
>> + 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
>> @@ -2486,6 +2492,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 9672904..59ba9d8 100644
>> --- a/hw/xfree86/Makefile.am
>> +++ b/hw/xfree86/Makefile.am
>> @@ -76,9 +76,14 @@ Xorg_DEPENDENCIES = $(LOCAL_LIBS)
>>
>> Xorg_LDFLAGS = $(LD_EXPORT_SYMBOLS_FLAG)
>>
>> +if SUID_WRAPPER
>> +bin_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:
>> @@ -93,6 +98,11 @@ if INSTALL_SETUID
>> chown root $(DESTDIR)$(bindir)/Xorg
>> chmod u+s $(DESTDIR)$(bindir)/Xorg
>> endif
>> +if SUID_WRAPPER
>> + mv $(DESTDIR)$(bindir)/Xorg $(DESTDIR)$(bindir)/Xorg.bin
>> + ${INSTALL} -m 755 Xorg.sh $(DESTDIR)$(bindir)/Xorg
>> + -chown root $(DESTDIR)$(bindir)/Xorg.wrap && chmod u+s $(DESTDIR)$(bindir)/Xorg.wrap
>> +endif
>>
>> uninstall-local:
>> if CYGWIN
>> @@ -114,7 +124,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..99f1d5b
>> --- /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.
>> +
>> +bindir=@PROJECTROOT@/bin
>> +if [ -x "$bindir"/Xorg.wrap ]; then
>> + exec "$bindir"/Xorg.wrap "$@"
>> +else
>> + exec "$bindir"/Xorg.bin "$@"
>> +fi
>
> I think libexecdir would be the best directory to put the wrapper script and
> the binary.
Fixed for v2.
>> diff --git a/hw/xfree86/xorg-wrapper.c b/hw/xfree86/xorg-wrapper.c
>> new file mode 100644
>> index 0000000..9e9bc45
>> --- /dev/null
>> +++ b/hw/xfree86/xorg-wrapper.c
>> @@ -0,0 +1,211 @@
>> +/*
>> + * 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 <hdegoede at redhat.com>
>> + */
>> +
>> +#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>
>> +
>> +#define CONFIG_FILE "/etc/X11/Xwrapper.config"
>
> this should probably use SYSCONFDIR
Fixed for v2.
>> +
>> +enum { ROOT_ONLY, CONSOLE_ONLY, ANYBODY };
>> +
>> +static int allowed = CONSOLE_ONLY;
>> +static int needs_root_rights = -1;
>> +
>> +/* 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(void)
>> +{
>> + 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 */
>> +
>> + /* 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",
>> + value, CONFIG_FILE, line);
>> + exit(1);
>> + }
>> + }
>> + else if (strcmp(key, "needs_root_rights") == 0) {
>> + needs_root_rights = atoi(value);
>
> I'd use strtol here, because otherwise "y", "yes", "true", "prettyplease",
> etc. all resolve to false. or, my preference, strcmp to "1" and "0" and
> whatever else you want to support and bail out with an error otherwise.
Fixed for v2.
> btw: a man page describing what this wrapper does and what keywords are
> allowed is definitely needed.
Agreed, I've added manpages for both to v2.
>
>> + }
>> + else if (strcmp(key, "nice_value") == 0) {
>> + /* Backwart compat with older Debian Xwrapper, ignore */
>
> typo: backward.
>
Fixed for v2.
>> + }
>> + else {
>> + fprintf(stderr, "Unknown keyword '%s' at %s line %d\n", key,
>> + CONFIG_FILE, line);
>> + exit(1);
>> + }
>> + }
>> + fclose(f);
>> +}
>> +
>> +int main(int argc, char *argv[])
>> +{
>> + struct drm_mode_card_res res;
>> + struct stat st;
>> + char buf[PATH_MAX];
>> + int i, r, fd;
>> + int kms_cards = 0;
>> + int total_cards = 0;
>> +
>> + parse_config();
>
> fwiw, parse_config is simple enough and only has two outputs, so instead of
> having global variables you could just return them here. Less chance of
> surprise.
Fixed for v2.
>> +
>> + /* For non root users check if they are allowed to run the X server */
>> + if (getuid() != 0) {
>> + switch (allowed) {
>> + case ROOT_ONLY:
>> + /* Already checked above */
>> + fprintf(stderr, "%s: Only root is allowed to run the X server\n", argv[0]);
>> + exit(1);
>> + break;
>> + case CONSOLE_ONLY:
>> + /* Some of stdin / stdout / stderr maybe redirected to a file */
>> + for (i = STDIN_FILENO; i <= STDERR_FILENO; i++) {
>> + r = fstat(i, &st);
>> + if (r == 0 && S_ISCHR(st.st_mode) && major(st.st_rdev) == 4)
>> + break;
>> + }
>
> I honestly don't know this: what's the difference to this approach vs using
> isatty() and is one preferred over the other?
isatty will return true for any tty, also for serial console's and pty's (so ssh sessions
and xterms), where as we want to test for the tty being a vc here.
>
>> + if (i > STDERR_FILENO) {
>> + fprintf(stderr, "%s: Only console users are allowed to run the X server\n", argv[0]);
>> + exit(1);
>> + }
>
> please add a break statement here, no effect, but it does avoid potential confusion.
Fixed for v2.
>
>> + case ANYBODY:
>> + break;
>> + }
>> + }
>> +
>> + /* Detect if we need root rights, except when overriden by the config */
>> + if (needs_root_rights == -1) {
>> + for (i = 0; i < 16; i++) {
>> + snprintf(buf, PATH_MAX, "/dev/dri/card%d", i);
>
> sizeof(buf) please, even if it has no effective difference here.
> also, should we change this to a #define, Mark said that there are other
> paths on other systems?
Both fixed for v2.
>
>> + fd = open(buf, O_RDWR);
>> + if (fd == -1)
>> + continue;
>> +
>> + total_cards++;
>> +
>> + memset(&res, 0, sizeof(struct drm_mode_card_res));
>> + r = ioctl(fd, DRM_IOCTL_MODE_GETRESOURCES, &res);
>> + if (r == 0 && res.count_connectors > 0)
>> + kms_cards++;
>> +
>> + close(fd);
>> + }
>> + }
>> +
>> + /* If we've found cards, and all cards support kms, drop root rights */
>> + if (needs_root_rights == 0 || (total_cards && kms_cards == total_cards)) {
>> + gid_t realgid = getgid();
>> + uid_t realuid = getuid();
>> +
>> + if (setresgid(-1, realgid, realgid) != 0) {
>> + perror("Could not drop setgid privileges");
>> + exit(1);
>> + }
>> + if (setresuid(-1, realuid, realuid) != 0) {
>> + perror("Could not drop setuid privileges");
>> + exit(1);
>> + }
>> + }
>> +
>> + snprintf(buf, PATH_MAX, "%s/bin/Xorg.bin", PROJECTROOT);
>> +
>> + /* Check if the server is exetable by our real uid */
>
> typo: executable
>
>> + if (access(buf, X_OK) != 0) {
>> + perror("Missing execute permissions for " PROJECTROOT "/bin/Xorg.bin");
>> + exit(1);
>> + }
>> +
>> + argv[0] = buf;
>> + (void) execv(argv[0], argv);
>> + perror("Failed to execute " PROJECTROOT "/bin/Xorg.bin");
>> + exit(1);
>
> Having the paths hardcoded here will likely bite us. IMO we should
> just #define this in whatever config.h.in applies for this part of the
> server and let it be set by autotools.
Fixed for v2.
Thanks for the review!
Regards,
Hans
More information about the xorg-devel
mailing list