<div dir="ltr"><div><div><div><div>Sent that a little too soon; please consider "Reviewed-by: Ben Crocker <<a href="mailto:bcrocker@redhat.com">bcrocker@redhat.com</a>>" to be </div>added after each of the "Signed-off-by: Nicolai Haehnle <<a href="mailto:nicolai.haehnle@amd.com">nicolai.haehnle@amd.com</a>>" lines. </div>Also please note that I rebased Nicolai's original patch, <a href="https://patchwork.freedesktop.org/series/18684/">https://patchwork.freedesktop.org/series/18684/</a>, </div>against a very recent copy of master. </div> Thanks, <div> Ben </div></div><div class="gmail_extra"> <div class="gmail_quote">On Thu, Mar 8, 2018 at 4:53 PM, Ben Crocker <<a href="mailto:bcrocker@redhat.com" target="_blank">bcrocker@redhat.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">From: Nicolai Hähnle <<a href="mailto:nhaehnle@gmail.com">nhaehnle@gmail.com</a>> <div><div class="h5"> From: Nicolai Hähnle <<a href="mailto:nicolai.haehnle@amd.com">nicolai.haehnle@amd.com</a>> Having different types of code all trying to check for elevated privileges is a bad idea. This implementation is the most thorough one. Signed-off-by: Nicolai Hähnle <<a href="mailto:nicolai.haehnle@amd.com">nicolai.haehnle@amd.com</a>> --- hw/xfree86/common/xf86Init.c | 59 +---------------------------------------- include/os.h | 3 +++ os/utils.c | 63 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 58 deletions(-) diff --git a/hw/xfree86/common/xf86Init.c b/hw/xfree86/common/xf86Init.c index e61fe66..758b926 100644 --- a/hw/xfree86/common/xf86Init.c +++ b/hw/xfree86/common/xf86Init.c @@ -230,78 +230,21 @@ xf86PrintBanner(void) xf86ErrorFVerb(0, "Current version of pixman: %s\n", pixman_version_string()); xf86ErrorFVerb(0, "\tBefore reporting problems, check " "" __VENDORDWEBSUPPORT__ "\n" "\tto make sure that you have the latest version.\n"); } Bool xf86PrivsElevated(void) { - static Bool privsTested = FALSE; - static Bool privsElevated = TRUE; - - if (!privsTested) { -#if defined(WIN32) - privsElevated = FALSE; -#else - if ((getuid() != geteuid()) || (getgid() != getegid())) { - privsElevated = TRUE; - } - else { -#if defined(HAVE_ISSETUGID) - privsElevated = issetugid(); -#elif defined(HAVE_GETRESUID) - uid_t ruid, euid, suid; - gid_t rgid, egid, sgid; - - if ((getresuid(&ruid, &euid, &suid) == 0) && - (getresgid(&rgid, &egid, &sgid) == 0)) { - privsElevated = (euid != suid) || (egid != sgid); - } - else { - printf("Failed getresuid or getresgid"); - /* Something went wrong, make defensive assumption */ - privsElevated = TRUE; - } -#else - if (getuid() == 0) { - /* running as root: uid==euid==0 */ - privsElevated = FALSE; - } - else { - /* - * If there are saved ID's the process might still be privileged - * even though the above test succeeded. If issetugid() and - * getresgid() aren't available, test this by trying to set - * euid to 0. - */ - unsigned int oldeuid; - - oldeuid = geteuid(); - - if (seteuid(0) != 0) { - privsElevated = FALSE; - } - else { - if (seteuid(oldeuid) != 0) { - FatalError("Failed to drop privileges. Exiting\n"); - } - privsElevated = TRUE; - } - } -#endif - } -#endif - privsTested = TRUE; - } - return privsElevated; + return PrivsElevated(); } static void </div></div> TrapSignals(void) { if (xf86Info.notrapSignals) { OsSignal(SIGSEGV, SIG_DFL); OsSignal(SIGABRT, SIG_DFL); OsSignal(SIGILL, SIG_DFL); #ifdef SIGEMT <div><div class="h5">diff --git a/include/os.h b/include/os.h index d2c41b4..686f6d6 100644 --- a/include/os.h +++ b/include/os.h @@ -355,20 +355,23 @@ Fclose(void *); extern const char * Win32TempDir(void); extern int System(const char *cmdline); #define Fopen(a,b) fopen(a,b) #define Fclose(a) fclose(a) #endif +extern _X_EXPORT Bool +PrivsElevated(void); + extern _X_EXPORT void CheckUserParameters(int argc, char **argv, char **envp); extern _X_EXPORT void CheckUserAuthorization(void); extern _X_EXPORT int AddHost(ClientPtr /*client */ , int /*family */ , unsigned /*length */ , const void * /*pAddr */ ); diff --git a/os/utils.c b/os/utils.c index ac55cd7..024989e 100644 --- a/os/utils.c +++ b/os/utils.c @@ -1717,20 +1717,83 @@ System(const char *cmdline) /* Close process and thread handles. */ CloseHandle(pi.hProcess); CloseHandle(pi.hThread); free(cmd); return dwExitCode; } #endif +Bool +PrivsElevated(void) +{ + static Bool privsTested = FALSE; + static Bool privsElevated = TRUE; + + if (!privsTested) { +#if defined(WIN32) + privsElevated = FALSE; +#else + if ((getuid() != geteuid()) || (getgid() != getegid())) { + privsElevated = TRUE; + } + else { +#if defined(HAVE_ISSETUGID) + privsElevated = issetugid(); +#elif defined(HAVE_GETRESUID) + uid_t ruid, euid, suid; + gid_t rgid, egid, sgid; + + if ((getresuid(&ruid, &euid, &suid) == 0) && + (getresgid(&rgid, &egid, &sgid) == 0)) { + privsElevated = (euid != suid) || (egid != sgid); + } + else { + printf("Failed getresuid or getresgid"); + /* Something went wrong, make defensive assumption */ + privsElevated = TRUE; + } +#else + if (getuid() == 0) { + /* running as root: uid==euid==0 */ + privsElevated = FALSE; + } + else { + /* + * If there are saved ID's the process might still be privileged + * even though the above test succeeded. If issetugid() and + * getresgid() aren't available, test this by trying to set + * euid to 0. + */ + unsigned int oldeuid; + + oldeuid = geteuid(); + + if (seteuid(0) != 0) { + privsElevated = FALSE; + } + else { + if (seteuid(oldeuid) != 0) { + FatalError("Failed to drop privileges. Exiting\n"); + } + privsElevated = TRUE; + } + } +#endif + } +#endif + privsTested = TRUE; + } + return privsElevated; +} + /* * CheckUserParameters: check for long command line arguments and long * environment variables. By default, these checks are only done when * the server's euid != ruid. In 3.3.x, these checks were done in an * external wrapper utility. */ /* Consider LD* variables insecure? */ #ifndef REMOVE_ENV_LD #define REMOVE_ENV_LD 1 </div></div>From patchwork Fri Jan 27 13:37:36 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [xserver,2/4] os: use PrivsElevated instead of a manual check From: =?utf-8?q?Nicolai_H=C3=A4hnle?= <<a href="mailto:nhaehnle@gmail.com">nhaehnle@gmail.com</a>> X-Patchwork-Id: 135711 Message-Id: <<a href="mailto:1485524258-6482-3-git-send-email-nhaehnle@gmail.com">1485524258-6482-3-git-send-email-nhaehnle@gmail.com</a>> To: <a href="mailto:xorg-devel@lists.x.org">xorg-devel@lists.x.org</a> Cc: =?UTF-8?q?Nicolai=20H=C3=A4hnle?= <<a href="mailto:nicolai.haehnle@amd.com">nicolai.haehnle@amd.com</a>> Date: Fri, 27 Jan 2017 14:37:36 +0100 From: Nicolai Hähnle <<a href="mailto:nicolai.haehnle@amd.com">nicolai.haehnle@amd.com</a>> Signed-off-by: Nicolai Hähnle <<a href="mailto:nicolai.haehnle@amd.com">nicolai.haehnle@amd.com</a>> --- os/utils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/os/utils.c b/os/utils.c index 024989e..05733b0 100644 --- a/os/utils.c +++ b/os/utils.c @@ -1861,21 +1861,21 @@ enum BadCode { #endif void CheckUserParameters(int argc, char **argv, char **envp) { enum BadCode bad = NotBad; int i = 0, j; char *a, *e = NULL; #if CHECK_EUID - if (geteuid() == 0 && getuid() != geteuid()) + if (PrivsElevated()) #endif { /* Check each argv[] */ for (i = 1; i < argc; i++) { if (strcmp(argv[i], "-fp") == 0) { i++; /* continue with next argument. skip the length check */ if (i >= argc) break; } else { >From patchwork Fri Jan 27 13:37:37 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [xserver, 3/4] xfree86: replace all uses of xf86PrivsElevated with PrivsElevated From: =?utf-8?q?Nicolai_H=C3=A4hnle?= <<a href="mailto:nhaehnle@gmail.com">nhaehnle@gmail.com</a>> X-Patchwork-Id: 135712 Message-Id: <<a href="mailto:1485524258-6482-4-git-send-email-nhaehnle@gmail.com">1485524258-6482-4-git-send-email-nhaehnle@gmail.com</a>> To: <a href="mailto:xorg-devel@lists.x.org">xorg-devel@lists.x.org</a> Cc: =?UTF-8?q?Nicolai=20H=C3=A4hnle?= <<a href="mailto:nicolai.haehnle@amd.com">nicolai.haehnle@amd.com</a>> Date: Fri, 27 Jan 2017 14:37:37 +0100 From: Nicolai Hähnle <<a href="mailto:nicolai.haehnle@amd.com">nicolai.haehnle@amd.com</a>> Signed-off-by: Nicolai Hähnle <<a href="mailto:nicolai.haehnle@amd.com">nicolai.haehnle@amd.com</a>> --- hw/xfree86/common/xf86Config.c | 2 +- hw/xfree86/common/xf86Init.c | 12 +++--------- hw/xfree86/common/xf86Priv.h | 2 -- 3 files changed, 4 insertions(+), 12 deletions(-) diff --git a/hw/xfree86/common/xf86Config.c b/hw/xfree86/common/xf86Config.c index f03acf3..5d2cf0a 100644 --- a/hw/xfree86/common/xf86Config.c +++ b/hw/xfree86/common/xf86Config.c @@ -2309,21 +2309,21 @@ xf86HandleConfigFile(Bool autoconfig) #endif Bool implicit_layout = FALSE; XF86ConfLayoutPtr layout; if (!autoconfig) { char *filename, *dirname, *sysdirname; const char *filesearch, *dirsearch; MessageType filefrom = X_DEFAULT; MessageType dirfrom = X_DEFAULT; - if (!xf86PrivsElevated()) { + if (!PrivsElevated()) { filesearch = ALL_CONFIGPATH; dirsearch = ALL_CONFIGDIRPATH; } else { filesearch = RESTRICTED_CONFIGPATH; dirsearch = RESTRICTED_CONFIGDIRPATH; } if (xf86ConfigFile) filefrom = X_CMDLINE; diff --git a/hw/xfree86/common/xf86Init.c b/hw/xfree86/common/xf86Init.c index 758b926..deed91f 100644 --- a/hw/xfree86/common/xf86Init.c +++ b/hw/xfree86/common/xf86Init.c @@ -227,26 +227,20 @@ xf86PrintBanner(void) #if defined(BUILDERSTRING) xf86ErrorFVerb(0, "%s \n", BUILDERSTRING); #endif xf86ErrorFVerb(0, "Current version of pixman: %s\n", pixman_version_string()); xf86ErrorFVerb(0, "\tBefore reporting problems, check " "" __VENDORDWEBSUPPORT__ "\n" "\tto make sure that you have the latest version.\n"); } -Bool -xf86PrivsElevated(void) -{ - return PrivsElevated(); -} - static void TrapSignals(void) { if (xf86Info.notrapSignals) { OsSignal(SIGSEGV, SIG_DFL); OsSignal(SIGABRT, SIG_DFL); OsSignal(SIGILL, SIG_DFL); #ifdef SIGEMT OsSignal(SIGEMT, SIG_DFL); #endif @@ -886,21 +880,21 @@ OsVendorInit(void) /* Set stderr to non-blocking. */ #ifndef O_NONBLOCK #if defined(FNDELAY) #define O_NONBLOCK FNDELAY #elif defined(O_NDELAY) #define O_NONBLOCK O_NDELAY #endif #ifdef O_NONBLOCK if (!beenHere) { - if (xf86PrivsElevated()) { + if (PrivsElevated()) { int status; status = fcntl(fileno(stderr), F_GETFL, 0); if (status != -1) { fcntl(fileno(stderr), F_SETFL, status | O_NONBLOCK); } } } #endif #endif @@ -1041,21 +1035,21 @@ xf86PrintDefaultModulePath(void) static void xf86PrintDefaultLibraryPath(void) { ErrorF("%s\n", DEFAULT_LIBRARY_PATH); } static void xf86CheckPrivs(const char *option, const char *arg) { - if (xf86PrivsElevated() && !xf86PathIsSafe(arg)) { + if (PrivsElevated() && !xf86PathIsSafe(arg)) { FatalError("\nInvalid argument for %s - \"%s\"\n" "\tWith elevated privileges %s must specify a relative path\n" "\twithout any \"..\" elements.\n\n", option, arg, option); } } /* * ddxProcessArgument -- * Process device-dependent command line args. Returns 0 if argument is * not device dependent, otherwise Count of number of elements of argv @@ -1342,21 +1336,21 @@ ddxProcessArgument(int argc, char **argv, int i) * Print out correct use of device dependent commandline options. * Maybe the user now knows what really to do ... */ void ddxUseMsg(void) { ErrorF("\n"); ErrorF("\n"); ErrorF("Device Dependent Usage\n"); - if (!xf86PrivsElevated()) { + if (!PrivsElevated()) { ErrorF("-modulepath paths specify the module search path\n"); ErrorF("-logfile file specify a log file name\n"); ErrorF("-configure probe for devices and write an " XCONFIGFILE "\n"); ErrorF ("-showopts print available options for all installed drivers\n"); } ErrorF ("-config file specify a configuration file, relative to the\n"); ErrorF(" " XCONFIGFILE diff --git a/hw/xfree86/common/xf86Priv.h b/hw/xfree86/common/xf86Priv.h index c1f8a18..d78e8f6 100644 --- a/hw/xfree86/common/xf86Priv.h +++ b/hw/xfree86/common/xf86Priv.h @@ -154,16 +154,14 @@ xf86CloseLog(enum ExitCode error); /* xf86Init.c */ extern _X_EXPORT Bool xf86LoadModules(const char **list, void **optlist); extern _X_EXPORT int xf86SetVerbosity(int verb); extern _X_EXPORT int xf86SetLogVerbosity(int verb); extern _X_EXPORT Bool xf86CallDriverProbe(struct _DriverRec *drv, Bool detect_only); -extern _X_EXPORT Bool -xf86PrivsElevated(void); #endif /* _NO_XF86_PROTOTYPES */ #endif /* _XF86PRIV_H */ >From patchwork Fri Jan 27 13:37:38 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [xserver,4/4] glx: honor LIBGL_DRIVERS_PATH when loading DRI drivers From: =?utf-8?q?Nicolai_H=C3=A4hnle?= <<a href="mailto:nhaehnle@gmail.com">nhaehnle@gmail.com</a>> X-Patchwork-Id: 135713 Message-Id: <<a href="mailto:1485524258-6482-5-git-send-email-nhaehnle@gmail.com">1485524258-6482-5-git-send-email-nhaehnle@gmail.com</a>> To: <a href="mailto:xorg-devel@lists.x.org">xorg-devel@lists.x.org</a> Cc: =?UTF-8?q?Nicolai=20H=C3=A4hnle?= <<a href="mailto:nicolai.haehnle@amd.com">nicolai.haehnle@amd.com</a>> Date: Fri, 27 Jan 2017 14:37:38 +0100 From: Nicolai Hähnle <<a href="mailto:nicolai.haehnle@amd.com">nicolai.haehnle@amd.com</a>> Allow switching to another driver build without a full installation. Glamor already takes LIBGL_DRIVERS_PATH into account, so this change makes sure that the same driver is used in both parts of the server. Signed-off-by: Nicolai Hähnle <<a href="mailto:nicolai.haehnle@amd.com">nicolai.haehnle@amd.com</a>> --- glx/glxdricommon.c | 38 ++++++++++++++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/glx/glxdricommon.c b/glx/glxdricommon.c index f6c6fcd..a370845 100644 --- a/glx/glxdricommon.c +++ b/glx/glxdricommon.c @@ -241,28 +241,58 @@ static const char dri_driver_path[] = DRI_DRIVER_PATH; void * glxProbeDriver(const char *driverName, void **coreExt, const char *coreName, int coreVersion, void **renderExt, const char *renderName, int renderVersion) { int i; void *driver; char filename[PATH_MAX]; char *get_extensions_name; const __DRIextension **extensions = NULL; + const char *path = NULL; + + /* Search in LIBGL_DRIVERS_PATH if we're not setuid. */ + if (!PrivsElevated()) + path = getenv("LIBGL_DRIVERS_PATH"); + + if (!path) + path = dri_driver_path; + + do { + const char *next; + int path_len; + + next = strchr(path, ':'); + if (next) { + path_len = next - path; + next++; + } else { + path_len = strlen(path); + next = NULL; + } - snprintf(filename, sizeof filename, "%s/%s_dri.so", - dri_driver_path, driverName); + snprintf(filename, sizeof filename, "%.*s/%s_dri.so", path_len, path, + driverName); + + driver = dlopen(filename, RTLD_LAZY | RTLD_LOCAL); + if (driver != NULL) + break; - driver = dlopen(filename, RTLD_LAZY | RTLD_LOCAL); - if (driver == NULL) { LogMessage(X_ERROR, "AIGLX error: dlopen of %s failed (%s)\n", filename, dlerror()); + + path = next; + } while (path); + + if (driver == NULL) { + LogMessage(X_ERROR, "AIGLX error: unable to load driver %s\n", + driverName); goto cleanup_failure; } if (asprintf(&get_extensions_name, "%s_%s", __DRI_DRIVER_GET_EXTENSIONS, driverName) != -1) { const __DRIextension **(*get_extensions)(void); get_extensions = dlsym(driver, get_extensions_name); if (get_extensions) extensions = get_extensions(); </blockquote></div> </div>