[BusyBox 0004174]: handle_errors() buffer underflow

[bugs at busybox.net]

A NOTE has been added to this issue. 
====================================================================== 
http://busybox.net/bugs/view.php?id=4174 
====================================================================== 
Reported By:                cristic
Assigned To:                BusyBox
====================================================================== 
Project:                    BusyBox
Issue ID:                   4174
Category:                   Other
Reproducibility:            always
Severity:                   minor
Priority:                   normal
Status:                     assigned
====================================================================== 
Date Submitted:             07-16-2008 16:11 PDT
Last Modified:              07-16-2008 16:28 PDT
====================================================================== 
Summary:                    handle_errors() buffer underflow
Description: 
In handle_errors() (libbb/bb_strtonum.c), the line "if (endptr[-1] == '-')"

can lead to a buffer underflow, and the outcome of the branch will
usually
depend on uninitialized memory.  Here is an example that triggers the
bug:

chmod a.b c

This leads to a call to bb_strtoul("a", NULL, 10), which calls
strtoul("a",
&endptr, 10), which will set endptr to point to the beginning of the
buffer
storing "a".  Then endptr is passed to handle_errors() which is illegally
examining endptr[-1]. 

Looking at the code, I was confused about the comment on the "weird"
feature. Is that still needed?  On my libc, calling strtol('-", &endptr,
10) sets endptr as expected to the beginning of the buffer storing "-", so
that particular if statement doesn't seem to be needed at all.

BTW, this bug can be hit by various other tools, here are some other
examples:
kill -l a
printf "% *" B
setuidgid a ""

Thanks,
Cristian

====================================================================== 

---------------------------------------------------------------------- 
 vda - 07-16-08 16:25  
---------------------------------------------------------------------- 
Nope.

unsigned long FAST_FUNC bb_strtoul(const char *arg, char **endp, int
base)
{
        unsigned long v;
        char *endptr;

        if (!isalnum(arg[0])) return ret_ERANGE();  <==========!!!
        errno = 0;
        v = strtoul(arg, &endptr, base);
        return handle_errors(v, endp, endptr);
}

We definitely know that arg[0] is a digit. Therefore strtoul will not set
endptr to &arg[0], at the very worst, it will be &arg[1]. And thus
endptr[-1]
is valid (it references arg[0]). 

---------------------------------------------------------------------- 
 vda - 07-16-08 16:28  
---------------------------------------------------------------------- 
> Is that still needed? On my libc, calling strtol("-", &endptr, 10) sets
endptr as expected to the beginning of the buffer storing "-"...

Yes. This is not a problem. The problem is, strtol returns 0 and does not
set errno. I refuse to believe that string "-" is a valid representation
of number 'zero', and I don't want busybox tools to accept that as a valid
numeric input. 

Issue History 
Date Modified   Username       Field                    Change               
====================================================================== 
07-16-08 16:11  cristic        New Issue                                    
07-16-08 16:11  cristic        Status                   new => assigned     
07-16-08 16:11  cristic        Assigned To               => BusyBox         
07-16-08 16:25  vda            Note Added: 0009494                          
07-16-08 16:28  vda            Note Added: 0009504                          
======================================================================