lib/42636: [dM] t_getstr potential memory overrun

[der Mouse <mouse%Rodents-Montreal.ORG@localhost>]

>Number:         42636
>Category:       lib
>Synopsis:       [dM] t_getstr potential memory overrun
>Confidential:   no
>Severity:       serious
>Priority:       low
>Responsible:    lib-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Jan 18 07:30:00 +0000 2010
>Originator:     der Mouse
>Release:        NetBSD 4.0.1
>Organization:
        Dis-
>Environment:
System: NetBSD iMac-backup.Rodents-Montreal.ORG 4.0.1 NetBSD 4.0.1 
(IMAC-BACKUP) #2: Thu Dec 31 09:47:34 EST 2009 
mouse%iMac-backup.Rodents-Montreal.ORG@localhost:/home/mouse/kbuild/IMAC-BACKUP 
i386
Architecture: i386
Machine: i386
>Description:
        The manpage description of t_getstr describes the area and
        limit parameters saying that the "limit parameter that gives
        the number of characters that can be inserted in to the array
        pointed to by area", adding that it "is updated [...] to give
        the number of characters that remain available in area", and
        that if area is nil then "the size required to hold the
        capability string will be returned in limit".

        However, this is not quite the reality.  The area pointer is
        advanced by one more than the limit parameter is decreased by,
        leading to a potential buffer overrun by as many bytes as there
        are string capabilities involved - the limit advance does not
        take the \0 into account, while the area pointer advance does.
        Also, the returned limit value for a nil-pointer area call is
        the amount the limit value would have been decreased by, not
        the number of bytes that would have been copied if the area
        pointer had been non-nil, thus inviting the caller to allocate
        one byte too few.  (Fortunately, the buffer overrun is more
        difficult to exploit than most buffer overruns, since the
        overrun data is not very controllable by the attacker in most
        scenarios.)

        It's possible there's actually a standard somewhere that
        specifies this bizarre bug-inviting behaviour.  This is serious
        enough that I would actually recommend ignoring any such
        standard; if NetBSD does not want to do that, this should then
        be reclassified as a doc-bug and warnings splashed all over the
        manpage that the behaviour is severely counterintuitive and the
        pointer and limit variables must be treated very carefully to
        avoid buffer overrun bugs.
>How-To-Repeat:
        % cat z.c
        #include <stdio.h>
        #include <errno.h>
        #include <stdlib.h>
        #include <termcap.h>
        
        extern const char *__progname;
        
        static void fail(const char *why)
        {
         fprintf(stderr,"%s: %s\n",__progname,why);
         exit(1);
        }
        
        int main(void);
        int main(void)
        {
         struct tinfo *inf;
         char area[64];
         char *ap;
         size_t lim;
         char *cap;
        
         if (t_getent(&inf,"vt100") != 1) fail("can't look up vt100");
         ap = &area[0];
         lim = sizeof(area);
         cap = t_getstr(inf,"bl",&ap,&lim);
         if (cap == 0) fail("can't look up bl capability");
         printf("area pointer advanced by %d\n",(int)(ap-&area[0]));
         printf("limit reduced by %d\n",(int)(sizeof(area)-lim));
         errno = 0;
         lim = 0;
         t_getstr(inf,"bl",0,&lim);
         if (errno) fail("can't nil-pointer check bl capability");
         printf("nil-pointer length is %d\n",(int)lim);
         return(0);
        }
        % cc -o z z.c
        % ./z
        area pointer advanced by 2
        limit reduced by 1
        nil-pointer length is 1
        % 
>Fix:
        Relative to termcap.c,v 1.49:

        --- termcap.c.orig      2010-01-17 11:56:50.000000000 -0500
        +++ termcap.c   2010-01-17 11:58:22.000000000 -0500
        @@ -384,6 +384,7 @@
                        return NULL;
                }
         
        +       i ++; /* account for the NUL */
                if (area != NULL) {
                        /*
                         * check if there is room for the new entry to be put 
into
        @@ -398,7 +399,7 @@
                        (void)strcpy(*area, s);
                        free(s);
                        s = *area;
        -               *area += i + 1;
        +               *area += i;
                        if (limit != NULL) *limit -= i;
         
                        return (s);

/~\ The ASCII                             Mouse
\ / Ribbon Campaign
 X  Against HTML                mouse%rodents-montreal.org@localhost
/ \ Email!           7D C8 61 52 5D E7 2D 39  4E F1 31 3E E8 B3 27 4B

>Unformatted:
        Also believed present as far back as 1.4T - probably anything
        with the t_* termcap routines.  May be present in more recent
        versions too; I don't have anything newer at ready hand.