Auditing the file handling stuff, increase general paranoia and code

Thu Sep 25 10:37:38 2003  George Lebl <jirka@5z.com>

	* daemon/auth.c, daemon/filecheck.[ch], daemon/gdm.[ch],
	  daemon/misc.c, daemon/slave.c:  Auditing the file handling
	  stuff, increase general paranoia and code anality about
	  these things plus check pretty much every return of the
	  sete[ug]id even though they are pretty much guaranteed
	  to exist.  Being paranoid here is good.  Allow the
	  authentication cookie be given in upper case hex for
	  the socket protocol.

8 files changed, 315 insertions, 109 deletions

diff --git a/ChangeLog b/ChangeLog
index 75ea5f16..eeee15cc 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+Thu Sep 25 10:37:38 2003  George Lebl <jirka@5z.com>
+
+	* daemon/auth.c, daemon/filecheck.[ch], daemon/gdm.[ch],
+	  daemon/misc.c, daemon/slave.c:  Auditing the file handling
+	  stuff, increase general paranoia and code anality about
+	  these things plus check pretty much every return of the
+	  sete[ug]id even though they are pretty much guaranteed
+	  to exist.  Being paranoid here is good.  Allow the
+	  authentication cookie be given in upper case hex for
+	  the socket protocol.
+
 Wed Sep 24 18:01:06 2003  George Lebl <jirka@5z.com>
 
 	* daemon/slave.c: add some extra anality to the slave when looking
diff --git a/daemon/auth.c b/daemon/auth.c
index e8e44dbc..151e44d4 100644
--- a/daemon/auth.c
+++ b/daemon/auth.c
@@ -537,10 +537,20 @@ try_user_add_again:
 	    d->userauth = g_build_filename (authdir, GdmUserAuthFile, NULL);
 
     /* Find out if the Xauthority file passes the paranoia check */
