On Sun, Jul 11, 2010 at 4:53 AM, Amr Ali <[email protected]> wrote:
> On 07/10/2010 01:51 AM, Denys Vlasenko wrote:
>> On Friday 09 July 2010 21:12, Amr Ali wrote:
>>> I've noticed that there is an abundance of format string vulnerabilities all
>>> across busybox 1.17.0 source code. I understand that most of them are not
>>> practically exploitable or very hard to exploit, but I also noticed a
>>> couple of
>>> format string vulnerabilities that can be directly exploited.
>>>
>>> Attached is a patch to fix the found bugs.
>>
>> + char *res;
>> char line_old[80];
>> char line_new[80];
>> @@ -136,5 +137,5 @@
>> fp = fopen(argv[2], "r");
>> if (fp) {
>> - fgets(line_old, sizeof(line_old), fp);
>> + res = fgets(line_old, sizeof(line_old), fp);
>>
>> What exactly do you fix here? gcc will even generate the same code
>> with and without this patch. Yes, I did check that.
> That's just a slight fix to shut off GCC about a warning for unused return
> value, as fgets is declared with __attribute__((warn_unused_result));
I thought (void) cast is a usual method to do it. And both methods
obfuscate code. I think it's not worth it.
>> - printf(usage_array[i].aname);
>> + /*
>> + * Vulnerable to arbitrary format strings.
>> + * printf(usage_array[i].aname);
>> + */
>> + printf("%s", usage_array[i].aname);
>>
>> But applet names are not arbitrary format strings. They never contain
>> percent character.
> Indeed they won't, as I mentioned in my first email, it might never be
> exploitable, but it is nonetheless a good practice to not produce vulnerable
> code regardless if it's exploitable or not.
Busybox primary goal is small size. If I'm 100% sure printf(variable)
is safe, I should not make it bigger and slower.
> Besides, GCC won't allow such
> warnings to pass, it fails the compilation process as well, which was the
> reason
> I started looking into the code more.
Which version of gcc?
Unselect CONFIG_WERROR in your .config then.
>> In all, I see only two remotely-possible places in the whole patch
>> where it _might_ be a bug.
>
> I'm not all that familiar with BB code base, but how about lines: 618,668 in
> libbb/dump.c?
I don't know for sure, but I suspect F_TEXT format unit is meant to contain
just text. If it does not, then there is a bug in dump.c somewhere.
--
vda
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
