restorecon manpage: link back to fixfiles
diff mbox

Message ID 20170111124110.5721-2-alan.christopher.jenkins@gmail.com
State Not Applicable
Headers show

Commit Message

Alan Jenkins Jan. 11, 2017, 12:41 p.m. UTC
fixfiles links to restorecon.  However if you start with restorecon
"restore file(s) default SELinux security contexts", you can easily
miss the fixfiles script.  fixfiles is more generally useful than
`restorecon -R`.   For example `restorecon -R /` is not as good as
`fixfiles restore`, because the restorecon command will try to relabel
`/sys` and fail noisily.

Signed-off-by: Alan Jenkins <alan.christopher.jenkins@gmail.com>
---
 policycoreutils/setfiles/restorecon.8 | 1 +
 1 file changed, 1 insertion(+)

Comments

Stephen Smalley Jan. 12, 2017, 8:01 p.m. UTC | #1
On Wed, 2017-01-11 at 12:41 +0000, Alan Jenkins wrote:
> fixfiles links to restorecon.  However if you start with restorecon
> "restore file(s) default SELinux security contexts", you can easily
> miss the fixfiles script.  fixfiles is more generally useful than
> `restorecon -R`.   For example `restorecon -R /` is not as good as
> `fixfiles restore`, because the restorecon command will try to
> relabel
> `/sys` and fail noisily.

Thanks, applied both patches.  Wondering though about the behavior
you describe above; restorecon -R /sys only issues one error message
for me and otherwise works fine,
# restorecon -R /sys
Could not set context for /sys/fs/cgroup:  Read-only file system

> 
> Signed-off-by: Alan Jenkins <alan.christopher.jenkins@gmail.com>
> ---
>  policycoreutils/setfiles/restorecon.8 | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/policycoreutils/setfiles/restorecon.8
> b/policycoreutils/setfiles/restorecon.8
> index b00bf4e..bd27113 100644
> --- a/policycoreutils/setfiles/restorecon.8
> +++ b/policycoreutils/setfiles/restorecon.8
> @@ -214,6 +214,7 @@ The program was written by Dan Walsh <dwalsh@redh
> at.com>.
>  
>  .SH "SEE ALSO"
>  .BR setfiles (8),
> +.BR fixfiles (8),
>  .BR load_policy (8),
>  .BR checkpolicy (8),
>  .BR customizable_types (5)
Alan Jenkins Jan. 12, 2017, 8:47 p.m. UTC | #2
On 12/01/17 20:01, Stephen Smalley wrote:
> On Wed, 2017-01-11 at 12:41 +0000, Alan Jenkins wrote:
>> fixfiles links to restorecon.  However if you start with restorecon
>> "restore file(s) default SELinux security contexts", you can easily
>> miss the fixfiles script.  fixfiles is more generally useful than
>> `restorecon -R`.   For example `restorecon -R /` is not as good as
>> `fixfiles restore`, because the restorecon command will try to
>> relabel
>> `/sys` and fail noisily.
> Thanks, applied both patches.
yay!

>    Wondering though about the behavior
> you describe above; restorecon -R /sys only issues one error message
> for me and otherwise works fine,
> # restorecon -R /sys
> Could not set context for /sys/fs/cgroup:  Read-only file system

It turned out fixfiles also generated similar noise.  I suspect this 
involved `-v` (in both cases), sorry.

Fedora Workstation 25:
"fixfiles spams warnings about debugfs. (docs say it only touches "real" 
filesystems!)" https://bugzilla.redhat.com/show_bug.cgi?id=1412747

Perhaps the root cause is actually the same.  I still prefer the 
messages from fixfiles though.  It explicitly detected conflicting 
labels on hardlinks

https://bugzilla.redhat.com/show_bug.cgi?id=1411371

and informed me in advance when it decided to traverse and relabel five 
of my virtual filesystems

    Checking / /boot /dev /dev/hugepages /dev/mqueue /dev/pts /dev/shm
    /home /run /run/user/1000 /run/user/1001 /run/user/42 /sys
    /sys/fs/pstore /sys/kernel/debug /tmp

