On Fri, 2017-05-12 at 15:02 -0700, William Roberts wrote: > > > 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/9cc62ce35d099acf > > 7897b6259228479737521709/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 :-) Looks like the logic for appending to an existing list provided by the caller is a legacy of the original code, obsoleted by fd56c5230cea6b81fbe74d1d0a228936a6797923 when dist_subs was split out. In which case we don't need to pass in list at all; it will always be NULL as you say. However, we should distinguish error cases; it should not be a fatal error if the path doesn't exist (the subs configurations are optional), but memory allocation or other failures likely should be fatal.