Reviewed-by: Jamey Sharp <ja...@minilop.net> Since this has gone through so many changes I tried to carefully re-review the whole thing, and it still looks good to me. Thanks, Antoine!
We don't actually put "xserver:" in the commit message for a server patch, though. It wouldn't be wrong to tag this "xfree86:" even though it touches autoconf-ery because it's really all for the benefit of the xfree86 DDX, as written; but it would be OK to leave off the tag entirely, too. The next trick is to get Keith to merge this patch, and/or get more reviewed-by/tested-by tags. Jamey On Tue, Oct 11, 2011 at 03:02:50PM +0700, Antoine Martin wrote: > This allows us to run the server as a normal user whilst still > being able to use the -modulepath, -logfile and -config switches > We define a xf86PrivsElevated which will do the checks and cache > the result in case it is called more than once. > Also renamed the paths #defines to match their new meaning. > Original discussion which led to this patch can be found here: > http://lists.freedesktop.org/archives/xorg-devel/2011-September/025853.html > > Signed-off-by: Antoine Martin <anto...@nagafix.co.uk> > --- > > Changes from the previous version of this patch: > * use FatalError() rather than exit() (thx to Julien Cristau) > * remove comment superseded by FatalError message (thx to Tormod Volden) > * more defensive code: print warning and assume privs are elevated > if getresuid or getresgid fail, ensure all codepaths set > privsElevated explicitly and make it default to TRUE (in case we > ever miss one) > * email title includes patch version in the right location > (btw, there are changes in xserver's autoconf, not just hw/xfree86) > > Thanks > Antoine > > configure.ac | 4 ++- > hw/xfree86/common/xf86Config.c | 28 +++++++------- > hw/xfree86/common/xf86Init.c | 76 > +++++++++++++++++++++++++++++++++++----- > hw/xfree86/common/xf86Priv.h | 1 + > include/xorg-config.h.in | 6 +++ > 5 files changed, 91 insertions(+), 24 deletions(-) > > diff --git a/configure.ac b/configure.ac > index 67a6836..a79e2be 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -208,9 +208,11 @@ AC_SUBST(DLOPEN_LIBS) > > dnl Checks for library functions. > AC_FUNC_VPRINTF > -AC_CHECK_FUNCS([geteuid getuid link memmove memset mkstemp strchr strrchr \ > +AC_CHECK_FUNCS([geteuid getuid issetugid getresuid \ > + link memmove memset mkstemp strchr strrchr \ > strtol getopt getopt_long vsnprintf walkcontext backtrace \ > getisax getzoneid shmctl64 strcasestr ffs vasprintf]) > + > AC_FUNC_ALLOCA > dnl Old HAS_* names used in os/*.c. > AC_CHECK_FUNC([getdtablesize], > diff --git a/hw/xfree86/common/xf86Config.c b/hw/xfree86/common/xf86Config.c > index f8c1b65..8ccc52a 100644 > --- a/hw/xfree86/common/xf86Config.c > +++ b/hw/xfree86/common/xf86Config.c > @@ -72,8 +72,8 @@ > * These paths define the way the config file search is done. The escape > * sequences are documented in parser/scan.c. > */ > -#ifndef ROOT_CONFIGPATH > -#define ROOT_CONFIGPATH "%A," "%R," \ > +#ifndef ALL_CONFIGPATH > +#define ALL_CONFIGPATH "%A," "%R," \ > "/etc/X11/%R," "%P/etc/X11/%R," \ > "%E," "%F," \ > "/etc/X11/%F," "%P/etc/X11/%F," \ > @@ -83,8 +83,8 @@ > "%P/lib/X11/%X.%H," \ > "%P/lib/X11/%X" > #endif > -#ifndef USER_CONFIGPATH > -#define USER_CONFIGPATH "/etc/X11/%S," "%P/etc/X11/%S," \ > +#ifndef RESTRICTED_CONFIGPATH > +#define RESTRICTED_CONFIGPATH "/etc/X11/%S," "%P/etc/X11/%S," \ > "/etc/X11/%G," "%P/etc/X11/%G," \ > "/etc/X11/%X," "/etc/%X," \ > "%P/etc/X11/%X.%H," \ > @@ -92,14 +92,14 @@ > "%P/lib/X11/%X.%H," \ > "%P/lib/X11/%X" > #endif > -#ifndef ROOT_CONFIGDIRPATH > -#define ROOT_CONFIGDIRPATH "%A," "%R," \ > +#ifndef ALL_CONFIGDIRPATH > +#define ALL_CONFIGDIRPATH "%A," "%R," \ > "/etc/X11/%R," "%C/X11/%R," \ > "/etc/X11/%X," "%C/X11/%X" > #endif > -#ifndef USER_CONFIGDIRPATH > -#define USER_CONFIGDIRPATH "/etc/X11/%R," "%C/X11/%R," \ > - "/etc/X11/%X," "%C/X11/%X" > +#ifndef RESTRICTED_CONFIGDIRPATH > +#define RESTRICTED_CONFIGDIRPATH "/etc/X11/%R," "%C/X11/%R," \ > + "/etc/X11/%X," "%C/X11/%X" > #endif > #ifndef SYS_CONFIGDIRPATH > #define SYS_CONFIGDIRPATH "/usr/share/X11/%X," "%D/X11/%X" > @@ -2302,12 +2302,12 @@ xf86HandleConfigFile(Bool autoconfig) > Bool implicit_layout = FALSE; > > if (!autoconfig) { > - if (getuid() == 0) { > - filesearch = ROOT_CONFIGPATH; > - dirsearch = ROOT_CONFIGDIRPATH; > + if (!xf86PrivsElevated()) { > + filesearch = ALL_CONFIGPATH; > + dirsearch = ALL_CONFIGDIRPATH; > } else { > - filesearch = USER_CONFIGPATH; > - dirsearch = USER_CONFIGDIRPATH; > + filesearch = RESTRICTED_CONFIGPATH; > + dirsearch = RESTRICTED_CONFIGDIRPATH; > } > > if (xf86ConfigFile) > diff --git a/hw/xfree86/common/xf86Init.c b/hw/xfree86/common/xf86Init.c > index 350918d..3fead6f 100644 > --- a/hw/xfree86/common/xf86Init.c > +++ b/hw/xfree86/common/xf86Init.c > @@ -237,6 +237,63 @@ xf86PrintMarkers(void) > LogPrintMarkers(); > } > > +Bool xf86PrivsElevated(void) > +{ > + static Bool privsTested = FALSE; > + static Bool privsElevated = TRUE; > + > + if (!privsTested) { > +#if !defined(WIN32) > + 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; > +} > + > static Bool > xf86CreateRootWindow(WindowPtr pWin) > { > @@ -896,7 +953,7 @@ OsVendorInit(void) > > #ifdef O_NONBLOCK > if (!beenHere) { > - if (geteuid() == 0 && getuid() != geteuid()) > + if (xf86PrivsElevated()) > { > int status; > > @@ -1067,10 +1124,11 @@ ddxProcessArgument(int argc, char **argv, int i) > FatalError("Required argument to %s not specified\n", argv[i]); > \ > } > > - /* First the options that are only allowed for root */ > + /* First the options that are not allowed with elevated privileges */ > if (!strcmp(argv[i], "-modulepath") || !strcmp(argv[i], "-logfile")) { > - if ( (geteuid() == 0) && (getuid() != 0) ) { > - FatalError("The '%s' option can only be used by root.\n", argv[i]); > + if (xf86PrivsElevated()) { > + FatalError("The '%s' option cannot be used with " > + "elevated privileges.\n", argv[i]); > } > else if (!strcmp(argv[i], "-modulepath")) > { > @@ -1098,9 +1156,9 @@ ddxProcessArgument(int argc, char **argv, int i) > if (!strcmp(argv[i], "-config") || !strcmp(argv[i], "-xf86config")) > { > CHECK_FOR_REQUIRED_ARGUMENT(); > - if (getuid() != 0 && !xf86PathIsSafe(argv[i + 1])) { > + if (xf86PrivsElevated() && !xf86PathIsSafe(argv[i + 1])) { > FatalError("\nInvalid argument for %s\n" > - "\tFor non-root users, the file specified with %s must be\n" > + "\tWith elevated privileges, the file specified with %s must be\n" > "\ta relative path and must not contain any \"..\" elements.\n" > "\tUsing default "__XCONFIGFILE__" search path.\n\n", > argv[i], argv[i]); > @@ -1111,9 +1169,9 @@ ddxProcessArgument(int argc, char **argv, int i) > if (!strcmp(argv[i], "-configdir")) > { > CHECK_FOR_REQUIRED_ARGUMENT(); > - if (getuid() != 0 && !xf86PathIsSafe(argv[i + 1])) { > + if (xf86PrivsElevated() && !xf86PathIsSafe(argv[i + 1])) { > FatalError("\nInvalid argument for %s\n" > - "\tFor non-root users, the file specified with %s must be\n" > + "\tWith elevated privileges, the file specified with %s must be\n" > "\ta relative path and must not contain any \"..\" elements.\n" > "\tUsing default "__XCONFIGDIR__" search path.\n\n", > argv[i], argv[i]); > @@ -1397,7 +1455,7 @@ ddxUseMsg(void) > ErrorF("\n"); > ErrorF("\n"); > ErrorF("Device Dependent Usage\n"); > - if (getuid() == 0 || geteuid() != 0) > + if (!xf86PrivsElevated()) > { > ErrorF("-modulepath paths specify the module search path\n"); > ErrorF("-logfile file specify a log file name\n"); > diff --git a/hw/xfree86/common/xf86Priv.h b/hw/xfree86/common/xf86Priv.h > index 1fe3d7e..49cae87 100644 > --- a/hw/xfree86/common/xf86Priv.h > +++ b/hw/xfree86/common/xf86Priv.h > @@ -147,6 +147,7 @@ extern _X_EXPORT Bool xf86LoadModules(char **list, > pointer *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 */ > > diff --git a/include/xorg-config.h.in b/include/xorg-config.h.in > index 0d1ea91..ae0c8ec 100644 > --- a/include/xorg-config.h.in > +++ b/include/xorg-config.h.in > @@ -139,4 +139,10 @@ > /* Build with libdrm support */ > #undef WITH_LIBDRM > > +/* Have setugid */ > +#undef HAVE_ISSETUGID > + > +/* Have getresuid */ > +#undef HAVE_GETRESUID > + > #endif /* _XORG_CONFIG_H_ */ > -- > 1.7.6.4 >
signature.asc
Description: Digital signature
_______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel