[PATCH] use mkstemp instead of mktemp

[PATCH] use mkstemp instead of mktemp

[Elia Pinto]

As described in the manual page mktemp(3) the use of this feature
is strongly discouraged in favor of mkstemp(3).

In fact the mkstemp() function generates a unique temporary file
name from the supplied template,
opens a file of that name using the O_EXCL flag (guaranteeing the
current process to be the only user) and returns a file descriptor.

But the POSIX specification does not say anything about file modes,
so the application should make sure its umask
is set appropriately before calling mkstemp.
( ref. https://buildsecurityin.us-cert.gov/bsi/articles/knowledge/coding/781-BSI.html)

Signed-off-by: Elia Pinto <gitter.spiros-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 mount.cifs.c |   18 +++++++++++++-----
 1 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/mount.cifs.c b/mount.cifs.c
index c90ce3e..bdd9d28 100644
--- a/mount.cifs.c
+++ b/mount.cifs.c
@@ -1627,6 +1627,9 @@ del_mtab(char *mountpoint)
 	FILE *mnttmp, *mntmtab;
 	struct mntent *mountent;
 	char *mtabfile, *mtabdir, *mtabtmpfile;
+	mode_t mode;
+	FILE *spf;
+	int fd = -1 ;
 
 	mtabfile = strdup(MOUNTED);
 	mtabdir = dirname(mtabfile);
@@ -1652,12 +1655,17 @@ del_mtab(char *mountpoint)
 		goto del_mtab_exit;
 	}
 
