[RFC] fixfiles: correctly restore context of mountpoints
diff mbox series

Message ID 330f12f0-44d6-3659-0153-32b3cddf2db6@gmail.com
State Superseded
Headers show
Series
  • [RFC] fixfiles: correctly restore context of mountpoints
Related show

Commit Message

bauen1 June 30, 2020, 2:59 p.m. UTC
By bind mounting every filesystem we want to relabel we can access all
files without anything hidden due to active mounts.

This comes at the cost of user experience, because setfiles only
displays the percentage if no path is given or the path is /

Signed-off-by: bauen1 <j2468h@gmail.com>
---
 policycoreutils/scripts/fixfiles | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

Comments

Stephen Smalley July 6, 2020, 6:25 p.m. UTC | #1
On Tue, Jun 30, 2020 at 11:01 AM bauen1 <j2468h@googlemail.com> wrote:
>
> By bind mounting every filesystem we want to relabel we can access all
> files without anything hidden due to active mounts.
>
> This comes at the cost of user experience, because setfiles only
> displays the percentage if no path is given or the path is /

Perhaps this should be opt-in via a new command-line option rather
than the default, given the user-visible difference in behavior and
the potential for something to go wrong for existing users.  We might
also want to look at improving setfiles / selinux_restorecon() to
support percentage progress without this limitation.

>
> Signed-off-by: bauen1 <j2468h@gmail.com>

Generally I think a real name is required for Signed-off-by lines in
the DCO since otherwise it isn't truly meaningful from a legal
perspective.

> ---
>  policycoreutils/scripts/fixfiles | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles
> index 5d777034..dc5be195 100755
> --- a/policycoreutils/scripts/fixfiles
> +++ b/policycoreutils/scripts/fixfiles
> @@ -243,7 +243,19 @@ case "$RESTORE_MODE" in
>         if [ -n "${FILESYSTEMSRW}" ]; then
>             LogReadOnly
>             echo "${OPTION}ing `echo ${FILESYSTEMSRW}`"
> -           ${SETFILES} ${VERBOSE} ${EXCLUDEDIRS} ${FORCEFLAG} $* -q ${FC} ${FILESYSTEMSRW}
> +
> +           # we bind mount so we can fix the labels of files that have already been
> +           # mounted over
> +           for m in `echo $FILESYSTEMSRW`; do
> +               TMP_MOUNT="$(mktemp -d)"
> +               test -z ${TMP_MOUNT+x} && echo "Unable to find temporary directory!" && exit 1
> +
> +               mkdir -p "${TMP_MOUNT}${m}" || exit 1
> +               mount --bind "${m}" "${TMP_MOUNT}${m}" || exit 1
> +               ${SETFILES} ${VERBOSE} ${EXCLUDEDIRS} ${FORCEFLAG} $* -q ${FC} -r "${TMP_MOUNT}" "${TMP_MOUNT}${m}"
> +               umount "${TMP_MOUNT}${m}" || exit 1
> +               rm -rf "${TMP_MOUNT}" || echo "Error cleaning up."
> +           done;
>         else
>             echo >&2 "fixfiles: No suitable file systems found"
>         fi
> --
> 2.27.0
>
bauen1 July 6, 2020, 7:16 p.m. UTC | #2
Thank you for reviewing:

On 7/6/20 8:25 PM, Stephen Smalley wrote:
> On Tue, Jun 30, 2020 at 11:01 AM bauen1 <j2468h@googlemail.com> wrote:
>>
>> By bind mounting every filesystem we want to relabel we can access all
>> files without anything hidden due to active mounts.
>>
>> This comes at the cost of user experience, because setfiles only
>> displays the percentage if no path is given or the path is /
> 
> Perhaps this should be opt-in via a new command-line option rather
> than the default, given the user-visible difference in behavior and
> the potential for something to go wrong for existing users.  We might
> also want to look at improving setfiles / selinux_restorecon() to
> support percentage progress without this limitation.

I would argue that the new behavior is in theory "better" and allows removing a few questionable mounton allow rules from policies. If a user has files in a directory that was mounted over it could lead to surprises, so keeping a backwards compatible behavior is probably preferable. I will implement a new command-line option for it

