NetBSD Problem Report #40693

From www@NetBSD.org  Thu Feb 19 16:28:00 2009
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by narn.NetBSD.org (Postfix) with ESMTP id 7D18163C0D5
	for <gnats-bugs@gnats.netbsd.org>; Thu, 19 Feb 2009 16:28:00 +0000 (UTC)
Message-Id: <20090219162800.43CB063C0CF@narn.NetBSD.org>
Date: Thu, 19 Feb 2009 16:28:00 +0000 (UTC)
From: persgray@gmail.com
Reply-To: persgray@gmail.com
To: gnats-bugs@NetBSD.org
Subject: _gettemp() flawed
X-Send-Pr-Version: www-1.0

>Number:         40693
>Category:       lib
>Synopsis:       _gettemp() flawed
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    lib-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Feb 19 16:30:00 +0000 2009
>Last-Modified:  Fri Feb 20 09:05:03 +0000 2009
>Originator:     Vadim Zhukov
>Release:        CURRENT
>Organization:
>Environment:
>Description:
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.

Thank you for your attention.
>How-To-Repeat:
/*
 * May die if memory exhausted before actual memory allocation
 * occurs at page start
 */
#include <stdio.h>

void
main() {
        char    *s;
        size_t   sz;

        for (sz = 1024;; sz *= 2) {
                if ((s = malloc(sz)) == NULL)
                        err(1, "malloc");
                *s = '\0';
                mktemp(s);
        }
}

>Fix:
I recommend to replace it via OpenBSD's _gettemp() implementation:

http://www.openbsd.org/cgi-bin/cvsweb/src/lib/libc/stdio/mktemp.c?rev=1.25

>Audit-Trail:
From: Alan Barrett <apb@cequrux.com>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: lib/40693: _gettemp() flawed
Date: Thu, 19 Feb 2009 21:07:07 +0200

 On Thu, 19 Feb 2009, persgray@gmail.com 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.

 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?rev=1.25

 That implementation calls strlen(path) without verifying that path != NULL.

 --apb (Alan Barrett)

From: Martin Husemann <martin@duskware.de>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: lib/40693: _gettemp() flawed
Date: Thu, 19 Feb 2009 20:19:36 +0100

 How is crashing (or not) a problem when passing invalid arguments to a
 libary function?

 Martin

From: Vadim Zhukov <persgray@gmail.com>
To: gnats-bugs@netbsd.org
Cc: lib-bug-people@netbsd.org,
 gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org
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>
 > To: gnats-bugs@NetBSD.org
 > Cc:
 > Subject: Re: lib/40693: _gettemp() flawed
 > Date: Thu, 19 Feb 2009 21:07:07 +0200
 >
 >  On Thu, 19 Feb 2009, persgray@gmail.com 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

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

 On 19 February 2009 =C7. 22:20:04 Martin Husemann wrote:
 > The following reply was made to PR lib/40693; it has been noted by
 > GNATS.
 >
 > From: Martin Husemann <martin@duskware.de>
 > To: gnats-bugs@NetBSD.org
 > Cc:
 > Subject: Re: lib/40693: _gettemp() flawed
 > Date: Thu, 19 Feb 2009 20:19:36 +0100
 >
 >  How is crashing (or not) a problem when passing invalid arguments to
 > a libary function?

 It's all about what to consider "invalid". Of course, this is NetBSD=20
 developer's decision, not my.

 Thanks to all for you attention.

 =2D-=20
   Best wishes,
     Vadim Zhukov

NetBSD Home
NetBSD PR Database Search

(Contact us) $NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2007 The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.