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
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
