Subject: sail(6) security
To: None <tech-security@netbsd.org>
From: Joseph S. Myers <jsm28@cam.ac.uk>
List: tech-security
Date: 02/04/2000 12:08:09
by redmail.netbsd.org with SMTP; 4 Feb 2000 12:08:12 -0000
by red.csi.cam.ac.uk with local-esmtp (Exim 3.13 #1)
id 12GhWz-0000Am-00
for tech-security@netbsd.org; Fri, 04 Feb 2000 12:08:09 +0000
Date: Fri, 4 Feb 2000 12:08:09 +0000 (GMT)
From: "Joseph S. Myers" <jsm28@cam.ac.uk>
To: tech-security@netbsd.org
Subject: sail(6) security
Message-ID: <Pine.SOL.4.10a.10002041206010.28310-100000@red.csi.cam.ac.uk>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Currently, the game sail(6) handles interaction between multiple
players using files in /tmp with fixed names; it will follow symlinks
when opening them. The security problems are obvious.
The best fix might involve a daemon to manage games of sail instead; I
believe Paul Janzen of OpenBSD was thinking of doing this at some
point. However, most of the problems can be fixed more easily.
The patch below attempts such a fix by using a dedicated directory for
sail's sync files; the directory is made non-world-accessible so that
the user starting sail, who will own the sync files, cannot fiddle
with them from other processes. (They might still potentially be able
to leave corrupted sync files through suitably timed signals.)
Any comments on this solution and the proposed patch below? (Please
CC me on any replies.)
diff -ruN sail+/Makefile sail++/Makefile
--- sail+/Makefile Wed Feb 18 22:37:32 1998
+++ sail++/Makefile Sat Jan 29 22:08:29 2000
@@ -11,4 +11,11 @@
HIDEGAME=hidegame
SETGIDGAME=yes
+afterinstall:
+.if !defined(UNPRIVILEGED)
+ mkdir ${DESTDIR}/var/games/sail
+ chown root.games ${DESTDIR}/var/games/sail
+ chmod 2770 ${DESTDIR}/var/games/sail
+.endif
+
.include <bsd.prog.mk>
diff -ruN sail+/dr_main.c sail++/dr_main.c
--- sail+/dr_main.c Mon Oct 13 19:43:54 1997
+++ sail++/dr_main.c Sat Jan 29 22:04:18 2000
@@ -57,8 +57,6 @@
(void) signal(SIGINT, SIG_IGN);
(void) signal(SIGQUIT, SIG_IGN);
(void) signal(SIGTSTP, SIG_IGN);
- if (issetuid)
- (void) setuid(geteuid());
if (game < 0 || game >= NSCENE) {
fprintf(stderr, "DRIVER: Bad game number %d\n", game);
exit(1);
diff -ruN sail+/extern.h sail++/extern.h
--- sail+/extern.h Tue Dec 28 18:05:24 1999
+++ sail++/extern.h Sat Jan 29 22:04:18 2000
@@ -40,6 +40,7 @@
#include <string.h>
#include <ctype.h>
#include <setjmp.h>
+#include <sys/types.h>
#include "machdep.h"
/* program mode */
@@ -56,7 +57,8 @@
extern char nobells; /* -b, don't ring bell before Signal */
/* other initial modes */
-extern char issetuid; /* running setuid */
+extern gid_t gid;
+extern gid_t egid;
#define die() ((rand() >> 3) % 6 + 1)
#define sqr(a) ((a) * (a))
diff -ruN sail+/globals.c sail++/globals.c
--- sail+/globals.c Tue Dec 28 18:05:24 1999
+++ sail++/globals.c Sat Jan 29 22:04:18 2000
@@ -553,7 +553,8 @@
char longfmt; /* -l, print score in long format */
char nobells; /* -b, don't ring bell before Signal */
-char issetuid;
+gid_t gid;
+gid_t egid;
struct scenario *cc; /* the current scenario */
struct ship *ls; /* &cc->ship[cc->vessels] */
diff -ruN sail+/main.c sail++/main.c
--- sail+/main.c Mon Oct 13 21:03:55 1997
+++ sail++/main.c Sat Jan 29 22:04:18 2000
@@ -48,6 +48,7 @@
#endif /* not lint */
#include "extern.h"
+#include <fcntl.h>
#include <unistd.h>
#include <stdlib.h>
#include <string.h>
@@ -62,9 +63,18 @@
{
char *p;
int i;
+ int fd;
+
+ gid = getgid();
+ egid = getegid();
+ setegid(gid);
+
+ fd = open("/dev/null", O_RDONLY);
+ if (fd < 3)
+ exit(1);
+ close(fd);
(void) srand(getpid());
- issetuid = getuid() != geteuid();
if ((p = strrchr(*argv, '/')) != NULL)
p++;
else
diff -ruN sail+/misc.c sail++/misc.c
--- sail+/misc.c Mon Oct 13 19:44:38 1997
+++ sail++/misc.c Sat Jan 29 22:04:40 2000
@@ -208,8 +208,12 @@
float net;
struct logs *lp;
- if ((fp = fopen(_PATH_LOGFILE, "r+")) == NULL)
+ setegid(egid);
+ if ((fp = fopen(_PATH_LOGFILE, "r+")) == NULL) {
+ setegid(gid);
return;
+ }
+ setegid(gid);
#ifdef LOCK_EX
if (flock(fileno(fp), LOCK_EX) < 0)
return;
diff -ruN sail+/pathnames.h sail++/pathnames.h
--- sail+/pathnames.h Sat Apr 22 10:37:06 1995
+++ sail++/pathnames.h Sat Jan 29 22:05:58 2000
@@ -36,3 +36,5 @@
*/
#define _PATH_LOGFILE "/var/games/saillog"
+#define _PATH_SYNC "/var/games/sail/#sailsink.%d"
+#define _PATH_LOCK "/var/games/sail/#saillock.%d"
diff -ruN sail+/sync.c sail++/sync.c
--- sail+/sync.c Thu Sep 9 17:30:20 1999
+++ sail++/sync.c Sat Jan 29 22:04:49 2000
@@ -55,17 +55,18 @@
#include <sys/stat.h>
#include <time.h>
#include "extern.h"
+#include "pathnames.h"
#define BUFSIZE 4096
+static const char SF[] = _PATH_SYNC;
+static const char LF[] = _PATH_LOCK;
static char sync_buf[BUFSIZE];
static char *sync_bp = sync_buf;
-static char sync_lock[25];
-static char sync_file[25];
+static char sync_lock[sizeof SF];
+static char sync_file[sizeof LF];
static long sync_seek;
static FILE *sync_fp;
-#define SF "/tmp/#sailsink.%d"
-#define LF "/tmp/#saillock.%d"
void
fmtship(buf, len, fmt, ship)
@@ -160,30 +161,39 @@
(void) sprintf(buf, SF, game);
(void) time(&t);
- if (stat(buf, &s) < 0)
+ setegid(egid);
+ if (stat(buf, &s) < 0) {
+ setegid(gid);
return 0;
+ }
if (s.st_mtime < t - 60*60*2) { /* 2 hours */
(void) unlink(buf);
(void) sprintf(buf, LF, game);
(void) unlink(buf);
+ setegid(gid);
return 0;
- } else
+ } else {
+ setegid(gid);
return 1;
+ }
}
int
sync_open()
{
+ struct stat tmp;
if (sync_fp != NULL)
(void) fclose(sync_fp);
(void) sprintf(sync_lock, LF, game);
(void) sprintf(sync_file, SF, game);
- if (access(sync_file, 0) < 0) {
- int omask = umask(issetuid ? 077 : 011);
+ setegid(egid);
+ if (stat(sync_file, &tmp) < 0) {
+ mode_t omask = umask(002);
sync_fp = fopen(sync_file, "w+");
(void) umask(omask);
} else
sync_fp = fopen(sync_file, "r+");
+ setegid(gid);
if (sync_fp == NULL)
return -1;
sync_seek = 0;
@@ -196,8 +206,11 @@
{
if (sync_fp != 0)
(void) fclose(sync_fp);
- if (remove)
+ if (remove) {
+ setegid(egid);
(void) unlink(sync_file);
+ setegid(gid);
+ }
}
void
@@ -254,8 +267,12 @@
if (errno != EWOULDBLOCK)
return -1;
#else
- if (link(sync_file, sync_lock) >= 0)
+ setegid(egid);
+ if (link(sync_file, sync_lock) >= 0) {
+ setegid(gid);
break;
+ }
+ setegid(gid);
if (errno != EEXIST)
return -1;
#endif
@@ -319,7 +336,9 @@
#ifdef LOCK_EX
(void) flock(fileno(sync_fp), LOCK_UN);
#else
+ setegid(egid);
(void) unlink(sync_lock);
+ setegid(gid);
#endif
(void) signal(SIGHUP, sighup);
(void) signal(SIGINT, sigint);
--
Joseph S. Myers
jsm28@cam.ac.uk