[xserver,1/4] os: move xf86PrivsElevated here
Ben Crocker
bcrocker at redhat.com
Thu Mar 8 22:11:43 UTC 2018
Sent that a little too soon; please consider "Reviewed-by: Ben Crocker <
bcrocker at redhat.com>" to be
added after each of the "Signed-off-by: Nicolai Haehnle <
nicolai.haehnle at amd.com>" lines.
Also please note that I rebased Nicolai's original patch,
https://patchwork.freedesktop.org/series/18684/,
against a very recent copy of master.
Thanks,
Ben
On Thu, Mar 8, 2018 at 4:53 PM, Ben Crocker <bcrocker at redhat.com> wrote:
> From: Nicolai Hähnle <nhaehnle at gmail.com>
>
> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>
> 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 <nicolai.haehnle at amd.com>
> ---
> 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
> TrapSignals(void)
> {
> if (xf86Info.notrapSignals) {
> OsSignal(SIGSEGV, SIG_DFL);
> OsSignal(SIGABRT, SIG_DFL);
> OsSignal(SIGILL, SIG_DFL);
> #ifdef SIGEMT
> 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
>
> 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?= <nhaehnle at gmail.com>
> X-Patchwork-Id: 135711
> Message-Id: <1485524258-6482-3-git-send-email-nhaehnle at gmail.com>
> To: xorg-devel at lists.x.org
> Cc: =?UTF-8?q?Nicolai=20H=C3=A4hnle?= <nicolai.haehnle at amd.com>
> Date: Fri, 27 Jan 2017 14:37:36 +0100
>
> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>
> Signed-off-by: Nicolai Hähnle <nicolai.haehnle at amd.com>
> ---
> 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?= <nhaehnle at gmail.com>
> X-Patchwork-Id: 135712
> Message-Id: <1485524258-6482-4-git-send-email-nhaehnle at gmail.com>
> To: xorg-devel at lists.x.org
> Cc: =?UTF-8?q?Nicolai=20H=C3=A4hnle?= <nicolai.haehnle at amd.com>
> Date: Fri, 27 Jan 2017 14:37:37 +0100
>
> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>
> Signed-off-by: Nicolai Hähnle <nicolai.haehnle at amd.com>
> ---
> 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?= <nhaehnle at gmail.com>
> X-Patchwork-Id: 135713
> Message-Id: <1485524258-6482-5-git-send-email-nhaehnle at gmail.com>
> To: xorg-devel at lists.x.org
> Cc: =?UTF-8?q?Nicolai=20H=C3=A4hnle?= <nicolai.haehnle at amd.com>
> Date: Fri, 27 Jan 2017 14:37:38 +0100
>
> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>
> 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 <nicolai.haehnle at amd.com>
> ---
> 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();
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.x.org/archives/xorg-devel/attachments/20180308/1f10775a/attachment-0001.html>
More information about the xorg-devel
mailing list