Re: [RFC PATCH v2 2/9] libselinux/utils: introduce selabel_compare

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Mar 11, 2024 at 1:21 PM Christian Göttsche
<cgzones@xxxxxxxxxxxxxx> wrote:
>
> On Thu, 7 Mar 2024 at 20:50, James Carter <jwcart2@xxxxxxxxx> wrote:
> >
> > On Wed, Jan 31, 2024 at 8:42 AM Christian Göttsche
> > <cgzones@xxxxxxxxxxxxxx> wrote:
> > >
> > > Add a utility around selabel_cmp(3).
> > >
> > > Can be used by users to compare a pre-compiled fcontext file to an
> > > original text-based file context definition file.
> > >
> > > Can be used for development to verify compilation and parsing of the
> > > pre-compiled fcontext format works correctly.
> > >
> > > Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
> >
>
> First of all many thanks for testing!
>
> > I don't have any comments on the code.
> >
> > It took me a while to get this to work. It was very confusing. Not
> > because of your code, but because of the behavior of selabel_cmp(). It
> > is not intuitive and I think that it needs to be better documented. It
> > will absolutely not work as expected on a copy of the contexts
> > directory.
> >
> >
> > With all that, not everything worked as I thought it would.
> >
> > current = installed contexts directory
> > the other file is by itself (and not in a copy of the contexts directory)
> >
> > 1) current file_contexts.bin vs file_contexts [Expected]
> > selabel_compare /etc/selinux/targeted/contexts/files/file_contexts.bin
> > file_contexts
> > spec /etc/selinux/targeted/contexts/files/file_contexts.bin is equal
> > to spec file_contexts
> >
> > 2) current file_contexts vs file_contexts [Not expected, diff says
> > they are equal]
> > selabel_compare /etc/selinux/targeted/contexts/files/file_contexts file_contexts
> > selabel_cmp: mismatch regex_str left remnant in stem tmp
> > selabel_cmp: mismatch child node tmp stem (root)
> > spec /etc/selinux/targeted/contexts/files/file_contexts is
> > uncomparable to spec file_contexts
>
> Since the database is opened via selabel_open(3)
> filecontexts.(homedirs|local) are implicitly loaded too.
> It would be confusing if the selabel_compare helper would deviate from
> that default behavior.
> Also currently there is no information in the in-memory database from
> which/how-many files the database was loaded.
> Maybe that should be added so the selabel_compare helper could give a hint?
>

I am not very familiar with the selabel code, but I could see what it was doing.
It is just very confusing behavior. The behavior is different
depending on where the files being compared are located. If neither is
in something that looks like /etc/selinux/targeted/contexts/files/,
then it works as expected, otherwise it works differently. I think the
expected behavior needs to be explained better.

> > 3) current file_contexts.bin vs file_contexts_1 [Expected]
> > selabel_compare /etc/selinux/targeted/contexts/files/file_contexts.bin
> > file_contexts_1
> > spec /etc/selinux/targeted/contexts/files/file_contexts.bin is equal
> > to spec file_contexts_1
> >
> > 4) file_contexts vs file_contexts_1 [Expected]
> > selabel_compare file_contexts file_contexts_1
> > spec file_contexts is equal to spec file_contexts_1
> >
> >
> > I realize now that selabel_cmp() can only find a superset or a subset
> > if the only difference occurs at the very end. The longer file must be
> > exactly like the shorter file except for the additional lines at the
> > end.
>
> Thanks, this is indeed a shortcoming in v2 (regex prefix length were
> not considered). Fixed in wip-v3:
> https://github.com/SELinuxProject/selinux/compare/f07e4c3b66416f6337d6a2ea1e0d69555f8704ad..090af7e6510edb90f4e7df55346e5023ac190b66
>
> > I tried testing with the zebra module disabled, so it should have been
> > a subset of the current policy, but, of course, selabel_cmp() was not
> > sophisticated enough to say that. It just says that everything is
> > uncomparable. It would be nice if something said that.
> >
> >
> > The following worked only in the most trivial case where I could just
> > use diff instead.
> > contexts1 = copy of original contexts directory
> > contexts2 = copy of new contexts directory with new selabel db
> >
> > 1) current file_contexts vs contexts1 file_contexts [Expected, but Confusing]
> > selabel_compare /etc/selinux/targeted/contexts/files/file_contexts
> > contexts1/files/file_contexts
> > contexts1/files/file_contexts.bin:  Old compiled fcontext format, skipping
> > contexts1/files/file_contexts.homedirs.bin:  Old compiled fcontext
> > format, skipping
> > spec /etc/selinux/targeted/contexts/files/file_contexts is equal to
> > spec contexts1/files/file_contexts
>
> What is confusing here?
>