-	mtabtmpfile = mktemp(mtabtmpfile);
-	if (!mtabtmpfile) {
-		fprintf(stderr, "del_mtab: cannot setup tmp file destination");
-		rc = EX_FILEIO;
-		goto del_mtab_exit;
+	mode = umask(0077);
+	(void) umask(mode);
+	if ((fd = mkstemp(mtabtmpfile)) == -1 ||
+		(spf = fdopen(fd, "w+")) == NULL) {
+		if (fd != -1) {
+			fprintf(stderr, "del_mtab: cannot setup tmp file destination");
+			rc = EX_FILEIO;
+			goto del_mtab_exit;
+                }
 	}
+	(void) fclose(spf);
 
 	mntmtab = setmntent(MOUNTED, "r");
 	if (!mntmtab) {
-- 
1.7.1

Re: [PATCH] use mkstemp instead of mktemp

[Jeff Layton]

On Thu, 26 Apr 2012 11:36:38 -0400
Elia Pinto <gitter.spiros-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> As described in the manual page mktemp(3) the use of this feature
> is strongly discouraged in favor of mkstemp(3).
> 
> In fact the mkstemp() function generates a unique temporary file
> name from the supplied template,
> opens a file of that name using the O_EXCL flag (guaranteeing the
> current process to be the only user) and returns a file descriptor.
> 
> But the POSIX specification does not say anything about file modes,
> so the application should make sure its umask
> is set appropriately before calling mkstemp.
> ( ref. https://buildsecurityin.us-cert.gov/bsi/articles/knowledge/coding/781-BSI.html)
> 
> Signed-off-by: Elia Pinto <gitter.spiros-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  mount.cifs.c |   18 +++++++++++++-----
>  1 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/mount.cifs.c b/mount.cifs.c
> index c90ce3e..bdd9d28 100644
> --- a/mount.cifs.c
> +++ b/mount.cifs.c
> @@ -1627,6 +1627,9 @@ del_mtab(char *mountpoint)
>  	FILE *mnttmp, *mntmtab;
>  	struct mntent *mountent;
>  	char *mtabfile, *mtabdir, *mtabtmpfile;
> +	mode_t mode;
> +	FILE *spf;
> +	int fd = -1 ;
>  
>  	mtabfile = strdup(MOUNTED);
>  	mtabdir = dirname(mtabfile);
> @@ -1652,12 +1655,17 @@ del_mtab(char *mountpoint)
>  		goto del_mtab_exit;
>  	}
>  
> -	mtabtmpfile = mktemp(mtabtmpfile);
> -	if (!mtabtmpfile) {
> -		fprintf(stderr, "del_mtab: cannot setup tmp file destination");
> -		rc = EX_FILEIO;
> -		goto del_mtab_exit;
> +	mode = umask(0077);

(cc'ing Carlos since he did most of this work originally)

I think we want the final mtab to be group and world-readable, right?
So maybe umask(0033) would be better? We should probably also reset the
umask afterward too just to be safe...

> +	(void) umask(mode);
> +	if ((fd = mkstemp(mtabtmpfile)) == -1 ||
> +		(spf = fdopen(fd, "w+")) == NULL) {
> +		if (fd != -1) {
> +			fprintf(stderr, "del_mtab: cannot setup tmp file destination");
> +			rc = EX_FILEIO;
> +			goto del_mtab_exit;
> +                }
>  	}
> +	(void) fclose(spf);
>  

Looks reasonable to change to mkstemp, but what's the point of doing the
the fdopen there if you're just going to immediately close it
afterward? Why not just close(fd) there?

...or, is it legit to call fdopen on fd and and then just pretend
that's equivalent to a setmntent and get rid of the later setmntent to
open the file?

>  	mntmtab = setmntent(MOUNTED, "r");
>  	if (!mntmtab) {


-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH] use mkstemp instead of mktemp

[Carlos Maiolino]

Hi,

> >  	FILE *mnttmp, *mntmtab;
> >  	struct mntent *mountent;
> >  	char *mtabfile, *mtabdir, *mtabtmpfile;
> > +	mode_t mode;
> > +	FILE *spf;
> > +	int fd = -1 ;
> >  
> >  	mtabfile = strdup(MOUNTED);
> >  	mtabdir = dirname(mtabfile);
> > @@ -1652,12 +1655,17 @@ del_mtab(char *mountpoint)
> >  		goto del_mtab_exit;
> >  	}
> >  
> > -	mtabtmpfile = mktemp(mtabtmpfile);
> > -	if (!mtabtmpfile) {
> > -		fprintf(stderr, "del_mtab: cannot setup tmp file destination");
> > -		rc = EX_FILEIO;
> > -		goto del_mtab_exit;
> > +	mode = umask(0077);
> 
> (cc'ing Carlos since he did most of this work originally)
> 
> I think we want the final mtab to be group and world-readable, right?
> So maybe umask(0033) would be better? We should probably also reset the
> umask afterward too just to be safe...
> 
> > +	(void) umask(mode);
> > +	if ((fd = mkstemp(mtabtmpfile)) == -1 ||
> > +		(spf = fdopen(fd, "w+")) == NULL) {
> > +		if (fd != -1) {
> > +			fprintf(stderr, "del_mtab: cannot setup tmp file destination");
> > +			rc = EX_FILEIO;
> > +			goto del_mtab_exit;
> > +                }
> >  	}
> > +	(void) fclose(spf);
> >  
> 
> Looks reasonable to change to mkstemp, but what's the point of doing the
> the fdopen there if you're just going to immediately close it
> afterward? Why not just close(fd) there?
> 
The mainly reason I chose mktemp() instead of mkstemp was due its return value.
The mktemp() returns a template, while mkstemp returns a file descriptor, which
makes the need to handle the file descriptor.
Due our usage of mktemp() here, I didn't think we were subjected to both risks
it implies (the limit of at most 26 different names and the race), and also,
mktemp() made it easier to implement. But I have no objection to change it to
mkstemp if anyone believes it's a risk.

 
Sorry for my delay though, I was enjoying some vacations :)

-- 
--Carlos

Re: [PATCH] use mkstemp instead of mktemp

[Jeff Layton]

On Wed, 9 May 2012 09:56:11 -0300
Carlos Maiolino <cmaiolino-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> Hi,
> 
> > >  	FILE *mnttmp, *mntmtab;
> > >  	struct mntent *mountent;
> > >  	char *mtabfile, *mtabdir, *mtabtmpfile;
> > > +	mode_t mode;
> > > +	FILE *spf;
> > > +	int fd = -1 ;
> > >  
> > >  	mtabfile = strdup(MOUNTED);
> > >  	mtabdir = dirname(mtabfile);
> > > @@ -1652,12 +1655,17 @@ del_mtab(char *mountpoint)
> > >  		goto del_mtab_exit;
> > >  	}
> > >  
> > > -	mtabtmpfile = mktemp(mtabtmpfile);
> > > -	if (!mtabtmpfile) {
> > > -		fprintf(stderr, "del_mtab: cannot setup tmp file destination");
> > > -		rc = EX_FILEIO;
> > > -		goto del_mtab_exit;
> > > +	mode = umask(0077);
> > 
> > (cc'ing Carlos since he did most of this work originally)
> > 
> > I think we want the final mtab to be group and world-readable, right?
> > So maybe umask(0033) would be better? We should probably also reset the
> > umask afterward too just to be safe...
> > 
> > > +	(void) umask(mode);
> > > +	if ((fd = mkstemp(mtabtmpfile)) == -1 ||
> > > +		(spf = fdopen(fd, "w+")) == NULL) {
> > > +		if (fd != -1) {
> > > +			fprintf(stderr, "del_mtab: cannot setup tmp file destination");
> > > +			rc = EX_FILEIO;
> > > +			goto del_mtab_exit;
> > > +                }
> > >  	}
> > > +	(void) fclose(spf);
> > >  
> > 
> > Looks reasonable to change to mkstemp, but what's the point of doing the
> > the fdopen there if you're just going to immediately close it
> > afterward? Why not just close(fd) there?
> > 
> The mainly reason I chose mktemp() instead of mkstemp was due its return value.
> The mktemp() returns a template, while mkstemp returns a file descriptor, which
> makes the need to handle the file descriptor.
> Due our usage of mktemp() here, I didn't think we were subjected to both risks
> it implies (the limit of at most 26 different names and the race), and also,
> mktemp() made it easier to implement. But I have no objection to change it to
> mkstemp if anyone believes it's a risk.
> 
>  
> Sorry for my delay though, I was enjoying some vacations :)
> 

Well, there is a minor risk here. We don't open the file with O_EXCL and
we should. It's possible someone could guess the filename and be able
to create it during the race window. Presumably they'd already need to
be root to do that in which case they could already clobber /etc/mtab,
but it doesn't hurt to be safe there I guess...

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH] Use mkstemp instead of mktemp

[Jeff Layton]

On Thu, 3 May 2012 10:16:35 +0200
Elia Pinto <gitter.spiros-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> 2012/5/2 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> > On Wed, 2 May 2012 18:40:05 +0200
> > Elia Pinto <gitter.spiros-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> 2012/4/28 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> >> > On Fri, 27 Apr 2012 11:12:48 -0400
> >> > Elia Pinto <gitter.spiros-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 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-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH] Use mkstemp instead of mktemp

[Elia Pinto]

2012/5/2 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> On Wed, 2 May 2012 18:40:05 +0200
> Elia Pinto <gitter.spiros-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> 2012/4/28 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
>> > On Fri, 27 Apr 2012 11:12:48 -0400
>> > Elia Pinto <gitter.spiros-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
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-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH] Use mkstemp instead of mktemp

[Jeff Layton]

On Wed, 2 May 2012 18:40:05 +0200
Elia Pinto <gitter.spiros-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> 2012/4/28 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> > On Fri, 27 Apr 2012 11:12:48 -0400
> > Elia Pinto <gitter.spiros-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH] Use mkstemp instead of mktemp

