Alias path subbing results in unexpected policy labelling

Alias path subbing results in unexpected policy labelling

[Joe Kirwin]

[-- Attachment #1.1: Type: text/plain, Size: 3122 bytes --]

*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

*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

[-- Attachment #1.2: Type: text/html, Size: 4405 bytes --]

[-- Attachment #2: label_file.patch --]
[-- Type: text/x-patch, Size: 1330 bytes --]

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;

Re: Alias path subbing results in unexpected policy labelling

[Stephen Smalley]

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
> 

Re: Alias path subbing results in unexpected policy labelling

[Joe Kirwin]

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

Petr, Daniel,

Have you had time to verify this issue yet?
Any comments to add?

Thanks,
Joe

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: Type: text/html, Size: 6544 bytes --]

Re: Alias path subbing results in unexpected policy labelling

[Petr Lautrbach]

[-- 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 --]