From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Ulrich Mueller Newsgroups: gmane.emacs.devel Subject: [PATCH] Allow update-game-score to run sgid instead of suid. Date: Fri, 16 Jan 2015 12:59:13 +0100 Message-ID: <21688.64785.754456.35609@a1i15.kph.uni-mainz.de> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable X-Trace: ger.gmane.org 1421409606 23004 80.91.229.3 (16 Jan 2015 12:00:06 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Fri, 16 Jan 2015 12:00:06 +0000 (UTC) To: emacs-devel@gnu.org Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Fri Jan 16 13:00:03 2015 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1YC5ZJ-0005aV-Qk for ged-emacs-devel@m.gmane.org; Fri, 16 Jan 2015 13:00:02 +0100 Original-Received: from localhost ([::1]:55209 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YC5ZJ-0002S7-DF for ged-emacs-devel@m.gmane.org; Fri, 16 Jan 2015 07:00:01 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:34026) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YC5Yf-0001mU-9M for emacs-devel@gnu.org; Fri, 16 Jan 2015 06:59:25 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YC5Ya-0001fR-FT for emacs-devel@gnu.org; Fri, 16 Jan 2015 06:59:21 -0500 Original-Received: from a1www.kph.uni-mainz.de ([134.93.134.1]:48572) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YC5Ya-0001ep-6J for emacs-devel@gnu.org; Fri, 16 Jan 2015 06:59:16 -0500 Original-Received: from a1i15.kph.uni-mainz.de (a1i15.kph.uni-mainz.de [134.93.134.92]) by a1www.kph.uni-mainz.de (8.14.9/8.14.7) with ESMTP id t0GBxEeE005036 for ; Fri, 16 Jan 2015 12:59:14 +0100 Original-Received: from a1i15.kph.uni-mainz.de (localhost [127.0.0.1]) by a1i15.kph.uni-mainz.de (8.14.8/8.14.2) with ESMTP id t0GBxE7Y019285; Fri, 16 Jan 2015 12:59:14 +0100 Original-Received: (from ulm@localhost) by a1i15.kph.uni-mainz.de (8.14.8/8.14.8/Submit) id t0GBxDpn019281; Fri, 16 Jan 2015 12:59:13 +0100 X-Mailer: VM 8.2.0b under 24.3.1 (x86_64-pc-linux-gnu) X-MIME-Autoconverted: from 8bit to quoted-printable by a1www.kph.uni-mainz.de id t0GBxEeE005036 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 134.93.134.1 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:181329 Archived-At: 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: =3D?UTF-8?q?Ulrich=3D20M=3DC3=3DBCller?=3D 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=FCller + + * 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 =20 Give up on -Wsuggest-attribute=3Dconst 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=3D install]) =20 AC_ARG_WITH(gameuser,dnl -[AS_HELP_STRING([--with-gameuser=3DUSER],[user for shared game score fil= es])]) -test "X${with_gameuser}" !=3D X && test "${with_gameuser}" !=3D yes \ - && gameuser=3D"${with_gameuser}" -test "X$gameuser" =3D X && gameuser=3Dgames +[AS_HELP_STRING([--with-gameuser=3DUSER_OR_GROUP], + [user for shared game score files. + An argument prefixed by ':' specifies a group instead.])]) +gameuser=3D +gamegroup=3D +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=3Dgames + else + AC_MSG_RESULT([no]) + fi + rm -f conftest.tmp + ;; + :*) gamegroup=3D${with_gameuser#:} ;; + *) gameuser=3D${with_gameuser} ;; +esac =20 AC_ARG_WITH([gnustep-conf],dnl [AS_HELP_STRING([--with-gnustep-conf=3DFILENAME], @@ -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=FCller + + * 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 =20 Give up on -Wsuggest-attribute=3Dconst 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=3D@archlibdir@ =20 gamedir=3D@gamedir@ gameuser=3D@gameuser@ +gamegroup=3D@gamegroup@ =20 # =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D Utility P= rograms for the Build =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D =20 @@ -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${EXEEX= T}" && chmod u+s "$(DESTDIR)${archlibdir}/update-game-score${EXEEXT}"; th= en \ - chown ${gameuser} "$(DESTDIR)${gamedir}"; \ - chmod u=3Drwx,g=3Drwx,o=3Drx "$(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=3Drwx,g=3Drx,o=3Drx "$(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=3Drwx,g=3Drwx,o=3Drx "$(DESTDIR)${gamedir}" +endif exp_archlibdir=3D`cd "$(DESTDIR)${archlibdir}" && /bin/pwd`; \ if [ "$$exp_archlibdir" !=3D "`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 . */ =20 =20 /* 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'. =20 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, ptr= diff_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); =20 static _Noreturn void @@ -122,18 +122,19 @@ get_user_id (void) } =20 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 =3D=3D 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 a= nd 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 =3D=3D NULL) + lose ("Not using a shared game directory, and no prefix given."); return user_prefix; } =20 @@ -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); =20 running_suid =3D (getuid () !=3D geteuid ()); + running_sgid =3D (getgid () !=3D getegid ()); + if (running_suid && running_sgid) + lose ("This program can run either suid or sgid, but not both."); =20 - prefix =3D get_prefix (running_suid, user_prefix); + prefix =3D get_prefix (running_suid || running_sgid, user_prefix); =20 scorefile =3D malloc (strlen (prefix) + strlen (argv[optind]) + 2); if (!scorefile) @@ -270,7 +274,8 @@ main (int argc, char **argv) scores +=3D scorecount - max_scores; scorecount =3D 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 co= unt, bool reverse) } =20 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 scor= e_entry *scores, if (fd < 0) return -1; #ifndef DOS_NT - if (fchmod (fd, 0644) !=3D 0) + if (fchmod (fd, mode) !=3D 0) return -1; #endif f =3D 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=FCller + + * 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 =20 * 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 --=20 2.2.1