(I doubt devtmpfs files are _intended_ to be labeled like this either.  
OTOH the stupidity doesn't seem to affect it, so I won't complain there).
Stephen Smalley Jan. 12, 2017, 9:23 p.m. UTC | #3
On Thu, 2017-01-12 at 20:47 +0000, Alan Jenkins wrote:
> On 12/01/17 20:01, Stephen Smalley wrote:
> > On Wed, 2017-01-11 at 12:41 +0000, Alan Jenkins wrote:
> > > fixfiles links to restorecon.  However if you start with
> > > restorecon
> > > "restore file(s) default SELinux security contexts", you can
> > > easily
> > > miss the fixfiles script.  fixfiles is more generally useful than
> > > `restorecon -R`.   For example `restorecon -R /` is not as good
> > > as
> > > `fixfiles restore`, because the restorecon command will try to
> > > relabel
> > > `/sys` and fail noisily.
> > Thanks, applied both patches.
>  yay!
> 
> >   Wondering though about the behavior
> > you describe above; restorecon -R /sys only issues one error
> > message
> > for me and otherwise works fine,
> > # restorecon -R /sys
> > Could not set context for /sys/fs/cgroup:  Read-only file system
>  
> It turned out fixfiles also generated similar noise.  I suspect this
> involved `-v` (in both cases), sorry.
> 
> Fedora Workstation 25:
> "fixfiles spams warnings about debugfs. (docs say it only touches
> "real" filesystems!)" https://bugzilla.redhat.com/show_bug.cgi?id=141
> 2747
> 
> Perhaps the root cause is actually the same.  I still prefer the
> messages from fixfiles though.  It explicitly detected conflicting
> labels on hardlinks
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1411371
> 
> and informed me in advance when it decided to traverse and relabel
> five of my virtual filesystems
> Checking / /boot /dev /dev/hugepages /dev/mqueue /dev/pts /dev/shm
> /home /run /run/user/1000 /run/user/1001 /run/user/42 /sys
> /sys/fs/pstore /sys/kernel/debug /tmp
> (I doubt devtmpfs files are _intended_ to be labeled like this
> either.  OTOH the stupidity doesn't seem to affect it, so I won't
> complain there).

By default, it includes any filesystem with the seclabel mount option
from /proc/mounts; this is a kernel-generated option that indicates
that the filesystem is known to support setting of security labels by
userspace.  restorecon (and setfiles) can be further instructed to skip
specific directories via the -e option.  fixfiles is just a wrapper
script around setfiles and/or restorecon depending on its options.
 setfiles was the original SELinux filesystem labeling utility,
oriented to labeling a list of filesystems based on a specified
file_contexts configuration, potentially even on a SELinux-disabled
host.  fixfiles and restorecon were later additions by Red Hat as more
user-friendly options, and then still later restorecon kept growing in
functionality and duplicating setfiles that we coalesced them
(restorecon is just a link that alters the default options and command-
line interface).

setfiles walks a filesystem at a time, which facilitates the hard link
conflict detection logic.  restorecon in contrast just recurses a
directory tree (with -R) and doesn't care about filesystem boundaries,
so if we wanted to turn on hard link conflict detection for it, we'd
need to augment that logic to save the device numbers too.  Possible
but hasn't been a priority.
Alan Jenkins Jan. 12, 2017, 11:42 p.m. UTC | #4
On 12/01/17 21:23, Stephen Smalley wrote:
> On Thu, 2017-01-12 at 20:47 +0000, Alan Jenkins wrote:
>> On 12/01/17 20:01, Stephen Smalley wrote:
>>> On Wed, 2017-01-11 at 12:41 +0000, Alan Jenkins wrote:
>>>> fixfiles links to restorecon.  However if you start with
>>>> restorecon
>>>> "restore file(s) default SELinux security contexts", you can
>>>> easily
>>>> miss the fixfiles script.  fixfiles is more generally useful than
>>>> `restorecon -R`.   For example `restorecon -R /` is not as good
>>>> as
>>>> `fixfiles restore`, because the restorecon command will try to
>>>> relabel
>>>> `/sys` and fail noisily.
>>> Thanks, applied both patches.
>>   yay!
>>
>>>    Wondering though about the behavior
>>> you describe above; restorecon -R /sys only issues one error
>>> message
>>> for me and otherwise works fine,
>>> # restorecon -R /sys
>>> Could not set context for /sys/fs/cgroup:  Read-only file system
>>   
>> It turned out fixfiles also generated similar noise.  I suspect this
>> involved `-v` (in both cases), sorry.
>>
>> Fedora Workstation 25:
>> "fixfiles spams warnings about debugfs. (docs say it only touches
>> "real" filesystems!)" https://bugzilla.redhat.com/show_bug.cgi?id=141
>> 2747
>>
>> Perhaps the root cause is actually the same.  I still prefer the
>> messages from fixfiles though.  It explicitly detected conflicting
>> labels on hardlinks
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1411371
>>
>> and informed me in advance when it decided to traverse and relabel
>> five of my virtual filesystems
>> Checking / /boot /dev /dev/hugepages /dev/mqueue /dev/pts /dev/shm
>> /home /run /run/user/1000 /run/user/1001 /run/user/42 /sys
>> /sys/fs/pstore /sys/kernel/debug /tmp
>> (I doubt devtmpfs files are _intended_ to be labeled like this
>> either.  OTOH the stupidity doesn't seem to affect it, so I won't
>> complain there).
> By default, it includes any filesystem with the seclabel mount option
> from /proc/mounts; this is a kernel-generated option that indicates
> that the filesystem is known to support setting of security labels by
> userspace.  restorecon (and setfiles) can be further instructed to skip
> specific directories via the -e option.  fixfiles is just a wrapper
> script around setfiles and/or restorecon depending on its options.
>   setfiles was the original SELinux filesystem labeling utility,
> oriented to labeling a list of filesystems based on a specified
> file_contexts configuration, potentially even on a SELinux-disabled
> host.  fixfiles and restorecon were later additions by Red Hat as more
> user-friendly options, and then still later restorecon kept growing in
> functionality and duplicating setfiles that we coalesced them
> (restorecon is just a link that alters the default options and command-
> line interface).
>
> setfiles walks a filesystem at a time, which facilitates the hard link
> conflict detection logic.  restorecon in contrast just recurses a
> directory tree (with -R) and doesn't care about filesystem boundaries,
> so if we wanted to turn on hard link conflict detection for it, we'd
> need to augment that logic to save the device numbers too.  Possible
> but hasn't been a priority.