+    /* Note that this is not very efficient, we stat the files over
+       and over, but we don't care, we don't do this too often */
     if (automatic_tmp_dir ||
 	authdir == NULL ||
+
+	/* first the standard paranoia check (this checks the home dir
+	 * too which is useful here) */
 	! gdm_file_check ("gdm_auth_user_add", user, authdir, GdmUserAuthFile, 
 			  TRUE, FALSE, GdmUserMaxFile, GdmRelaxPerms) ||
+
+	/* now the auth file checking routine */
+	! gdm_auth_file_check ("gdm_auth_user_add", user, d->userauth, TRUE /* absentok */, NULL) ||
+
+	/* now see if we can actually append this file */
 	! try_open_append (d->userauth) ||
 
 	/* try opening as root, if we can't open as root,
@@ -726,12 +736,21 @@ gdm_auth_user_remove (GdmDisplay *d, uid_t user)
     authfile = g_path_get_basename (d->userauth);
     authdir = g_path_get_dirname (d->userauth);
 
+    if (ve_string_empty (authfile) ||
+	ve_string_empty (authdir)) {
+	    g_free (authdir);
+	    g_free (authfile);
+	    return;
+    }
+
     /* Now, the cookie file could be owned by a malicious user who
      * decided to concatenate something like his entire MP3 collection
      * to it. So we better play it safe... */
 
     if G_UNLIKELY ( ! gdm_file_check ("gdm_auth_user_remove", user, authdir, authfile, 
-			   TRUE, FALSE, GdmUserMaxFile, GdmRelaxPerms)) {
+			   TRUE, FALSE, GdmUserMaxFile, GdmRelaxPerms) ||
+		    /* be even paranoider with permissions */
+		    ! gdm_auth_file_check ("gdm_auth_user_remove", user, d->userauth, FALSE /* absentok */, NULL)) {
 	    g_free (authdir);
 	    g_free (authfile);
 	    gdm_error (_("%s: Ignoring suspiciously looking cookie file %s"),
@@ -825,6 +844,7 @@ gdm_auth_purge (GdmDisplay *d, FILE *af, gboolean remove_when_empty)
 {
     Xauth *xa;
     GSList *keep = NULL, *li;
+    int cnt;
 
     if G_UNLIKELY (!d || !af)
 	return af;
@@ -837,6 +857,8 @@ gdm_auth_purge (GdmDisplay *d, FILE *af, gboolean remove_when_empty)
      * temporary file issues. Then remove any instance of this display
      * in the cookie jar... */
 
+    cnt = 0;
+
     while ( (xa = XauReadAuth (af)) != NULL ) {
 	GSList *li;
 	/* We look at the current auths, but those may
@@ -853,6 +875,11 @@ gdm_auth_purge (GdmDisplay *d, FILE *af, gboolean remove_when_empty)
 	}
 	if (xa != NULL)
 		keep = g_slist_append (keep, xa);
+
+	/* just being ultra anal */
+	cnt ++;
+	if (cnt > 500)
+		break;
     }
 
     fclose (af);
diff --git a/daemon/filecheck.c b/daemon/filecheck.c
index 212d724c..3bbe4db4 100644
--- a/daemon/filecheck.c
+++ b/daemon/filecheck.c
@@ -21,9 +21,13 @@
 #include <syslog.h>
 #include <sys/stat.h>
 
+#include <vicious.h>
+
 #include "gdm.h"
 #include "filecheck.h"
 
+extern int GdmUserMaxFile;
+
 /**
  * gdm_file_check:
  * @caller: String to be prepended to syslog error messages.
@@ -38,6 +42,7 @@
  * Examines a file to determine whether it is safe for the daemon to write to it.
  */
 
+/* we should be euid the user BTW */
 gboolean
 gdm_file_check (const gchar *caller, uid_t user, const gchar *dir,
 		const gchar *file, gboolean absentok,
@@ -47,6 +52,10 @@ gdm_file_check (const gchar *caller, uid_t user, const gchar *dir,
     gchar *fullpath;
     int r;
 
+    if (ve_string_empty (dir) ||
+	ve_string_empty (file))
+	    return FALSE;
+
     /* Stat directory */
     IGNORE_EINTR (r = stat (dir, &statbuf));
     if (r < 0) {
@@ -57,19 +66,19 @@ gdm_file_check (const gchar *caller, uid_t user, const gchar *dir,
     }
 
     /* Check if dir is owned by the user ... */
-    if (statbuf.st_uid != user) {
+    if G_UNLIKELY (statbuf.st_uid != user) {
 	syslog (LOG_WARNING, _("%s: %s is not owned by uid %d."), caller, dir, user);
 	return FALSE;
     }
     
     /* ... if group has write permission ... */
-    if (perms < 1 && (statbuf.st_mode & S_IWGRP) == S_IWGRP) {
+    if G_UNLIKELY (perms < 1 && (statbuf.st_mode & S_IWGRP) == S_IWGRP) {
 	syslog (LOG_WARNING, _("%s: %s is writable by group."), caller, dir);
 	return FALSE;
     }
 
     /* ... and if others have write permission. */
-    if (perms < 2 && (statbuf.st_mode & S_IWOTH) == S_IWOTH) {
+    if G_UNLIKELY (perms < 2 && (statbuf.st_mode & S_IWOTH) == S_IWOTH) {
 	syslog (LOG_WARNING, _("%s: %s is writable by other."), caller, dir);
 	return FALSE;
     }
@@ -92,35 +101,35 @@ gdm_file_check (const gchar *caller, uid_t user, const gchar *dir,
     }
 
     /* Check that it is a regular file ... */
-    if (! S_ISREG (statbuf.st_mode)) {
+    if G_UNLIKELY (! S_ISREG (statbuf.st_mode)) {
 	syslog (LOG_WARNING, _("%s: %s is not a regular file."), caller, fullpath);
 	g_free (fullpath);
 	return FALSE;
     }
 
     /* ... owned by the user ... */
-    if (statbuf.st_uid != user) {
+    if G_UNLIKELY (statbuf.st_uid != user) {
 	syslog (LOG_WARNING, _("%s: %s is not owned by uid %d."), caller, fullpath, user);
 	g_free (fullpath);
 	return FALSE;
     }
 
     /* ... unwritable by group ... */
-    if (perms < 1 && (statbuf.st_mode & S_IWGRP) == S_IWGRP) {
+    if G_UNLIKELY (perms < 1 && (statbuf.st_mode & S_IWGRP) == S_IWGRP) {
 	syslog (LOG_WARNING, _("%s: %s is writable by group."), caller, fullpath);
 	g_free (fullpath);
 	return FALSE;
     }
 
     /* ... unwritable by others ... */
-    if (perms < 2 && (statbuf.st_mode & S_IWOTH) == S_IWOTH) {
+    if G_UNLIKELY (perms < 2 && (statbuf.st_mode & S_IWOTH) == S_IWOTH) {
 	syslog (LOG_WARNING, _("%s: %s is writable by group/other."), caller, fullpath);
 	g_free (fullpath);
 	return FALSE;
     }
 
     /* ... and smaller than sysadmin specified limit. */
-    if (maxsize && statbuf.st_size > maxsize) {
+    if G_UNLIKELY (maxsize && statbuf.st_size > maxsize) {
 	syslog (LOG_WARNING, _("%s: %s is bigger than sysadmin specified maximum file size."), 
 		caller, fullpath);
 	g_free (fullpath);
@@ -133,4 +142,54 @@ gdm_file_check (const gchar *caller, uid_t user, const gchar *dir,
     return TRUE;
 }
 
+/* we should be euid the user BTW */
+gboolean
+gdm_auth_file_check (const gchar *caller, uid_t user, const gchar *authfile, gboolean absentok, struct stat *s)
+{
+    struct stat statbuf;
+    int r;
+
+    if (ve_string_empty (authfile))
+	    return FALSE;
+
+    /* Stat file */
+    IGNORE_EINTR (r = lstat (authfile, &statbuf));
+    if (s != NULL)
+	    *s = statbuf;
+    if (r < 0) {
+	    if (absentok)
+		    return TRUE;
+	    syslog (LOG_WARNING, _("%s: %s does not exist but must exist."), caller, authfile);
+	    return FALSE;
+    }
+
+    /* Check that it is a regular file ... */
+    if G_UNLIKELY (! S_ISREG (statbuf.st_mode)) {
+	syslog (LOG_WARNING, _("%s: %s is not a regular file."), caller, authfile);
+	return FALSE;
+    }
+
+    /* ... owned by the user ... */
+    if G_UNLIKELY (statbuf.st_uid != user) {
+	syslog (LOG_WARNING, _("%s: %s is not owned by uid %d."), caller, authfile, user);
+	return FALSE;
+    }
+
+    /* ... has right permissions ... */
+    if G_UNLIKELY (statbuf.st_mode & 0077) {
+	syslog (LOG_WARNING, "%s: %s has wrong permissions (should be 0600)", caller, authfile);
+	return FALSE;
+    }
+
+    /* ... and smaller than sysadmin specified limit. */
+    if G_UNLIKELY (GdmUserMaxFile && statbuf.st_size > GdmUserMaxFile) {
+	syslog (LOG_WARNING, _("%s: %s is bigger than sysadmin specified maximum file size."), 
+		caller, authfile);
+	return FALSE;
+    }
+
+    /* Yeap, this file is ok */
+    return TRUE;
+}
+
 /* EOF */
diff --git a/daemon/filecheck.h b/daemon/filecheck.h
index 127a4cdc..6a0b239b 100644
--- a/daemon/filecheck.h
+++ b/daemon/filecheck.h
@@ -21,7 +21,12 @@
 
 gboolean gdm_file_check (const gchar *caller, uid_t user, const gchar *dir,
 			 const gchar *file, gboolean absentok,
-			 gboolean absentdirok, gint maxsize, gint perms);
+			 gboolean absentdirok, gint maxsize,
+			 gint perms);
+
+/* more paranoid on the file itself, doesn't check directory (for all we know
+   it could be /tmp) */
+gboolean gdm_auth_file_check (const gchar *caller, uid_t user, const gchar *authfile, gboolean absentok, struct stat *s);
 
 #endif /* GDM_FILECHECK_H */
 
diff --git a/daemon/gdm.c b/daemon/gdm.c
index c2f4019c..f358f1c8 100644
--- a/daemon/gdm.c
+++ b/daemon/gdm.c
@@ -57,6 +57,7 @@
 #include "getvt.h"
 #include "gdm-net.h"
 #include "cookie.h"
+#include "filecheck.h"
 
 /* Local functions */
 static void gdm_config_parse (void);
@@ -680,8 +681,8 @@ gdm_config_parse (void)
     GdmRebootReal = ve_get_first_working_command (GdmReboot, FALSE);
     GdmSuspendReal = ve_get_first_working_command (GdmSuspend, FALSE);
 
-    setegid (GdmGroupId);	/* gid remains `gdm' */
-    seteuid (GdmUserId);
+    NEVER_FAILS_setegid (GdmGroupId);	/* gid remains `gdm' */
+    NEVER_FAILS_seteuid (GdmUserId);
 
     /* Check that the greeter can be executed */
     bin = ve_first_word (GdmGreeter);
@@ -727,15 +728,15 @@ gdm_config_parse (void)
     /* Enter paranoia mode */
     check_servauthdir (&statbuf);
 
-    seteuid (0);
-    setegid (0);
+    NEVER_FAILS_seteuid (0);
+    NEVER_FAILS_setegid (0);
 
     /* Now set things up for us as  */
     chown (GdmServAuthDir, 0, GdmGroupId);
     chmod (GdmServAuthDir, (S_IRWXU|S_IRWXG|S_ISVTX));
 
-    setegid (GdmGroupId);
-    seteuid (GdmUserId);
+    NEVER_FAILS_setegid (GdmGroupId);
+    NEVER_FAILS_seteuid (GdmUserId);
 
     /* again paranoid */
     check_servauthdir (&statbuf);
@@ -774,8 +775,8 @@ gdm_config_parse (void)
 		      GdmServAuthDir, statbuf.st_mode, (S_IRWXU|S_IRWXG|S_ISVTX));
     }
 
-    seteuid (0);
-    setegid (0);
+    NEVER_FAILS_seteuid (0);
+    NEVER_FAILS_setegid (0);
 
     check_logdir ();
 
@@ -2757,6 +2758,7 @@ dehex_cookie (const char *cookie, int *len)
 	return bcookie;
 }
 
+/* This runs as the user who owns the file */
 static gboolean
 check_cookie (const char *file, const char *disp, const char *cookie)
 {
@@ -2765,6 +2767,7 @@ check_cookie (const char *file, const char *disp, const char *cookie)
 	char *bcookie;
 	int cookielen;
 	gboolean ret = FALSE;
+	int cnt = 0;
 
 	FILE *fp = fopen (file, "r");
 	if (fp == NULL)
@@ -2792,6 +2795,11 @@ check_cookie (const char *file, const char *disp, const char *cookie)
 			break; 
 		}
 		XauDisposeAuth (xa);
+
+		/* just being ultra anal */
+		cnt ++;
+		if (cnt > 500)
+			break;
 	}
 
 	g_free (number);
@@ -2817,24 +2825,33 @@ handle_flexi_server (GdmConnection *conn, int type, const char *server,
 	gdm_debug ("server: '%s'", server);
 
 	if (type == TYPE_FLEXI_XNEST) {
-		struct stat s;
-		int r;
 		gboolean authorized = TRUE;
+		struct passwd *pw;
+		gid_t oldgid = getegid ();
 
-		seteuid (xnest_uid);
+		pw = getpwuid (xnest_uid);
+		if (pw == NULL) {
+			gdm_connection_write (conn,
+					      "ERROR 100 Not authenticated\n");
+			return;
+		}
+
+		if (seteuid (xnest_uid) < 0) {
+			gdm_connection_write (conn,
+					      "ERROR 100 Not authenticated\n");
+			return;
+		}
+		if (setegid (pw->pw_gid) < 0)
+			NEVER_FAILS_setegid (GdmGroupId);
 
 		gdm_assert (xnest_auth_file != NULL);
 		gdm_assert (xnest_disp != NULL);
 		gdm_assert (xnest_cookie != NULL);
 
-		IGNORE_EINTR (r = stat (xnest_auth_file, &s));
-		if (r < 0)
-			authorized = FALSE;
 		if (authorized &&
-		    /* if readable or writable by group or others,
-		     * we are NOT authorized */
-		    s.st_mode & 0077)
+		    ! gdm_auth_file_check ("handle_flexi_server", xnest_uid, xnest_auth_file, FALSE /* absentok */, NULL))
 			authorized = FALSE;
+
 		if (authorized &&
 		    ! check_cookie (xnest_auth_file,
 				    xnest_disp,
@@ -2842,10 +2859,9 @@ handle_flexi_server (GdmConnection *conn, int type, const char *server,
 			authorized = FALSE;
 		}
 
-		if (s.st_uid != xnest_uid)
-			authorized = FALSE;
-
-		seteuid (0);
+		/* this must always work, thus the asserts */
+		NEVER_FAILS_seteuid (0);
+		NEVER_FAILS_setegid (oldgid);
 
 		if ( ! authorized) {
 			/* Sorry dude, you're not doing something
@@ -2855,7 +2871,7 @@ handle_flexi_server (GdmConnection *conn, int type, const char *server,
 			return;
 		}
 
-		server_uid = s.st_uid;
+		server_uid = xnest_uid;
 	}
 
 	if (flexi_servers >= GdmFlexibleXServers) {
@@ -3217,7 +3233,7 @@ gdm_handle_user_message (GdmConnection *conn, const char *msg, gpointer data)
 			GdmDisplay *disp = li->data;
 			if (disp->console &&
 			    disp->cookie != NULL &&
-			    strcmp (disp->cookie, cookie) == 0) {
+			    g_ascii_strcasecmp (disp->cookie, cookie) == 0) {
 				g_free (cookie);
 				GDM_CONNECTION_SET_USER_FLAG
 					(conn, GDM_SUP_FLAG_AUTHENTICATED);
diff --git a/daemon/gdm.h b/daemon/gdm.h
index 7fe2a56a..ef52d0d9 100644
--- a/daemon/gdm.h
+++ b/daemon/gdm.h
@@ -693,6 +693,23 @@ enum {
 		errno = 0;	\
 		expr;		\
 	} while G_UNLIKELY (errno == EINTR);
+
+#define NEVER_FAILS_seteuid(uid) \
+	{ int r = seteuid (uid); \
+	  if G_UNLIKELY (r != 0) \
+        gdm_fail ("GDM file %s: line %d (%s): Cannot run seteuid to %d", \
+		  __FILE__,						\
+		  __LINE__,						\
+		  __PRETTY_FUNCTION__,					\
+                  (int)uid);			}
+#define NEVER_FAILS_setegid(gid) \
+	{ int r = setegid (gid); \
+	  if G_UNLIKELY (r != 0) \
+        gdm_fail ("GDM file %s: line %d (%s): Cannot run setegid to %d", \
+		  __FILE__,						\
+		  __LINE__,						\
+		  __PRETTY_FUNCTION__,					\
+                  (int)gid);			}
 		
 
 #endif /* GDM_H */
diff --git a/daemon/misc.c b/daemon/misc.c
index 1c491f0e..9cef6738 100644
--- a/daemon/misc.c
+++ b/daemon/misc.c
@@ -822,8 +822,8 @@ gdm_ensure_sanity (void)
 	old_euid = geteuid ();
 	old_egid = getegid ();
 
-	seteuid (0);
-	setegid (0);
+	NEVER_FAILS_seteuid (0);
+	NEVER_FAILS_setegid (0);
 
 	/* The /tmp/.ICE-unix check, note that we do
 	 * ignore errors, since it's not deadly to run
@@ -850,8 +850,8 @@ gdm_ensure_sanity (void)
 
 	umask (old_umask);
 
-	seteuid (old_euid);
-	setegid (old_egid);
+	NEVER_FAILS_seteuid (old_euid);
+	NEVER_FAILS_setegid (old_egid);
 }
 
 const GList *
diff --git a/daemon/slave.c b/daemon/slave.c
index f873b13a..cf939617 100644
--- a/daemon/slave.c
+++ b/daemon/slave.c
@@ -287,10 +287,20 @@ run_session_output (gboolean read_until_eof)
 	/* make sure we are the user when we do this,
 	   for purposes of file limits and all that kind of
 	   stuff */
-	if G_LIKELY (logged_in_gid >= 0)
-		setegid (logged_in_gid);
-	if G_LIKELY (logged_in_uid >= 0)
-		seteuid (logged_in_uid);
+	if G_LIKELY (logged_in_gid >= 0) {
+		if G_UNLIKELY (setegid (logged_in_gid) != 0) {
+			gdm_error ("Can't set GID to user GID");
+			return;
+		}
+	}
+
+	if G_LIKELY (logged_in_uid >= 0) {
+		if G_UNLIKELY (seteuid (logged_in_uid) != 0) {
+			gdm_error ("Can't set UID to user UID");
+			NEVER_FAILS_seteuid (old);
+			return;
+		}
+	}
 
 	/* the fd is non-blocking */
 	for (;;) {
@@ -356,8 +366,8 @@ run_session_output (gboolean read_until_eof)
 			break;
 	}
 
-	seteuid (old);
-	setegid (oldg);
+	NEVER_FAILS_seteuid (old);
+	NEVER_FAILS_setegid (oldg);
 }
 
 /* must call slave_waitpid_setpid before calling this */
@@ -1617,8 +1627,8 @@ gdm_slave_wait_for_login (void)
 		check_notifies_now ();
 
 		/* just for paranoia's sake */
-		seteuid (0);
-		setegid (0);
+		NEVER_FAILS_seteuid (0);
+		NEVER_FAILS_setegid (0);
 
 		gdm_debug ("gdm_slave_wait_for_login: In loop");
 		login = gdm_verify_user (d,
@@ -1843,8 +1853,13 @@ run_pictures (void)
 
 		picfile = NULL;
 
-		setegid (pwent->pw_gid);
-		seteuid (pwent->pw_uid);
+		if G_UNLIKELY (setegid (pwent->pw_gid) != 0 ||
+			       seteuid (pwent->pw_uid) != 0) {
+			NEVER_FAILS_seteuid (0);
+			NEVER_FAILS_setegid (GdmGroupId);
+			gdm_slave_greeter_ctl_no_ret (GDM_READPIC, "");
+			continue;
+		}
 
 		if G_LIKELY (picfile == NULL) {
 			picfile = g_build_filename (pwent->pw_dir, ".face", NULL);
@@ -1856,8 +1871,8 @@ run_pictures (void)
 								 GdmRelaxPerms)) {
 				g_free (picfile);
 
-				seteuid (0);
-				setegid (GdmGroupId);
+				NEVER_FAILS_seteuid (0);
+				NEVER_FAILS_setegid (GdmGroupId);
 
 				gdm_slave_greeter_ctl_no_ret (GDM_READPIC, "");
 				continue;
@@ -1874,8 +1889,8 @@ run_pictures (void)
 								 GdmRelaxPerms)) {
 				g_free (picfile);
 
-				seteuid (0);
-				setegid (GdmGroupId);
+				NEVER_FAILS_seteuid (0);
+				NEVER_FAILS_setegid (GdmGroupId);
 
 				gdm_slave_greeter_ctl_no_ret (GDM_READPIC, "");
 				continue;
@@ -1913,8 +1928,8 @@ run_pictures (void)
 
 					/* if in trusted dir, just use it */
 					if (is_in_trusted_pic_dir (picfile)) {
-						seteuid (0);
-						setegid (GdmGroupId);
+						NEVER_FAILS_seteuid (0);
+						NEVER_FAILS_setegid (GdmGroupId);
 
 						g_free (cfgdir);
 
@@ -1932,9 +1947,11 @@ run_pictures (void)
 					 * on this file.  Even if it may not even be owned by the
 					 * user.  This setting should ONLY point to pics in trusted
 					 * dirs. */
-					if ( ! gdm_file_check ("run_pictures", pwent->pw_uid,
-							       dir, base, TRUE, TRUE, GdmUserMaxFile,
-							       GdmRelaxPerms)) {
+					if (ve_string_empty (dir) ||
+					    ve_string_empty (base) ||
+					    ! gdm_file_check ("run_pictures", pwent->pw_uid,
+							      dir, base, TRUE, TRUE, GdmUserMaxFile,
+							      GdmRelaxPerms)) {
 						g_free (picfile);
 						picfile = NULL;
 					}
@@ -1960,8 +1977,8 @@ run_pictures (void)
 				picdir = g_build_filename (pwent->pw_dir, ".gnome", NULL);
 			}
 			if (access (picfile, F_OK) != 0) {
-				seteuid (0);
-				setegid (GdmGroupId);
+				NEVER_FAILS_seteuid (0);
+				NEVER_FAILS_setegid (GdmGroupId);
 
 				/* Try the global face directory */
 
@@ -1999,8 +2016,8 @@ run_pictures (void)
 					       GdmRelaxPerms)) {
 				g_free (picdir);
 
-				seteuid (0);
-				setegid (GdmGroupId);
+				NEVER_FAILS_seteuid (0);
+				NEVER_FAILS_setegid (GdmGroupId);
 
 				gdm_slave_greeter_ctl_no_ret (GDM_READPIC, "");
 				continue;
@@ -2010,8 +2027,8 @@ run_pictures (void)
 
 		IGNORE_EINTR (r = stat (picfile, &s));
 		if G_UNLIKELY (r < 0 || s.st_size > GdmUserMaxFile) {
-			seteuid (0);
-			setegid (GdmGroupId);
+			NEVER_FAILS_seteuid (0);
+			NEVER_FAILS_setegid (GdmGroupId);
 
 			gdm_slave_greeter_ctl_no_ret (GDM_READPIC, "");
 			continue;
@@ -2020,8 +2037,8 @@ run_pictures (void)
 		fp = fopen (picfile, "r");
 		g_free (picfile);
 		if G_UNLIKELY (fp == NULL) {
-			seteuid (0);
-			setegid (GdmGroupId);
+			NEVER_FAILS_seteuid (0);
+			NEVER_FAILS_setegid (GdmGroupId);
 
 			gdm_slave_greeter_ctl_no_ret (GDM_READPIC, "");
 			continue;
@@ -2034,8 +2051,10 @@ run_pictures (void)
 		if G_UNLIKELY (ret == NULL || strcmp (ret, "OK") != 0) {
 			fclose (fp);
 			g_free (ret);
-			seteuid (0);
-			setegid (GdmGroupId);
+
+			NEVER_FAILS_seteuid (0);
+			NEVER_FAILS_setegid (GdmGroupId);
+
 			continue;
 		}
 		g_free (ret);
@@ -2108,8 +2127,8 @@ run_pictures (void)
 			
 		gdm_slave_greeter_ctl_no_ret (GDM_READPIC, "done");
 
-		seteuid (0);
-		setegid (GdmGroupId);
+		NEVER_FAILS_seteuid (0);
+		NEVER_FAILS_setegid (GdmGroupId);
 	}
 	g_free (response);
 }
@@ -2119,22 +2138,45 @@ static char *
 copy_auth_file (uid_t fromuid, uid_t touid, const char *file)
 {
 	uid_t old = geteuid ();
+	gid_t oldg = getegid ();
 	char *name;
 	int authfd;
 	int fromfd;
 	int bytes;
 	char buf[2048];
+	int cnt;
+
+	NEVER_FAILS_setegid (GdmGroupId);
 
-	seteuid (fromuid);
+	if G_UNLIKELY (seteuid (fromuid) != 0) {
+		NEVER_FAILS_setegid (oldg);
+		return NULL;
+	}
+
+	if ( ! gdm_auth_file_check ("copy_auth_file", fromuid,
+				    file, FALSE /* absentok */, NULL)) {
+		NEVER_FAILS_seteuid (old);
+		NEVER_FAILS_seteuid (oldg);
+		return NULL;
+	}
 
-	fromfd = open (file, O_RDONLY);
+	fromfd = open (file, O_RDONLY
+#ifdef O_NOCTTY
+		       |O_NOCTTY
+#endif
+#ifdef O_NOFOLLOW
+		       |O_NOFOLLOW
+#endif
+		      );
 
 	if G_UNLIKELY (fromfd < 0) {
-		seteuid (old);
+		NEVER_FAILS_seteuid (old);
+		NEVER_FAILS_seteuid (oldg);
 		return NULL;
 	}
 
-	seteuid (0);
+	NEVER_FAILS_seteuid (0);
+	NEVER_FAILS_setegid (0);
 
 	name = gdm_make_filename (GdmServAuthDir, d->name, ".XnestAuth");
 
@@ -2143,14 +2185,15 @@ copy_auth_file (uid_t fromuid, uid_t touid, const char *file)
 
 	if G_UNLIKELY (authfd < 0) {
 		IGNORE_EINTR (close (fromfd));
-		seteuid (old);
+		NEVER_FAILS_seteuid (old);
+		NEVER_FAILS_setegid (oldg);
 		g_free (name);
 		return NULL;
 	}
 
-	/* Make it owned by the user that Xnest is started as */
 	IGNORE_EINTR (fchown (authfd, touid, -1));
 
+	cnt = 0;
 	for (;;) {
 		int written, n;
 		IGNORE_EINTR (bytes = read (fromfd, buf, sizeof (buf)));
@@ -2159,12 +2202,13 @@ copy_auth_file (uid_t fromuid, uid_t touid, const char *file)
 		if (bytes == 0)
 			break;
 
-		if (bytes < 0) {
+		if G_UNLIKELY (bytes < 0) {
 			/* Error reading */
 			gdm_error ("Error reading %s: %s", file, strerror (errno));
 			IGNORE_EINTR (close (fromfd));
 			IGNORE_EINTR (close (authfd));
-			setuid (old);
+			NEVER_FAILS_seteuid (old);
+			NEVER_FAILS_setegid (oldg);
 			g_free (name);
 			return NULL;
 		}
@@ -2177,18 +2221,26 @@ copy_auth_file (uid_t fromuid, uid_t touid, const char *file)
 				gdm_error ("Error writing %s: %s", name, strerror (errno));
 				IGNORE_EINTR (close (fromfd));
 				IGNORE_EINTR (close (authfd));
-				setuid (old);
+				NEVER_FAILS_seteuid (old);
+				NEVER_FAILS_setegid (oldg);
 				g_free (name);
 				return NULL;
 			}
 			written += n;
 		} while (written < bytes);
+
+		cnt = cnt + written;
+		/* this should never occur (we check above)
+		   but we're paranoid) */
+		if G_UNLIKELY (cnt > GdmUserMaxFile)
+			return NULL;
 	}
 
 	IGNORE_EINTR (close (fromfd));
 	IGNORE_EINTR (close (authfd));
 
-	seteuid (old);
+	NEVER_FAILS_seteuid (old);
+	NEVER_FAILS_setegid (oldg);
 
 	return name;
 }
@@ -3031,13 +3083,14 @@ open_xsession_errors (struct passwd *pwent,
 		uid_t old = geteuid ();
 		uid_t oldg = getegid ();
 
-		setegid (pwent->pw_gid);
-		seteuid (pwent->pw_uid);
-		/* unlink to be anal */
-		IGNORE_EINTR (unlink (filename));
-		logfd = open (filename, O_EXCL|O_CREAT|O_TRUNC|O_WRONLY, 0644);
-		seteuid (old);
-		setegid (oldg);
+		if G_LIKELY (setegid (pwent->pw_gid) == 0 &&
+			     seteuid (pwent->pw_uid) == 0) {
+			/* unlink to be anal */
+			IGNORE_EINTR (unlink (filename));
+			logfd = open (filename, O_EXCL|O_CREAT|O_TRUNC|O_WRONLY, 0644);
+		}
+		NEVER_FAILS_seteuid (old);
+		NEVER_FAILS_setegid (oldg);
 
 		if G_UNLIKELY (logfd < 0) {
 			gdm_error (_("%s: Could not open ~/.xsession-errors"),
@@ -3057,15 +3110,15 @@ open_xsession_errors (struct passwd *pwent,
 		uid_t old = geteuid ();
 		uid_t oldg = getegid ();
 
-		setegid (pwent->pw_gid);
-		seteuid (pwent->pw_uid);
-
-		oldmode = umask (077);
-		logfd = mkstemp (filename);
-		umask (oldmode);
+		if G_LIKELY (setegid (pwent->pw_gid) == 0 &&
+			     seteuid (pwent->pw_uid) == 0) {
+			oldmode = umask (077);
+			logfd = mkstemp (filename);
+			umask (oldmode);
+		}
 
-		seteuid (old);
-		setegid (oldg);
+		NEVER_FAILS_seteuid (old);
+		NEVER_FAILS_setegid (oldg);
 
 		if G_LIKELY (logfd >= 0) {
 			d->xsession_errors_filename = filename;
@@ -3222,8 +3275,9 @@ session_child_run (struct passwd *pwent,
 				"gdm_slave_session_start", login);
 
 	/* setup egid to the correct group,
-	 * not to leave the egid around */
-	setegid (pwent->pw_gid);
+	 * not to leave the egid around.  It's
+	 * ok to gdm_fail here */
+	NEVER_FAILS_setegid (pwent->pw_gid);
 
 	IGNORE_EINTR (chdir (home_dir));
 	if G_UNLIKELY (errno != 0) {
@@ -3563,8 +3617,13 @@ gdm_slave_session_start (void)
 	    home_dir = pwent->pw_dir;
     }
 
-    setegid (pwent->pw_gid);
-    seteuid (pwent->pw_uid);
+    if G_UNLIKELY (setegid (pwent->pw_gid) != 0 ||
+		   seteuid (pwent->pw_uid) != 0) {
+	    gdm_error ("Cannot set effective user/group id");
+	    gdm_verify_cleanup (d);
+	    session_started = FALSE;
+	    return;
+    }
 
     if G_LIKELY (home_dir_ok) {
 	    /* Sanity check on ~user/.dmrc */
@@ -3613,8 +3672,8 @@ gdm_slave_session_start (void)
 	usrlang = g_strdup ("");
     }
 
-    seteuid (0);
-    setegid (GdmGroupId);
+    NEVER_FAILS_seteuid (0);
+    NEVER_FAILS_setegid (GdmGroupId);
 
     if (greet) {
 	    tmp = gdm_ensure_extension (usrsess, ".desktop");
@@ -3682,16 +3741,19 @@ gdm_slave_session_start (void)
     /* Setup cookie -- We need this information during cleanup, thus
      * cookie handling is done before fork()ing */
 
-    setegid (pwent->pw_gid);
-    seteuid (pwent->pw_uid);
+    if G_UNLIKELY (setegid (pwent->pw_gid) != 0 ||
+		   seteuid (pwent->pw_uid) != 0) {
+	    gdm_error ("Cannot set effective user/group id");
+	    gdm_slave_quick_exit (DISPLAY_REMANAGE);
+    }
 
     authok = gdm_auth_user_add (d, pwent->pw_uid,
 				/* Only pass the home_dir if
 				 * it was ok */
 				home_dir_ok ? home_dir : NULL);
 
-    seteuid (0);
-    setegid (GdmGroupId);
+    NEVER_FAILS_seteuid (0);
+    NEVER_FAILS_setegid (GdmGroupId);
     
     if G_UNLIKELY ( ! authok) {
 	    gdm_debug ("gdm_slave_session_start: Auth not OK");
@@ -3811,8 +3873,8 @@ gdm_slave_session_start (void)
     }
 
     /* We must be root for this, and we are, but just to make sure */
-    seteuid (0);
-    setegid (GdmGroupId);
+    NEVER_FAILS_seteuid (0);
+    NEVER_FAILS_setegid (GdmGroupId);
     /* Reset all the process limits, pam may have set some up for our process and that
        is quite evil.  But pam is generally evil, so this is to be expected. */
     gdm_reset_limits ();
@@ -3893,6 +3955,9 @@ gdm_slave_session_stop (gboolean run_post_session,
     local_login = login;
     login = NULL;
 
+    /* don't use NEVER_FAILS_ here this can be called from places
+       kind of exiting and it's ok if this doesn't work (when shouldn't
+       it work anyway? */
     seteuid (0);
     setegid (0);
 
@@ -3941,11 +4006,14 @@ gdm_slave_session_stop (gboolean run_post_session,
 
     if (pwent != NULL) {
 	    /* Remove display from ~user/.Xauthority */
-	    setegid (pwent->pw_gid);
-	    seteuid (pwent->pw_uid);
-
-	    gdm_auth_user_remove (d, pwent->pw_uid);
+	    if G_LIKELY (setegid (pwent->pw_gid) == 0 &&
+			 seteuid (pwent->pw_uid) == 0) {
+		    gdm_auth_user_remove (d, pwent->pw_uid);
+	    }
 
+	    /* don't use NEVER_FAILS_ here this can be called from places
+	       kind of exiting and it's ok if this doesn't work (when shouldn't
+	       it work anyway? */
 	    seteuid (0);
 	    setegid (0);
     }
@@ -4499,6 +4567,9 @@ static void
 gdm_slave_quick_exit (gint status)
 {
     /* just for paranoia's sake */
+    /* don't use NEVER_FAILS_ here this can be called from places
+       kind of exiting and it's ok if this doesn't work (when shouldn't
+       it work anyway? */
     seteuid (0);
     setegid (0);