On Thu, Oct 19, 2017 at 9:46 PM, Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
> On Thu, 2017-10-19 at 14:27 -0400, Stephen Smalley wrote:
>> On Thu, 2017-10-19 at 09:25 -0700, William Roberts wrote:
>> > On Thu, Oct 19, 2017 at 7:26 AM, Stephen Smalley <sds@xxxxxxxxxxxxx
>> > >
>> > wrote:
>> > > On Tue, 2017-10-17 at 09:33 -0700, Daniel Cashman wrote:
>> > > > [...]
>> > > > diff --git a/libselinux/src/label_file.c
>> > > > b/libselinux/src/label_file.c
>> > > > index 560d8c3d..b3b36bc2 100644
>> > > > --- a/libselinux/src/label_file.c
>> > > > +++ b/libselinux/src/label_file.c
>> > > > @@ -709,28 +709,61 @@ static int init(struct selabel_handle
>> > > > *rec,
>> > > > const struct selinux_opt *opts,
>> > > > unsigned n)
>> > > > {
>> > > > struct saved_data *data = (struct saved_data *)rec->data;
>> > > > - const char *path = NULL;
>> > > > + size_t num_paths = 0;
>> > > > + char **path = NULL;
>> > > > const char *prefix = NULL;
>> > > > - int status = -1, baseonly = 0;
>> > > > + int status = -1;
>> > > > + size_t i;
>> > > > + bool baseonly = false;
>> > > > + bool path_provided;
>> > > >
>> > > > /* Process arguments */
>> > > > - while (n--)
>> > > > - switch(opts[n].type) {
>> > > > + i = n;
>> > > > + while (i--)
>> > > > + switch(opts[i].type) {
>> > > > case SELABEL_OPT_PATH:
>> > > > - path = opts[n].value;
>> > > > + num_paths++;
>> > > > break;
>> > > > case SELABEL_OPT_SUBSET:
>> > > > - prefix = opts[n].value;
>> > > > + prefix = opts[i].value;
>> > > > break;
>> > > > case SELABEL_OPT_BASEONLY:
>> > > > - baseonly = !!opts[n].value;
>> > > > + baseonly = !!opts[i].value;
>> > > > break;
>> > > > }
>> > > >
>> > > > + if (!num_paths) {
>> > > > + num_paths = 1;
>> > > > + path_provided = false;
>> > > > + } else {
>> > > > + path_provided = true;
>> > > > + }
>> > > > +
>> > > > + path = calloc(num_paths, sizeof(*path));
>> > > > + if (path == NULL) {
>> > > > + goto finish;
>> > > > + }
>> > > > + rec->spec_files = path;
>> > > > + rec->spec_files_len = num_paths;
>> > > > +
>> > > > + if (path_provided) {
>> > > > + for (i = 0; i < n; i++) {
>> > > > + switch(opts[i].type) {
>> > > > + case SELABEL_OPT_PATH:
>> > > > + *path = strdup(opts[i].value);
>> > >
>> > > Perhaps surprisingly, opts[i].value can be NULL here and some
>> > > callers
>> > > rely on that. After applying your patch, coreutils,
>> > > selabel_lookup,
>> > > and other userspace programs all seg fault as a result. The use
>> > > case
>> > > is programs where the selinux_opt structure is declared with a
>> > > SELABEL_OPT_PATH entry whose value is subsequently filled in, and
>> > > left
>> > > NULL if they want to use the default path for file_contexts.
>> > > Internally, libselinux also does this from the
>> > > matchpathcon_init_prefix() function.
>> > >
>> > > In any event, you need to handle this case.
>> > >
>> >
>> > Poor Stephen has turned into your test bot :-)
>> >
>> > Does any of the test suite exercise this? Maybe make a PR
>> > on github first to get Travis to test these?
>>
>> Unfortunately the travis-ci tests don't catch this one currently.
>>
>> > Stephen can you share with how your testing this so Dan can follow
>> > suit?
>>
>> In this case, you could build and install to a private directory as
>> per
>> the README, then set your LD_LIBRARY_PATH=~/obj/lib or whatever and
>> run
>> selabel_lookup.
>
> Ala:
> $ make DESTDIR=~/obj install
> $ LD_LIBRARY_PATH=~/obj/lib selabel_lookup -b file -k /etc
> Segmentation fault (core dumped)
>
> BTW, I guess this exposes another issue with the patch; there is no
> support for exercising the new code included with it, e.g. an extension
> to utils/selabel_lookup.c to permit specifying multiple input files via
> multiple -f options. Otherwise, we have no upstream way of testing it.
For selabel_lookup, it would be possible to write a test script (or a
CUnit-based test program like libsepol and libsemanage have) which
does not need an SELinux policy to be installed on the system, thanks
to option -f.
Nevertheless other parts of libselinux (and programs) can not
currently be tested on a system without SELinux, as /etc/selinux and
/sys/fs/selinux are needed. If we want to automatize the testing of
such parts in a CI like Travis-CI, I suppose we would need to run a
virtual machine with its own filesystem, a kernel with SELinux
enabled, and some programs like coreutils compiled with the tested
version of libselinux/libsepol/... As implementing such an idea may
take quite a long time, would there be simpler ways to perform
automatic testing of libselinux? Perhaps using another continuous
integration system?
Cheers,
Nicolas
