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 ?
Best regards
--
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