Fixing selinux_restorecon() to display the correct percentage is just a matter of improving it to check if the relabel targets the root of a mounted filesystem instead of the currently hard coded "/" (I think).

>>
>> Signed-off-by: bauen1 <j2468h@gmail.com>
> 
> Generally I think a real name is required for Signed-off-by lines in
> the DCO since otherwise it isn't truly meaningful from a legal
> perspective.

I've now also read the guide on submitting patches to the linux kernel. What would be the best way to go about adding my real name and email address while also keeping my pseudonym and email in the commit, e.g. would just replacing the Signed-off-by with my real name and email address work ?
Stephen Smalley July 6, 2020, 7:48 p.m. UTC | #3
On Mon, Jul 6, 2020 at 3:16 PM bauen1 <j2468h@googlemail.com> wrote:
>
> Thank you for reviewing:
>
> On 7/6/20 8:25 PM, Stephen Smalley wrote:
> > On Tue, Jun 30, 2020 at 11:01 AM bauen1 <j2468h@googlemail.com> wrote:
> >>
> >> By bind mounting every filesystem we want to relabel we can access all
> >> files without anything hidden due to active mounts.
> >>
> >> This comes at the cost of user experience, because setfiles only
> >> displays the percentage if no path is given or the path is /
> >
> > Perhaps this should be opt-in via a new command-line option rather
> > than the default, given the user-visible difference in behavior and
> > the potential for something to go wrong for existing users.  We might
> > also want to look at improving setfiles / selinux_restorecon() to
> > support percentage progress without this limitation.
>
> I would argue that the new behavior is in theory "better" and allows removing a few questionable mounton allow rules from policies. If a user has files in a directory that was mounted over it could lead to surprises, so keeping a backwards compatible behavior is probably preferable. I will implement a new command-line option for it
>
> Fixing selinux_restorecon() to display the correct percentage is just a matter of improving it to check if the relabel targets the root of a mounted filesystem instead of the currently hard coded "/" (I think).
>
> >>
> >> Signed-off-by: bauen1 <j2468h@gmail.com>
> >
> > Generally I think a real name is required for Signed-off-by lines in
> > the DCO since otherwise it isn't truly meaningful from a legal
> > perspective.
>
> I've now also read the guide on submitting patches to the linux kernel. What would be the best way to go about adding my real name and email address while also keeping my pseudonym and email in the commit, e.g. would just replacing the Signed-off-by with my real name and email address work ?

I think the important part is that you use your real (legal) name in
the Signed-off-by line. You can use whatever email address you like in
the Signed-off-by line (as long as you can in fact receive email sent
there), and that need not match the email address in the From header.
Of course, IANAL and others may disagree.

Patch
diff mbox series

diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles
index 5d777034..dc5be195 100755
--- a/policycoreutils/scripts/fixfiles
+++ b/policycoreutils/scripts/fixfiles
@@ -243,7 +243,19 @@  case "$RESTORE_MODE" in
 	if [ -n "${FILESYSTEMSRW}" ]; then
 	    LogReadOnly
 	    echo "${OPTION}ing `echo ${FILESYSTEMSRW}`"
-	    ${SETFILES} ${VERBOSE} ${EXCLUDEDIRS} ${FORCEFLAG} $* -q ${FC} ${FILESYSTEMSRW}
+
+	    # we bind mount so we can fix the labels of files that have already been
+	    # mounted over
+	    for m in `echo $FILESYSTEMSRW`; do
+	        TMP_MOUNT="$(mktemp -d)"
+	        test -z ${TMP_MOUNT+x} && echo "Unable to find temporary directory!" && exit 1
+
+	        mkdir -p "${TMP_MOUNT}${m}" || exit 1
+	        mount --bind "${m}" "${TMP_MOUNT}${m}" || exit 1
+	        ${SETFILES} ${VERBOSE} ${EXCLUDEDIRS} ${FORCEFLAG} $* -q ${FC} -r "${TMP_MOUNT}" "${TMP_MOUNT}${m}"
+	        umount "${TMP_MOUNT}${m}" || exit 1
+	        rm -rf "${TMP_MOUNT}" || echo "Error cleaning up."
+	    done;
 	else
 	    echo >&2 "fixfiles: No suitable file systems found"
 	fi