Re: lib/40693: _gettemp() flawed

[Vadim Zhukov <persgray%gmail.com@localhost>]

On 19 February 2009 г. 22:15:03 Alan Barrett wrote:
> The following reply was made to PR lib/40693; it has been noted by
> GNATS.
>
> From: Alan Barrett <apb%cequrux.com@localhost>
> To: gnats-bugs%NetBSD.org@localhost
> Cc:
> Subject: Re: lib/40693: _gettemp() flawed
> Date: Thu, 19 Feb 2009 21:07:07 +0200
>
>  On Thu, 19 Feb 2009, persgray%gmail.com@localhost wrote:
>  > After fixing out-of-bounds access in OpenBSD's version of this
>  > function, I looked at NetBSD's one. As far as I can see, current
>  > implementation of _gettemp() in libc (core function for mk*temp(3))
>  > is flawed by many ways:
>  >
>  > - It produces highly predictable (i.e. insecure) values;
>  > - It may (should) cause SIGSEGV when path (template) provided has
>  > zero length;
>  > - Maybe more.
>
>  I am not sure that the "highly predictable values" is a real problem,
>  except for callers who use the deprecated mktemp(3) interface.  Users
>  of the mkstemp(3) and mkdtemp(3) inetrfaces should get the advertised
>  uniqueness guarantees.

Yes, mkstemp(3) and mkdtemp(3) are safe enough, but there is some legacy 
software that was not ported to using those routines. Of course, it's 
your (by "you" I mean NetBSD developers) politics whether to care or not 
about such programs. But if you don't care, you can just remove 
mktemp(3). And even mktemp(3) may be used safely using "open(O_CREAT|
O_EXCL); fdopen()" dance.

>  I do see a problem when strlen(path) = 0 (it tries to access
> path[-1]).
>
>  > I recommend to replace it via OpenBSD's _gettemp() implementation:
>  >
>  > http://www.openbsd.org/cgi-bin/cvsweb/src/lib/libc/stdio/mktemp.c?r
>  >ev=1.25
>
>  That implementation calls strlen(path) without verifying that path !=
> NULL.

The only function for which POSIX says it accepts NULL pointer is 
tmpnam() - it is logical to assume (though assuming anything in standard 
is bad practice in general, of course) that other functions should not 
care about NULL pointer. Moreover, open(2) returns EFAULT for NULL 
pointer passed, not EINVAL - this makes a little confusion.

If you care, it's not more than adding

        _DIAGASSERT(path != NULL);

to the OpenBSD's implementation. I can make a full patch, if you wish. 
And in NetBSD it's already checked in __gettemp() callers, and if 
someone will call __gettemp() directly they'll by definition shoot in 
their foot.

-- 
  Best wishes,
    Vadim Zhukov