The patch below improves the security of the game atc(6), by having it

open the score file at the start and then drop all setgid privileges
while keeping a (close-on-exec) file descriptor open to it.  In order
to allow this the static data files have to be made world readable.
In addition a potential buffer overrun with corrupted score files is
avoided by more careful use of scanf (note that SCORE_SCANF_FMT is
defined alongside the definition of the relevant structure).

Submitted in PR 8015 by Joseph Myers <jsm28@cam.ac.uk>

diff --git a/atc/Makefile b/atc/Makefile
index eb3a28d390f9c780cf6c3378c7e6648a987be968..ad5efc6ea081c7dbc185335fe0c198c895d1ec23 100644 (file)
--- a/atc/Makefile
+++ b/atc/Makefile
@@ -1,4 +1,4 @@
-#      $NetBSD: Makefile,v 1.21 1999/02/13 02:54:20 lukem Exp $
+#      $NetBSD: Makefile,v 1.22 1999/07/17 19:57:03 hubertf Exp $

 #      @(#)Makefile    8.1 (Berkeley) 5/31/93
 
 .include <bsd.own.mk>
 #      @(#)Makefile    8.1 (Berkeley) 5/31/93
 
 .include <bsd.own.mk>


@@ -19,7 +19,7 @@ SETGIDGAME=yes
 .if ${MKSHARE} != "no"
 FILES=${GAMES:S@^@${.CURDIR}/games/@g}
 FILESDIR=/usr/share/games/atc
 .if ${MKSHARE} != "no"
 FILES=${GAMES:S@^@${.CURDIR}/games/@g}
 FILESDIR=/usr/share/games/atc

-FILESMODE=440
+FILESMODE=444

 .endif
 
 lex.o: grammar.h
 .endif
 
 lex.o: grammar.h

diff --git a/atc/extern.h b/atc/extern.h
index 802ae9b2af5461dc11a205f431917c1e77ad7784..7931c3ec49fbb14e3c3584a1a813cd780260838c 100644 (file)
--- a/atc/extern.h
+++ b/atc/extern.h
@@ -1,4 +1,4 @@
-/*     $NetBSD: extern.h,v 1.7 1998/11/10 13:43:30 hubertf Exp $       */
+/*     $NetBSD: extern.h,v 1.8 1999/07/17 19:57:03 hubertf Exp $       */

 
 /*-
  * Copyright (c) 1990, 1993
 
 /*-
  * Copyright (c) 1990, 1993


@@ -97,6 +97,7 @@ char          name __P((const PLANE *));
 int            next_plane __P((void));
 void           noise __P((void));
 int            number __P((char));
 int            next_plane __P((void));
 void           noise __P((void));
 int            number __P((char));

+void           open_score_file __P((void));

 void           planewin __P((void));
 int            pop __P((void));
 void           push __P((int, int));
 void           planewin __P((void));
 int            pop __P((void));
 void           push __P((int, int));

diff --git a/atc/input.c b/atc/input.c
index a214a12f217d5c515186737d41362963535a2fd4..821d6055034103cb050cf64f798bdddd66ecddf2 100644 (file)
--- a/atc/input.c
+++ b/atc/input.c
@@ -1,4 +1,4 @@
-/*     $NetBSD: input.c,v 1.11 1998/11/10 13:43:31 hubertf Exp $       */
+/*     $NetBSD: input.c,v 1.12 1999/07/17 19:57:03 hubertf Exp $       */

 
 /*-
  * Copyright (c) 1990, 1993
 
 /*-
  * Copyright (c) 1990, 1993


@@ -50,7 +50,7 @@
 #if 0
 static char sccsid[] = "@(#)input.c    8.1 (Berkeley) 5/31/93";
 #else
 #if 0
 static char sccsid[] = "@(#)input.c    8.1 (Berkeley) 5/31/93";
 #else

-__RCSID("$NetBSD: input.c,v 1.11 1998/11/10 13:43:31 hubertf Exp $");
+__RCSID("$NetBSD: input.c,v 1.12 1999/07/17 19:57:03 hubertf Exp $");

 #endif
 #endif not lint
 
 #endif
 #endif not lint
 


@@ -316,7 +316,6 @@ gettoken()
                        {
                                char *shell, *base;
 
                        {
                                char *shell, *base;
 

-                               setuid(getuid()); /* turn off setuid bit */

                                done_screen();
 
                                                 /* run user's favorite shell */
                                done_screen();
 
                                                 /* run user's favorite shell */

