[Emacs-diffs] master 20f6648: Allow update-game-score to run sgid instead of suid.

[Ulrich Müller]

branch: master
commit 20f66485526b69eb26f2e70bd835a5e1333559d5
Author: Ulrich Müller <address@hidden>
Commit: Ulrich Müller <address@hidden>

    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 a 'games' user
    exists.
    * 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                |   24 ++++++++++++++++++++----
 etc/NEWS                    |    7 +++++++
 lib-src/ChangeLog           |   12 ++++++++++++
 lib-src/Makefile.in         |   16 ++++++++++++----
 lib-src/update-game-score.c |   33 +++++++++++++++++++--------------
 lisp/ChangeLog              |    5 +++++
 lisp/play/gamegrid.el       |    6 +++---
 8 files changed, 85 insertions(+), 25 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 309b04f..b02203d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2015-01-21  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 a 'games' user
+       exists.
+
 2015-01-16  Paul Eggert  <address@hidden>
 
        Give up on -Wsuggest-attribute=const
diff --git a/configure.ac b/configure.ac
index 9db4bde..47b36fe 100644
--- a/configure.ac
+++ b/configure.ac
@@ -392,10 +392,25 @@ 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 a 'games' user exists])
+    if id -u games >/dev/null 2>&1; then
+      AC_MSG_RESULT([yes])
+      gameuser=games
+    else
+      AC_MSG_RESULT([no])
+    fi
+    ;;
+  :*) gamegroup=`echo "${with_gameuser}" | sed -e "s/://"` ;;
+  *) gameuser=${with_gameuser} ;;
+esac
 
 AC_ARG_WITH([gnustep-conf],dnl
 [AS_HELP_STRING([--with-gnustep-conf=FILENAME],
@@ -4684,6 +4699,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/etc/NEWS b/etc/NEWS
index 548b54d..120d8b9 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -46,6 +46,13 @@ and silent rules are now quieter.  To get the old behavior 
where
 build with 'make V=1'.
 
 ---
+** The configure option '--with-gameuser' now allows to specify a
+group instead of a user if its argument is prefixed by ':' (a colon).
+This will cause the game score files in ${localstatedir}/games/emacs
+to be owned by that group, and the helper program for updating them to
+be installed setgid.
+
+---
 ** The `grep-changelog' script (and its manual page) are no longer included.
 It has no particular connection to Emacs and has not changed in years,
 so if you want to use it, you can always take a copy from an older Emacs.
diff --git a/lib-src/ChangeLog b/lib-src/ChangeLog
index 37f037e..b67038f 100644
--- a/lib-src/ChangeLog
+++ b/lib-src/ChangeLog
@@ -1,3 +1,15 @@
+2015-01-21  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  Eli Zaretskii  <address@hidden>
 
        * Makefile.in (AM_V_RC, am__v_RC_, am__v_RC_0, am__v_RC_1): New
diff --git a/lib-src/Makefile.in b/lib-src/Makefile.in
index 01592bd..2997f1b 100644
--- a/lib-src/Makefile.in
+++ b/lib-src/Makefile.in
@@ -122,6 +122,7 @@ address@hidden@
 
 address@hidden@
 address@hidden@
address@hidden@
 
 # ==================== Utility Programs for the Build =================
 
@@ -263,10 +264,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 d13bacf..7aa66bf 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,8 @@
+2015-01-21  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-21  Stefan Monnier  <address@hidden>
 
        * emacs-lisp/eieio.el: Use cl-defmethod.
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