[PATCH] ping6: segfault on incorrect command line

[walter harms]

Michael Abbott schrieb:
> On Thu, 19 Nov 2009, walter harms wrote:
>> Michael Abbott schrieb:
>>> On Wed, 18 Nov 2009, Leonid Lisovskiy wrote:
>>>> ping6 segfaults on incorrect command line in case of busybox linked 
>>>> against uClibc 0.9.30.1 example:
>>>>
>>>> $ ping6 -Z google.com
>>>> Segmentation fault
>>>>
>>>> Seems to be this happens due to uClibc don't initialize argv[-1] at
>>>> all. Simple patch is below:
>>>> --- a/networking/ping.c 2009-09-26 17:14:57.000000000 +0400
>>>> +++ a/networking/ping.c 2009-11-18 20:39:19.000000000 +0300
>>>> @@ -769,6 +769,7 @@ int ping_main(int argc UNUSED_PARAM, cha
>>>>  int ping6_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
>>>>  int ping6_main(int argc UNUSED_PARAM, char **argv)
>>>>  {
>>>> +       argv[-1] = argv[0];
>>>>         argv[0] = (char*)"-6";
>>>>         return ping_main(0 /* argc+1 - but it's unused anyway */,
>>>>                         argv - 1);
>>> Is argv[-1] even a valid address?  Surely you're overwriting unknown 
>>> memory here, with entirely unpredictable consequences?
>>>
>>> Or is this argv defined elsewhere to extend into negative offsets?  
>>> If you're saying it's come from uClibc I doubt it, in which case the 
>>> bug has to be passing argv-1 to ping_main.
>> I guess the basic idea is to rewrite
>> ping6 arg ->   ping -6 arg
>>
>> ping: usage.h: -4, -6          Force IPv4 or IPv6 hostname resolution
> 
> I guess so.  Looks as if the trick is just a little bit *too* cute, 
> though: somebody's trying to add extra storage to an array (argv[]) that 
> doesn't have room for it!
> 
> Solutions: 1. alloca() a copy of argv, or use a dynamically sized array 
> (pity that argc is undefined ... or can we actually rely on it here) and 
> make room for the extra parameter: is this proper busybox style?  Not sure 
> about alloca in restricted environments, and my alloca(3) man page 
> deprecates it!
> 
> 2. Refactor ping_main and ping6_main to share a common routine with an 
> extra flag.
> 
> Sorry, I'm not so familiar with the innards of busybox.  Looks like this 
> trick goes a fair way back, so maybe there's some reason for believing it 
> should work?
> 

I think it worked because the original was simply:
 argv[0] = (char*)"-6";
the argv[-1] trick is a loop to much by using the wrong memory
the ping_main has this trick here:



#if ENABLE_PING6
    while ((++argv)[0] && argv[0][0] == '-') {  <-- so the argv-1 is important for ping6
                if (argv[0][1] == '4') {
                        af = AF_INET;
                        continue;
                }
                if (argv[0][1] == '6') {
                        af = AF_INET6;
                        continue;
                }
                bb_show_usage();
        }

#else
        argv++;
#endif


@leonid:
i can not reproduce your problem, what version of bb do you use ?

 ./busybox ping6 -Z google.com
BusyBox v1.15.0 (2009-11-19 16:17:14 CET) multi-call binary

Usage: ping6 host

Send ICMP ECHO_REQUEST packets to network hosts