[xserver,1/4] os: move xf86PrivsElevated here
Ben Crocker
bcrocker at redhat.com
Tue Mar 13 21:40:51 UTC 2018
Antoine, thanks for the suggestion and the additional review. V2 patch
series forthcoming.
-- Ben
On Sat, Mar 10, 2018 at 11:58 PM, Antoine Martin <antoine at nagafix.co.uk>
wrote:
> On 09/03/18 05:11, Ben Crocker wrote:
> > Sent that a little too soon; please consider "Reviewed-by: Ben Crocker
> > <bcrocker at redhat.com <mailto:bcrocker at redhat.com>>" to be
> > added after each of the "Signed-off-by: Nicolai Haehnle
> > <nicolai.haehnle at amd.com <mailto:nicolai.haehnle at amd.com>>" lines.
> You can add:
> Reviewed-by: Antoine Martin <antoine at nagafix.co.uk>
>
> You should probably re-send the patch series as individual emails.
>
> Cheers
> Antoine
>
>
> >
> > 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
> > <mailto:bcrocker at redhat.com>> wrote:
> >
> > From: Nicolai Hähnle <nhaehnle at gmail.com <mailto:nhaehnle at gmail.com
> >>
> >
> > From: Nicolai Hähnle <nicolai.haehnle at amd.com
> > <mailto: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
> > <mailto: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
> > <mailto:nhaehnle at gmail.com>>
> > X-Patchwork-Id: 135711
> > Message-Id: <1485524258-6482-3-git-send-email-nhaehnle at gmail.com
> > <mailto:1485524258-6482-3-git-send-email-nhaehnle at gmail.com>>
> > To: xorg-devel at lists.x.org <mailto:xorg-devel at lists.x.org>
> > Cc: =?UTF-8?q?Nicolai=20H=C3=A4hnle?= <nicolai.haehnle at amd.com
> > <mailto:nicolai.haehnle at amd.com>>
> > Date: Fri, 27 Jan 2017 14:37:36 +0100
> >
> > From: Nicolai Hähnle <nicolai.haehnle at amd.com
> > <mailto:nicolai.haehnle at amd.com>>
> >
> > Signed-off-by: Nicolai Hähnle <nicolai.haehnle at amd.com
> > <mailto: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
> > <mailto:nhaehnle at gmail.com>>
> > X-Patchwork-Id: 135712
> > Message-Id: <1485524258-6482-4-git-send-email-nhaehnle at gmail.com
> > <mailto:1485524258-6482-4-git-send-email-nhaehnle at gmail.com>>
> > To: xorg-devel at lists.x.org <mailto:xorg-devel at lists.x.org>
> > Cc: =?UTF-8?q?Nicolai=20H=C3=A4hnle?= <nicolai.haehnle at amd.com
> > <mailto:nicolai.haehnle at amd.com>>
> > Date: Fri, 27 Jan 2017 14:37:37 +0100
> >
> > From: Nicolai Hähnle <nicolai.haehnle at amd.com
> > <mailto:nicolai.haehnle at amd.com>>
> >
> > Signed-off-by: Nicolai Hähnle <nicolai.haehnle at amd.com
> > <mailto: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
> > <mailto:nhaehnle at gmail.com>>
> > X-Patchwork-Id: 135713
> > Message-Id: <1485524258-6482-5-git-send-email-nhaehnle at gmail.com
> > <mailto:1485524258-6482-5-git-send-email-nhaehnle at gmail.com>>
> > To: xorg-devel at lists.x.org <mailto:xorg-devel at lists.x.org>
> > Cc: =?UTF-8?q?Nicolai=20H=C3=A4hnle?= <nicolai.haehnle at amd.com
> > <mailto:nicolai.haehnle at amd.com>>
> > Date: Fri, 27 Jan 2017 14:37:38 +0100
> >
> > From: Nicolai Hähnle <nicolai.haehnle at amd.com
> > <mailto: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
> > <mailto: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();
> >
> >
> >
> >
> > _______________________________________________
> > xorg-devel at lists.x.org: X.Org development
> > Archives: http://lists.x.org/archives/xorg-devel
> > Info: https://lists.x.org/mailman/listinfo/xorg-devel
> >
>
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.x.org/archives/xorg-devel/attachments/20180313/e272dfbf/attachment-0001.html>
More information about the xorg-devel
mailing list