On Fri, May 12, 2017 at 1:26 PM, Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote:
Hi,
Currently libselinux/src/label.c defines selabel_subs_init() like this [1]:
struct selabel_sub *selabel_subs_init(/* ... */)
{
/* ... */
while (fgets_unlocked(buf, sizeof(buf) - 1, cfg)) {
/* ... allocate and fill "sub" ... */
sub->next = list;
list = sub;
}
if (digest_add_specfile(digest, cfg, NULL, sb.st_size, path) < 0)
goto err;
out:
fclose(cfg);
return list;
err:
if (sub)
free(sub->src);
free(sub);
goto out;
}
When digest_add_specfile() fails, sub is freed (in the err label), but
as list=sub, it means that the return value, list, is freed. This
leads to a use-after-free when this value is used.
What should selabel_subs_init() do (and return) when
digest_add_specfile() fails?
Nicolas
[1] https://github.com/SELinuxProject/selinux/blob/ 9cc62ce35d099acf7897b625922847 9737521709/libselinux/src/ label.c#L94
From what I can tell, and I may be wrong, is that the only callers are from init() in label_file.c. So the first two failure returns where it returns list,
list i think would be NULL there. Which makes sense since the first iteration, the linked list next pointer would be NULL. I think we would want to free
the whole list and return NULL and have callers check for NULL. This seems like a fatal error path. But I am wrong frequently :-)
