On Wed, 9 May 2012 09:56:11 -0300
Carlos Maiolino <cmaiolino@xxxxxxxxxx> 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@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
