>Number:         8015
>Category:       bin
>Synopsis:       [PATCH] Atc security
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    bin-bug-people (Utility Bug People)
>State:          open
>Class:          change-request
>Submitter-Id:   net
>Arrival-Date:   Sat Jul 17 02:35:01 1999
>Last-Modified:
>Originator:     Joseph S. Myers
>Organization:
Trinity College, University of Cambridge, UK
>Release:        NetBSD-current of 1999-07-16
>Environment:
[
System: Linux decomino 2.2.10 #1 Mon Jun 14 07:48:53 UTC 1999 i686 unknown
Architecture: i686
]
>Description:

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).

This patch is loosely based on OpenBSD.

>How-To-Repeat:

At present you can shell out with setgid games privileges, since atc
only revokes setuid on shelling out.  Corrupting the score file in
such a way as to cause a buffer overrun on reading and get access to
your target atc player's account is left as an exercise.

>Fix:

diff -ruN atc/Makefile atc+/Makefile
--- atc/Makefile	Tue Sep 29 11:05:09 1998
+++ atc+/Makefile	Sat Jul 17 09:13:33 1999
@@ -17,7 +17,7 @@
 .if !defined(NOSHARE)
 FILES=${GAMES:S@^@${.CURDIR}/games/@g}
 FILESDIR=/usr/share/games/atc
-FILESMODE=440
+FILESMODE=444
 .endif
 
 lex.o:	grammar.h
diff -ruN atc/extern.h atc+/extern.h
--- atc/extern.h	Wed Nov 11 12:11:25 1998
+++ atc+/extern.h	Wed Jan  6 07:47:04 1999
@@ -97,6 +97,7 @@
 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 -ruN atc/input.c atc+/input.c
--- atc/input.c	Wed Nov 11 12:11:25 1998
+++ atc+/input.c	Wed Jan  6 07:47:04 1999
@@ -316,7 +316,6 @@
 			{
 				char *shell, *base;
 
-				setuid(getuid()); /* turn off setuid bit */
 				done_screen();
 
 						 /* run user's favorite shell */
diff -ruN atc/log.c atc+/log.c
--- atc/log.c	Wed Nov 11 12:11:26 1998
+++ atc+/log.c	Wed Jan  6 07:47:04 1999
@@ -57,6 +57,8 @@
 #include "include.h"
 #include "pathnames.h"
 
+static FILE *score_fp;
+
 int
 compar(va, vb)
 	const void *va, *vb;
@@ -101,44 +103,69 @@
 	return (s);
 }
 
+void
+open_score_file()
+{
+	mode_t old_mask;
+	int score_fd;
+	int flags;
+
+	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;
+	}
+	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.
+	 */
+	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, fd, num_scores = 0, good, changed = 0, found = 0;
+	int		i, num_scores = 0, good, changed = 0, found = 0;
 	struct passwd	*pw;
-	FILE		*fp;
 	char		*cp;
 	SCORE		score[100], thisscore;
 	struct utsname	name;
+	long		offset;
 
-	umask(0);
-	fd = open(_PATH_SCORE, O_CREAT|O_RDWR, 0644);
-	if (fd < 0) {
-		warn("open %s", _PATH_SCORE);
-		return (-1);
-	}
-	/*
-	 * 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) {
-		warn("fdopen %s", _PATH_SCORE);
+	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 @@
 		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 @@
 			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 @@
 		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 -ruN atc/main.c atc+/main.c
--- atc/main.c	Wed Nov 11 12:11:26 1998
+++ atc+/main.c	Wed Jan  6 07:47:04 1999
@@ -78,6 +78,10 @@
 	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 -ruN atc/struct.h atc+/struct.h
--- atc/struct.h	Fri Oct 13 23:55:45 1995
+++ atc+/struct.h	Wed Jan  6 07:47:04 1999
@@ -107,6 +107,8 @@
 	int	real_time;
 } SCORE;
 
+#define SCORE_SCANF_FMT	"%9s %255s %255s %d %d %d"
+
 typedef struct displacement {
 	int	dx;
 	int	dy;
>Audit-Trail:
>Unformatted: