On (13/08/13 19:45), Michal Židek wrote:
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.
Nice catch.
I also removed the triple check at Lukas's request in the second
patch, since it modifies the same parts already).
Thank you
New patches are attached.
Thanks
Michal
+ strs_offset = offsetof(struct sss_mc_grp_data, strs);
data = (struct sss_mc_grp_data *)rec->data;
+ /* Integrity check
+ * - name_len cannot be longer than all strings
+ * - data->name cannot point outside strings
+ * - all strings must be within data_table */
+ if (name_len > data->strs_len
+ || data->name > strs_offset + data->strs_len
+ || (uint8_t *)data->strs + data->strs_len > max_addr) {
+ return ENOENT;
replace return ENOENT with "ret = ENOENT; goto
done", because it
introduce coverity warning:
sssd-1.10.93/src/sss_client/nss_mc_passwd.c:190: leaked_storage: Variable "rec"
going out of scope leaks the storage it points to.
And the same in src/sss_client/nss_mc_group.c
+ }
+
rec_name = (char *)data + data->name;
if (strcmp(name, rec_name) == 0) {
break;
LS