Please ignore the patch sent in the previous email, it was missing the
autoconf bits. The correct patch is now attached to this email.
I have copied the macros from the libX11 example and I have verified
that the HASGETRESUID/(HASSETUGID) are defined during the build on F15 -
still, I would prefer if someone could double check it..
The good thing is that this explains why Michal was using the fallback
code when testing, bonus is that this got tested properly, otherwise it
might not have.
Sorry it took so long to get it into shape..
Antoine
On 10/10/2011 09:07 PM, 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
Changes from the previous version of this patch:
* moved variables inside the function (thx to Tormod Volden)
* fallback code (when both HASSETUGID and HASGETRESUID are unset) now
correctly returns FALSE if uid is already 0. (thx to Michal Suchanek)
* consistently use "TRUE" constant rather than "1" value
You can find some ready made Fedora 15 test RPMs here:
http://xpra.org/src/Xdummy/
I have been using this patch for days without any visible side effects.
Can I please get some reviewed-by / acks?
Thanks
Antoine
>From 7412ccc6c837d36956dc00a7a44e595496f652f9 Mon Sep 17 00:00:00 2001
From: Antoine Martin <[email protected]>
Date: Mon, 10 Oct 2011 23:27:29 +0700
Subject: [PATCH xserver] check for elevated privileges not uid=0 (V3)
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]>
---
configure.ac | 7 ++++
hw/xfree86/common/xf86Config.c | 28 +++++++++---------
hw/xfree86/common/xf86Init.c | 63 ++++++++++++++++++++++++++++++++++------
hw/xfree86/common/xf86Priv.h | 1 +
include/xorg-config.h.in | 6 ++++
5 files changed, 82 insertions(+), 23 deletions(-)
diff --git a/configure.ac b/configure.ac
index 67a6836..fe836cc 100644
--- a/configure.ac
+++ b/configure.ac
@@ -211,6 +211,13 @@ AC_FUNC_VPRINTF
AC_CHECK_FUNCS([geteuid getuid link memmove memset mkstemp strchr strrchr \
strtol getopt getopt_long vsnprintf walkcontext backtrace \
getisax getzoneid shmctl64 strcasestr ffs vasprintf])
+
+dnl Checks for uid/gid functions used for testing elevated privileges
+AC_CHECK_FUNC([issetugid],
+ AC_DEFINE(HASSETUGID,1,[Has issetugid() function]))
+AC_CHECK_FUNC([getresuid],
+ AC_DEFINE(HASGETRESUID,1,[Has getresuid() & getresgid() functions]))
+
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..29f7638 100644
--- a/hw/xfree86/common/xf86Init.c
+++ b/hw/xfree86/common/xf86Init.c
@@ -237,6 +237,50 @@ xf86PrintMarkers(void)
LogPrintMarkers();
}
+static Bool privsTested = FALSE;
+static Bool privsElevated = FALSE;
+Bool xf86PrivsElevated(void)
+{
+ if (!privsTested) {
+#if !defined(WIN32)
+ if ((getuid() != geteuid()) || (getgid() != getegid())) {
+ privsElevated = TRUE;
+ } else {
+#if defined(HASSETUGID)
+ privsElevated = issetugid();
+#elif defined(HASGETRESUID)
+ 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
+ /*
+ * If there are saved ID's the process might still be priviledged
+ * 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) == -1) {
+ /* XXX ouch, coudn't get back to original uid
+ what can we do ??? */
+ _exit(127);
+ }
+ privsElevated = 1;
+ }
+#endif
+ }
+#endif
+ privsTested = TRUE;
+ }
+ return privsElevated;
+}
+
static Bool
xf86CreateRootWindow(WindowPtr pWin)
{
@@ -896,7 +940,7 @@ OsVendorInit(void)
#ifdef O_NONBLOCK
if (!beenHere) {
- if (geteuid() == 0 && getuid() != geteuid())
+ if (xf86PrivsElevated())
{
int status;
@@ -1067,10 +1111,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 +1143,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 +1156,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 +1442,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..18e5fb7 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 HASSETUGID
+
+/* Have getresuid */
+#undef HASGETRESUID
+
#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
