On 08/14/2013 04:51 PM, Simo Sorce wrote:
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.
Ok. I split the second patch into two. So now the second patch is about the triple check and the third one about the off-by-one error. The first patch is unchanged.
Thanks for the review
Michal