[PATCH V4] xserver: check for elevated privileges not uid=0

Antoine Martin antoine at nagafix.co.uk
Sat Oct 29 06:47:16 PDT 2011


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/18/2011 12:41 AM, Jamey Sharp wrote:
> Reviewed-by: Jamey Sharp <jamey at 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.
There's also this one (in a reply to this same thread):
Tested-by: Michal Suchanek <hramrach at centrum.cz>

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 <antoine at 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
>> 
>> 
>> 
>> _______________________________________________ 
>> xorg-devel at lists.x.org: 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-----


More information about the xorg-devel mailing list