`fixfiles -C` does not apply to all paths

`fixfiles -C` does not apply to all paths

[Cedric Buissart]

Hello all,

I would like to discuss the possible removal of the static path list
in fixfiles' differential update mode (`fixfiles -C`).

Here is how it works :

160 # Compare PREVious File Context to currently installed File Context and
161 # run restorecon on all files affected by the differences.
162 #
163 diff_filecontext() {
164 EXCLUDEDIRS="`exclude_dirs_from_relabelling`"
165 for i in /sys /proc /dev /run /mnt /var/tmp /var/lib/BackupPC
/home /tmp /dev; do
166     [ -e $i ]  && EXCLUDEDIRS="${EXCLUDEDIRS} -e $i";
167 done
168 LogExcluded
169
170 if [ -f ${PREFC} -a -x /usr/bin/diff ]; then
171     TEMPFILE=`mktemp ${FC}.XXXXXXXXXX`
172     test -z "$TEMPFILE" && exit
173     PREFCTEMPFILE=`mktemp ${PREFC}.XXXXXXXXXX`
174     sed -r -e 's,:s0, ,g' $PREFC | sort -u > ${PREFCTEMPFILE}
175     sed -r -e 's,:s0, ,g' $FC | sort -u | \
176     /usr/bin/diff -b ${PREFCTEMPFILE} - | \
177         grep '^[<>]'|cut -c3-| grep ^/ | \
178         egrep -v '(^/home|^/root|^/tmp|^/dev)' |\
179     sed -r -e 's,[[:blank:]].*,,g' \
[...]
199     ${RESTORECON} ${VERBOSE} ${EXCLUDEDIRS} ${FORCEFLAG} $* -i -R -f -; \


lines 165-167 and 178 statically prevent some paths to be updated with
the new policy. I suspect this was done for efficiency and historical
reasons.

I would propose the removal of these path because :

- restorecon is (by default) automatically ignoring paths that are not
mounted with `seclabel`. There shouldn't be a need to statically treat
paths differently
- Some paths currently in this list (e.g. `/home`) may require
updating. During a policy update, packages (at least RHEL and Fedora)
are using `fixfiles -C` to make the policy more efficient, resulting
in a possibly incomplete policy update.
- The admin may not be aware of the manual steps required to fully
apply the new policy after an update.


How about removing these lines ?

Best regards,

Cedric

Re: `fixfiles -C` does not apply to all paths

[Stephen Smalley]

On Thu, Sep 17, 2020 at 10:44 AM Cedric Buissart <cbuissar@redhat.com> wrote:
>
> Hello all,
>
> I would like to discuss the possible removal of the static path list
> in fixfiles' differential update mode (`fixfiles -C`).
>
> Here is how it works :
>
> 160 # Compare PREVious File Context to currently installed File Context and
> 161 # run restorecon on all files affected by the differences.
> 162 #
> 163 diff_filecontext() {
> 164 EXCLUDEDIRS="`exclude_dirs_from_relabelling`"
> 165 for i in /sys /proc /dev /run /mnt /var/tmp /var/lib/BackupPC
> /home /tmp /dev; do
> 166     [ -e $i ]  && EXCLUDEDIRS="${EXCLUDEDIRS} -e $i";
> 167 done
> 168 LogExcluded
> 169
> 170 if [ -f ${PREFC} -a -x /usr/bin/diff ]; then
> 171     TEMPFILE=`mktemp ${FC}.XXXXXXXXXX`
> 172     test -z "$TEMPFILE" && exit
> 173     PREFCTEMPFILE=`mktemp ${PREFC}.XXXXXXXXXX`
> 174     sed -r -e 's,:s0, ,g' $PREFC | sort -u > ${PREFCTEMPFILE}
> 175     sed -r -e 's,:s0, ,g' $FC | sort -u | \
> 176     /usr/bin/diff -b ${PREFCTEMPFILE} - | \
> 177         grep '^[<>]'|cut -c3-| grep ^/ | \
> 178         egrep -v '(^/home|^/root|^/tmp|^/dev)' |\
> 179     sed -r -e 's,[[:blank:]].*,,g' \
> [...]
> 199     ${RESTORECON} ${VERBOSE} ${EXCLUDEDIRS} ${FORCEFLAG} $* -i -R -f -; \
>
>
> lines 165-167 and 178 statically prevent some paths to be updated with
> the new policy. I suspect this was done for efficiency and historical
> reasons.
>
> I would propose the removal of these path because :
>
> - restorecon is (by default) automatically ignoring paths that are not
> mounted with `seclabel`. There shouldn't be a need to statically treat
> paths differently
> - Some paths currently in this list (e.g. `/home`) may require
> updating. During a policy update, packages (at least RHEL and Fedora)
> are using `fixfiles -C` to make the policy more efficient, resulting
> in a possibly incomplete policy update.
> - The admin may not be aware of the manual steps required to fully
> apply the new policy after an update.
>
>
> How about removing these lines ?

Looking at the list, I note that several of them have seclabel set in
/proc/mounts so they would no longer be excluded after such a change.
The biggest concern is probably /home due to making fixfiles very
slow.  I think the whole idea of fixfiles -C was to try to minimize
time spent on a policy update.  Maybe we need to re-think the whole
approach.  Android has taken a different approach to allowing
efficient relabeling on Android upgrades.  They save a hash of the
matching file_contexts entries as an extended attribute of
directories, and only descend into the directory during relabeling if
the hash no longer matches.  Upstream, this is only enabled if the -D
option is passed to setfiles/restorecon since it requires
CAP_SYS_ADMIN to set the additional xattr.  Perhaps fixfiles should be
extended with this option and we should be using it instead of -C?

Re: `fixfiles -C` does not apply to all paths

[Petr Lautrbach]

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

On Thu, Sep 17, 2020 at 02:53:06PM -0400, Stephen Smalley wrote:
> On Thu, Sep 17, 2020 at 10:44 AM Cedric Buissart <cbuissar@redhat.com> wrote:
> >
> > Hello all,
> >
> > I would like to discuss the possible removal of the static path list
> > in fixfiles' differential update mode (`fixfiles -C`).
> >
> > Here is how it works :
> >
> > 160 # Compare PREVious File Context to currently installed File Context and
> > 161 # run restorecon on all files affected by the differences.
> > 162 #
> > 163 diff_filecontext() {
> > 164 EXCLUDEDIRS="`exclude_dirs_from_relabelling`"
> > 165 for i in /sys /proc /dev /run /mnt /var/tmp /var/lib/BackupPC
> > /home /tmp /dev; do
> > 166     [ -e $i ]  && EXCLUDEDIRS="${EXCLUDEDIRS} -e $i";
> > 167 done
> > 168 LogExcluded
> > 169
> > 170 if [ -f ${PREFC} -a -x /usr/bin/diff ]; then
> > 171     TEMPFILE=`mktemp ${FC}.XXXXXXXXXX`
> > 172     test -z "$TEMPFILE" && exit
> > 173     PREFCTEMPFILE=`mktemp ${PREFC}.XXXXXXXXXX`
> > 174     sed -r -e 's,:s0, ,g' $PREFC | sort -u > ${PREFCTEMPFILE}
> > 175     sed -r -e 's,:s0, ,g' $FC | sort -u | \
> > 176     /usr/bin/diff -b ${PREFCTEMPFILE} - | \
> > 177         grep '^[<>]'|cut -c3-| grep ^/ | \
> > 178         egrep -v '(^/home|^/root|^/tmp|^/dev)' |\
> > 179     sed -r -e 's,[[:blank:]].*,,g' \
> > [...]
> > 199     ${RESTORECON} ${VERBOSE} ${EXCLUDEDIRS} ${FORCEFLAG} $* -i -R -f -; \
> >
> >
> > lines 165-167 and 178 statically prevent some paths to be updated with
> > the new policy. I suspect this was done for efficiency and historical
> > reasons.
> >
> > I would propose the removal of these path because :
> >
> > - restorecon is (by default) automatically ignoring paths that are not
> > mounted with `seclabel`. There shouldn't be a need to statically treat
> > paths differently
> > - Some paths currently in this list (e.g. `/home`) may require
> > updating. During a policy update, packages (at least RHEL and Fedora)
> > are using `fixfiles -C` to make the policy more efficient, resulting
> > in a possibly incomplete policy update.
> > - The admin may not be aware of the manual steps required to fully
> > apply the new policy after an update.
> >
> >
> > How about removing these lines ?
> 
> Looking at the list, I note that several of them have seclabel set in
> /proc/mounts so they would no longer be excluded after such a change.
> The biggest concern is probably /home due to making fixfiles very
> slow.  I think the whole idea of fixfiles -C was to try to minimize
> time spent on a policy update.  Maybe we need to re-think the whole
> approach.  Android has taken a different approach to allowing
> efficient relabeling on Android upgrades.  They save a hash of the
> matching file_contexts entries as an extended attribute of
> directories, and only descend into the directory during relabeling if
> the hash no longer matches.  Upstream, this is only enabled if the -D
> option is passed to setfiles/restorecon since it requires
> CAP_SYS_ADMIN to set the additional xattr.  Perhaps fixfiles should be
> extended with this option and we should be using it instead of -C?
> 

I'd like to say that I'm aware about this problem but I don't have answers yet.

It seems to be related to the way how `fixfiles -C` translates regexps to
glob's, e.g. '/home/[^/]+/\.yubico(/.*)?' would be translated to '/home/*' and
relabeling whole /home could be long and delicate? action as it would touch users
data.

As a short term workaround, I'd suggest that policy package maintainers enforce relabeling
of particular directories inside /home when they know it's really needed.

Petr

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

Re: `fixfiles -C` does not apply to all paths

[Cedric Buissart]

On Thu, Sep 24, 2020 at 12:12 PM Petr Lautrbach <plautrba@redhat.com> wrote:
>
> On Thu, Sep 17, 2020 at 02:53:06PM -0400, Stephen Smalley wrote:
> > On Thu, Sep 17, 2020 at 10:44 AM Cedric Buissart <cbuissar@redhat.com> wrote:
> > >
> > > Hello all,
> > >
> > > I would like to discuss the possible removal of the static path list
> > > in fixfiles' differential update mode (`fixfiles -C`).
> > >
> > > Here is how it works :
> > >
> > > 160 # Compare PREVious File Context to currently installed File Context and
> > > 161 # run restorecon on all files affected by the differences.
> > > 162 #
> > > 163 diff_filecontext() {
> > > 164 EXCLUDEDIRS="`exclude_dirs_from_relabelling`"
> > > 165 for i in /sys /proc /dev /run /mnt /var/tmp /var/lib/BackupPC
> > > /home /tmp /dev; do
> > > 166     [ -e $i ]  && EXCLUDEDIRS="${EXCLUDEDIRS} -e $i";
> > > 167 done
> > > 168 LogExcluded
> > > 169
> > > 170 if [ -f ${PREFC} -a -x /usr/bin/diff ]; then
> > > 171     TEMPFILE=`mktemp ${FC}.XXXXXXXXXX`
> > > 172     test -z "$TEMPFILE" && exit
> > > 173     PREFCTEMPFILE=`mktemp ${PREFC}.XXXXXXXXXX`
> > > 174     sed -r -e 's,:s0, ,g' $PREFC | sort -u > ${PREFCTEMPFILE}
> > > 175     sed -r -e 's,:s0, ,g' $FC | sort -u | \
> > > 176     /usr/bin/diff -b ${PREFCTEMPFILE} - | \
> > > 177         grep '^[<>]'|cut -c3-| grep ^/ | \
> > > 178         egrep -v '(^/home|^/root|^/tmp|^/dev)' |\
> > > 179     sed -r -e 's,[[:blank:]].*,,g' \
> > > [...]
> > > 199     ${RESTORECON} ${VERBOSE} ${EXCLUDEDIRS} ${FORCEFLAG} $* -i -R -f -; \
> > >
> > >
> > > lines 165-167 and 178 statically prevent some paths to be updated with
> > > the new policy. I suspect this was done for efficiency and historical
> > > reasons.
> > >
> > > I would propose the removal of these path because :
> > >
> > > - restorecon is (by default) automatically ignoring paths that are not
> > > mounted with `seclabel`. There shouldn't be a need to statically treat
> > > paths differently
> > > - Some paths currently in this list (e.g. `/home`) may require
> > > updating. During a policy update, packages (at least RHEL and Fedora)
> > > are using `fixfiles -C` to make the policy more efficient, resulting
> > > in a possibly incomplete policy update.
> > > - The admin may not be aware of the manual steps required to fully
> > > apply the new policy after an update.
> > >
> > >
> > > How about removing these lines ?
> >
> > Looking at the list, I note that several of them have seclabel set in
> > /proc/mounts so they would no longer be excluded after such a change.
> > The biggest concern is probably /home due to making fixfiles very
> > slow.  I think the whole idea of fixfiles -C was to try to minimize
> > time spent on a policy update.  Maybe we need to re-think the whole
> > approach.  Android has taken a different approach to allowing
> > efficient relabeling on Android upgrades.  They save a hash of the
> > matching file_contexts entries as an extended attribute of
> > directories, and only descend into the directory during relabeling if
> > the hash no longer matches.  Upstream, this is only enabled if the -D
> > option is passed to setfiles/restorecon since it requires
> > CAP_SYS_ADMIN to set the additional xattr.  Perhaps fixfiles should be
> > extended with this option and we should be using it instead of -C?

As a side note : since non-system directories do not need immediate
update, how about delegating the policy update of these ones to a
one-off service running in the background ?
> >
>
> I'd like to say that I'm aware about this problem but I don't have answers yet.
>
> It seems to be related to the way how `fixfiles -C` translates regexps to
> glob's, e.g. '/home/[^/]+/\.yubico(/.*)?' would be translated to '/home/*' and
> relabeling whole /home could be long and delicate? action as it would touch users
> data.
>
> As a short term workaround, I'd suggest that policy package maintainers enforce relabeling
> of particular directories inside /home when they know it's really needed.
>
> Petr



-- 
Cedric Buissart,
Product Security