Ok.

My main puzzle here[*] is why `fixfiles` handles sysfs (/sys/) fine, but 
then there's floods of warnings about debugfs (/sys/kernel/debug/).  The 
same seems to happen with /dev/ being fine, but not the other virtual 
fs's with seclabel which are mounted in subdirectories of /dev/.

What frustrates me is that `fixfiles check` generates this flood of 
messages even immediately after `fixfiles restore`.  (Or equivalently, 
that `fixfiles restore -v` does not converge to a smaller number of 
messages).

My use case was to install the latest version of some software from 
source (fwupd), apply the labels expected by the distribution policy, 
and also check what that involved.

I learnt about fixfiles from /usr/libexec/selinux/selinux-autorelabel.

(I notice the autorelabel script "handles" any messages by redirecting 
to /dev/null.  But, it's using `fixfiles restore` without `-v`, which 
doesn't generate these warnings.  It seems it would also miss out on the 
nice percentage progress indicator. Weird).

Thanks
Alan

[*] Ok, I was also surprised that fixfiles dug into my home directory.  
It's happy with `touch /home/alan-sysop/t` (user_home_t), but after I 
"move to wastebasket", it complains the file needs to marked as 
data_home_t.  No idea what happens if you relabel and then try to 
restore files from the wastebasket, e.g. `.ssh` or `public_html`.  But 
then `public_html` doesn't seem to get labeled when I create it either.  
Clearly there's something I don't understand.
Stephen Smalley Jan. 13, 2017, 2:48 p.m. UTC | #5
On Thu, 2017-01-12 at 23:42 +0000, Alan Jenkins wrote:
> My main puzzle here[*] is why `fixfiles` handles sysfs (/sys/) fine,
> but 
> then there's floods of warnings about debugfs
> (/sys/kernel/debug/).  The 
> same seems to happen with /dev/ being fine, but not the other
> virtual 
> fs's with seclabel which are mounted in subdirectories of /dev/.

This is a bug/regression.  Thanks for reporting it.  In commit
36f1ccbb5743749c404ad8f960867923544b90d9, Dan added this warning but
only if the user explicitly does a restorecon /path/to/foo and
/path/to/foo does not have any matching label in file_contexts; in the
case of a restorecon -R or setfiles, it isn't supposed to happen.  The
check on the recursive flag got dropped when this logic was taken into
selinux_restorecon(3) in libselinux
(commit f2e77865e144ab2e1313aa78d99b969f8f48695e).  Will fix.  The
reason it occurs for /sys/kernel/debug is that there is an explicit
entry in file_contexts that indicates that setfiles/restorecon should
not try to label anything under /sys/kernel/debug (/sys/kernel/debug/.*
<<none>>).

With regard to sysfs, a bit of historical context: originally all sysfs
nodes were created with a single security context, specified in kernel
policy. Later, we needed finer-grained control to support libvirt
labeling of specific nodes that needed to be accessible to VMs, so
support was added to permit userspace labeling of sysfs nodes (hence
restorecon/setfiles started working on sysfs).  Then in Android we
needed to label a number of sysfs nodes, so Android started doing the
equivalent of restorecon -R /sys on boot (using an optimization to
avoid walking parts of the sysfs tree that do not need to change from
the default).  Linux distributions don't do that presently but do label
a few specific sysfs nodes via /usr/lib/tmpfiles.d/selinux-policy.conf.
More recently, we introduced improved support in the kernel for per-
file labeling of sysfs nodes at creation time as well (based on
pathname from the root of sysfs), so we can specify those via genfscon
statements in policy.  So the kernel does assign an initial default,
but userspace retains the ability to override selectively (if
authorized by policy).

