On Tue, 2013-08-13 at 19:42 +0200, Michal Židek wrote:
Thanks for the review Simo.
On 08/12/2013 11:07 PM, Simo Sorce wrote:
> What you need to check is somehing like:
> if (data->name > offsetof(struct sss_mc_pwd_data, strs) +
> data->strs_len) { return ENOENT; }
>
> ... except you should probably not trust strs_len entirely at this point
> if you are trying to catch malformed data and you should also check that
> data + strs_len is within the mmaped memory region.
>
Ok. The new check tests if data + strs_len is in the data_table
(if it is somewhere else in the mmaped region it is already corrupted).
Sure.
> Also at this point it may make sense to do a strlen(name)
upfront and
> check that strs_len > name and return immediately if not.
>
I added this one check too... I think it is not bad to have another
line of defense.
Thanks.
Btw. I think we have off-by-one error in cases where we use pattern:
if (slot > MC_SIZE_TO_SLOTS(data_table_size) {
return something (ENOENT/NULL);
}
If the slots are numbered from 0 and MC_SIZE_TO_SLOTS returns
number of slots needed to store some amount of data, there should
be '>=' no '>'. Please check my thinking. If I am correct then the
second patch should fix it.
Let's look at MC_SIZE_TO_SLOTS() definition:
We always add (MC_SLOT_SIZE -1) to the requested size.
This means if you ask 1 byte you get 1 slot, if you ask for MC_SLOT_SIZE
+ 1 you get 2 slots.
Ie you get the right number of slots required for the size when you call
that macro.
So yeah good catch!
However I think I'd like to see this fixed in a different way, by using
a macro as we use this check elsewhere.
Something like:
#define MC_SLOT_WITHIN_BOUNDS(slot, size) \
(slot <= (size / MC_SLOT_SIZE))
and change the check to:
if (!MC_SLOT_WITHIN_BOUNDS(slot, mcc->dt_size)) { ...
This would be more error proof if someone should add code in future to
check bounds.
I also removed the triple check at Lukas's request in the second
patch, since it modifies the same parts already).
I would prefer this to be done in a separate patch please.
Simo.
--
Simo Sorce * Red Hat, Inc * New York