Re: lib/40693: _gettemp() flawed

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

The following reply was made to PR lib/40693; it has been noted by GNATS.

From: Vadim Zhukov <persgray%gmail.com@localhost>
To: gnats-bugs%netbsd.org@localhost
Cc: lib-bug-people%netbsd.org@localhost,
 gnats-admin%netbsd.org@localhost,
 netbsd-bugs%netbsd.org@localhost
Subject: Re: lib/40693: _gettemp() flawed
Date: Fri, 20 Feb 2009 12:00:02 +0300

 On 19 February 2009 =C7. 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=20
 software that was not ported to using those routines. Of course, it's=20
 your (by "you" I mean NetBSD developers) politics whether to care or not=20
 about such programs. But if you don't care, you can just remove=20
 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) =3D 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=3D1.25
 >
 >  That implementation calls strlen(path) without verifying that path !=3D
 > NULL.
 
 The only function for which POSIX says it accepts NULL pointer is=20
 tmpnam() - it is logical to assume (though assuming anything in standard=20
 is bad practice in general, of course) that other functions should not=20
 care about NULL pointer. Moreover, open(2) returns EFAULT for NULL=20
 pointer passed, not EINVAL - this makes a little confusion.
 
 If you care, it's not more than adding
 
         _DIAGASSERT(path !=3D NULL);
 
 to the OpenBSD's implementation. I can make a full patch, if you wish.=20
 And in NetBSD it's already checked in __gettemp() callers, and if=20
 someone will call __gettemp() directly they'll by definition shoot in=20
 their foot.
 
 =2D-=20
   Best wishes,
     Vadim Zhukov