>Number:         5945
>Category:       bin
>Synopsis:       [PATCH] Security of NetBSD games 1
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    bin-bug-people (Utility Bug People)
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Aug  9 10:35:00 1998
>Last-Modified:
>Originator:     Joseph Samuel Myers
>Organization:
Trinity College, University of Cambridge, UK
>Release:        NetBSD-current of 1998-07-27
>Environment:
	
[
System: Linux octomino 2.0.35 #1 Tue Jul 14 19:09:50 UTC 1998 i586 unknown
Architecture: i586
]
>Description:

The NetBSD games are liable to end up running setgid games, either to
access score files, or simply because they were invoked through dm(8);
but in general they are not designed to operate this way and do not
drop privileges when they should, or were expected to be setuid and
only drop their effective uid.  These problems are for the most part
fixed in OpenBSD; I have been adapting their changes for the Linux
port.  The patch for the first few games is appended; others should
follow.

The general principles of these changes are:

(i) Games not needing to run setgid drop all privileges (including
saved gid) on startup.

(ii) Games needing to run setgid to access score files open the score
file on startup, and then drop all privileges (including saved gid)
while leaving the score file open.

>How-To-Repeat:

>Fix:

This patch covers the games: adventure, arithmetic, atc, backgammon,
banner.  Please check that the use of setregid(getgid(), getgid())
(the appropriate operation under Linux) will have the desired effect
(including dropping saved gid) under NetBSD before applying.

diff -ruN netbsd/adventure/main.c netbsd+security/adventure/main.c
--- netbsd/adventure/main.c	Sat Oct 11 11:49:23 1997
+++ netbsd+security/adventure/main.c	Sun Aug  9 17:06:59 1998
@@ -55,7 +55,6 @@
 /*      Re-coding of advent in C: main program */
 
 #include <sys/file.h>
-#include <err.h>
 #include <signal.h>
 #include <stdio.h>
 #include <unistd.h>
@@ -71,9 +70,8 @@
 	int     rval, ll;
 	struct text *kk;
 
-	/* adventure doesn't need setuid-ness, so, just get rid of it */
-	if (setuid(getuid()) < 0)
-		warn("setuid");
+	/* revoke setgid privileges */
+	setregid(getgid(), getgid());
 
 	init(NULL);		/* Initialize everything */
 	signal(SIGINT, trapdel);
diff -ruN netbsd/arithmetic/arithmetic.c netbsd+security/arithmetic/arithmetic.c
--- netbsd/arithmetic/arithmetic.c	Tue Feb  3 12:33:56 1998
+++ netbsd+security/arithmetic/arithmetic.c	Sun Aug  9 17:06:59 1998
@@ -122,6 +122,9 @@
 	extern int optind;
 	int ch, cnt;
 
+	/* Revoke setgid privileges */
+	setregid(getgid(), getgid());
+
 	while ((ch = getopt(argc, argv, "r:o:")) != -1)
 		switch(ch) {
 		case 'o': {
diff -ruN netbsd/atc/extern.h netbsd+security/atc/extern.h
--- netbsd/atc/extern.h	Fri Oct 10 11:21:28 1997
+++ netbsd+security/atc/extern.h	Sun Aug  9 17:06:59 1998
@@ -96,6 +96,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 netbsd/atc/log.c netbsd+security/atc/log.c
--- netbsd/atc/log.c	Sat Jul 25 11:06:20 1998
+++ netbsd+security/atc/log.c	Sun Aug  9 17:11:57 1998
@@ -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,57 @@
 	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;
 
-	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;
 	}
 	/*
 	 * 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;
+
+	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, "%s %s %s %d %d %d",
 			score[num_scores].name, 
 			score[num_scores].host, 
 			score[num_scores].game,
@@ -152,7 +167,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 +230,21 @@
 			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 lseek is required for stream/fd
+			 * synchronisation by POSIX.1.  */
+			lseek(fileno(score_fp), 0, SEEK_END);
+			ftruncate(fileno(score_fp), ftell(score_fp));
 		} else {
 			if (found)
 				puts("You didn't beat your previous score.");
@@ -230,12 +254,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 netbsd/atc/main.c netbsd+security/atc/main.c
--- netbsd/atc/main.c	Sat Oct 11 11:49:42 1997
+++ netbsd+security/atc/main.c	Sun Aug  9 17:06:59 1998
@@ -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 netbsd/backgammon/backgammon/main.c netbsd+security/backgammon/backgammon/main.c
--- netbsd/backgammon/backgammon/main.c	Sun Mar 29 12:13:44 1998
+++ netbsd+security/backgammon/backgammon/main.c	Sun Aug  9 17:08:00 1998
@@ -110,6 +110,9 @@
 	char    c;		/* non-descript character storage */
 	long    t;		/* time for random num generator */
 
+	/* revoke setgid privileges */
+	setregid(getgid(), getgid());
+
 	/* initialization */
 	bflag = 2;		/* default no board */
 	signal(2, getout);	/* trap interrupts */
diff -ruN netbsd/backgammon/teachgammon/teach.c netbsd+security/backgammon/teachgammon/teach.c
--- netbsd/backgammon/teachgammon/teach.c	Fri Oct 10 11:22:16 1997
+++ netbsd+security/backgammon/teachgammon/teach.c	Sun Aug  9 17:08:27 1998
@@ -72,6 +72,9 @@
 {
 	int     i;
 
+	/* revoke setgid privileges */
+	setregid(getgid(), getgid());
+
 	signal(2, getout);
 	if (tcgetattr(0, &old) == -1)	/* get old tty mode */
 		errexit("teachgammon(gtty)");
diff -ruN netbsd/banner/banner.c netbsd+security/banner/banner.c
--- netbsd/banner/banner.c	Fri Oct 10 11:22:45 1997
+++ netbsd+security/banner/banner.c	Sun Aug  9 17:06:59 1998
@@ -1039,6 +1039,9 @@
 { 
 	int ch;
 
+	/* revoke setgid privileges */
+	setregid(getgid(), getgid());
+
 	while ((ch = getopt(argc, argv, "w:td")) != -1)
 		switch (ch) {
 		case 'd':
>Audit-Trail:
>Unformatted: