[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?= <address@hidden>
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  <address@hidden>
+
+       * 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  <address@hidden>
 
        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 @address@hidden
 ## 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  <address@hidden>
+
+       * 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  <address@hidden>
 
        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 @@ address@hidden@
 
 address@hidden@
 address@hidden@
address@hidden@
 
 # ==================== 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  <address@hidden>
+
+       * 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  <address@hidden>
 
        * 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