Thanks for the review Simo.
On 08/12/2013 11:07 PM, Simo Sorce wrote:
On Mon, 2013-08-12 at 19:56 +0200, Michal Židek wrote:
> This should have been part of the patch:
> [PATCH] mmap_cache: Check if slot and name_ptr are not invalid.
>
> I missed the fact that name_ptr is called just name in the
> client code and did not add check there.
>
> Using manually corrupted cache (setting name_ptr to some high value
> in hexeditor) this caused segfault in the client code. The attached
> patch fixes this.
>
> Please review and push this patch to master, 1-9 and 1-10.
I do not think this patch is correct.
Although normally data->name will point to data->strs it is a separate
pointer exactly becsaue the server code may decide to put the name
string somewhere in the strs buffer but not necessarily at the start.
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).
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.
Simo.
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.
I also removed the triple check at Lukas's request in the second
patch, since it modifies the same parts already).
New patches are attached.
Thanks
Michal