From 3809a8fdebc7087514443871f875d64c9e24e447 Mon Sep 17 00:00:00 2001 From: hubertf Date: Sat, 17 Jul 1999 19:57:03 +0000 Subject: 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 --- atc/Makefile | 4 +-- atc/extern.h | 3 ++- atc/input.c | 5 ++-- atc/log.c | 88 +++++++++++++++++++++++++++++++++++++++++++----------------- atc/main.c | 8 ++++-- atc/struct.h | 4 ++- 6 files changed, 78 insertions(+), 34 deletions(-) (limited to 'atc') 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 @@ -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; -- cgit v1.2.3-56-ge451