On Thu, 3 May 2012 10:16:35 +0200
Elia Pinto <gitter.spiros@xxxxxxxxx> wrote:
> 2012/5/2 Jeff Layton <jlayton@xxxxxxxxx>:
> > 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?
> Hello. See below
>
> gcc -DHAVE_CONFIG_H -I. -Wall -Wextra -Werror -D_FORTIFY_SOURCE=2
> -g -O2 -MT mount.cifs.o -MD -MP -MF .deps/mount.cifs.Tpo -c -o
> mount.cifs.o mount.cifs.c
> mv -f .deps/mount.cifs.Tpo .deps/mount.cifs.Po
> gcc -Wall -Wextra -Werror -D_FORTIFY_SOURCE=2 -g -O2 -o mount.cifs
> mount.cifs.o mtab.o resolve_host.o util.o -lcap-ng -lrt
> mount.cifs.o: In function `del_mtab':
> /home/machbuild/proj/cifs-utils/mount.cifs.c:1655: warning: the use of
> `mktemp' is dangerous, better use `mkstemp'
> make[2]: Leaving directory `/home/machbuild/proj/cifs-utils'
> make[1]: Leaving directory `/home/machbuild/proj/cifs-utils'
> [root@esil231 cifs-utils]# gcc --version
> gcc (GCC) 4.5.1 20100924 (Red Hat 4.5.1-4)
> Copyright (C) 2010 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions. There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>
> [root@esil231 cifs-utils]# ls -l /etc/mtab
> -rw-r--r--. 1 root root 386 2012-05-03 09:40 /etc/mtab
>
> This is a old FC12. del_mtab was introduced by commit id
> f46dd7661cfb87257c95081fc2071c934bfbbb16
>
> commit f46dd7661cfb87257c95081fc2071c934bfbbb16
> Author: Carlos Maiolino <cmaiolino@xxxxxxxxxx>
> Date: Mon Jan 16 12:29:49 2012 -0500
>
> mount.cifs: Properly update mtab during remount
>
> During a remount of a cifs filesystem, the mtab file is not properly
> updated, which leads to a doubled entry of the same filesystem in the
> /etc/mtab file. This patch adds a new function del_mtab() which is
> called before the add_mtab() in case the fs is being remounted.
>
> In fact del_mtab it is only called by mount(8), or mount.cifs, in a
> remount phase(i was wrong when i have told that it is not used
> anymore, sorry).
>
> So, to get rid of this warning, i have to do a better patch. Thanks
>
>
So I guess this is on fedora 14 or so?
Hrm...as best I can tell this is a link_warning that comes from glibc,
but I don't see it on more recent Fedora machines and I'm building with
the same flags.
OTOH, I do see it when I build on RHEL6, but luckily this doesn't seem
to cause the build to fail, even when -Werror is set.
FWIW, The manpage for mktemp(3) and this warning seem unnecessarily
alarmist, in my opinion:
"Since on the one hand the names are easy to guess, and on the
other hand there is a race between testing whether the name exists
and opening the file, every use of mktemp() is a security risk."
...which is of course bullshit since you can use O_EXCL (or "x" for
fopen) and wrap the whole thing in:
do { ... } while (errno == EEXIST);
--
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