> What frustrates me is that `fixfiles check` generates this flood of 
> messages even immediately after `fixfiles restore`.  (Or
> equivalently, 
> that `fixfiles restore -v` does not converge to a smaller number of 
> messages).

Yes, agreed that we ought to fix that.

> 
> My use case was to install the latest version of some software from 
> source (fwupd), apply the labels expected by the distribution
> policy, 
> and also check what that involved.
> 
> I learnt about fixfiles from /usr/libexec/selinux/selinux-
> autorelabel.
> 
> (I notice the autorelabel script "handles" any messages by
> redirecting 
> to /dev/null.  But, it's using `fixfiles restore` without `-v`,
> which 
> doesn't generate these warnings.  It seems it would also miss out on
> the 
> nice percentage progress indicator. Weird).
> 
> Thanks
> Alan
> 
> [*] Ok, I was also surprised that fixfiles dug into my home
> directory.  
> It's happy with `touch /home/alan-sysop/t` (user_home_t), but after
> I 
> "move to wastebasket", it complains the file needs to marked as 
> data_home_t.  No idea what happens if you relabel and then try to 
> restore files from the wastebasket, e.g. `.ssh` or
> `public_html`.  But 
> then `public_html` doesn't seem to get labeled when I create it
> either.  
> Clearly there's something I don't understand.

Not sure about the different types on Trash vs Home.
But if I create public_html directory, it does get assigned
httpd_user_content_t automatically (probably a name-based type
transition or alternatively restorecond).
Stephen Smalley Jan. 13, 2017, 3:27 p.m. UTC | #6
On Fri, 2017-01-13 at 09:48 -0500, Stephen Smalley wrote:
> On Thu, 2017-01-12 at 23:42 +0000, Alan Jenkins wrote:
> > 
> > My main puzzle here[*] is why `fixfiles` handles sysfs (/sys/)
> > fine,
> > but 
> > then there's floods of warnings about debugfs
> > (/sys/kernel/debug/).  The 
> > same seems to happen with /dev/ being fine, but not the other
> > virtual 
> > fs's with seclabel which are mounted in subdirectories of /dev/.
> 
> This is a bug/regression.  Thanks for reporting it.  In commit
> 36f1ccbb5743749c404ad8f960867923544b90d9, Dan added this warning but
> only if the user explicitly does a restorecon /path/to/foo and
> /path/to/foo does not have any matching label in file_contexts; in
> the
> case of a restorecon -R or setfiles, it isn't supposed to happen.
>  The
> check on the recursive flag got dropped when this logic was taken
> into
> selinux_restorecon(3) in libselinux
> (commit f2e77865e144ab2e1313aa78d99b969f8f48695e).  Will fix.

Actually, I am wrong about this being a regression (and I should have
known that, since the buggy version is 2.5 and that precedes the latter
commit). Looking at the first commit, the original logic was to display
a warning if not recursive OR verbose, so it would unconditionally log
a warning if you did restorecon /path/to/foo or restorecon -v
/path/to/foo or restorecon -Rv /path/to/foo, just not if you did
restorecon -R /path/to/foo.  When it was moved to libselinux
selinux_restorecon(3), it was changed to log a warning if verbose, so
it logs a warning if you pass -v (with or without -R) but not if you
just do restorecon /path/to/foo. The patch I sent makes it only log the
warning if verbose and not recursive, so it will only log if you pass
-v without -R.

To be honest, I'm not sure what the point of this warning is; it is
perfectly valid for an entry to have <<none>> to indicate that it
should not be relabeled at all by restorecon/setfiles.  Maybe we should
just remove the warning altogether.
Stephen Smalley Jan. 13, 2017, 3:37 p.m. UTC | #7
On Thu, 2017-01-12 at 20:47 +0000, Alan Jenkins wrote:
> Perhaps the root cause is actually the same.  I still prefer the
> messages from fixfiles though.  It explicitly detected conflicting
> labels on hardlinks
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1411371

