[PATCH v1] Add bounds checking in sys_query_module

[Zubin Mithra]

Hi Mike,

Thank you for the review!


On Wed, Aug 6, 2014 at 7:14 PM, Mike Frysinger <vapier at gentoo.org> wrote:
> On Mon 04 Aug 2014 08:31:30 zubin.mithra at gmail.com wrote:
>> --- a/bjm.c
>> +++ b/bjm.c
>> @@ -121,6 +121,8 @@ sys_query_module(struct tcb *tcp)
>>                                                               (idx ? ", " : ""),
>>                                                               mod);
>>                                                       mod += strlen(mod)+1;
>> +                                                     if (mod-data >= tcp->u_arg[3])
>> +                                                             break;
>
> this check is incomplete.  if the buffer is not NUL terminated, then the
> string reading code will read beyond the end.  you can protect against that by
> doing:
>         char *data = malloc(tcp->u_arg[3] + 1);
> then before the for loop here, make sure the buffer is NUL terminated:
>         data[tcp->u_arg[3]] = '\0';
>
> as for this check, i think it's weirdly written.  it makes more sense to me:
>         if (mod >= data + tcp->u_arg[3])
>
> also, when you do break, don't you want to print out a ... to indicate ?

I've made these changes.

>
>> @@ -144,6 +146,8 @@ sys_query_module(struct tcb *tcp)
>>                                               tprintf(" /* %lu entries */ ", (unsigned long)ret);
>>                                       } else {
>>                                               for (idx = 0; idx < ret; idx++) {
>> +                                                     if ((long)sym->name >= tcp->u_arg[3])
>> +                                                             break;
>>                                                       tprintf("%s{name=%s, value=%lu}",
>>                                                               (idx ? " " : ""),
>>                                                               data+(long)sym->name,
>
> i think this too is incomplete.  the sym++ might walk past the end of the
> data, sym->name is unsigned while u_arg is signed, and the symbol name might
> start near the end of the valid region but then walk beyond it (no NUL
> termination).  so i guess what are you trying to protect against ?

The idea was to make sure that a dereference of "data+(long)sym->name"
inside tprintf(upon processing "%s") should not lead to an access of
an invalid address(i.e. the address at data+(long)sym->name needs to
be valid). However, the check I had added in the patch fails to do
that. Would there be a way to do that check elegantly?

I'll add a bounds check to sym in the loop.


Thanks,
-- zm