On Wed, 2 May 2012 18:40:05 +0200
Elia Pinto <gitter.spiros@xxxxxxxxx> wrote:
> 2012/4/28 Jeff Layton <jlayton@xxxxxxxxx>:
> > On Fri, 27 Apr 2012 11:12:48 -0400
> > Elia Pinto <gitter.spiros@xxxxxxxxx> wrote:
> >
> >
> > Looks like you're still setting the umask to 0077 here?
> I am sorry for the long delay.
>
>
> For the umask yes, it was my bad, sorry.
> >
> >> + if (close(mkstemp(mtabtmpfile)) == -1 ) {
> >> + fprintf(stderr, "del_mtab: cannot setup tmp file destination");
> >> + (void)umask(mode);
> >
> > You probably don't really need to reset the umask on error, but I guess
> > it doesn't really hurt...
> >
> >> + rc = EX_FILEIO;
> >> + goto del_mtab_exit;
> >> }
> >> + (void)umask(mode);
> >>
> >> mntmtab = setmntent(MOUNTED, "r");
> >> if (!mntmtab) {
> >
> > Now that I look at that us-cert page though, I'm not sure you're really
> > making anything safer with this patch...
> >
> > AFAICT, the thing that makes mkstemp safer than mktemp is the fact that
> > the filename generation and open are atomic. If you use mkstemp here
> > and then just close the file afterward, aren't you still subject to the
> > same set of races that you are with mktemp?
> >
> > In fact, it seems like this is even worse, really. With this, you're
> > creating the file and then reopening it. Within that window of time,
> > an attacker now has some idea of what file you plan to use here
> > whereas he didn't really before...
> >
> > I think what might be better is to continue to use mktemp, but do the
> > setmntent call with a type value of "wx" to make it use an exclusive
> > open. If that fails with EEXIST, we should call mktemp again and try
> > again until it succeeds. Of course, not all libc's will take "x" as an
> > arg there, so you may need an autoconf test to verify that that will
> > work...
> >
> > An alternate approach might be to use mkstemp and fdopen, and then
> > simply use the resulting FILE pointer in the addmntent calls. From a
> > quick look at glibc, it looks like setmntent is basically a wrapper
> > around fopen anyway, but that may not be universally true in all
> > libc's. Hmm...now that I look though, glibc extends the fopen mode with
> > "ce" too and you'd probably want to do the same. So you might still
> > need an autoconf test for that anyway...
> >
> > Either way, this code should probably be moved to a separate helper
> > function that does this for del_mtab instead of open-coding it there.
> >
> I agree with your analisys. The simple patch was only a standard way
> to get rid of a compiler warn
> of the use of mktemp, but in this case the technique is not correct.
>
> What is more, I am reasonably sure that these days del_mtab in linux
> cifs util it is no longer called anymore, and perhaps it would be best
> to drop the
> code all together. Opinions ?
>
I haven't seen a compiler warning from this. What compiler are you
using and with what options?
It's correct that it's not called when /etc/mtab is a symlink and a lot
of distros are doing that these days. But, people can and do use this
on older distros that have a real mtab, so I wouldn't be comfortable
removing that code outright.
--
Jeff Layton <jlayton@xxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
