From owner-freebsd-audit Fri Sep 14 2:27:20 2001 Delivered-To: freebsd-audit@freebsd.org Received: from whale.sunbay.crimea.ua (whale.sunbay.crimea.ua [212.110.138.65]) by hub.freebsd.org (Postfix) with ESMTP id E78B737B411 for ; Fri, 14 Sep 2001 02:27:01 -0700 (PDT) Received: (from ru@localhost) by whale.sunbay.crimea.ua (8.11.2/8.11.2) id f8E9QEV02656; Fri, 14 Sep 2001 12:26:14 +0300 (EEST) (envelope-from ru) Date: Fri, 14 Sep 2001 12:26:14 +0300 From: Ruslan Ermilov To: "Andrew R. Reiter" Cc: freebsd-audit@FreeBSD.ORG Subject: Re: dungeon master patch Message-ID: <20010914122614.A82568@sunbay.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.2.5i In-Reply-To: ; from arr@watson.org on Fri, Sep 14, 2001 at 12:08:35AM -0400 Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG On Fri, Sep 14, 2001 at 12:08:35AM -0400, Andrew R. Reiter wrote: > hey, > > I just started to go through -current (seemingly on default) s{g,u}id bins > and their source for security vulns. I found a few definet coding > problems in dungeon master (setgid games ;-)), however, since you can't > specify the config file, they are probably non-exploitable. but, hey, > it's being installed setgid (even if it is games), might as well use good > coding practice. > > the patch is attached, and also can be found at: > At a quick glance. > --- dm.c.orig Thu Sep 13 22:44:25 2001 > +++ dm.c Thu Sep 13 23:02:43 2001 > @@ -111,15 +111,16 @@ > play(args) > char **args; > { > - char pbuf[MAXPATHLEN]; > + char pbuf[MAXPATHLEN+1]; > Err, the correct value is MAXPATHLEN. Search intro(2) for more details. > - if (sizeof(_PATH_HIDE) + strlen(game) > sizeof(pbuf)) { > + if (sizeof(_PATH_HIDE) + strlen(game) + 1 > sizeof(pbuf)) { Err, sizeof("foo") is 4, not 3. Hence the space for final '\0' is already accumulated with sizeof(_PATH_HIDE). > (void)fprintf(stderr, "dm: %s/%s: %s\n", _PATH_HIDE, game, > strerror(ENAMETOOLONG)); > exit(1); > } > - (void)strcpy(pbuf, _PATH_HIDE); > - (void)strcpy(pbuf + sizeof(_PATH_HIDE) - 1, game); > + bzero((void *)&pbuf, MAXPATHLEN+1); > + strlcpy(pbuf, _PATH_HIDE, sizeof(pbuf)); > + strlcat(pbuf+strlen(_PATH_HIDE), game, sizeof(pbuf)-strlen(_PATH_HIDE)); There was nothing wrong with this. > if (priority > 0) /* < 0 requires root */ > (void)setpriority(PRIO_PROCESS, 0, priority); > execv(pbuf, args); > @@ -135,30 +136,37 @@ > read_config() > { > FILE *cfp; > - char lbuf[BUFSIZ], f1[40], f2[40], f3[40], f4[40], f5[40]; > + char lbuf[BUFSIZ+1], f1[40], f2[40], f3[40], f4[40], f5[40]; > > if (!(cfp = fopen(_PATH_CONFIG, "r"))) > return; > - while (fgets(lbuf, sizeof(lbuf), cfp)) > + while (fgets(lbuf, sizeof(lbuf)-1, cfp)) { The fgets() function reads at most one less than the number of characters specified by size from the given stream and stores them in the string str. > + bzero(&f1, sizeof(f1)); > + bzero(&f2, sizeof(f2)); > + bzero(&f3, sizeof(f3)); > + bzero(&f4, sizeof(f4)); > + bzero(&f5, sizeof(f5)); Umm, sscanf(3) places a trailing NUL character: : s Matches a sequence of non-white-space characters; the next pointer : must be a pointer to char, and the array must be large enough to : accept all the sequence and the terminating NUL character. The : input string stops at white space or at the maximum field width, : whichever occurs first. > switch(*lbuf) { > case 'b': /* badtty */ > - if (sscanf(lbuf, "%s%s", f1, f2) != 2 || > + if (sscanf(lbuf, "%39s%39s", f1, f2) != 2 || Use "%.s" and "sizeof(f1) - 1", don't hardcode. > strcasecmp(f1, "badtty")) > break; > c_tty(f2); > break; > case 'g': /* game */ > - if (sscanf(lbuf, "%s%s%s%s%s", > + if (sscanf(lbuf, "%39s%39s%39s%39s%39s", > f1, f2, f3, f4, f5) != 5 || strcasecmp(f1, "game")) > break; > c_game(f2, f3, f4, f5); > break; > case 't': /* time */ > - if (sscanf(lbuf, "%s%s%s%s", f1, f2, f3, f4) != 4 || > - strcasecmp(f1, "time")) > + if (sscanf(lbuf, "%39s%39s%39s%39s", > + f1, f2, f3, f4) != 4 || strcasecmp(f1, "time")) > break; > c_day(f2, f3, f4); > } > + bzero(&lbuf, sizeof(lbuf)); > + } > (void)fclose(cfp); > } > -- Ruslan Ermilov Oracle Developer/DBA, ru@sunbay.com Sunbay Software AG, ru@FreeBSD.org FreeBSD committer, +380.652.512.251 Simferopol, Ukraine http://www.FreeBSD.org The Power To Serve http://www.oracle.com Enabling The Information Age To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message