[PATCH] Allow update-game-score to run sgid instead of suid.

[PATCH] Allow update-game-score to run sgid instead of suid.

[Ulrich Mueller]

Currently the update-game-score program is installed with the setuid
flag, per default to the "games" user. It is more common for games
with a shared score file to be installed setgid "games" instead.

The patch included below would allow running update-game-score setgid
instead.

Questions:
- I have kept the previous default behaviour, namely suid to "games"
  if such a user exists. IMHO it would be better not to install any
  suid/sgid binary by default, but only if explicitly requested by a
  configure option.
- In the --with-gameuser option, I am using the [USER][:GROUP] syntax
  from GNU chown to distinguish between user and group. Is this o.k.,
  or would it be cleaner to have a separate --with-gamegroup option
  (which would have to be mutually exclusive with the --with-gameuser
  option)?

Ulrich


From 63275cb8c22ef6cef6a5b233438d0c39608e4448 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ulrich=20M=C3=BCller?= <ulm@gentoo.org>
Date: Fri, 16 Jan 2015 09:25:25 +0100
Subject: [PATCH] Allow update-game-score to run sgid instead of suid.

* configure.ac (gamegroup): New AC_SUBST.
(--with-gameuser): Allow to specify a group instead of a user.
In the default case, check at configure time if we can chown
to the 'games' user.
* lib-src/update-game-score.c: Allow the program to run sgid
instead of suid, in order to match common practice for most games.
(main): Check if we are running sgid.  Pass appropriate file
permission bits to 'write_scores'.
(write_scores): New 'mode' argument, instead of hardcoding 0644.
(get_prefix): Update error message.
* lib-src/Makefile.in (gamegroup): New variable, set by configure.
($(DESTDIR)${archlibdir}): Handle both suid or sgid when
installing the 'update-game-score' program.
* lisp/play/gamegrid.el (gamegrid-add-score-with-update-game-score):
Allow the 'update-game-score' helper program to run suid or sgid.
---
 ChangeLog                   |  7 +++++++
 configure.ac                | 26 ++++++++++++++++++++++----
 lib-src/ChangeLog           | 12 ++++++++++++
 lib-src/Makefile.in         | 16 ++++++++++++----
 lib-src/update-game-score.c | 33 +++++++++++++++++++--------------
 lisp/ChangeLog              |  5 +++++
 lisp/play/gamegrid.el       |  6 +++---
 7 files changed, 80 insertions(+), 25 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 309b04f..9a94a71 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2015-01-16  Ulrich Müller  <ulm@gentoo.org>
+
+	* configure.ac (gamegroup): New AC_SUBST.
+	(--with-gameuser): Allow to specify a group instead of a user.
+	In the default case, check at configure time if we can chown
+	to the 'games' user.
+
 2015-01-16  Paul Eggert  <eggert@cs.ucla.edu>
 
 	Give up on -Wsuggest-attribute=const
