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>

6 files changed, 78 insertions, 34 deletions

diff --git a/atc/Makefile b/atc/Makefile
index eb3a28d3..ad5efc6e 100644
--- 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>
@@ -19,7 +19,7 @@ SETGIDGAME=yes
 .if ${MKSHARE} != "no"
 FILES=${GAMES:S@^@${.CURDIR}/games/@g}
 FILESDIR=/usr/share/games/atc
-FILESMODE=440
+FILESMODE=444
 .endif
 
 lex.o:	grammar.h
diff --git a/atc/extern.h b/atc/extern.h
index 802ae9b2..7931c3ec 100644
--- 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
@@ -97,6 +97,7 @@ char		name __P((const PLANE *));
 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));
diff --git a/atc/input.c b/atc/input.c
index a214a12f..821d6055 100644
--- 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
@@ -50,7 +50,7 @@
 #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
 
@@ -316,7 +316,6 @@ gettoken()
 			{
 				char *shell, *base;
 
-				setuid(getuid()); /* turn off setuid bit */
 				done_screen();
 
 						 /* run user's favorite shell */
diff --git a/atc/log.c b/atc/log.c
index 1a515266..746912b4 100644
--- 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
@@ -50,13 +50,15 @@
 #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"
 
+static FILE *score_fp;
+
 int
 compar(va, vb)
 	const void *va, *vb;
@@ -101,44 +103,69 @@ timestr(t)
 	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);
-		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.
 	 */
-	fp = fdopen(fd, "r+");
-	if (fp == NULL) {
+	score_fp = fdopen(score_fd, "r+");
+	if (score_fp == NULL) {
 		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);
 	}
+
 #ifdef BSD
-	if (flock(fileno(fp), LOCK_EX) < 0)
+	if (flock(fileno(score_fp), LOCK_EX) < 0)
 #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 (;;) {
-		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,
@@ -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",
-				getuid());
+				(int)getuid());
 			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);
-			rewind(fp);
+			rewind(score_fp);
 			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);
+			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.");
@@ -230,12 +268,12 @@ log_score(list_em)
 		putchar('\n');
 	}
 #ifdef BSD
-	flock(fileno(fp), LOCK_UN);
+	flock(fileno(score_fp), LOCK_UN);
 #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("-------------------------------------------------------------------------------");
diff --git a/atc/main.c b/atc/main.c
index 8bc180d5..3b543547 100644
--- 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
@@ -55,7 +55,7 @@ __COPYRIGHT("@(#) Copyright (c) 1990, 1993\n\
 #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 */
 
@@ -78,6 +78,10 @@ main(ac, av)
 	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++;
diff --git a/atc/struct.h b/atc/struct.h
index 016f1d6c..35e302f5 100644
--- 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
@@ -107,6 +107,8 @@ typedef struct {
 	int	real_time;
 } SCORE;
 
+#define SCORE_SCANF_FMT	"%9s %255s %255s %d %d %d"
+
 typedef struct displacement {
 	int	dx;
 	int	dy;