On this topic, I have opened an issue for selinux,
https://github.com/SELinuxProject/selinux/issues/43
Daniel Walsh Jan. 13, 2017, 6:29 p.m. UTC | #8
On 01/13/2017 10:27 AM, Stephen Smalley wrote:
> On Fri, 2017-01-13 at 09:48 -0500, Stephen Smalley wrote:
>> On Thu, 2017-01-12 at 23:42 +0000, Alan Jenkins wrote:
>>> My main puzzle here[*] is why `fixfiles` handles sysfs (/sys/)
>>> fine,
>>> but 
>>> then there's floods of warnings about debugfs
>>> (/sys/kernel/debug/).  The 
>>> same seems to happen with /dev/ being fine, but not the other
>>> virtual 
>>> fs's with seclabel which are mounted in subdirectories of /dev/.
>> This is a bug/regression.  Thanks for reporting it.  In commit
>> 36f1ccbb5743749c404ad8f960867923544b90d9, Dan added this warning but
>> only if the user explicitly does a restorecon /path/to/foo and
>> /path/to/foo does not have any matching label in file_contexts; in
>> the
>> case of a restorecon -R or setfiles, it isn't supposed to happen.
>>  The
>> check on the recursive flag got dropped when this logic was taken
>> into
>> selinux_restorecon(3) in libselinux
>> (commit f2e77865e144ab2e1313aa78d99b969f8f48695e).  Will fix.
> Actually, I am wrong about this being a regression (and I should have
> known that, since the buggy version is 2.5 and that precedes the latter
> commit). Looking at the first commit, the original logic was to display
> a warning if not recursive OR verbose, so it would unconditionally log
> a warning if you did restorecon /path/to/foo or restorecon -v
> /path/to/foo or restorecon -Rv /path/to/foo, just not if you did
> restorecon -R /path/to/foo.  When it was moved to libselinux
> selinux_restorecon(3), it was changed to log a warning if verbose, so
> it logs a warning if you pass -v (with or without -R) but not if you
> just do restorecon /path/to/foo. The patch I sent makes it only log the
> warning if verbose and not recursive, so it will only log if you pass
> -v without -R.
>
> To be honest, I'm not sure what the point of this warning is; it is
> perfectly valid for an entry to have <<none>> to indicate that it
> should not be relabeled at all by restorecon/setfiles.  Maybe we should
> just remove the warning altogether.
>
The problem is people don't understand this.  If a user sees a
user_home_t on /tmp and runs
restorecon on it he expects it to have a label with tmp_t in the name,
and if the tool finishes
silently he thinks he is done.  This reveals to him that their is no
default label, so perhaps he
will do a chcon.  Or `rm -f`.
Stephen Smalley Jan. 13, 2017, 7:38 p.m. UTC | #9
On Fri, 2017-01-13 at 13:29 -0500, Daniel J Walsh wrote:
> 
> On 01/13/2017 10:27 AM, Stephen Smalley wrote:
> > 
> > On Fri, 2017-01-13 at 09:48 -0500, Stephen Smalley wrote:
> > > 
> > > On Thu, 2017-01-12 at 23:42 +0000, Alan Jenkins wrote:
> > > > 
> > > > My main puzzle here[*] is why `fixfiles` handles sysfs (/sys/)
> > > > fine,
> > > > but 
> > > > then there's floods of warnings about debugfs
> > > > (/sys/kernel/debug/).  The 
> > > > same seems to happen with /dev/ being fine, but not the other
> > > > virtual 
> > > > fs's with seclabel which are mounted in subdirectories of
> > > > /dev/.
> > > This is a bug/regression.  Thanks for reporting it.  In commit
> > > 36f1ccbb5743749c404ad8f960867923544b90d9, Dan added this warning
> > > but
> > > only if the user explicitly does a restorecon /path/to/foo and
> > > /path/to/foo does not have any matching label in file_contexts;
> > > in
> > > the
> > > case of a restorecon -R or setfiles, it isn't supposed to happen.
> > >  The
> > > check on the recursive flag got dropped when this logic was taken
> > > into
> > > selinux_restorecon(3) in libselinux
> > > (commit f2e77865e144ab2e1313aa78d99b969f8f48695e).  Will fix.
> > Actually, I am wrong about this being a regression (and I should
> > have
> > known that, since the buggy version is 2.5 and that precedes the
> > latter
> > commit). Looking at the first commit, the original logic was to
> > display
> > a warning if not recursive OR verbose, so it would unconditionally
> > log
> > a warning if you did restorecon /path/to/foo or restorecon -v
> > /path/to/foo or restorecon -Rv /path/to/foo, just not if you did
> > restorecon -R /path/to/foo.  When it was moved to libselinux
> > selinux_restorecon(3), it was changed to log a warning if verbose,
> > so
> > it logs a warning if you pass -v (with or without -R) but not if
> > you
> > just do restorecon /path/to/foo. The patch I sent makes it only log
> > the
> > warning if verbose and not recursive, so it will only log if you
> > pass
> > -v without -R.
> > 
> > To be honest, I'm not sure what the point of this warning is; it is
> > perfectly valid for an entry to have <<none>> to indicate that it
> > should not be relabeled at all by restorecon/setfiles.  Maybe we
> > should
> > just remove the warning altogether.
> > 
> The problem is people don't understand this.  If a user sees a
> user_home_t on /tmp and runs
> restorecon on it he expects it to have a label with tmp_t in the
> name,
> and if the tool finishes
> silently he thinks he is done.  This reveals to him that their is no
> default label, so perhaps he
> will do a chcon.  Or `rm -f`.