What was confusing was the fact that it is referring to files that I
did not ask it to compare. I understand that is the behavior of the
library, but that is not what I would have expected from running the
command.

> > 2) current file_contexts.bin vs contexts1 file_contexts [Not expected.
> > Expected to be able to compare the current file_contexts.bin with old
> > text file_contexts]
> > selabel_compare /etc/selinux/targeted/contexts/files/file_contexts.bin
> > contexts1/files/file_contexts
> > contexts1/files/file_contexts.bin:  Old compiled fcontext format, skipping
> > contexts1/files/file_contexts.homedirs.bin:  Old compiled fcontext
> > format, skipping
> > selabel_cmp: mismatch regex_str right remnant in stem tmp
> > selabel_cmp: mismatch child node tmp stem (root)
> > spec /etc/selinux/targeted/contexts/files/file_contexts.bin is
> > uncomparable to spec contexts1/files/file_contexts
>
> Gives subset with v3 (since when using a .bin extension .homedirs and
> .local are not loaded).
>
> > 3) current file_contexts.bin vs contexts1 file_contexts.bin [Expected,
> > but confusing]
> > selabel_compare /etc/selinux/targeted/contexts/files/file_contexts.bin
> > contexts1/files/file_contexts.bin
> > contexts1/files/file_contexts.bin:  Old compiled fcontext format, skipping
> > contexts1/files/file_contexts.bin:  Old compiled fcontext format, skipping
> > ERROR: selabel_open - Could not obtain handle for
> > contexts1/files/file_contexts.bin:  No such file or directory
>
> Updated errno to EINVAL for this case.
>
> > 4) current file_contexts vs contexts2 file_contexts [Expected]
> > selabel_compare /etc/selinux/targeted/contexts/files/file_contexts
> > contexts2/files/file_contexts
> > spec /etc/selinux/targeted/contexts/files/file_contexts is equal to
> > spec contexts2/files/file_contexts
> >
> > 5) current file_contexts.bin vs contexts2 file_contexts [Not expected.
> > Expected to be able to compare the current file_contexts.bin with
> > contexts2 text file_contexts]
> > selabel_compare /etc/selinux/targeted/contexts/files/file_contexts.bin
> > contexts2/files/file_contexts
> > selabel_cmp: mismatch regex_str right remnant in stem tmp
> > selabel_cmp: mismatch child node tmp stem (root)
> > spec /etc/selinux/targeted/contexts/files/file_contexts.bin is
> > uncomparable to spec contexts2/files/file_contexts
>
> Should give subset (due to the missing .homedirs|.local) now.
>
> > 6) current file_contexts.bin vs contexts2 file_contexts.bin [Expected]
> > selabel_compare /etc/selinux/targeted/contexts/files/file_contexts.bin
> > contexts2/files/file_contexts.bin
> > spec /etc/selinux/targeted/contexts/files/file_contexts.bin is equal
> > to spec contexts2/files/file_contexts.bin
> >
> > 7) contexts1 file_contexts vs contexts2 file_contexts [Expected, but confusing]
> > selabel_compare contexts1/files/file_contexts  contexts2/files/file_contexts
> > contexts1/files/file_contexts.bin:  Old compiled fcontext format, skipping
> > contexts1/files/file_contexts.homedirs.bin:  Old compiled fcontext
> > format, skipping
> > spec contexts1/files/file_contexts is equal to spec
> > contexts2/files/file_contexts
>
> What is confusing here? selabel_open(3) tries to open binary fcontext
> files first for performance.
>