diff --git a/configure.ac b/configure.ac
index 9db4bde..0fd78da 100644
--- a/configure.ac
+++ b/configure.ac
@@ -392,10 +392,27 @@ OPTION_DEFAULT_ON([compress-install],
 make GZIP_PROG= install])
 
 AC_ARG_WITH(gameuser,dnl
-[AS_HELP_STRING([--with-gameuser=USER],[user for shared game score files])])
-test "X${with_gameuser}" != X && test "${with_gameuser}" != yes \
-  && gameuser="${with_gameuser}"
-test "X$gameuser" = X && gameuser=games
+[AS_HELP_STRING([--with-gameuser=USER_OR_GROUP],
+		[user for shared game score files.
+		An argument prefixed by ':' specifies a group instead.])])
+gameuser=
+gamegroup=
+case "${with_gameuser}" in
+  no) ;;
+  "" | yes)
+    AC_MSG_CHECKING([whether we can chown to 'games'])
+    touch conftest.tmp
+    if chown games conftest.tmp 2>/dev/null; then
+      AC_MSG_RESULT([yes])
+      gameuser=games
+    else
+      AC_MSG_RESULT([no])
+    fi
+    rm -f conftest.tmp
+    ;;
+  :*) gamegroup=${with_gameuser#:} ;;
+  *) gameuser=${with_gameuser} ;;
+esac
 
 AC_ARG_WITH([gnustep-conf],dnl
 [AS_HELP_STRING([--with-gnustep-conf=FILENAME],
@@ -4684,6 +4701,7 @@ AC_SUBST(etcdocdir)
 AC_SUBST(bitmapdir)
 AC_SUBST(gamedir)
 AC_SUBST(gameuser)
+AC_SUBST(gamegroup)
 ## FIXME? Nothing uses @LD_SWITCH_X_SITE@.
 ## src/Makefile.in did add LD_SWITCH_X_SITE (as a cpp define) to the
 ## end of LIBX_BASE, but nothing ever set it.
diff --git a/lib-src/ChangeLog b/lib-src/ChangeLog
index 7cbf327..8c1e5ca 100644
--- a/lib-src/ChangeLog
+++ b/lib-src/ChangeLog
@@ -1,3 +1,15 @@
+2015-01-16  Ulrich Müller  <ulm@gentoo.org>
+
+	* update-game-score.c: Allow the program to run sgid instead
+	of suid, in order to match common practice for most games.
+	(main): Check if we are running sgid.  Pass appropriate file
+	permission bits to 'write_scores'.
+	(write_scores): New 'mode' argument, instead of hardcoding 0644.
+	(get_prefix): Update error message.
+	* Makefile.in (gamegroup): New variable, set by configure.
+	($(DESTDIR)${archlibdir}): Handle both suid or sgid when
+	installing the 'update-game-score' program.
+
 2015-01-16  Paul Eggert  <eggert@cs.ucla.edu>
 
 	Give up on -Wsuggest-attribute=const
diff --git a/lib-src/Makefile.in b/lib-src/Makefile.in
index 22a5eca..878e394 100644
--- a/lib-src/Makefile.in
+++ b/lib-src/Makefile.in
@@ -117,6 +117,7 @@ archlibdir=@archlibdir@
 
 gamedir=@gamedir@
 gameuser=@gameuser@
+gamegroup=@gamegroup@
 
 # ==================== Utility Programs for the Build =================
 
@@ -258,10 +259,17 @@ $(DESTDIR)${archlibdir}: all
 	umask 022; ${MKDIR_P} "$(DESTDIR)${gamedir}"; \
 	touch "$(DESTDIR)${gamedir}/snake-scores"; \
 	touch "$(DESTDIR)${gamedir}/tetris-scores"
-	-if chown ${gameuser} "$(DESTDIR)${archlibdir}/update-game-score${EXEEXT}" && chmod u+s "$(DESTDIR)${archlibdir}/update-game-score${EXEEXT}"; then \
-	  chown ${gameuser} "$(DESTDIR)${gamedir}"; \
-	  chmod u=rwx,g=rwx,o=rx "$(DESTDIR)${gamedir}"; \
-	fi
+ifneq ($(gameuser),)
+	chown ${gameuser} "$(DESTDIR)${archlibdir}/update-game-score${EXEEXT}"
+	chmod u+s,go-r "$(DESTDIR)${archlibdir}/update-game-score${EXEEXT}"
+	chown ${gameuser} "$(DESTDIR)${gamedir}"
+	chmod u=rwx,g=rx,o=rx "$(DESTDIR)${gamedir}"
+else ifneq ($(gamegroup),)
+	chgrp ${gamegroup} "$(DESTDIR)${archlibdir}/update-game-score${EXEEXT}"
+	chmod g+s,o-r "$(DESTDIR)${archlibdir}/update-game-score${EXEEXT}"
+	chgrp ${gamegroup} "$(DESTDIR)${gamedir}"
+	chmod u=rwx,g=rwx,o=rx "$(DESTDIR)${gamedir}"
+endif
 	exp_archlibdir=`cd "$(DESTDIR)${archlibdir}" && /bin/pwd`; \
 	if [ "$$exp_archlibdir" != "`cd ${srcdir} && /bin/pwd`" ]; then \
 	  for file in ${SCRIPTS}; do \
diff --git a/lib-src/update-game-score.c b/lib-src/update-game-score.c
index d3354af..4f15483 100644
--- a/lib-src/update-game-score.c
+++ b/lib-src/update-game-score.c
@@ -21,8 +21,8 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 
 
 /* This program allows a game to securely and atomically update a
-   score file.  It should be installed setuid, owned by an appropriate
-   user like `games'.
+   score file.  It should be installed either setuid or setgid, owned
+   by an appropriate user or group like `games'.
 
    Alternatively, it can be compiled without HAVE_SHARED_GAME_DIR
    defined, and in that case it will store scores in the user's home
@@ -88,7 +88,7 @@ static int push_score (struct score_entry **scores, ptrdiff_t *count,
 		       ptrdiff_t *size, struct score_entry const *newscore);
 static void sort_scores (struct score_entry *scores, ptrdiff_t count,
 			 bool reverse);
-static int write_scores (const char *filename,
+static int write_scores (const char *filename, mode_t mode,
 			 const struct score_entry *scores, ptrdiff_t count);
 
 static _Noreturn void
@@ -122,18 +122,19 @@ get_user_id (void)
 }
 
 static const char *
-get_prefix (bool running_suid, const char *user_prefix)
+get_prefix (bool privileged, const char *user_prefix)
 {
-  if (!running_suid && user_prefix == NULL)
-    lose ("Not using a shared game directory, and no prefix given.");
-  if (running_suid)
+  if (privileged)
     {
 #ifdef HAVE_SHARED_GAME_DIR
       return HAVE_SHARED_GAME_DIR;
 #else
-      lose ("This program was compiled without HAVE_SHARED_GAME_DIR,\n and should not be suid.");
+      lose ("This program was compiled without HAVE_SHARED_GAME_DIR,\n"
+	    "and should not run with elevated privileges.");
 #endif
     }
+  if (user_prefix == NULL)
+    lose ("Not using a shared game directory, and no prefix given.");
   return user_prefix;
 }
 
@@ -173,7 +174,7 @@ int
 main (int argc, char **argv)
 {
   int c;
-  bool running_suid;
+  bool running_suid, running_sgid;
   void *lockstate;
   char *scorefile;
   char *end, *nl, *user, *data;
@@ -214,8 +215,11 @@ main (int argc, char **argv)
     usage (EXIT_FAILURE);
 
   running_suid = (getuid () != geteuid ());
+  running_sgid = (getgid () != getegid ());
+  if (running_suid && running_sgid)
+    lose ("This program can run either suid or sgid, but not both.");
 
-  prefix = get_prefix (running_suid, user_prefix);
+  prefix = get_prefix (running_suid || running_sgid, user_prefix);
 
   scorefile = malloc (strlen (prefix) + strlen (argv[optind]) + 2);
   if (!scorefile)
@@ -270,7 +274,8 @@ main (int argc, char **argv)
 	scores += scorecount - max_scores;
       scorecount = max_scores;
     }
-  if (write_scores (scorefile, scores, scorecount) < 0)
+  if (write_scores (scorefile, running_sgid ? 0664 : 0644,
+		    scores, scorecount) < 0)
     {
       unlock_file (scorefile, lockstate);
       lose_syserr ("Failed to write scores file");
@@ -421,8 +426,8 @@ sort_scores (struct score_entry *scores, ptrdiff_t count, bool reverse)
 }
 
 static int
-write_scores (const char *filename, const struct score_entry *scores,
-	      ptrdiff_t count)
+write_scores (const char *filename, mode_t mode,
+	      const struct score_entry *scores, ptrdiff_t count)
 {
   int fd;
   FILE *f;
@@ -435,7 +440,7 @@ write_scores (const char *filename, const struct score_entry *scores,
   if (fd < 0)
     return -1;
 #ifndef DOS_NT
-  if (fchmod (fd, 0644) != 0)
+  if (fchmod (fd, mode) != 0)
     return -1;
 #endif
   f = fdopen (fd, "w");
diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index fbfd68e..044493d 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,8 @@
+2015-01-16  Ulrich Müller  <ulm@gentoo.org>
+
+	* play/gamegrid.el (gamegrid-add-score-with-update-game-score):
+	Allow the 'update-game-score' helper program to run suid or sgid.
+
 2015-01-16  Daniel Colascione  <dancol@dancol.org>
 
 	* cus-start.el (all): Make `ring-bell-function' customizable.
diff --git a/lisp/play/gamegrid.el b/lisp/play/gamegrid.el
index 1e265a6..b4c3c59 100644
--- a/lisp/play/gamegrid.el
+++ b/lisp/play/gamegrid.el
@@ -486,13 +486,13 @@ FILE is created there."
 	 (not (zerop (logand (file-modes
 			      (expand-file-name "update-game-score"
 						exec-directory))
-			     #o4000)))))
+			     #o6000)))))
     (cond ((file-name-absolute-p file)
 	   (gamegrid-add-score-insecure file score))
 	  ((and gamegrid-shared-game-dir
 		(file-exists-p (expand-file-name file shared-game-score-directory)))
-	   ;; Use the setuid "update-game-score" program to update a
-	   ;; system-wide score file.
+	   ;; Use the setuid (or setgid) "update-game-score" program
+	   ;; to update a system-wide score file.
 	   (gamegrid-add-score-with-update-game-score-1 file
 	    (expand-file-name file shared-game-score-directory) score))
 	  ;; Else: Add the score to a score file in the user's home
-- 
2.2.1

Re: [PATCH] Allow update-game-score to run sgid instead of suid.

[Ulrich Mueller]

>>>>> On Fri, 16 Jan 2015, I wrote:

> Currently the update-game-score program is installed with the setuid
> flag, per default to the "games" user. It is more common for games
> with a shared score file to be installed setgid "games" instead.

> The patch included below would allow running update-game-score setgid
> instead.

This I have committed some time ago.

> Questions:
> - I have kept the previous default behaviour, namely suid to "games"
>   if such a user exists. IMHO it would be better not to install any
>   suid/sgid binary by default, but only if explicitly requested by a
>   configure option.

Coming back to this. I'd expect the suid/sgid setup to exist mostly
when Emacs is being built for a distro. Otherwise, I'd rather not have
the build system pick up some random username (I admit that "games" is
fairly common, but it is not specified by any standard) and install a
suid binary for it. Such a thing should be explicitly requested by a
configure option.

So, any objections against changing the default, as indicated above?

Ulrich

Re: [PATCH] Allow update-game-score to run sgid instead of suid.

[Stefan Monnier]

> So, any objections against changing the default, as indicated above?

I'm OK with changing to setgid instead of setuid, yes (I don't have
a strong opinion either way on this).

As for defaulting to "neither setuid nor setgid", I'm less convinced.
After all, I'd expect that most cases where Emacs is built "by hand"
(rather than installed from a distro), the build will be done by
a non-privileged user, so it will already end up being neither
setgid/setuid.  IOW changing the default will end up catching distros by
surprise with no real upside.


        Stefan

Re: [PATCH] Allow update-game-score to run sgid instead of suid.

[Ulrich Mueller]

>>>>> On Thu, 05 Feb 2015, Stefan Monnier wrote:

>> So, any objections against changing the default, as indicated above?

> I'm OK with changing to setgid instead of setuid, yes (I don't have
> a strong opinion either way on this).

A small problem with this is that there seems to be no portable
command to check for existence of a group. (There is getent(1) in
GNU/Linux and {Free,Open,Net}BSD but it's not specified by POSIX.)

But I guess we can set the default in configure and try to chgrp to it
during make install. It fails softly, so installation won't abort if
the group doesn't exist.

> As for defaulting to "neither setuid nor setgid", I'm less convinced.
> After all, I'd expect that most cases where Emacs is built "by hand"
> (rather than installed from a distro), the build will be done by
> a non-privileged user, so it will already end up being neither
> setgid/setuid.  IOW changing the default will end up catching distros
> by surprise with no real upside.

There's a note in etc/NEWS ... But point taken.

Ulrich