From owner-freebsd-audit Tue Oct 24 5: 5:14 2000 Delivered-To: freebsd-audit@freebsd.org Received: from lucifer.ninth-circle.org (lucifer.bart.nl [194.158.168.74]) by hub.freebsd.org (Postfix) with ESMTP id 1178A37B479 for ; Tue, 24 Oct 2000 05:05:12 -0700 (PDT) Received: (from asmodai@localhost) by lucifer.ninth-circle.org (8.11.0/8.11.0) id e9OC5A595360 for audit@freebsd.org; Tue, 24 Oct 2000 14:05:10 +0200 (CEST) (envelope-from asmodai) Date: Tue, 24 Oct 2000 14:05:10 +0200 From: Jeroen Ruigrok van der Werven To: audit@freebsd.org Subject: printjob.c mktemp() problem Message-ID: <20001024140510.G93799@lucifer.bart.nl> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.2.5i Organisation: VIA Net.Works The Netherlands Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG In printjob.c in the dir src/usr.sbin/lpr/lpd we see a mktemp() call which creates a file accroding to the template. I also see that it is getting unlink()'d again a bunch of lines later. I later on see some open() call on the same tempfile array which does exactly what the mktemp(3) manpage warns about. Am I right into thinking this might be a good candidate for a mktemp()->mkstemp() conversion? -- Jeroen Ruigrok van der Werven Network- and systemadministrator VIA Net.Works The Netherlands BSD: Technical excellence at its best http://www.via-net-works.nl Asleep is the Rose, in tired innocence dreaming Time away... To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Tue Oct 24 5:59:47 2000 Delivered-To: freebsd-audit@freebsd.org Received: from rover.village.org (rover.village.org [204.144.255.49]) by hub.freebsd.org (Postfix) with ESMTP id E913B37B4CF for ; Tue, 24 Oct 2000 05:59:44 -0700 (PDT) Received: from harmony.village.org (harmony.village.org [10.0.0.6]) by rover.village.org (8.11.0/8.11.0) with ESMTP id e9OCxhn86371; Tue, 24 Oct 2000 06:59:44 -0600 (MDT) (envelope-from imp@harmony.village.org) Received: from harmony.village.org (localhost.village.org [127.0.0.1]) by harmony.village.org (8.9.3/8.8.3) with ESMTP id GAA15161; Tue, 24 Oct 2000 06:59:43 -0600 (MDT) Message-Id: <200010241259.GAA15161@harmony.village.org> To: Jeroen Ruigrok van der Werven Subject: Re: printjob.c mktemp() problem Cc: audit@FreeBSD.ORG In-reply-to: Your message of "Tue, 24 Oct 2000 14:05:10 +0200." <20001024140510.G93799@lucifer.bart.nl> References: <20001024140510.G93799@lucifer.bart.nl> Date: Tue, 24 Oct 2000 06:59:43 -0600 From: Warner Losh Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG In message <20001024140510.G93799@lucifer.bart.nl> Jeroen Ruigrok van der Werven writes: : Am I right into thinking this might be a good candidate for a : mktemp()->mkstemp() conversion? Yes. Almost all of the uses of mktemp in the tree are good candidates for conversion. Warner To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Tue Oct 24 6: 6:56 2000 Delivered-To: freebsd-audit@freebsd.org Received: from peitho.fxp.org (peitho.fxp.org [209.26.95.40]) by hub.freebsd.org (Postfix) with ESMTP id 2393937B4CF for ; Tue, 24 Oct 2000 06:06:55 -0700 (PDT) Received: by peitho.fxp.org (Postfix, from userid 1501) id 965DD1360E; Tue, 24 Oct 2000 09:06:59 -0400 (EDT) Date: Tue, 24 Oct 2000 09:06:59 -0400 From: Chris Faulhaber To: Jeroen Ruigrok van der Werven Cc: audit@freebsd.org Subject: Re: printjob.c mktemp() problem Message-ID: <20001024090659.A80998@peitho.fxp.org> References: <20001024140510.G93799@lucifer.bart.nl> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.2.5i In-Reply-To: <20001024140510.G93799@lucifer.bart.nl>; from jruigrok@via-net-works.nl on Tue, Oct 24, 2000 at 02:05:10PM +0200 Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG On Tue, Oct 24, 2000 at 02:05:10PM +0200, Jeroen Ruigrok van der Werven wrote: > In printjob.c in the dir src/usr.sbin/lpr/lpd we see a mktemp() call > which creates a file accroding to the template. > > I also see that it is getting unlink()'d again a bunch of lines later. > > I later on see some open() call on the same tempfile array which does > exactly what the mktemp(3) manpage warns about. > > Am I right into thinking this might be a good candidate for a > mktemp()->mkstemp() conversion? > It looks like a prime candidate for mktemp()->mkstemp() conversion. -- Chris D. Faulhaber - jedgar@fxp.org - jedgar@FreeBSD.org -------------------------------------------------------- FreeBSD: The Power To Serve - http://www.FreeBSD.org To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Tue Oct 24 8: 0:59 2000 Delivered-To: freebsd-audit@freebsd.org Received: from lucifer.ninth-circle.org (lucifer.bart.nl [194.158.168.74]) by hub.freebsd.org (Postfix) with ESMTP id F2F0E37B4C5 for ; Tue, 24 Oct 2000 08:00:55 -0700 (PDT) Received: (from asmodai@localhost) by lucifer.ninth-circle.org (8.11.0/8.11.0) id e9OF0sS98388 for audit@FreeBSD.ORG; Tue, 24 Oct 2000 17:00:54 +0200 (CEST) (envelope-from asmodai) Date: Tue, 24 Oct 2000 17:00:54 +0200 From: Jeroen Ruigrok van der Werven To: audit@FreeBSD.ORG Subject: Re: printjob.c mktemp() problem Message-ID: <20001024170054.Q93799@lucifer.bart.nl> References: <20001024140510.G93799@lucifer.bart.nl> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="sm4nu43k4a2Rpi4c" Content-Disposition: inline User-Agent: Mutt/1.2.5i In-Reply-To: <20001024140510.G93799@lucifer.bart.nl>; from jruigrok@via-net-works.nl on Tue, Oct 24, 2000 at 02:05:10PM +0200 Organisation: VIA Net.Works The Netherlands Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG --sm4nu43k4a2Rpi4c Content-Type: text/plain; charset=us-ascii Content-Disposition: inline -On [20001024 14:10], Jeroen Ruigrok van der Werven (jruigrok@via-net-works.nl) wrote: >Am I right into thinking this might be a good candidate for a >mktemp()->mkstemp() conversion? See attached patch. The patch might not be totally accurate yet, but that due to the code throwing me off at one point, namely the dup2(n, 2); and the unlink(tempfile); Also, is the syslog level ok for a tempfile which couldn't be created? -- Jeroen Ruigrok van der Werven Network- and systemadministrator VIA Net.Works The Netherlands BSD: Technical excellence at its best http://www.via-net-works.nl These days get so long and I got nothing to do... --sm4nu43k4a2Rpi4c Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="printjob.c.diff" --- printjob.c.orig Tue Oct 24 15:38:17 2000 +++ printjob.c Tue Oct 24 16:59:20 2000 @@ -168,8 +168,6 @@ signal(SIGQUIT, abortpr); signal(SIGTERM, abortpr); - (void) mktemp(tempfile); - /* * uses short form file names */ @@ -553,7 +551,7 @@ int format; char *file; { - register int n, i; + register int n, i = -1; register char *prog; int fi, fo; FILE *fp; @@ -733,7 +731,11 @@ if ((child = dofork(pp, DORETURN)) == 0) { /* child */ dup2(fi, 0); dup2(fo, 1); - n = open(tempfile, O_WRONLY|O_CREAT|O_TRUNC, 0664); + if ((n = mkstemp(tempfile)) == -1) { + syslog(LOG_WARNING, "%s: %s: %m", pp->printer, tempfile); + return -1; + } + fchmod(n, 0664); if (n >= 0) dup2(n, 2); closelog(); --sm4nu43k4a2Rpi4c-- To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Tue Oct 24 10:25:46 2000 Delivered-To: freebsd-audit@freebsd.org Received: from gvr.gvr.org (gvr.gvr.org [194.151.74.97]) by hub.freebsd.org (Postfix) with ESMTP id BC09337B479 for ; Tue, 24 Oct 2000 10:25:41 -0700 (PDT) Received: by gvr.gvr.org (Postfix, from userid 657) id 227DE5806; Tue, 24 Oct 2000 19:25:40 +0200 (CEST) Date: Tue, 24 Oct 2000 19:25:39 +0200 From: Guido van Rooij To: Jeroen Ruigrok van der Werven Cc: audit@FreeBSD.ORG Subject: Re: printjob.c mktemp() problem Message-ID: <20001024192539.A65599@gvr.gvr.org> References: <20001024140510.G93799@lucifer.bart.nl> <20001024170054.Q93799@lucifer.bart.nl> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="PEIAKu/WMn1b1Hv9" Content-Disposition: inline In-Reply-To: <20001024170054.Q93799@lucifer.bart.nl>; from jruigrok@via-net-works.nl on Tue, Oct 24, 2000 at 05:00:54PM +0200 Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG --PEIAKu/WMn1b1Hv9 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Oct 24, 2000 at 05:00:54PM +0200, Jeroen Ruigrok van der Werven wrote: > > The patch might not be totally accurate yet, but that due to the code > throwing me off at one point, namely the dup2(n, 2); and the > unlink(tempfile); The dup2 is on the already opened tempfile so should not cause any problems. You should remove the addition of int i = -1 since it isn't used. Furthermore, I would exit after the syslog in stead of the return -1 as it seems all errors result in an exit. Did I mention that this code is awfull? Btw: there might be another problem. In line 956, another open() is done with tempfile which you did not cover. (the one you covered is within printit(). This one is reached via sendit(). I guess it is best to also replace this one with mkstemp(). Attached my attempt (which also fixes: printjob.c:1339: warning: int format, long int arg (arg 3) -Guido --PEIAKu/WMn1b1Hv9 Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename=pp Index: lpd/printjob.c =================================================================== RCS file: /scratch/cvsup/freebsd/CVS/src/usr.sbin/lpr/lpd/printjob.c,v retrieving revision 1.23 diff -u -r1.23 printjob.c --- lpd/printjob.c 2000/05/24 11:38:41 1.23 +++ lpd/printjob.c 2000/10/24 17:24:06 @@ -168,8 +168,6 @@ signal(SIGQUIT, abortpr); signal(SIGTERM, abortpr); - (void) mktemp(tempfile); - /* * uses short form file names */ @@ -733,7 +731,11 @@ if ((child = dofork(pp, DORETURN)) == 0) { /* child */ dup2(fi, 0); dup2(fo, 1); - n = open(tempfile, O_WRONLY|O_CREAT|O_TRUNC, 0664); + if ((n = mkstemp(tempfile)) == -1) { + syslog(LOG_WARNING, "%s: %s: %m", pp->printer, tempfile); + exit(-1); + } + fchmod(n, 0664); if (n >= 0) dup2(n, 2); closelog(); @@ -953,8 +955,12 @@ if ((ifilter = dofork(pp, DORETURN)) == 0) { /* child */ dup2(f, 0); dup2(tfd, 1); - n = open(tempfile, O_WRONLY|O_CREAT|O_TRUNC, - TEMP_FILE_MODE); + if ((n = mkstemp(tempfile)) == -1) { + syslog(LOG_WARNING, + "%s: %s: %m", pp->printer, tempfile); + exit(-1); + } + fchmod(n, 0664); if (n >= 0) dup2(n, 2); closelog(); @@ -1329,7 +1335,7 @@ */ if (pid == 0) { if ((pwd = getpwuid(pp->daemon_user)) == NULL) { - syslog(LOG_ERR, "Can't lookup default daemon uid (%d) in password file", + syslog(LOG_ERR, "Can't lookup default daemon uid (%ld) in password file", pp->daemon_user); break; } --PEIAKu/WMn1b1Hv9-- To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Tue Oct 24 10:56:49 2000 Delivered-To: freebsd-audit@freebsd.org Received: from mail.rpi.edu (mail.rpi.edu [128.113.100.7]) by hub.freebsd.org (Postfix) with ESMTP id D3CE737B4C5 for ; Tue, 24 Oct 2000 10:56:41 -0700 (PDT) Received: from [128.113.24.47] (gilead.acs.rpi.edu [128.113.24.47]) by mail.rpi.edu (8.9.3/8.9.3) with ESMTP id NAA82434; Tue, 24 Oct 2000 13:56:36 -0400 Mime-Version: 1.0 X-Sender: drosih@mail.rpi.edu Message-Id: In-Reply-To: <20001024140510.G93799@lucifer.bart.nl> References: <20001024140510.G93799@lucifer.bart.nl> Date: Tue, 24 Oct 2000 13:56:34 -0400 To: Jeroen Ruigrok van der Werven , audit@FreeBSD.ORG From: Garance A Drosihn Subject: Re: printjob.c mktemp() problem Content-Type: text/plain; charset="us-ascii" ; format="flowed" Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG At 2:05 PM +0200 10/24/00, Jeroen Ruigrok van der Werven wrote: >In printjob.c in the dir src/usr.sbin/lpr/lpd we see a mktemp() >call which creates a file accroding to the template. > >I also see that it is getting unlink()'d again a bunch of lines >later. It is unlinked some 150 lines later, and a lot happens between that mktemp and the call to unlink... >I later on see some open() call on the same tempfile array which >does exactly what the mktemp(3) manpage warns about. > >Am I right into thinking this might be a good candidate for a >mktemp()->mkstemp() conversion? If I ever get my commit status, I would have fixed this. Also notice that mktemp() is called for a file-pattern which does not have a '/' in it, and just a few lines later 'chdir' is called. Thus, mktemp is potentially checking in a different directory than the directory which is active when the file is actually created. Thus, even ignoring the possible race condition (which is probably not much of a problem in this case), the current mktemp call is just plain wrong. It is a great candidate for replacement. IMO. --- Garance Alistair Drosehn = gad@eclipse.acs.rpi.edu Senior Systems Programmer or drosih@rpi.edu Rensselaer Polytechnic Institute To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Tue Oct 24 11:29:54 2000 Delivered-To: freebsd-audit@freebsd.org Received: from lucifer.ninth-circle.org (lucifer.bart.nl [194.158.168.74]) by hub.freebsd.org (Postfix) with ESMTP id 838CB37B6AA for ; Tue, 24 Oct 2000 11:29:48 -0700 (PDT) Received: (from asmodai@localhost) by lucifer.ninth-circle.org (8.11.0/8.11.0) id e9OITgJ01271; Tue, 24 Oct 2000 20:29:42 +0200 (CEST) (envelope-from asmodai) Date: Tue, 24 Oct 2000 20:29:42 +0200 From: Jeroen Ruigrok van der Werven To: Guido van Rooij Cc: audit@FreeBSD.ORG Subject: Re: printjob.c mktemp() problem Message-ID: <20001024202942.C209@lucifer.bart.nl> References: <20001024140510.G93799@lucifer.bart.nl> <20001024170054.Q93799@lucifer.bart.nl> <20001024192539.A65599@gvr.gvr.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="6c2NcOVqGQ03X4Wi" Content-Disposition: inline User-Agent: Mutt/1.2.5i In-Reply-To: <20001024192539.A65599@gvr.gvr.org>; from guido@gvr.org on Tue, Oct 24, 2000 at 07:25:39PM +0200 Organisation: VIA Net.Works The Netherlands Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG --6c2NcOVqGQ03X4Wi Content-Type: text/plain; charset=us-ascii Content-Disposition: inline -On [20001024 19:30], Guido van Rooij (guido@gvr.org) wrote: >On Tue, Oct 24, 2000 at 05:00:54PM +0200, Jeroen Ruigrok van der Werven wrote: >> >> The patch might not be totally accurate yet, but that due to the code >> throwing me off at one point, namely the dup2(n, 2); and the >> unlink(tempfile); > >The dup2 is on the already opened tempfile so should not cause any problems. That was not my concern, only problem was that later on I see no obvious references back to the fd 2. BTW, the mkstemp() eliminates the n >= 2 testcase. [Thanks to Eivind] >You should remove the addition of int i = -1 since it isn't used. Yeah, I fixed that in my latest patches. >Furthermore, I would exit after the syslog in stead of the return -1 >as it seems all errors result in an exit. Done. I also fixed the syslog() reporting to LOG_ERR as done elsewhere in the code. >Did I mention that this code is awfull? That was my thought as well. =) I wrapped my lines as per style(9) btw, there were too long before. >Btw: there might be another problem. In line 956, another open() is done >with tempfile which you did not cover. (the one you covered is within >printit(). This one is reached via sendit(). I guess it is best to >also replace this one with mkstemp(). Thanks. >Attached my attempt (which also fixes: printjob.c:1339: warning: int >format, long int arg (arg 3) Cool. I see your patch and I'll raise you mine, which also does proper checking on the fchmod(). [Thanks to Eivind] Btw, Eivind and me thought that using mode 0664 was slightly unnecessary and 0660 should be way better and still useable. Further opinions? Please keep in mind that this is an audit patch only. There way more to improved on the code, but that's outside the scope. I still need to look at Garance's mention of the tempfile name which doesn't include a /tmp/ path. -- Jeroen Ruigrok van der Werven Network- and systemadministrator VIA Net.Works The Netherlands BSD: Technical excellence at its best http://www.via-net-works.nl All that we are is the result of what we have thought. The mind is everything. What we think, we become... --6c2NcOVqGQ03X4Wi Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="printjob.c.diff" --- printjob.c.orig Tue Oct 24 15:38:17 2000 +++ printjob.c Tue Oct 24 20:18:12 2000 @@ -168,8 +168,6 @@ signal(SIGQUIT, abortpr); signal(SIGTERM, abortpr); - (void) mktemp(tempfile); - /* * uses short form file names */ @@ -733,9 +731,15 @@ if ((child = dofork(pp, DORETURN)) == 0) { /* child */ dup2(fi, 0); dup2(fo, 1); - n = open(tempfile, O_WRONLY|O_CREAT|O_TRUNC, 0664); - if (n >= 0) - dup2(n, 2); + if ((n = mkstemp(tempfile)) == -1) { + syslog(LOG_ERR, "mkstemp: %m"); + exit(-1); + } + if ((i = fchmod(n, 0664)) == -1) { /* O660 should also work */ + syslog(LOG_ERR, "fchmod: %m"); + exit(-1); + } + dup2(n, 2); closelog(); closeallfds(3); execv(prog, av); @@ -953,10 +957,15 @@ if ((ifilter = dofork(pp, DORETURN)) == 0) { /* child */ dup2(f, 0); dup2(tfd, 1); - n = open(tempfile, O_WRONLY|O_CREAT|O_TRUNC, - TEMP_FILE_MODE); - if (n >= 0) - dup2(n, 2); + if ((n = mkstemp(tempfile)) == -1) { + syslog(LOG_ERR, "mkstemp: %m"); + exit(-1); + } + if ((i = fchmod(n, 0664)) == -1) { + syslog(LOG_ERR, "fchmod: %m"); + exit(-1); + } + dup2(n, 2); closelog(); closeallfds(3); execv(pp->filters[LPF_INPUT], av); @@ -1329,7 +1338,7 @@ */ if (pid == 0) { if ((pwd = getpwuid(pp->daemon_user)) == NULL) { - syslog(LOG_ERR, "Can't lookup default daemon uid (%d) in password file", + syslog(LOG_ERR, "Can't lookup default daemon uid (%ld) in password file", pp->daemon_user); break; } --6c2NcOVqGQ03X4Wi-- To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Tue Oct 24 12:15:29 2000 Delivered-To: freebsd-audit@freebsd.org Received: from lucifer.ninth-circle.org (lucifer.bart.nl [194.158.168.74]) by hub.freebsd.org (Postfix) with ESMTP id BC5B737B479 for ; Tue, 24 Oct 2000 12:15:22 -0700 (PDT) Received: (from asmodai@localhost) by lucifer.ninth-circle.org (8.11.0/8.11.0) id e9OJFId02003; Tue, 24 Oct 2000 21:15:18 +0200 (CEST) (envelope-from asmodai) Date: Tue, 24 Oct 2000 21:15:18 +0200 From: Jeroen Ruigrok van der Werven To: Guido van Rooij Cc: audit@FreeBSD.ORG Subject: Re: printjob.c mktemp() problem Message-ID: <20001024211518.E209@lucifer.bart.nl> References: <20001024140510.G93799@lucifer.bart.nl> <20001024170054.Q93799@lucifer.bart.nl> <20001024192539.A65599@gvr.gvr.org> <20001024202942.C209@lucifer.bart.nl> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="Pd0ReVV5GZGQvF3a" Content-Disposition: inline User-Agent: Mutt/1.2.5i In-Reply-To: <20001024202942.C209@lucifer.bart.nl>; from jruigrok@via-net-works.nl on Tue, Oct 24, 2000 at 08:29:42PM +0200 Organisation: VIA Net.Works The Netherlands Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG --Pd0ReVV5GZGQvF3a Content-Type: text/plain; charset=us-ascii Content-Disposition: inline -On [20001024 20:35], Jeroen Ruigrok van der Werven (jruigrok@via-net-works.nl) wrote: >-On [20001024 19:30], Guido van Rooij (guido@gvr.org) wrote: >>You should remove the addition of int i = -1 since it isn't used. > >Yeah, I fixed that in my latest patches. Deleted bogus assignment of i = fchmod(). No need for it. >I still need to look at Garance's mention of the tempfile name which >doesn't include a /tmp/ path. My new patch should also fix this case. Given that lpd is a daemon I didn't use warnx/errx but instead used syslog(), but it might be that some logging is not yet fully clear what went wrong with my patch. I'll try to test that tomorrow. I haven't sanitized the code further except make my changes as sane possible as I could by myself and guidance from my mentors and peers. Opinions welcome. -- Jeroen Ruigrok van der Werven Network- and systemadministrator VIA Net.Works The Netherlands BSD: Technical excellence at its best http://www.via-net-works.nl I'm a child of the air, I'm a witch of the wind... --Pd0ReVV5GZGQvF3a Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="printjob.c.diff" --- printjob.c.orig Tue Oct 24 15:38:17 2000 +++ printjob.c Tue Oct 24 21:13:29 2000 @@ -114,7 +114,7 @@ static char logname[32]; /* user's login name */ static char pxlength[10] = "-y"; /* page length in pixels */ static char pxwidth[10] = "-x"; /* page width in pixels */ -static char tempfile[] = "errsXXXXXX"; /* file name for filter errors */ +static char *tempfile; /* file name for filter errors */ static char width[10] = "-w"; /* page width in static characters */ #define TFILENAME "fltXXXXXX" static char tfile[] = TFILENAME; /* file name for filter output */ @@ -153,6 +153,13 @@ off_t pidoff; int errcnt, count = 0; + if (getenv("TMPDIR") == NULL) + tempfile = strdup("/tmp/errsXXXXXXXXX"); + else + if (asprintf(&tempfile, "%s/errsXXXXXXXXX", + getenv("TMPDIR")) == -1) + syslog(LOG_ERR, "asprintf: %m"); + init(pp); /* set up capabilities */ (void) write(1, "", 1); /* ack that daemon is started */ (void) close(2); /* set up log file */ @@ -168,8 +175,6 @@ signal(SIGQUIT, abortpr); signal(SIGTERM, abortpr); - (void) mktemp(tempfile); - /* * uses short form file names */ @@ -733,9 +738,15 @@ if ((child = dofork(pp, DORETURN)) == 0) { /* child */ dup2(fi, 0); dup2(fo, 1); - n = open(tempfile, O_WRONLY|O_CREAT|O_TRUNC, 0664); - if (n >= 0) - dup2(n, 2); + if ((n = mkstemp(tempfile)) == -1) { + syslog(LOG_ERR, "mkstemp: %m"); + exit(-1); + } + if ((fchmod(n, 0664)) == -1) { /* O660 should also work */ + syslog(LOG_ERR, "fchmod: %m"); + exit(-1); + } + dup2(n, 2); closelog(); closeallfds(3); execv(prog, av); @@ -953,10 +964,15 @@ if ((ifilter = dofork(pp, DORETURN)) == 0) { /* child */ dup2(f, 0); dup2(tfd, 1); - n = open(tempfile, O_WRONLY|O_CREAT|O_TRUNC, - TEMP_FILE_MODE); - if (n >= 0) - dup2(n, 2); + if ((n = mkstemp(tempfile)) == -1) { + syslog(LOG_ERR, "mkstemp: %m"); + exit(-1); + } + if ((fchmod(n, 0664)) == -1) { + syslog(LOG_ERR, "fchmod: %m"); + exit(-1); + } + dup2(n, 2); closelog(); closeallfds(3); execv(pp->filters[LPF_INPUT], av); @@ -1329,7 +1345,7 @@ */ if (pid == 0) { if ((pwd = getpwuid(pp->daemon_user)) == NULL) { - syslog(LOG_ERR, "Can't lookup default daemon uid (%d) in password file", + syslog(LOG_ERR, "Can't lookup default daemon uid (%ld) in password file", pp->daemon_user); break; } --Pd0ReVV5GZGQvF3a-- To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Tue Oct 24 12:16:19 2000 Delivered-To: freebsd-audit@freebsd.org Received: from mail.utfors.se (mail.utfors.se [195.58.103.125]) by hub.freebsd.org (Postfix) with ESMTP id 1193737B666 for ; Tue, 24 Oct 2000 12:16:17 -0700 (PDT) Received: from ludd.luth.se (md469298f.utfors.se [212.105.41.143]) by mail.utfors.se (8.8.8/8.8.8) with ESMTP id VAA14804 for ; Tue, 24 Oct 2000 21:16:08 +0200 (MET DST) Message-ID: <39F5DFF7.8ABBAA99@ludd.luth.se> Date: Tue, 24 Oct 2000 21:16:07 +0200 From: Joachim =?iso-8859-1?Q?Str=F6mbergson?= Organization: Acne X-Mailer: Mozilla 4.75 [en] (X11; U; FreeBSD 4.1-STABLE i386) X-Accept-Language: en-US MIME-Version: 1.0 To: FreeBSD-Audit Subject: Which mktemp()s are bad? Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG Hi! I saw Joeroens posting on the mktemp() issue in printjob.c. During buildworlds I have seen warnings about mktemp() being used if not frequently, at least on quite a few places. This got me thinking - Generally, are all mktemp() calls bad? Which are the one that *really* should be eliminated? Can they all be emliminated? I did some grepping on the Stable tree as of 2000-10-24 and found a few places where mktemp was used, though mostly in contributed stuff, like GNU binutils. (Side note: Should those be fixed for FreeBSD or feedbacked to the respective team at FSF?) -- Cheers! Joachim - Alltid i harmonisk svängning --- FairLight ------ FairLight ------ FairLight ------ FairLight --- Joachim Strömbergson ASIC SoC designer, nice to CUTE animals Phone: +46(0)31 - 27 98 47 Web: http://www.ludd.luth.se/~watchman --------------- Spamfodder: regeringen@regeringen.se --------------- To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Tue Oct 24 12:18:19 2000 Delivered-To: freebsd-audit@freebsd.org Received: from rover.village.org (rover.village.org [204.144.255.49]) by hub.freebsd.org (Postfix) with ESMTP id D4CEE37B4CF for ; Tue, 24 Oct 2000 12:18:16 -0700 (PDT) Received: from harmony.village.org (harmony.village.org [10.0.0.6]) by rover.village.org (8.11.0/8.11.0) with ESMTP id e9OJIEn88131; Tue, 24 Oct 2000 13:18:15 -0600 (MDT) (envelope-from imp@harmony.village.org) Received: from harmony.village.org (localhost.village.org [127.0.0.1]) by harmony.village.org (8.9.3/8.8.3) with ESMTP id NAA18110; Tue, 24 Oct 2000 13:18:14 -0600 (MDT) Message-Id: <200010241918.NAA18110@harmony.village.org> To: Joachim =?iso-8859-1?Q?Str=F6mbergson?= Subject: Re: Which mktemp()s are bad? Cc: FreeBSD-Audit In-reply-to: Your message of "Tue, 24 Oct 2000 21:16:07 +0200." <39F5DFF7.8ABBAA99@ludd.luth.se> References: <39F5DFF7.8ABBAA99@ludd.luth.se> Date: Tue, 24 Oct 2000 13:18:14 -0600 From: Warner Losh Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG In message <39F5DFF7.8ABBAA99@ludd.luth.se> Joachim =?iso-8859-1?Q?Str=F6mbergson?= writes: : I saw Joeroens posting on the mktemp() issue in printjob.c. During : buildworlds I have seen warnings about mktemp() being used if not : frequently, at least on quite a few places. This got me thinking - : Generally, are all mktemp() calls bad? Which are the one that *really* : should be eliminated? Can they all be emliminated? Generally speaking, they are all bad. However, there are some safe ones in the tree. It would hurt nothing to convert the safe ones to using mkstemp, modulo repository issues. : I did some grepping on the Stable tree as of 2000-10-24 and found a few : places where mktemp was used, though mostly in contributed stuff, like : GNU binutils. : : (Side note: Should those be fixed for FreeBSD or feedbacked to the : respective team at FSF?) Yes. But likely the pain of doing it in the tree is too large for David O'Brien to want to deal with. Warner To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Tue Oct 24 13:27:20 2000 Delivered-To: freebsd-audit@freebsd.org Received: from lucifer.ninth-circle.org (lucifer.bart.nl [194.158.168.74]) by hub.freebsd.org (Postfix) with ESMTP id 2160037B4CF for ; Tue, 24 Oct 2000 13:27:18 -0700 (PDT) Received: (from asmodai@localhost) by lucifer.ninth-circle.org (8.11.0/8.11.0) id e9OKRGX02944 for audit@FreeBSD.ORG; Tue, 24 Oct 2000 22:27:16 +0200 (CEST) (envelope-from asmodai) Date: Tue, 24 Oct 2000 22:27:16 +0200 From: Jeroen Ruigrok van der Werven To: audit@FreeBSD.ORG Subject: Re: printjob.c mktemp() problem Message-ID: <20001024222716.B2020@lucifer.bart.nl> References: <20001024140510.G93799@lucifer.bart.nl> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.2.5i In-Reply-To: <20001024140510.G93799@lucifer.bart.nl>; from jruigrok@via-net-works.nl on Tue, Oct 24, 2000 at 02:05:10PM +0200 Organisation: VIA Net.Works The Netherlands Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG New patch at http://lucifer.bart.nl/~asmodai/printjob.c.diff However, there are some things I don't like in the current patch. I have these feeling that I am missing something. My memory keeps nagging about calling mkstemp() twice and only unlink() once, and then in the general case, whilst the original code only had one mktemp() and subsequently open()'d the tempfile. But that's for tomorrow. :) -- Jeroen Ruigrok van der Werven Network- and systemadministrator VIA Net.Works The Netherlands BSD: Technical excellence at its best http://www.via-net-works.nl If Winter comes, can Spring be far behind..? To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Tue Oct 24 13:39: 7 2000 Delivered-To: freebsd-audit@freebsd.org Received: from mail.rpi.edu (mail.rpi.edu [128.113.100.7]) by hub.freebsd.org (Postfix) with ESMTP id 1D5D637B4CF for ; Tue, 24 Oct 2000 13:39:03 -0700 (PDT) Received: from [128.113.24.47] (gilead.acs.rpi.edu [128.113.24.47]) by mail.rpi.edu (8.9.3/8.9.3) with ESMTP id QAA499608; Tue, 24 Oct 2000 16:38:47 -0400 Mime-Version: 1.0 X-Sender: drosih@mail.rpi.edu Message-Id: In-Reply-To: <20001024211518.E209@lucifer.bart.nl> References: <20001024140510.G93799@lucifer.bart.nl> <20001024170054.Q93799@lucifer.bart.nl> <20001024192539.A65599@gvr.gvr.org> <20001024202942.C209@lucifer.bart.nl> <20001024211518.E209@lucifer.bart.nl> Date: Tue, 24 Oct 2000 16:38:46 -0400 To: Jeroen Ruigrok van der Werven , Guido van Rooij From: Garance A Drosihn Subject: Re: printjob.c mktemp() problem Cc: audit@FreeBSD.ORG Content-Type: text/plain; charset="us-ascii" ; format="flowed" Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG At 9:15 PM +0200 10/24/00, Jeroen Ruigrok van der Werven wrote: >-On [20001024 20:35], Jeroen Ruigrok van der Werven >(jruigrok@via-net-works.nl) wrote: > >-On [20001024 19:30], Guido van Rooij (guido@gvr.org) wrote: > >>You should remove the addition of int i = -1 since it isn't used. > > > >Yeah, I fixed that in my latest patches. > >Deleted bogus assignment of i = fchmod(). No need for it. > > > I still need to look at Garance's mention of the tempfile name > > which doesn't include a /tmp/ path. > >My new patch should also fix this case. Uh, the tempfile name is NOT supposed to include /tmp in the pathname. The file is supposed to be created in the spool directory, and not in /tmp. 'mktemp' does not imply the file must be in /tmp. In the case of lpd, moving the filenames into /tmp would make matters worse instead of better. (at least for my print servers it would, in other cases it might not make any difference). I was just saying that given the pattern being used, which is a correct pattern, the bug was that mktemp was called BEFORE the chdir, when it should only be called AFTER the current-directory has been changed. If I remember right, the suggested mkstemp patch already fixed that particular problem. --- Garance Alistair Drosehn = gad@eclipse.acs.rpi.edu Senior Systems Programmer or drosih@rpi.edu Rensselaer Polytechnic Institute To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Tue Oct 24 13:39:38 2000 Delivered-To: freebsd-audit@freebsd.org Received: from mail.rpi.edu (mail.rpi.edu [128.113.100.7]) by hub.freebsd.org (Postfix) with ESMTP id 1F2C737B479 for ; Tue, 24 Oct 2000 13:39:36 -0700 (PDT) Received: from [128.113.24.47] (gilead.acs.rpi.edu [128.113.24.47]) by mail.rpi.edu (8.9.3/8.9.3) with ESMTP id QAA451772; Tue, 24 Oct 2000 16:39:31 -0400 Mime-Version: 1.0 X-Sender: drosih@mail.rpi.edu Message-Id: In-Reply-To: <20001024211518.E209@lucifer.bart.nl> References: <20001024140510.G93799@lucifer.bart.nl> <20001024170054.Q93799@lucifer.bart.nl> <20001024192539.A65599@gvr.gvr.org> <20001024202942.C209@lucifer.bart.nl> <20001024211518.E209@lucifer.bart.nl> Date: Tue, 24 Oct 2000 16:39:29 -0400 To: Jeroen Ruigrok van der Werven , Guido van Rooij From: Garance A Drosihn Subject: Re: printjob.c mktemp() problem Cc: audit@FreeBSD.ORG Content-Type: text/plain; charset="us-ascii" ; format="flowed" Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG At 9:15 PM +0200 10/24/00, Jeroen Ruigrok van der Werven wrote: > >I still need to look at Garance's mention of the tempfile name which > >doesn't include a /tmp/ path. > >My new patch should also fix this case. > >Given that lpd is a daemon I didn't use warnx/errx but instead used >syslog(), but it might be that some logging is not yet fully clear what >went wrong with my patch. I'll try to test that tomorrow. > >I haven't sanitized the code further except make my changes as sane >possible as I could by myself and guidance from my mentors and peers. Oops, I didn't notice your new patch was in the same message. The new patch is not good, because the temp files should not be in /tmp. It might be best to let this particular mktemp call alone for now, and I'll fix it when I get a chance to. I'd say more, but something odd seems to be going on in our (RPI) network right now... --- Garance Alistair Drosehn = gad@eclipse.acs.rpi.edu Senior Systems Programmer or drosih@rpi.edu Rensselaer Polytechnic Institute To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Tue Oct 24 14: 6:12 2000 Delivered-To: freebsd-audit@freebsd.org Received: from mail.rpi.edu (mail.rpi.edu [128.113.100.7]) by hub.freebsd.org (Postfix) with ESMTP id 37D6037B4C5 for ; Tue, 24 Oct 2000 14:06:07 -0700 (PDT) Received: from [128.113.24.47] (gilead.acs.rpi.edu [128.113.24.47]) by mail.rpi.edu (8.9.3/8.9.3) with ESMTP id RAA819398; Tue, 24 Oct 2000 17:06:01 -0400 Mime-Version: 1.0 X-Sender: drosih@mail.rpi.edu Message-Id: In-Reply-To: <20001024222716.B2020@lucifer.bart.nl> References: <20001024140510.G93799@lucifer.bart.nl> <20001024222716.B2020@lucifer.bart.nl> Date: Tue, 24 Oct 2000 17:06:00 -0400 To: Jeroen Ruigrok van der Werven , audit@FreeBSD.ORG From: Garance A Drosihn Subject: Re: printjob.c mktemp() problem Content-Type: text/plain; charset="us-ascii" ; format="flowed" Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG At 10:27 PM +0200 10/24/00, Jeroen Ruigrok van der Werven wrote: >New patch at http://lucifer.bart.nl/~asmodai/printjob.c.diff > >However, there are some things I don't like in the current patch. > >I have these feeling that I am missing something. It is pretty easy to miss things in lpr/lpd. While everything they do is pretty simple, the code is so disorganized that often it is not-obvious what is going on. In the general case of mktemp calls, the security issue is that mktemp is called to create a file in /tmp. Due to the window between the call to 'mktemp' and any call to create that file, there is a security exposure that some OTHER task will deliberately try to create a file of the same name. (by trial-and-error, if nothing else). That problem does not arise in the specific case of this mktemp call in lpd. The temp-file in question is being created in the spool directory, and the spool directory is (HOPEFULLY!!) permitted such that untrusted users do not have any ability to create any files in it. If they do have access, then you have serious security problems which have nothing to do with this call to mktemp. However, I did plan to replace this mktemp call, partly on general principles, and partly because the code is just plain wrong. It is wrong because mktemp is called, and then the current directory is changed, and then the filename returned by mktemp is created. What is wrong there is the order of mktemp/chdir calls, and not the template used for the new filename. In practice this isn't a problem, but it isn't right either. Given that this is not a security exposure, it would probably be just as well to leave it alone for now. --- Garance Alistair Drosehn = gad@eclipse.acs.rpi.edu Senior Systems Programmer or drosih@rpi.edu Rensselaer Polytechnic Institute To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Wed Oct 25 10:38:48 2000 Delivered-To: freebsd-audit@freebsd.org Received: from mail.utfors.se (mail.utfors.se [195.58.103.125]) by hub.freebsd.org (Postfix) with ESMTP id CE88A37B4CF for ; Wed, 25 Oct 2000 10:38:45 -0700 (PDT) Received: from ludd.luth.se (md4692537.utfors.se [212.105.37.55]) by mail.utfors.se (8.8.8/8.8.8) with ESMTP id TAA03269; Wed, 25 Oct 2000 19:38:15 +0200 (MET DST) Message-ID: <39F71A84.FF077F4A@ludd.luth.se> Date: Wed, 25 Oct 2000 19:38:12 +0200 From: Joachim =?iso-8859-1?Q?Str=F6mbergson?= Organization: Acne X-Mailer: Mozilla 4.75 [en] (X11; U; FreeBSD 4.1-STABLE i386) X-Accept-Language: en-US MIME-Version: 1.0 To: Warner Losh Cc: FreeBSD-Audit Subject: Re: Which mktemp()s are bad? References: <39F5DFF7.8ABBAA99@ludd.luth.se> <200010241918.NAA18110@harmony.village.org> Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG Hi! Warner Losh wrote: > In message <39F5DFF7.8ABBAA99@ludd.luth.se> Joachim =?iso-8859-1?Q?Str=F6mbergson?= writes: > : I did some grepping on the Stable tree as of 2000-10-24 and found a few > : places where mktemp was used, though mostly in contributed stuff, like > : GNU binutils. > : > : (Side note: Should those be fixed for FreeBSD or feedbacked to the > : respective team at FSF?) > > Yes. But likely the pain of doing it in the tree is too large for > David O'Brien to want to deal with. ;-) I'm sorry Warner, but does that "Yes" mean "fix in FreeBSD" OR "feedback to FSF" ? An alternative intepretation might be "fix in FreeBSD" AND "feedback to FSF". What's the correct way to handle these situations? (... And I hope the answer isn't (simply) "Yes" .... ;-) -- Cheers! Joachim - Alltid i harmonisk svängning --- FairLight ------ FairLight ------ FairLight ------ FairLight --- Joachim Strömbergson ASIC SoC designer, nice to CUTE animals Phone: +46(0)31 - 27 98 47 Web: http://www.ludd.luth.se/~watchman --------------- Spamfodder: regeringen@regeringen.se --------------- To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Wed Oct 25 12:40:29 2000 Delivered-To: freebsd-audit@freebsd.org Received: from gvr.gvr.org (gvr.gvr.org [194.151.74.97]) by hub.freebsd.org (Postfix) with ESMTP id 8420B37B479 for ; Wed, 25 Oct 2000 12:40:27 -0700 (PDT) Received: by gvr.gvr.org (Postfix, from userid 657) id D51FF5806; Wed, 25 Oct 2000 21:40:25 +0200 (CEST) Date: Wed, 25 Oct 2000 21:40:25 +0200 From: Guido van Rooij To: =?iso-8859-1?Q?Joachim_Str=F6mbergson?= Cc: Warner Losh , FreeBSD-Audit Subject: Re: Which mktemp()s are bad? Message-ID: <20001025214025.A71776@gvr.gvr.org> References: <39F5DFF7.8ABBAA99@ludd.luth.se> <200010241918.NAA18110@harmony.village.org> <39F71A84.FF077F4A@ludd.luth.se> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <39F71A84.FF077F4A@ludd.luth.se>; from watchman@ludd.luth.se on Wed, Oct 25, 2000 at 07:38:12PM +0200 Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG On Wed, Oct 25, 2000 at 07:38:12PM +0200, Joachim Strömbergson wrote: > > I'm sorry Warner, but does that "Yes" mean "fix in FreeBSD" OR "feedback > to FSF" ? > An alternative intepretation might be "fix in FreeBSD" AND "feedback to > FSF". > > What's the correct way to handle these situations? > (... And I hope the answer isn't (simply) "Yes" .... ;-) The correct way is to feedback to FSF and wait for them to be imported into FreeBSD. -Guido To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Wed Oct 25 13:25: 0 2000 Delivered-To: freebsd-audit@freebsd.org Received: from rover.village.org (rover.village.org [204.144.255.49]) by hub.freebsd.org (Postfix) with ESMTP id 2C64837B4C5 for ; Wed, 25 Oct 2000 13:24:53 -0700 (PDT) Received: from harmony.village.org (harmony.village.org [10.0.0.6]) by rover.village.org (8.11.0/8.11.0) with ESMTP id e9PKOpn93942; Wed, 25 Oct 2000 14:24:51 -0600 (MDT) (envelope-from imp@harmony.village.org) Received: from harmony.village.org (localhost.village.org [127.0.0.1]) by harmony.village.org (8.9.3/8.8.3) with ESMTP id OAA35656; Wed, 25 Oct 2000 14:24:50 -0600 (MDT) Message-Id: <200010252024.OAA35656@harmony.village.org> To: Joachim =?iso-8859-1?Q?Str=F6mbergson?= Subject: Re: Which mktemp()s are bad? Cc: FreeBSD-Audit In-reply-to: Your message of "Wed, 25 Oct 2000 19:38:12 +0200." <39F71A84.FF077F4A@ludd.luth.se> References: <39F71A84.FF077F4A@ludd.luth.se> <39F5DFF7.8ABBAA99@ludd.luth.se> <200010241918.NAA18110@harmony.village.org> Date: Wed, 25 Oct 2000 14:24:50 -0600 From: Warner Losh Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG In message <39F71A84.FF077F4A@ludd.luth.se> Joachim =?iso-8859-1?Q?Str=F6mbergson?= writes: : Hi! : : Warner Losh wrote: : > In message <39F5DFF7.8ABBAA99@ludd.luth.se> Joachim =?iso-8859-1?Q?Str=F6mbergson?= writes: : > : I did some grepping on the Stable tree as of 2000-10-24 and found a few : > : places where mktemp was used, though mostly in contributed stuff, like : > : GNU binutils. : > : : > : (Side note: Should those be fixed for FreeBSD or feedbacked to the : > : respective team at FSF?) : > : > Yes. But likely the pain of doing it in the tree is too large for : > David O'Brien to want to deal with. : : ;-) : : I'm sorry Warner, but does that "Yes" mean "fix in FreeBSD" OR "feedback : to FSF" ? : An alternative intepretation might be "fix in FreeBSD" AND "feedback to : FSF". Fix freebsd. Try to feed the patches back to FSF. If they can't/won't take them, fix freebsd repo. Warner To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Thu Oct 26 10:31: 4 2000 Delivered-To: freebsd-audit@freebsd.org Received: from salmon.maths.tcd.ie (salmon.maths.tcd.ie [134.226.81.11]) by hub.freebsd.org (Postfix) with SMTP id B98FA37B479; Thu, 26 Oct 2000 10:30:57 -0700 (PDT) Received: from walton.maths.tcd.ie by salmon.maths.tcd.ie with SMTP id ; 26 Oct 2000 18:30:56 +0100 (BST) To: audit@freebsd.org Cc: kris@freebsd.org, rwatson@freebsd.org Subject: Patch for crontab. X-Request-Do: Date: Thu, 26 Oct 2000 18:30:55 +0100 From: David Malone Message-ID: <200010261830.aa94658@salmon.maths.tcd.ie> Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG Can someone review this for me? It's a fix for the crontab problem based on what OpenBSD have done. We used to reopen NewCrontab after editing as root and reread the file. The patch I suggested last night just changed who we opened the file as (which works OK). What OpenBSD have done is arange to keep NewCrontab open during the edit. Then they just rewind and reread the file. This has the advantage that people just can't make a symlink to /dev/zero or /dev/random. Crontab would happily copy these byte-by-byte into the crontab spool directory. The down side to the OpenBSD patch is that it won't work with editors which unlink the file they are editing. Vi, vim, emacs, xemacs and ee all do the right thing so it looks like it isn't going to bite too many people. To try to alert people who might get caught by this I've aranged for the temp file to be stated and compared to the results of fstat for the file crontab has open. If they don't match it gives a warning and exits. This way edits made to crontab with fooedit don't silently fail. David. Index: crontab.c =================================================================== RCS file: /cvs/FreeBSD-CVS/src/usr.sbin/cron/crontab/crontab.c,v retrieving revision 1.13 diff -u -r1.13 crontab.c --- crontab.c 2000/10/15 00:35:34 1.13 +++ crontab.c 2000/10/26 16:59:47 @@ -285,7 +285,7 @@ char n[MAX_FNAME], q[MAX_TEMPSTR], *editor; FILE *f; int ch, t, x; - struct stat statbuf; + struct stat statbuf, fsbuf; time_t mtime; WAIT_T waiter; PID_T pid, xpid; @@ -317,7 +317,7 @@ warn("fchown"); goto fatal; } - if (!(NewCrontab = fdopen(t, "w"))) { + if (!(NewCrontab = fdopen(t, "r+"))) { warn("fdopen"); goto fatal; } @@ -347,14 +347,20 @@ while (EOF != (ch = get_char(f))) putc(ch, NewCrontab); fclose(f); - if (fclose(NewCrontab)) + if (fflush(NewCrontab)) err(ERROR_EXIT, "%s", Filename); + if (fstat(t, &fsbuf) < 0) { + warn("unable to fstat temp file"); + goto fatal; + } again: if (stat(Filename, &statbuf) < 0) { warn("stat"); fatal: unlink(Filename); exit(ERROR_EXIT); } + if (statbuf.st_dev != fsbuf.st_dev || statbuf.st_ino != fsbuf.st_ino) + errx(ERROR_EXIT, "temp file must be edited in place"); mtime = statbuf.st_mtime; if ((!(editor = getenv("VISUAL"))) @@ -419,15 +425,13 @@ warn("stat"); goto fatal; } + if (statbuf.st_dev != fsbuf.st_dev || statbuf.st_ino != fsbuf.st_ino) + errx(ERROR_EXIT, "temp file must be edited in place"); if (mtime == statbuf.st_mtime) { warnx("no changes made to crontab"); goto remove; } warnx("installing new crontab"); - if (!(NewCrontab = fopen(Filename, "r"))) { - warn("%s", Filename); - goto fatal; - } switch (replace_cmd()) { case 0: break; @@ -497,10 +501,10 @@ /* copy the crontab to the tmp */ + rewind(NewCrontab); Set_LineNum(1) while (EOF != (ch = get_char(NewCrontab))) putc(ch, tmp); - fclose(NewCrontab); ftruncate(fileno(tmp), ftell(tmp)); fflush(tmp); rewind(tmp); To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message