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
>
> 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
