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

Jamey Sharp jamey at minilop.net
Mon Oct 17 10:41:49 PDT 2011


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.

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
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.x.org/archives/xorg-devel/attachments/20111017/e661b060/attachment.pgp>


More information about the xorg-devel mailing list