[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
re: bin/46500: Permission of created files in lpr.c wrong.
The following reply was made to PR bin/46500; it has been noted by GNATS.
From: jnemeth%victoria.tc.ca@localhost (John Nemeth)
To: matthew green <mrg%eterna.com.au@localhost>,
Subject: re: bin/46500: Permission of created files in lpr.c wrong.
Date: Sat, 2 Jun 2012 12:24:47 -0700
On Oct 20, 10:06pm, matthew green wrote:
} > > >Description:
} > > Permission of created files */.seq is wrong.
} > > >How-To-Repeat:
} > >
} > > >Fix:
} > > diff -u -p -r1.45 lpr.c
} > > --- usr.sbin/lpr/lpr/lpr.c 30 Aug 2011 19:27:37 -0000 1.45
} > > +++ usr.sbin/lpr/lpr/lpr.c 30 May 2012 09:07:55 -0000
} > > @@ -698,7 +698,7 @@ mktemps(void)
} > >
} > > (void)snprintf(buf, sizeof(buf), "%s/.seq", SD);
} > > seteuid(euid);
} > > - if ((fd = open(buf, O_RDWR|O_CREAT, 0661)) < 0)
} > > + if ((fd = open(buf, O_RDWR|O_CREAT, 0664)) < 0)
} > > err(1, "cannot create %s", buf);
} > > if (flock(fd, LOCK_EX))
} > > err(1, "cannot lock %s", buf);
} > what's wrong with this? your change makes the temp files world
} > readable which seems like a security issue to me.
All it contains is a three digit sequence number for the next
queue entry. That doesn't seem to be particularly security critical,
but if you like, it could be changed to 660. Certainly execute is
} additionally, this will break lpd as it uses these execute bits
} modes specific meanings.
Uh, read the code. The .seq file is opened, locked, read,
written, and closed (implicitly unlocking) all within the same function
and the fd is never passed to any functions other then the libc
functions required to perform the above operations. Also, the
containing function is only called from with one place in lpr.c. In
other words, lpd doesn't even look at the file.
}-- End of excerpt from matthew green
Main Index |
Thread Index |