I understand what it is doing, but I think that this would be
confusing to anyone using the tool.

Thanks,
Jim


> > 8) contexts1 file_contexts vs contexts2 file_contexts.cpy (which is a
> > copy of file_contexts) [Not expected. Expected them to be equal]
> > selabel_compare contexts1/files/file_contexts contexts2/files/file_contexts.cpy
> > contexts1/files/file_contexts.bin:  Old compiled fcontext format, skipping
> > contexts1/files/file_contexts.homedirs.bin:  Old compiled fcontext
> > format, skipping
> > selabel_cmp: mismatch regex_str left remnant in stem tmp
> > selabel_cmp: mismatch child node tmp stem (root)
> > spec contexts1/files/file_contexts is uncomparable to spec
> > contexts2/files/file_contexts.cpy
> >
> > With all of the issues, I wonder how useful this actually would be. It
> > might cause more confusion then anything.
> >
> > Thanks,
> > Jim
> >
> > > ---
> > > v2:
> > >    split nested block into own function
> > > ---
> > >  libselinux/utils/.gitignore        |   1 +
> > >  libselinux/utils/selabel_compare.c | 122 +++++++++++++++++++++++++++++
> > >  2 files changed, 123 insertions(+)
> > >  create mode 100644 libselinux/utils/selabel_compare.c
> > >
> > > diff --git a/libselinux/utils/.gitignore b/libselinux/utils/.gitignore
> > > index b3311360..2e10b14f 100644
> > > --- a/libselinux/utils/.gitignore
> > > +++ b/libselinux/utils/.gitignore
> > > @@ -16,6 +16,7 @@ getseuser
> > >  matchpathcon
> > >  policyvers
> > >  sefcontext_compile
> > > +selabel_compare
> > >  selabel_digest
> > >  selabel_get_digests_all_partial_matches
> > >  selabel_lookup
> > > diff --git a/libselinux/utils/selabel_compare.c b/libselinux/utils/selabel_compare.c
> > > new file mode 100644
> > > index 00000000..9ca6eff1
> > > --- /dev/null
> > > +++ b/libselinux/utils/selabel_compare.c
> > > @@ -0,0 +1,122 @@
> > > +#include <getopt.h>
> > > +#include <stdio.h>
> > > +#include <stdlib.h>
> > > +#include <string.h>
> > > +
> > > +#include <selinux/label.h>
> > > +
> > > +
> > > +static void usage(const char *progname)
> > > +{
> > > +       fprintf(stderr,
> > > +               "usage: %s [-b backend] [-v] file1 file2\n\n"
> > > +               "Where:\n\t"
> > > +               "-b           The backend - \"file\", \"media\", \"x\", \"db\" or \"prop\" (defaults to \"file\")\n\t"
> > > +               "-v           Validate entries against loaded policy.\n\t"
> > > +               "file1/file2  Files containing the specs.\n",
> > > +               progname);
> > > +}
> > > +
> > > +static int compare(const char *file1, const char *file2, const char *validate, unsigned int backend)
> > > +{
> > > +       struct selabel_handle *hnd1, *hnd2;
> > > +       const struct selinux_opt selabel_option1[] = {
> > > +               { SELABEL_OPT_PATH, file1 },
> > > +               { SELABEL_OPT_VALIDATE, validate }
> > > +       };
> > > +       const struct selinux_opt selabel_option2[] = {
> > > +               { SELABEL_OPT_PATH, file2 },
> > > +               { SELABEL_OPT_VALIDATE, validate }
> > > +       };
> > > +       enum selabel_cmp_result result;
> > > +
> > > +       hnd1 = selabel_open(backend, selabel_option1, 2);
> > > +       if (!hnd1) {
> > > +               fprintf(stderr, "ERROR: selabel_open - Could not obtain handle for %s:  %m\n", file1);
> > > +               return EXIT_FAILURE;
> > > +       }
> > > +
> > > +       hnd2 = selabel_open(backend, selabel_option2, 2);
> > > +       if (!hnd2) {
> > > +               fprintf(stderr, "ERROR: selabel_open - Could not obtain handle for %s:  %m\n", file2);
> > > +               selabel_close(hnd1);
> > > +               return EXIT_FAILURE;
> > > +       }
> > > +
> > > +       result = selabel_cmp(hnd1, hnd2);
> > > +
> > > +       selabel_close(hnd2);
> > > +       selabel_close(hnd1);
> > > +
> > > +       switch (result) {
> > > +       case SELABEL_SUBSET:
> > > +               printf("spec %s is a subset of spec %s\n", file1, file2);
> > > +               break;
> > > +       case SELABEL_EQUAL:
> > > +               printf("spec %s is equal to spec %s\n", file1, file2);
> > > +               break;
> > > +       case SELABEL_SUPERSET:
> > > +               printf("spec %s is a superset of spec %s\n", file1, file2);
> > > +               break;
> > > +       case SELABEL_INCOMPARABLE:
> > > +               printf("spec %s is uncomparable to spec %s\n", file1, file2);
> > > +               break;
> > > +       default:
> > > +               fprintf(stderr, "ERROR: selabel_cmp - Unexpected result %d\n", result);
> > > +               return EXIT_FAILURE;
> > > +       }
> > > +
> > > +       return EXIT_SUCCESS;
> > > +}
> > > +
> > > +int main(int argc, char *argv[])
> > > +{
> > > +       unsigned int backend = SELABEL_CTX_FILE;
> > > +       int opt;
> > > +       const char *validate = NULL, *file1 = NULL, *file2 = NULL;
> > > +
> > > +       if (argc < 3) {
> > > +               usage(argv[0]);
> > > +               return EXIT_FAILURE;
> > > +       }
> > > +
> > > +       while ((opt = getopt(argc, argv, "b:v")) > 0) {
> > > +               switch (opt) {
> > > +               case 'b':
> > > +                       if (!strcasecmp(optarg, "file")) {
> > > +                               backend = SELABEL_CTX_FILE;
> > > +                       } else if (!strcmp(optarg, "media")) {
> > > +                               backend = SELABEL_CTX_MEDIA;
> > > +                       } else if (!strcmp(optarg, "x")) {
> > > +                               backend = SELABEL_CTX_X;
> > > +                       } else if (!strcmp(optarg, "db")) {
> > > +                               backend = SELABEL_CTX_DB;
> > > +                       } else if (!strcmp(optarg, "prop")) {
> > > +                               backend = SELABEL_CTX_ANDROID_PROP;
> > > +                       } else if (!strcmp(optarg, "service")) {
> > > +                               backend = SELABEL_CTX_ANDROID_SERVICE;
> > > +                       } else {
> > > +                               fprintf(stderr, "Unknown backend: %s\n", optarg);
> > > +                               usage(argv[0]);
> > > +                               return EXIT_FAILURE;
> > > +                       }
> > > +                       break;
> > > +               case 'v':
> > > +                       validate = (char *)1;
> > > +                       break;
> > > +               default:
> > > +                       usage(argv[0]);
> > > +                       return EXIT_FAILURE;
> > > +               }
> > > +       }
> > > +
> > > +       if (argc != optind + 2) {
> > > +               usage(argv[0]);
> > > +               return EXIT_FAILURE;
> > > +       }
> > > +
> > > +       file1 = argv[optind++];
> > > +       file2 = argv[optind];
> > > +
> > > +       return compare(file1, file2, validate, backend);
> > > +}
> > > --
> > > 2.43.0
> > >
> > >





[Index of Archives]     [Selinux Refpolicy]     [Linux SGX]     [Fedora Users]     [Fedora Desktop]     [Yosemite Photos]     [Yosemite Camping]     [Yosemite Campsites]     [KDE Users]     [Gnome Users]

  Powered by Linux