Re: Alias path subbing results in unexpected policy labelling

[Petr Lautrbach <plautrba@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5234 bytes --]

On Mon, Apr 23, 2018 at 04:21:22PM +0000, Joe Kirwin wrote:
> Petr, Daniel,
> 
> Have you had time to verify this issue yet?
> Any comments to add?
> 

I consider this as the expected behavior.

It's defined as "Substitute target path with sourcepath when generating default
 label." It means that /apple is substituted for /banana and the lookup is made
 for /banana/orange/foo.

On the other hand, `semanage-fcontext` man page and `semanage fcontext -h`
output could be misleading a bit as they use words "EQUAL" and "equivalent"
while it's not a symmetric relation, it's just a substitution.

I don't have an opinion about proposed change to have a real equivalence. It
could complicate some things a lot and the benefit is not clear to me right now.

Petr

>
> On Tue, Mar 20, 2018 at 8:14 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> 
> > On 03/19/2018 10:29 PM, Joe Kirwin wrote:
> > > *_Empirical Observations _*
> > > *
> > > *
> > > If I was to create an SELinux policy containing the following
> > file_contexts (fruits.fc)
> > > ```
> > > /apple/orange/.*                  --
> > gen_context(system_u:object_r:atype_t,s0)
> > > /banana/.*                           --
> > gen_context(system_u:object_r:btype_t,s0)
> > > ```
> > >
> > > If I then take the file
> > > /etc/selinux/default/contexts/files/file_contexts.subs_dist and append
> > to it the alias
> > > ```
> > > /apple /banana
> > > ```
> > >
> > > The resulting behavior is that when running:
> > > ```
> > > $ ./libselinux/utils/selabel_lookup_best_match -p /apple/orange/foo
> > > Best match context: system_u:object_r:btype_t:s0
> > >
> > > But the expected behavior is to match `atype_t` as that is a
> > "more-specific" match pattern
> >
> > I don't think this is a bug based on the documented behavior for
> > file_contexts.subs.  That said,
> > that support was added by Red Hat so I'll let them speak to it.
> >
> > >
> > > *_Looking into why_*
> > >
> > > From the method in `libselinux/src/label_file.c` :
> > >                       lookup_common(struct selabel_handle *rec, const
> > char *key, int type, bool partial)
> > >
> > > we encounter a call to :
> > >                      selabel_sub_key(struct saved_data *data, const char
> > *key)
> > >
> > > In the example above the candidate path we're trying to match (referred
> > to as the key in the code) is "canonicalized" to the /banana alias but the
> > regex being evaluated is not
> > >
> > > *_A proposed fix_*
> > > *
> > > *
> > > /Also attached (label_file.patch), if the patch formatting is off on
> > this thread, apologies./
> > > *
> > > *
> > > diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
> > > index 560d8c3..98a8d1b 100644
> > > --- a/libselinux/src/label_file.c
> > > +++ b/libselinux/src/label_file.c
> > > @@ -848,7 +848,7 @@ static struct spec *lookup_common(struct
> > selabel_handle *rec,
> > >  {
> > >     struct saved_data *data = (struct saved_data *)rec->data;
> > >     struct spec *spec_arr = data->spec_arr;
> > > -   int i, rc, file_stem;
> > > +   int i, rc, file_stem, orig_file_stem;
> > >     mode_t mode = (mode_t)type;
> > >     const char *buf;
> > >     struct spec *ret = NULL;
> > > @@ -879,8 +879,12 @@ static struct spec *lookup_common(struct
> > selabel_handle *rec,
> > >     }
> > >
> > >     sub = selabel_sub_key(data, key);
> > > -   if (sub)
> > > +   orig_file_stem = -1;
> > > +   if (sub) {
> > > +      orig_file_stem = find_stem_from_file(data, &key);
> > >         key = sub;
> > > +   }
> > >
> > >     buf = key;
> > >     file_stem = find_stem_from_file(data, &buf);
> > > @@ -896,7 +900,8 @@ static struct spec *lookup_common(struct
> > selabel_handle *rec,
> > >          * stem as the file AND if the spec in question has no mode
> > >          * specified or if the mode matches the file mode then we do
> > >          * a regex check        */
> > > -       if ((spec->stem_id == -1 || spec->stem_id == file_stem) &&
> > > +       if ((spec->stem_id == -1 || spec->stem_id == file_stem ||
> > > +            spec->stem_id == orig_file_stem) &&
> > >             (!mode || !spec->mode || mode == spec->mode)) {
> > >             if (compile_regex(data, spec, NULL) < 0)
> > >                 goto finish;
> > >
> > >
> > >
> > > I think there is still some simplification that could be done with
> > aliases, in that they really shouldn't have a direction (e.g. alias ->
> > original) instead they should go both ways and if there is a tie it should
> > go by the ordering of the specs.
> > > Reason for this is that a developer of an SELinux policy, may not know
> > the contents or directionality of file_contexts.subs_dist ahead of time or
> > when it might change.
> > >
> > > Thanks,
> > > Joe Kirwin and Travis Szucs
> > >
> >
> > --
> -- 
> *Joe Kirwin*  |  *Senior Security Developer_*
> *E:* jpk@cmd.com <bam@cmd.com>   *M:* 1.604.365.2823
> 
> <https://app.salesforceiq.com/r?target=5a6243eae4b0af727465cb94&t=AFwhZf1CQsv7FDLhSeW59giQrg545clQ2ksOeCxqTY5CK4AoaKjZJqLqF4FBhplxxtKw68VNNcp3ThHgNAMyfYlitWTAFUx2WymfWC2lJKWbtcBVzXz7rzKynmKi0AIVRaiN70T6bDHU>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]