Old behavior (before moving to selinux_restorecon(3), <= 2.5):
$ touch /tmp/foo
$ chcon -t etc_t /tmp/foo
$ restorecon /tmp/foo
restorecon:  Warning no default label for /tmp/foo
$ restorecon -v /tmp/foo
restorecon:  Warning no default label for /tmp/foo
$ restorecon -R /tmp/foo
$ restorecon -Rv /tmp/foo
restorecon:  Warning no default label for /tmp/foo

So we get the warning without -R or with -v.  Seems kind of surprising
that -R suppresses it but -Rv does not.

New behavior (after moving to selinux_restorecon(3), 2.6, before my
patch):
$ restorecon /tmp/foo
$ restorecon -v /tmp/foo
Warning no default label
for /tmp/foo
$ restorecon -R /tmp/foo
$ restorecon -Rv /tmp/foo
Warning no
default label for /tmp/foo

Here we get the warning only with -v, independent of -R.
Seems more consistent from a UI point of view.
This however doesn't help Alan with his goal of enabling fixfiles check or fixfiles restore -v to show no extraneous output if everything is labeled correctly.

New behavior after my patch:
$ restorecon /tmp/foo
$ restorecon -v /tmp/foo
Warning no default label for /tmp/foo
$ restorecon -R /tmp/foo
$ restorecon -Rv /tmp/foo

Here we only get the warning with -v without -R.
This avoids the problem for fixfiles check but doesn't help your situation and is confusing usage as well.

Also, I think you only want this warning if the user-supplied pathname
has no default label.  Which would mean we need to do this check and
warning early, not down where we are checking or applying the labels to individual files within a tree walk.
Alan Jenkins Jan. 13, 2017, 7:56 p.m. UTC | #10
On 13/01/17 19:38, Stephen Smalley wrote:
> On Fri, 2017-01-13 at 13:29 -0500, Daniel J Walsh wrote:
>> On 01/13/2017 10:27 AM, Stephen Smalley wrote:
>>> On Fri, 2017-01-13 at 09:48 -0500, Stephen Smalley wrote:
>>>> On Thu, 2017-01-12 at 23:42 +0000, Alan Jenkins wrote:
>>>>> My main puzzle here[*] is why `fixfiles` handles sysfs (/sys/)
>>>>> fine,
>>>>> but
>>>>> then there's floods of warnings about debugfs
>>>>> (/sys/kernel/debug/).  The
>>>>> same seems to happen with /dev/ being fine, but not the other
>>>>> virtual
>>>>> fs's with seclabel which are mounted in subdirectories of
>>>>> /dev/.
>>>> This is a bug/regression.  Thanks for reporting it.  In commit
>>>> 36f1ccbb5743749c404ad8f960867923544b90d9, Dan added this warning
>>>> but
>>>> only if the user explicitly does a restorecon /path/to/foo and
>>>> /path/to/foo does not have any matching label in file_contexts;
>>>> in
>>>> the
>>>> case of a restorecon -R or setfiles, it isn't supposed to happen.
>>>>   The
>>>> check on the recursive flag got dropped when this logic was taken
>>>> into
>>>> selinux_restorecon(3) in libselinux
>>>> (commit f2e77865e144ab2e1313aa78d99b969f8f48695e).  Will fix.
>>> Actually, I am wrong about this being a regression (and I should
>>> have
>>> known that, since the buggy version is 2.5 and that precedes the
>>> latter
>>> commit). Looking at the first commit, the original logic was to
>>> display
>>> a warning if not recursive OR verbose, so it would unconditionally
>>> log
>>> a warning if you did restorecon /path/to/foo or restorecon -v
>>> /path/to/foo or restorecon -Rv /path/to/foo, just not if you did
>>> restorecon -R /path/to/foo.  When it was moved to libselinux
>>> selinux_restorecon(3), it was changed to log a warning if verbose,
>>> so
>>> it logs a warning if you pass -v (with or without -R) but not if
>>> you
>>> just do restorecon /path/to/foo. The patch I sent makes it only log
>>> the
>>> warning if verbose and not recursive, so it will only log if you
>>> pass
>>> -v without -R.
>>>
>>> To be honest, I'm not sure what the point of this warning is; it is
>>> perfectly valid for an entry to have <<none>> to indicate that it
>>> should not be relabeled at all by restorecon/setfiles.  Maybe we
>>> should
>>> just remove the warning altogether.
>>>
>> The problem is people don't understand this.  If a user sees a
>> user_home_t on /tmp and runs
>> restorecon on it he expects it to have a label with tmp_t in the
>> name,
>> and if the tool finishes
>> silently he thinks he is done.  This reveals to him that their is no
>> default label, so perhaps he
>> will do a chcon.  Or `rm -f`.
> Old behavior (before moving to selinux_restorecon(3), <= 2.5):
> $ touch /tmp/foo
> $ chcon -t etc_t /tmp/foo
> $ restorecon /tmp/foo
> restorecon:  Warning no default label for /tmp/foo
> $ restorecon -v /tmp/foo
> restorecon:  Warning no default label for /tmp/foo
> $ restorecon -R /tmp/foo
> $ restorecon -Rv /tmp/foo
> restorecon:  Warning no default label for /tmp/foo
>
> So we get the warning without -R or with -v.  Seems kind of surprising
> that -R suppresses it but -Rv does not.
>
> New behavior (after moving to selinux_restorecon(3), 2.6, before my
> patch):
> $ restorecon /tmp/foo
> $ restorecon -v /tmp/foo
> Warning no default label
> for /tmp/foo
> $ restorecon -R /tmp/foo
> $ restorecon -Rv /tmp/foo
> Warning no
> default label for /tmp/foo
>
> Here we get the warning only with -v, independent of -R.
> Seems more consistent from a UI point of view.
> This however doesn't help Alan with his goal of enabling fixfiles check or fixfiles restore -v to show no extraneous output if everything is labeled correctly.
>
> New behavior after my patch:
> $ restorecon /tmp/foo
> $ restorecon -v /tmp/foo
> Warning no default label for /tmp/foo
> $ restorecon -R /tmp/foo
> $ restorecon -Rv /tmp/foo
>
> Here we only get the warning with -v without -R.
> This avoids the problem for fixfiles check but doesn't help your situation and is confusing usage as well.
>
> Also, I think you only want this warning if the user-supplied pathname
> has no default label.  Which would mean we need to do this check and
> warning early, not down where we are checking or applying the labels to individual files within a tree walk.