diff --git a/atc/log.c b/atc/log.c
index 1a51526625d31693c5172d058cf695e57461a416..746912b48b9a2bcef57d9b3132c9a9fd1446b233 100644 (file)
--- a/atc/log.c
+++ b/atc/log.c
@@ -1,4 +1,4 @@
-/*     $NetBSD: log.c,v 1.8 1998/11/10 13:43:31 hubertf Exp $  */
+/*     $NetBSD: log.c,v 1.9 1999/07/17 19:57:03 hubertf Exp $  */

 
 /*-
  * Copyright (c) 1990, 1993
 
 /*-
  * Copyright (c) 1990, 1993


@@ -50,13 +50,15 @@
 #if 0
 static char sccsid[] = "@(#)log.c      8.1 (Berkeley) 5/31/93";
 #else
 #if 0
 static char sccsid[] = "@(#)log.c      8.1 (Berkeley) 5/31/93";
 #else

-__RCSID("$NetBSD: log.c,v 1.8 1998/11/10 13:43:31 hubertf Exp $");
+__RCSID("$NetBSD: log.c,v 1.9 1999/07/17 19:57:03 hubertf Exp $");

 #endif
 #endif not lint
 
 #include "include.h"
 #include "pathnames.h"
 
 #endif
 #endif not lint
 
 #include "include.h"
 #include "pathnames.h"
 

+static FILE *score_fp;
+

 int
 compar(va, vb)
        const void *va, *vb;
 int
 compar(va, vb)
        const void *va, *vb;


@@ -101,44 +103,69 @@ timestr(t)
        return (s);
 }
 
        return (s);
 }
 

-int
-log_score(list_em)
-       int list_em;
+void
+open_score_file()

 {
 {

-       int             i, fd, num_scores = 0, good, changed = 0, found = 0;
-       struct passwd   *pw;
-       FILE            *fp;
-       char            *cp;
-       SCORE           score[100], thisscore;
-       struct utsname  name;
+       mode_t old_mask;
+       int score_fd;
+       int flags;

 
 

-       umask(0);
-       fd = open(_PATH_SCORE, O_CREAT|O_RDWR, 0644);
-       if (fd < 0) {
+       old_mask = umask(0);
+       score_fd = open(_PATH_SCORE, O_CREAT|O_RDWR, 0664);
+       umask(old_mask);
+       if (score_fd < 0) {

                warn("open %s", _PATH_SCORE);
                warn("open %s", _PATH_SCORE);

-               return (-1);
+               return;

        }
        }

+       if (score_fd < 3)
+               exit(1);
+       /* Set the close-on-exec flag.  If this fails for any reason, quit
+        * rather than leave the score file open to tampering.  */
+       flags = fcntl(score_fd, F_GETFD);
+       if (flags < 0)
+               err(1, "fcntl F_GETFD");
+       flags |= FD_CLOEXEC;
+       if (fcntl(score_fd, F_SETFD, flags) == -1)
+               err(1, "fcntl F_SETFD");

        /*
         * This is done to take advantage of stdio, while still 
         * allowing a O_CREAT during the open(2) of the log file.
         */
        /*
         * This is done to take advantage of stdio, while still 
         * allowing a O_CREAT during the open(2) of the log file.
         */

-       fp = fdopen(fd, "r+");
-       if (fp == NULL) {
+       score_fp = fdopen(score_fd, "r+");
+       if (score_fp == NULL) {

                warn("fdopen %s", _PATH_SCORE);
                warn("fdopen %s", _PATH_SCORE);

+               return;
+       }
+}
+
+int
+log_score(list_em)
+       int list_em;
+{
+       int             i, num_scores = 0, good, changed = 0, found = 0;
+       struct passwd   *pw;
+       char            *cp;
+       SCORE           score[100], thisscore;
+       struct utsname  name;
+       long            offset;
+
+       if (score_fp == NULL) {
+               warnx("no score file available");

                return (-1);
        }
                return (-1);
        }

+

 #ifdef BSD
 #ifdef BSD

-       if (flock(fileno(fp), LOCK_EX) < 0)
+       if (flock(fileno(score_fp), LOCK_EX) < 0)

 #endif
 #ifdef SYSV
 #endif
 #ifdef SYSV

