Date: Tue, 24 Oct 2000 20:29:42 +0200 From: Jeroen Ruigrok van der Werven <jruigrok@via-net-works.nl> To: Guido van Rooij <guido@gvr.org> Cc: audit@FreeBSD.ORG Subject: Re: printjob.c mktemp() problem Message-ID: <20001024202942.C209@lucifer.bart.nl> In-Reply-To: <20001024192539.A65599@gvr.gvr.org>; from guido@gvr.org on Tue, Oct 24, 2000 at 07:25:39PM %2B0200 References: <20001024140510.G93799@lucifer.bart.nl> <20001024170054.Q93799@lucifer.bart.nl> <20001024192539.A65599@gvr.gvr.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <jruigrok@via-net-works.nl> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20001024202942.C209>