[BusyBox 0004174]: handle_errors() buffer underflow
bugs at busybox.net
bugs at busybox.net
Fri Jul 18 18:10:37 UTC 2008
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-18-2008 11:10 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.
----------------------------------------------------------------------
cristic - 07-16-08 17:17
----------------------------------------------------------------------
> if (!isalnum(arg[0])) return ret_ERANGE(); <==========!!!
> ...
> 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]).
Sure, but isalnum() tests if the character is alphanumeric (digit _or_
letter), not if it's a digit. But I agree that if you replace isalnum
with isdigit you solve the problem; quite a short fix.
----------------------------------------------------------------------
vda - 07-17-08 01:45
----------------------------------------------------------------------
Oh. indeed, its isalnum! :(
replacing isalnum with isdigit is not good - it will break conversions to
base 16.
I think the working fix would be swapping "if (endptr[0])" and "if
(endptr[-1] == '-')" checks in handle_errors(). If strtoul indeed stopped
on the very first char, if (endptr[0])... will trigger and in the if body
it checks for isalnum() and returns. "if (endptr[-1]..." won't be
executed.
----------------------------------------------------------------------
cristic - 07-17-08 13:01
----------------------------------------------------------------------
I think the handle_errors() functions should not be more complicated than
this:
static unsigned long long handle_errors(unsigned long long v, char **endp,
char *endptr)
{
if (endp) *endp = endptr;
/* errno is already set to ERANGE by strtoXXX if value overflowed */
if (!errno && endptr[0]) {
/* good number, just suspicious terminator */
errno = EINVAL;
}
return v;
}
I tested it in conjunction with bb_strtol() and it seems to work
fine. I think the confusion is about the contract for
strto[u][l]. The contract for these functions is to convert the
prefix of the given string to a number, not the entire number.
And it looks the way EINVAL is set is not portable across
platforms. So, strtol("-", &endptr, 10) does indeed return "0"
and sets errno to 0, but so does strtol("w", &endptr, 10). But
endptr[0] is zero if and only if the entire string is valid, so I
think testing it is enough.
----------------------------------------------------------------------
vda - 07-17-08 14:47
----------------------------------------------------------------------
Try:
busybox printf '%d\n' -
For me it prints "-0", and this is WRONG. "-" is not 0 or -0, it's not a
number.
> strtol("-", &endptr, 10) does indeed return "0" and sets errno to 0, but
so does strtol("w", &endptr, 10)
But in case of "w" examining endptr[0] and seeing 'w' there clearly says
us that conversion failed, whereas with "-" endptr[0] is NUL. Many
programs conclude that if endptr[0] is NUL and errno is 0 it means that
entire string was a valid number. With "-" it is not true! I want to
detect that case.
And btw, printf fails to do it not because bb_strtol is buggy, but because
printf fails to test for errno!=0. Here is the instrumented output:
sh-3.2# ./busybox printf '%d\n' -
printf: bb_strtol('-'):0 errno:22
-printf: my_xstrtol('-'):0 errno:22
printf: bb_strtol('-'):0 errno:22
-0
And for comparison, this is what shell and coreutils do:
sh-3.2# printf '%d\n' -
sh: printf: -: invalid number
0
sh-3.2# /usr/bin/printf '%d\n' -
/usr/bin/printf: -: expected a numeric value
0
----------------------------------------------------------------------
cristic - 07-17-08 15:06
----------------------------------------------------------------------
>But in case of "w" examining endptr[0] and seeing 'w' there clearly says
us that
>conversion failed, whereas with "-" endptr[0] is NUL. Many programs
conclude that
>if endptr[0] is NUL and errno is 0 it means that entire string was a
valid
>number. With "-" it is not true! I want to detect that case.
Are you sure? What libc are you using? On my libc (GNU libc 2.6), if I
run strtol("-", &endptr, 10) and then examine endptr[0], I see "-", not
NUL. So with the version of handle_errors() that I proposed, bb_strtol()
sets errno to EINVAL, as desired. Maybe this is a bug in uclibc?
----------------------------------------------------------------------
vda - 07-18-08 11:10
----------------------------------------------------------------------
You are right. Please test 5.patch.
NB: I dont want to set EINVAL on "-", I want to set ERANGE. ERANGE is
considered to be "serious" error here - "this is not a number at all!"
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
07-16-08 17:17 cristic Note Added: 0009514
07-16-08 17:22 cristic Issue Monitored: cristic
07-17-08 01:45 vda Note Added: 0009564
07-17-08 01:45 vda Note Edited: 0009564
07-17-08 13:01 cristic Note Added: 0009674
07-17-08 14:46 vda Note Added: 0009684
07-17-08 14:47 vda Note Edited: 0009684
07-17-08 14:47 vda Note Edited: 0009684
07-17-08 15:06 cristic Note Added: 0009694
07-17-08 15:06 cristic Note Edited: 0009694
07-18-08 11:09 vda File Added: 5.patch
07-18-08 11:10 vda Note Added: 0009744
======================================================================
More information about the busybox-cvs
mailing list