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:
> > > > From: Dan Cashman <dcashman@xxxxxxxxxx>
> > > >
> > > > The file_contexts labeling backend, specified in label_file.c,
> > > > currently assumes
> > > > that only one path will be specified as an option to
> > > > selabel_open(). The split
> > > > of platform and non-platform policy on device, however, will
> > > > necessitate the
> > > > loading of two disparate policy files. Rather than combining
> > > > the
> > > > files and then
> > > > calling the existing API on a newly-formed file, just add the
> > > > ability
> > > > to specify
> > > > multiple files to use. Order of opt specification to
> > > > selabel_open
> > > > matters.
> > > >
> > > > This corresponds to AOSP commit
> > > > 50400d38203e4db08314168e60c281cc61a717a8, which
> > > > lead to a fork with upstream, which we'd like to correct.
> > > >
> > > > Signed-off-by: Dan Cashman <dcashman@xxxxxxxxxxx>
> > > > ---
> > > > libselinux/src/label.c | 21 +++++---
> > > > libselinux/src/label_db.c | 13 ++++-
> > > > libselinux/src/label_file.c | 104
> > > > +++++++++++++++++++++++++++++-
> > > > ----------
> > > > libselinux/src/label_internal.h | 5 +-
> > > > libselinux/src/label_media.c | 10 +++-
> > > > libselinux/src/label_x.c | 10 +++-
> > > > 6 files changed, 124 insertions(+), 39 deletions(-)
> > > >
> > > > diff --git a/libselinux/src/label.c b/libselinux/src/label.c
> > > > index 48f4d2d6..0dfa054c 100644
> > > > --- a/libselinux/src/label.c
> > > > +++ b/libselinux/src/label.c
> > > > @@ -143,7 +143,11 @@ static int selabel_fini(struct
> > > > selabel_handle
> > > > *rec,
> > > > struct selabel_lookup_rec *lr,
> > > > int translating)
> > > > {
> > > > - if (compat_validate(rec, lr, rec->spec_file, 0))
> > > > + char *path = NULL;
> > > > +
> > > > + if (rec->spec_files)
> > > > + path = rec->spec_files[0];
> > > > + if (compat_validate(rec, lr, path, 0))
> > > > return -1;
> > > >
> > > > if (translating && !lr->ctx_trans &&
> > > > @@ -226,11 +230,9 @@ struct selabel_handle
> > > > *selabel_open(unsigned
> > > > int
> > > > backend,
> > > > rec->digest = selabel_is_digest_set(opts, nopts, rec-
> > > > > digest);
> > > >
> > > > if ((*initfuncs[backend])(rec, opts, nopts)) {
> > > > - free(rec->spec_file);
> > > > - free(rec);
> > > > + selabel_close(rec);
> > > > rec = NULL;
> > > > }
> > > > -
> > > > out:
> > > > return rec;
> > > > }
> > > > @@ -337,10 +339,17 @@ int selabel_digest(struct selabel_handle
> > > > *rec,
> > > >
> > > > void selabel_close(struct selabel_handle *rec)
> > > > {
> > > > + size_t i;
> > > > +
> > > > + if (rec->spec_files) {
> > > > + for (i = 0; i < rec->spec_files_len; i++)
> > > > + free(rec->spec_files[i]);
> > > > + free(rec->spec_files);
> > > > + }
> > > > if (rec->digest)
> > > > selabel_digest_fini(rec->digest);
> > > > - rec->func_close(rec);
> > > > - free(rec->spec_file);
> > > > + if (rec->func_close)
> > > > + rec->func_close(rec);
> > > > free(rec);
> > > > }
> > > >
> > > > diff --git a/libselinux/src/label_db.c
> > > > b/libselinux/src/label_db.c
> > > > index c46d0a1d..9a35abea 100644
> > > > --- a/libselinux/src/label_db.c
> > > > +++ b/libselinux/src/label_db.c
> > > > @@ -290,7 +290,18 @@ db_init(const struct selinux_opt *opts,
> > > > unsigned
> > > > nopts,
> > > > errno = EINVAL;
> > > > return NULL;
> > > > }
> > > > - rec->spec_file = strdup(path);
> > > > + rec->spec_files_len = 1;
> > > > + rec->spec_files = calloc(rec->spec_files_len, sizeof(rec-
> > > > > spec_files[0]));
> > > >
> > > > + if (!rec->spec_files) {
> > > > + free(catalog);
> > > > + return NULL;
> > > > + }
> > > > + rec->spec_files[0] = strdup(path);
> > > > + if (!rec->spec_files[0]) {
> > > > + free(catalog);
> > > > + free(rec->spec_files);
> > > > + return NULL;
> > > > + }
> > > >
> > > > /*
> > > > * Parse for each lines
> > > > 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.
>
> >
> >
> > > > + if (*path == NULL)
> > > > + goto finish;
> > > > + path++;
> > > > + break;
> > > > + default:
> > > > + break;
> > > > + }
> > > > + }
> > > > + }
> > > > #if !defined(BUILD_HOST) && !defined(ANDROID)
> > > > char subs_file[PATH_MAX + 1];
> > > > /* Process local and distribution substitution files */
> > > > - if (!path) {
> > > > + if (!path_provided) {
> > > > status = selabel_subs_init(
> > > > selinux_file_context_subs_dist_path(),
> > > > rec->digest, &data->dist_subs);
> > > > @@ -740,43 +773,52 @@ static int init(struct selabel_handle
> > > > *rec,
> > > > const struct selinux_opt *opts,
> > > > rec->digest, &data->subs);
> > > > if (status)
> > > > goto finish;
> > > > - path = selinux_file_context_path();
> > > > + rec->spec_files[0] =
> > > > strdup(selinux_file_context_path());
> > > > + if (rec->spec_files[0] == NULL)
> > > > + goto finish;
> > > > } else {
> > > > - snprintf(subs_file, sizeof(subs_file),
> > > > "%s.subs_dist", path);
> > > > - status = selabel_subs_init(subs_file, rec-
> > > > >digest,
> > > > + for (i = 0; i < num_paths; i++) {
> > > > + snprintf(subs_file, sizeof(subs_file),
> > > > "%s.subs_dist", rec->spec_files[i]);
> > > > + status = selabel_subs_init(subs_file,
> > > > rec-
> > > > > digest,
> > > >
> > > > &data->dist_subs);
> > > > - if (status)
> > > > - goto finish;
> > > > - snprintf(subs_file, sizeof(subs_file), "%s.subs",
> > > > path);
> > > > - status = selabel_subs_init(subs_file, rec-
> > > > >digest,
> > > > + if (status)
> > > > + goto finish;
> > > > + snprintf(subs_file, sizeof(subs_file),
> > > > "%s.subs", rec->spec_files[i]);
> > > > + status = selabel_subs_init(subs_file,
> > > > rec-
> > > > > digest,
> > > >
> > > > &data->subs);
> > > > - if (status)
> > > > - goto finish;
> > > > + if (status)
> > > > + goto finish;
> > > > + }
> > > > + }
> > > > +#else
> > > > + if (!path_provided) {
> > > > + selinux_log(SELINUX_ERROR, "No path given to file
> > > > labeling backend\n");
> > > > + goto finish;
> > > > }
> > > > -
> > > > #endif
> > > > - rec->spec_file = strdup(path);
> > > >
> > > > /*
> > > > - * The do detailed validation of the input and fill the
> > > > spec
> > > > array
> > > > + * Do detailed validation of the input and fill the spec
> > > > array
> > > > */
> > > > - status = process_file(path, NULL, rec, prefix, rec-
> > > > > digest);
> > > >
> > > > - if (status)
> > > > - goto finish;
> > > > -
> > > > - if (rec->validating) {
> > > > - status = nodups_specs(data, path);
> > > > + for (i = 0; i < num_paths; i++) {
> > > > + status = process_file(rec->spec_files[i], NULL,
> > > > rec,
> > > > prefix, rec->digest);
> > > > if (status)
> > > > goto finish;
> > > > +
> > > > + if (rec->validating) {
> > > > + status = nodups_specs(data, rec-
> > > > > spec_files[i]);
> > > >
> > > > + if (status)
> > > > + goto finish;
> > > > + }
> > > > }
> > > >
> > > > if (!baseonly) {
> > > > - status = process_file(path, "homedirs", rec,
> > > > prefix,
> > > > + status = process_file(rec->spec_files[0],
> > > > "homedirs", rec, prefix,
> > > > rec-
> > > > > digest);
> > > >
> > > > if (status && errno != ENOENT)
> > > > goto finish;
> > > >
> > > > - status = process_file(path, "local", rec, prefix,
> > > > + status = process_file(rec->spec_files[0],
> > > > "local",
> > > > rec, prefix,
> > > > rec-
> > > > > digest);
> > > >
> > > > if (status && errno != ENOENT)
> > > > goto finish;
> > > > @@ -804,6 +846,12 @@ static void closef(struct selabel_handle
> > > > *rec)
> > > > struct stem *stem;
> > > > unsigned int i;
> > > >
> > > > + if (!data)
> > > > + return;
> > > > +
> > > > + /* make sure successive ->func_close() calls are harmless
> > > > */
> > > > + rec->data = NULL;
> > > > +
> > > > selabel_subs_fini(data->subs);
> > > > selabel_subs_fini(data->dist_subs);
> > > >
> > > > diff --git a/libselinux/src/label_internal.h
> > > > b/libselinux/src/label_internal.h
> > > > index c55efb75..43b63513 100644
> > > > --- a/libselinux/src/label_internal.h
> > > > +++ b/libselinux/src/label_internal.h
> > > > @@ -98,10 +98,11 @@ struct selabel_handle {
> > > > void *data;
> > > >
> > > > /*
> > > > - * The main spec file used. Note for file contexts the
> > > > local
> > > > and/or
> > > > + * The main spec file(s) used. Note for file contexts the
> > > > local and/or
> > > > * homedirs could also have been used to resolve a
> > > > context.
> > > > */
> > > > - char *spec_file;
> > > > + size_t spec_files_len;
> > > > + char **spec_files;
> > > >
> > > > /* ptr to SHA1 hash information if SELABEL_OPT_DIGEST set
> > > > */
> > > > struct selabel_digest *digest;
> > > > diff --git a/libselinux/src/label_media.c
> > > > b/libselinux/src/label_media.c
> > > > index d202e5d5..34260cbb 100644
> > > > --- a/libselinux/src/label_media.c
> > > > +++ b/libselinux/src/label_media.c
> > > > @@ -100,7 +100,15 @@ static int init(struct selabel_handle
> > > > *rec,
> > > > const struct selinux_opt *opts,
> > > > errno = EINVAL;
> > > > return -1;
> > > > }
> > > > - rec->spec_file = strdup(path);
> > > > + rec->spec_files_len = 1;
> > > > + rec->spec_files = calloc(rec->spec_files_len, sizeof(rec-
> > > > > spec_files[0]));
> > > >
> > > > + if (!rec->spec_files)
> > > > + return -1;
> > > > + rec->spec_files[0] = strdup(path);
> > > > + if (!rec->spec_files[0]) {
> > > > + free(rec->spec_files);
> > > > + return -1;
> > > > + }
> > > >
> > > > /*
> > > > * Perform two passes over the specification file.
> > > > diff --git a/libselinux/src/label_x.c
> > > > b/libselinux/src/label_x.c
> > > > index 96745299..fafe034a 100644
> > > > --- a/libselinux/src/label_x.c
> > > > +++ b/libselinux/src/label_x.c
> > > > @@ -127,7 +127,15 @@ static int init(struct selabel_handle
> > > > *rec,
> > > > const struct selinux_opt *opts,
> > > > errno = EINVAL;
> > > > return -1;
> > > > }
> > > > - rec->spec_file = strdup(path);
> > > > + rec->spec_files_len = 1;
> > > > + rec->spec_files = calloc(rec->spec_files_len, sizeof(rec-
> > > > > spec_files[0]));
> > > >
> > > > + if (!rec->spec_files)
> > > > + return -1;
> > > > + rec->spec_files[0] = strdup(path);
> > > > + if (!rec->spec_files[0]) {
> > > > + free(rec->spec_files);
> > > > + return -1;
> > > > + }
> > > >
> > > > /*
> > > > * Perform two passes over the specification file.
