-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 10/18/2011 12:41 AM, Jamey Sharp wrote: > Reviewed-by: Jamey Sharp <[email protected]> > > 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. There's also this one (in a reply to this same thread): Tested-by: Michal Suchanek <[email protected]>
Keith, is there anything else I should do to get this merged? Thanks Antoine > > 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 <[email protected]> >> --- >> >> 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 >> >> >> >> _______________________________________________ >> [email protected]: X.Org development Archives: >> http://lists.x.org/archives/xorg-devel Info: >> http://lists.x.org/mailman/listinfo/xorg-devel -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAk6sA+QACgkQGK2zHPGK1rsuAACcCP/TbPuACWoj1oe70pxTYHVy 1/YAn0vV9lY5G83oWSrvScI72xvLfarm =7qOV -----END PGP SIGNATURE----- _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