[Elia Pinto]

2012/4/28 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> On Fri, 27 Apr 2012 11:12:48 -0400
> Elia Pinto <gitter.spiros-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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 ?

Best regards

Re: [PATCH] Use mkstemp instead of mktemp

[Jeff Layton]

On Fri, 27 Apr 2012 11:12:48 -0400
Elia Pinto <gitter.spiros-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> As described in the manual page mktemp(3) the use of this function
> is strongly discouraged in favor of mkstemp(3).
> 
> In fact the mkstemp() function generates a unique temporary file
> name from the supplied template,
> opens a file of that name using the O_EXCL flag (guaranteeing the
> current process to be the only user) and returns a file descriptor.
> 
> The file is then created with mode read/write and permissions 0666 (glibc 2.0.6 and earlier),
> 0600 (glibc 2.0.7 and later).
> 
> But for portability the POSIX specification does not say anything about file modes,
> so the application should make sure its umask is set appropriately before calling mkstemp.
> ( ref. https://buildsecurityin.us-cert.gov/bsi/articles/knowledge/coding/781-BSI.html)
> 
> In this case we set umask to 033 before calling mkstemp
> and after we reset the original umask.
> 
> Signed-off-by: Elia Pinto <gitter.spiros-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> This is the second version of the patch. I have
> tried to follow the Jeff indication
> http://article.gmane.org/gmane.linux.kernel.cifs/5988
> 
> 
>  mount.cifs.c |   13 ++++++++-----
>  1 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/mount.cifs.c b/mount.cifs.c
> index c90ce3e..1cdaf38 100644
> --- a/mount.cifs.c
> +++ b/mount.cifs.c
> @@ -1627,6 +1627,7 @@ del_mtab(char *mountpoint)
>  	FILE *mnttmp, *mntmtab;
>  	struct mntent *mountent;
>  	char *mtabfile, *mtabdir, *mtabtmpfile;
> +	mode_t mode;
>  
>  	mtabfile = strdup(MOUNTED);
>  	mtabdir = dirname(mtabfile);
> @@ -1652,12 +1653,14 @@ del_mtab(char *mountpoint)
>  		goto del_mtab_exit;
>  	}
>  
> -	mtabtmpfile = mktemp(mtabtmpfile);
> -	if (!mtabtmpfile) {
> -		fprintf(stderr, "del_mtab: cannot setup tmp file destination");
> -		rc = EX_FILEIO;
> -		goto del_mtab_exit;
> +	mode = umask(0077);

Looks like you're still setting the umask to 0077 here?

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

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

[PATCH] Use mkstemp instead of mktemp

[Elia Pinto]

As described in the manual page mktemp(3) the use of this function
is strongly discouraged in favor of mkstemp(3).

In fact the mkstemp() function generates a unique temporary file
name from the supplied template,
opens a file of that name using the O_EXCL flag (guaranteeing the
current process to be the only user) and returns a file descriptor.

The file is then created with mode read/write and permissions 0666 (glibc 2.0.6 and earlier),
0600 (glibc 2.0.7 and later).

But for portability the POSIX specification does not say anything about file modes,
so the application should make sure its umask is set appropriately before calling mkstemp.
( ref. https://buildsecurityin.us-cert.gov/bsi/articles/knowledge/coding/781-BSI.html)

In this case we set umask to 033 before calling mkstemp
and after we reset the original umask.

Signed-off-by: Elia Pinto <gitter.spiros-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
This is the second version of the patch. I have
tried to follow the Jeff indication
http://article.gmane.org/gmane.linux.kernel.cifs/5988


 mount.cifs.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/mount.cifs.c b/mount.cifs.c
index c90ce3e..1cdaf38 100644
--- a/mount.cifs.c
+++ b/mount.cifs.c
@@ -1627,6 +1627,7 @@ del_mtab(char *mountpoint)
 	FILE *mnttmp, *mntmtab;
 	struct mntent *mountent;
 	char *mtabfile, *mtabdir, *mtabtmpfile;
+	mode_t mode;
 
 	mtabfile = strdup(MOUNTED);
 	mtabdir = dirname(mtabfile);
@@ -1652,12 +1653,14 @@ del_mtab(char *mountpoint)
 		goto del_mtab_exit;
 	}
 
-	mtabtmpfile = mktemp(mtabtmpfile);
-	if (!mtabtmpfile) {
-		fprintf(stderr, "del_mtab: cannot setup tmp file destination");
-		rc = EX_FILEIO;
-		goto del_mtab_exit;
+	mode = umask(0077);
+	if (close(mkstemp(mtabtmpfile)) == -1 ) {
+			fprintf(stderr, "del_mtab: cannot setup tmp file destination");
+			(void)umask(mode);
+			rc = EX_FILEIO;
+			goto del_mtab_exit;
 	}
+	(void)umask(mode);
 
 	mntmtab = setmntent(MOUNTED, "r");
 	if (!mntmtab) {
-- 
1.7.8.rc3.31.g017d1