[BusyBox 0004174]: handle_errors() buffer underflow
bugs at busybox.net
bugs at busybox.net
Sun Jul 20 22:12:56 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-20-2008 15:12 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!"
----------------------------------------------------------------------
cristic - 07-18-08 17:26
----------------------------------------------------------------------
OK, I tested it and it looks like the overflow is gone now.
But I have two other comments:
1. I'm not really sure I understand when you want to return EINVAL and
when
ERANGE. My understanding was that you want to return ERANGE when the
number is
valid, but out of range, and EINVAL when you encounter an invalid
character.
But this is definitely not the case. So for example:
bb_strtol("123456123456", &endptr, 10) -> sets ERANGE (correct number,
but too large)
bb_strtol("123x", &endptr, 10) -> sets ERANGE too (invalid
number)
2. In bb_strtol, you might want to replace
if (!isalnum(arg[0])) return ret_ERANGE();
by something like
if (!isalnum(first)) { *endp = (char*) arg; return ret_ERANGE();}
so that callers don't read uninitialized memory if they inspect **endp.
----------------------------------------------------------------------
vda - 07-19-08 01:23
----------------------------------------------------------------------
bb_strtonum.c says it all:
/* On exit: errno = 0 only if there was non-empty, '\0' terminated value
* errno = EINVAL if value was not '\0' terminated, but othervise ok
* Return value is still valid, caller should just check whether
end[0]
* is a valid terminating char for particular case. OTOH, if caller
* requires '\0' terminated input, [s]he can just check errno == 0.
* errno = ERANGE if value had alphanumeric terminating char
("1234abcg").
* errno = ERANGE if value is out of range, missing, etc.
* errno = ERANGE if value had minus sign for strtouXX (even "-0" is not
ok )
* return value is all-ones in this case.
*/
I found out that with this convention bb_strtoXXX() are easier to use than
strtoXXX. In many cases I don't need to analyse endptr, and thus I don't
need to _have_ endptr, I just pass NULL.
> - if (!isalnum(arg[0])) return ret_ERANGE();
> + if (!isalnum(first)) { *endp = (char*) arg; return ret_ERANGE();}
Adds more code. ERANGE according to the above is "hard error". No point in
examining anything, the number is *bogus* anyway, and you cannot "make it
right" by examining endptr.
----------------------------------------------------------------------
cristic - 07-19-08 18:13
----------------------------------------------------------------------
Thanks for the explanation. BTW, it might make more sense to use isxdigit
(0-9, a-f, A-F) instead of isalnum, but this is a minor issue.
----------------------------------------------------------------------
vda - 07-20-08 05:45
----------------------------------------------------------------------
think bb_strtou(str, NULL, 36)
----------------------------------------------------------------------
cristic - 07-20-08 12:16
----------------------------------------------------------------------
> think bb_strtou(str, NULL, 36)
Makes sense, my assumption was that base <= 16.
We can finally close it then, thanks again for the patch.
----------------------------------------------------------------------
vda - 07-20-08 15:12
----------------------------------------------------------------------
Thanks for your bug spotting. Please file more bugs! :)
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
07-18-08 17:25 cristic Note Added: 0009774
07-18-08 17:26 cristic Note Edited: 0009774
07-19-08 01:20 vda Note Added: 0009784
07-19-08 01:22 vda Note Edited: 0009784
07-19-08 01:23 vda Note Edited: 0009784
07-19-08 18:11 cristic Note Added: 0009814
07-19-08 18:12 cristic Note Edited: 0009814
07-19-08 18:13 cristic Note Edited: 0009814
07-20-08 05:45 vda Note Added: 0009824
07-20-08 12:16 cristic Note Added: 0009834
07-20-08 15:12 vda Note Added: 0009844
======================================================================
More information about the busybox-cvs
mailing list