Thank you for considering my report so seriously.

Your suggestion sounds very promising!

It sounds like a pretty nice compromise, if e.g. processing /var could 
omit the warnings caused by processing files under /var/tmp.

The corner case I wasn't 100% sure about, is if the path you pass is 
e.g. `/tmp`.  Looking at `file_contexts` it seems very sensible though.  
/tmp itself is ignored, so passing `/tmp` would still provide our 
friendly warning.  (I guess the label on /tmp comes from the package, 
e.g. the `filesystem` rpm on Fedora).

Alan
Alan Jenkins Jan. 13, 2017, 8:13 p.m. UTC | #11
On 13/01/17 19:56, Alan Jenkins wrote:
> On 13/01/17 19:38, Stephen Smalley wrote:
>> On Fri, 2017-01-13 at 13:29 -0500, Daniel J Walsh wrote:
>>> On 01/13/2017 10:27 AM, Stephen Smalley wrote:
>>>> On Fri, 2017-01-13 at 09:48 -0500, Stephen Smalley wrote:
>>>>> On Thu, 2017-01-12 at 23:42 +0000, Alan Jenkins wrote:
>>>>>> My main puzzle here[*] is why `fixfiles` handles sysfs (/sys/)
>>>>>> fine,
>>>>>> but
>>>>>> then there's floods of warnings about debugfs
>>>>>> (/sys/kernel/debug/).  The
>>>>>> same seems to happen with /dev/ being fine, but not the other
>>>>>> virtual
>>>>>> fs's with seclabel which are mounted in subdirectories of
>>>>>> /dev/.
>>>>> This is a bug/regression.  Thanks for reporting it.  In commit
>>>>> 36f1ccbb5743749c404ad8f960867923544b90d9, Dan added this warning
>>>>> but
>>>>> only if the user explicitly does a restorecon /path/to/foo and
>>>>> /path/to/foo does not have any matching label in file_contexts;
>>>>> in
>>>>> the
>>>>> case of a restorecon -R or setfiles, it isn't supposed to happen.
>>>>>   The
>>>>> check on the recursive flag got dropped when this logic was taken
>>>>> into
>>>>> selinux_restorecon(3) in libselinux
>>>>> (commit f2e77865e144ab2e1313aa78d99b969f8f48695e).  Will fix.
>>>> Actually, I am wrong about this being a regression (and I should
>>>> have
>>>> known that, since the buggy version is 2.5 and that precedes the
>>>> latter
>>>> commit). Looking at the first commit, the original logic was to
>>>> display
>>>> a warning if not recursive OR verbose, so it would unconditionally
>>>> log
>>>> a warning if you did restorecon /path/to/foo or restorecon -v
>>>> /path/to/foo or restorecon -Rv /path/to/foo, just not if you did
>>>> restorecon -R /path/to/foo.  When it was moved to libselinux
>>>> selinux_restorecon(3), it was changed to log a warning if verbose,
>>>> so
>>>> it logs a warning if you pass -v (with or without -R) but not if
>>>> you
>>>> just do restorecon /path/to/foo. The patch I sent makes it only log
>>>> the
>>>> warning if verbose and not recursive, so it will only log if you
>>>> pass
>>>> -v without -R.
>>>>
>>>> To be honest, I'm not sure what the point of this warning is; it is
>>>> perfectly valid for an entry to have <<none>> to indicate that it
>>>> should not be relabeled at all by restorecon/setfiles. Maybe we
>>>> should
>>>> just remove the warning altogether.
>>>>
>>> The problem is people don't understand this.  If a user sees a
>>> user_home_t on /tmp and runs
>>> restorecon on it he expects it to have a label with tmp_t in the
>>> name,
>>> and if the tool finishes
>>> silently he thinks he is done.  This reveals to him that their is no
>>> default label, so perhaps he
>>> will do a chcon.  Or `rm -f`.
>> Old behavior (before moving to selinux_restorecon(3), <= 2.5):
>> $ touch /tmp/foo
>> $ chcon -t etc_t /tmp/foo
>> $ restorecon /tmp/foo
>> restorecon:  Warning no default label for /tmp/foo
>> $ restorecon -v /tmp/foo
>> restorecon:  Warning no default label for /tmp/foo
>> $ restorecon -R /tmp/foo
>> $ restorecon -Rv /tmp/foo
>> restorecon:  Warning no default label for /tmp/foo
>>
>> So we get the warning without -R or with -v.  Seems kind of surprising
>> that -R suppresses it but -Rv does not.
>>
>> New behavior (after moving to selinux_restorecon(3), 2.6, before my
>> patch):
>> $ restorecon /tmp/foo
>> $ restorecon -v /tmp/foo
>> Warning no default label
>> for /tmp/foo
>> $ restorecon -R /tmp/foo
>> $ restorecon -Rv /tmp/foo
>> Warning no
>> default label for /tmp/foo
>>
>> Here we get the warning only with -v, independent of -R.
>> Seems more consistent from a UI point of view.
>> This however doesn't help Alan with his goal of enabling fixfiles 
>> check or fixfiles restore -v to show no extraneous output if 
>> everything is labeled correctly.
>>
>> New behavior after my patch:
>> $ restorecon /tmp/foo
>> $ restorecon -v /tmp/foo
>> Warning no default label for /tmp/foo
>> $ restorecon -R /tmp/foo
>> $ restorecon -Rv /tmp/foo
>>
>> Here we only get the warning with -v without -R.
>> This avoids the problem for fixfiles check but doesn't help your 
>> situation and is confusing usage as well.
>>
>> Also, I think you only want this warning if the user-supplied pathname
>> has no default label.  Which would mean we need to do this check and
>> warning early, not down where we are checking or applying the labels 
>> to individual files within a tree walk.
>
> Thank you for considering my report so seriously.
>
> Your suggestion sounds very promising!
>
> It sounds like a pretty nice compromise, if e.g. processing /var could 
> omit the warnings caused by processing files under /var/tmp.
>
> The corner case I wasn't 100% sure about, is if the path you pass is 
> e.g. `/tmp`.  Looking at `file_contexts` it seems very sensible 
> though.  /tmp itself is ignored, so passing `/tmp` would still provide 
> our friendly warning.  (I guess the label on /tmp comes from the 
> package, e.g. the `filesystem` rpm on Fedora).

oops, no.  We do label some files in /tmp, and /tmp _is_ labelled. So 
the compromise is not _quite_ as nice for providing warnings as I first 
thought.  I.e. processing /tmp would not provide any warning. However 
attempting to process 
/tmp/my-chroot-which-dnf-created-with-selinux-labels should still warn, 
which is something I agree with.

Alan

Patch
diff mbox

diff --git a/policycoreutils/setfiles/restorecon.8 b/policycoreutils/setfiles/restorecon.8
index b00bf4e..bd27113 100644
--- a/policycoreutils/setfiles/restorecon.8
+++ b/policycoreutils/setfiles/restorecon.8
@@ -214,6 +214,7 @@  The program was written by Dan Walsh <dwalsh@redhat.com>.
 
 .SH "SEE ALSO"
 .BR setfiles (8),
+.BR fixfiles (8),
 .BR load_policy (8),
 .BR checkpolicy (8),
 .BR customizable_types (5)