-       while (lockf(fileno(fp), F_LOCK, 1) < 0)
+       while (lockf(fileno(score_fp), F_LOCK, 1) < 0)

 #endif
        {
                warn("flock %s", _PATH_SCORE);
                return (-1);
        }
        for (;;) {
 #endif
        {
                warn("flock %s", _PATH_SCORE);
                return (-1);
        }
        for (;;) {

-               good = fscanf(fp, "%s %s %s %d %d %d",
+               good = fscanf(score_fp, SCORE_SCANF_FMT,

                        score[num_scores].name, 
                        score[num_scores].host, 
                        score[num_scores].game,
                        score[num_scores].name, 
                        score[num_scores].host, 
                        score[num_scores].game,


@@ -152,7 +179,7 @@ log_score(list_em)
                if ((pw = (struct passwd *) getpwuid(getuid())) == NULL) {
                        fprintf(stderr, 
                                "getpwuid failed for uid %d.  Who are you?\n",
                if ((pw = (struct passwd *) getpwuid(getuid())) == NULL) {
                        fprintf(stderr, 
                                "getpwuid failed for uid %d.  Who are you?\n",

-                               getuid());
+                               (int)getuid());

                        return (-1);
                }
                strcpy(thisscore.name, pw->pw_name);
                        return (-1);
                }
                strcpy(thisscore.name, pw->pw_name);


@@ -215,12 +242,23 @@ log_score(list_em)
                        else
                                puts("You made the top players list!");
                        qsort(score, num_scores, sizeof (*score), compar);
                        else
                                puts("You made the top players list!");
                        qsort(score, num_scores, sizeof (*score), compar);

-                       rewind(fp);
+                       rewind(score_fp);

                        for (i = 0; i < num_scores; i++)
                        for (i = 0; i < num_scores; i++)

-                               fprintf(fp, "%s %s %s %d %d %d\n",
+                               fprintf(score_fp, "%s %s %s %d %d %d\n",

                                        score[i].name, score[i].host, 
                                        score[i].game, score[i].planes,
                                        score[i].time, score[i].real_time);
                                        score[i].name, score[i].host, 
                                        score[i].game, score[i].planes,
                                        score[i].time, score[i].real_time);

+                       fflush(score_fp);
+                       if (ferror(score_fp))
+                               warn("error writing %s", _PATH_SCORE);
+                       /* It is just possible that updating an entry could
+                        * have reduced the length of the file, so we
+                        * truncate it.  The seeks are required for stream/fd
+                        * synchronisation by POSIX.1.  */
+                       offset = ftell(score_fp);
+                       lseek(fileno(score_fp), 0, SEEK_SET);
+                       ftruncate(fileno(score_fp), offset);
+                       rewind(score_fp);

                } else {
                        if (found)
                                puts("You didn't beat your previous score.");
                } else {
                        if (found)
                                puts("You didn't beat your previous score.");


@@ -230,12 +268,12 @@ log_score(list_em)
                putchar('\n');
        }
 #ifdef BSD
                putchar('\n');
        }
 #ifdef BSD

-       flock(fileno(fp), LOCK_UN);
+       flock(fileno(score_fp), LOCK_UN);

 #endif
 #ifdef SYSV
        /* lock will evaporate upon close */
 #endif
 #endif
 #ifdef SYSV
        /* lock will evaporate upon close */
 #endif

-       fclose(fp);
+       fclose(score_fp);

        printf("%2s:  %-8s  %-8s  %-18s  %4s  %9s  %4s\n", "#", "name", "host", 
                "game", "time", "real time", "planes safe");
        puts("-------------------------------------------------------------------------------");
        printf("%2s:  %-8s  %-8s  %-18s  %4s  %9s  %4s\n", "#", "name", "host", 
                "game", "time", "real time", "planes safe");
        puts("-------------------------------------------------------------------------------");

diff --git a/atc/main.c b/atc/main.c
index 8bc180d57d866b01ca3cd01cf88cd1b9022c45f2..3b5435478b0e03e283f600837d50868007b86c24 100644 (file)
--- a/atc/main.c
+++ b/atc/main.c
@@ -1,4 +1,4 @@
-/*     $NetBSD: main.c,v 1.8 1998/11/10 13:43:31 hubertf Exp $ */
+/*     $NetBSD: main.c,v 1.9 1999/07/17 19:57:03 hubertf Exp $ */

 
 /*-
  * Copyright (c) 1990, 1993
 
 /*-
  * Copyright (c) 1990, 1993


@@ -55,7 +55,7 @@ __COPYRIGHT("@(#) Copyright (c) 1990, 1993\n\
 #if 0
 static char sccsid[] = "@(#)main.c     8.1 (Berkeley) 5/31/93";
 #else
 #if 0
 static char sccsid[] = "@(#)main.c     8.1 (Berkeley) 5/31/93";
 #else

-__RCSID("$NetBSD: main.c,v 1.8 1998/11/10 13:43:31 hubertf Exp $");
+__RCSID("$NetBSD: main.c,v 1.9 1999/07/17 19:57:03 hubertf Exp $");

 #endif
 #endif /* not lint */
 
 #endif
 #endif /* not lint */
 


@@ -78,6 +78,10 @@ main(ac, av)
        struct itimerval        itv;
 #endif
 
        struct itimerval        itv;
 #endif
 

+       /* Open the score file then revoke setgid privileges */
+       open_score_file();
+       setregid(getgid(), getgid());
+

        start_time = seed = time(0);
 
        name = *av++;
        start_time = seed = time(0);
 
        name = *av++;

diff --git a/atc/struct.h b/atc/struct.h
index 016f1d6c0e38a52086b43895a5e5ea49b61892b8..35e302f598ad44ab9997424fd53090481d625083 100644 (file)
--- a/atc/struct.h
+++ b/atc/struct.h
@@ -1,4 +1,4 @@
-/*     $NetBSD: struct.h,v 1.3 1995/03/21 15:04:31 cgd Exp $   */
+/*     $NetBSD: struct.h,v 1.4 1999/07/17 19:57:03 hubertf Exp $       */

 
 /*-
  * Copyright (c) 1990, 1993
 
 /*-
  * Copyright (c) 1990, 1993


@@ -107,6 +107,8 @@ typedef struct {
        int     real_time;
 } SCORE;
 
        int     real_time;
 } SCORE;
 

+#define SCORE_SCANF_FMT        "%9s %255s %255s %d %d %d"
+

 typedef struct displacement {
        int     dx;
        int     dy;
 typedef struct displacement {
        int     dx;
        int     dy;