| Message ID | 20220915124418.27156-1-plautrba@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fixfiles: Unmount temporary bind mounts on SIGINT | expand |
On Thu, Sep 15, 2022 at 2:45 PM Petr Lautrbach <plautrba@redhat.com> wrote: > `fixfiles -M relabel` temporary bind mounts filestems before relabeling > but it leaves a directory mounted in /tmp/tmp.XXXX when a user hits > CTRL-C. It means that if the user run `fixfiles -M relabel` again and > answered Y to clean out /tmp directory, it would remove all data from > mounted fs. > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2125355 > > Signed-off-by: Petr Lautrbach <plautrba@redhat.com> > --- > policycoreutils/scripts/fixfiles | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles > index c72ca0eb9d61..6811921970f2 100755 > --- a/policycoreutils/scripts/fixfiles > +++ b/policycoreutils/scripts/fixfiles > @@ -207,6 +207,15 @@ rpm -q --qf '[%{FILESTATES} %{FILENAMES}\n]' "$1" | grep '^0 ' | cut -f2- -d ' ' > [ ${PIPESTATUS[0]} != 0 ] && echo "$1 not found" >/dev/stderr > } > > +# unmount tmp bind mount before exit > +umount_TMP_MOUNT() { > + if [ -n "$TMP_MOUNT" ]; then > + umount "${TMP_MOUNT}${m}" || exit 130 > + rm -rf "${TMP_MOUNT}" || echo "Error cleaning up." > + fi > + exit 130 > +} > + > # > # restore > # if called with -n will only check file context > @@ -251,6 +260,7 @@ case "$RESTORE_MODE" in > else > # we bind mount so we can fix the labels of files that have already been > # mounted over > + trap umount_TMP_MOUNT SIGINT > for m in `echo $FILESYSTEMSRW`; do > TMP_MOUNT="$(mktemp -d)" > test -z ${TMP_MOUNT+x} && echo "Unable to find temporary directory!" && exit 1 > @@ -261,6 +271,7 @@ case "$RESTORE_MODE" in > umount "${TMP_MOUNT}${m}" || exit 1 > rm -rf "${TMP_MOUNT}" || echo "Error cleaning up." > done; > + trap SIGINT > fi > else > echo >&2 "fixfiles: No suitable file systems found" > -- > 2.37.3 > And what if the fixfiles process is terminated via SIGTERM or even SIGKILL? Or a power failure occurs at the wrong time? What if some other process leaves behind a bind mount / other mount in /tmp? My suggestion would be to: 1) Change the trap from SIGINT to EXIT. That will cover both SIGTERM and SIGINT. 2) Additionally modify fullrelabel() to not traverse across mounts when doing the removing (+ possibly exit with an error whenever a mountpoint is encountered - OR - try to unmount the mounts instead of removing their contents).
Ondrej Mosnacek <omosnace@redhat.com> writes: > On Thu, Sep 15, 2022 at 2:45 PM Petr Lautrbach <plautrba@redhat.com> wrote: >> `fixfiles -M relabel` temporary bind mounts filestems before relabeling >> but it leaves a directory mounted in /tmp/tmp.XXXX when a user hits >> CTRL-C. It means that if the user run `fixfiles -M relabel` again and >> answered Y to clean out /tmp directory, it would remove all data from >> mounted fs. >> >> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2125355 >> >> Signed-off-by: Petr Lautrbach <plautrba@redhat.com> >> --- >> policycoreutils/scripts/fixfiles | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles >> index c72ca0eb9d61..6811921970f2 100755 >> --- a/policycoreutils/scripts/fixfiles >> +++ b/policycoreutils/scripts/fixfiles >> @@ -207,6 +207,15 @@ rpm -q --qf '[%{FILESTATES} %{FILENAMES}\n]' "$1" | grep '^0 ' | cut -f2- -d ' ' >> [ ${PIPESTATUS[0]} != 0 ] && echo "$1 not found" >/dev/stderr >> } >> >> +# unmount tmp bind mount before exit >> +umount_TMP_MOUNT() { >> + if [ -n "$TMP_MOUNT" ]; then >> + umount "${TMP_MOUNT}${m}" || exit 130 >> + rm -rf "${TMP_MOUNT}" || echo "Error cleaning up." >> + fi >> + exit 130 >> +} >> + >> # >> # restore >> # if called with -n will only check file context >> @@ -251,6 +260,7 @@ case "$RESTORE_MODE" in >> else >> # we bind mount so we can fix the labels of files that have already been >> # mounted over >> + trap umount_TMP_MOUNT SIGINT >> for m in `echo $FILESYSTEMSRW`; do >> TMP_MOUNT="$(mktemp -d)" >> test -z ${TMP_MOUNT+x} && echo "Unable to find temporary directory!" && exit 1 >> @@ -261,6 +271,7 @@ case "$RESTORE_MODE" in >> umount "${TMP_MOUNT}${m}" || exit 1 >> rm -rf "${TMP_MOUNT}" || echo "Error cleaning up." >> done; >> + trap SIGINT >> fi >> else >> echo >&2 "fixfiles: No suitable file systems found" >> -- >> 2.37.3 >> > > And what if the fixfiles process is terminated via SIGTERM or even > SIGKILL? Good point. > Or a power failure occurs at the wrong time? After power failure and reboot there's no bind mountpoints from fixfiles left. > What if some > other process leaves behind a bind mount / other mount in /tmp? I don't know. But it's up to users to decide. If they are not sure, they should answer 'No' to /tmp clean up. > My suggestion would be to: > 1) Change the trap from SIGINT to EXIT. That will cover both SIGTERM and SIGINT. I'll send updated patch with this. > 2) Additionally modify fullrelabel() to not traverse across mounts > when doing the removing (+ possibly exit with an error whenever a > mountpoint is encountered - OR - try to unmount the mounts instead of > removing their contents). Given that I don't know what was the original motivation for the cleaing code and users are asked whether they want to clean up /tmp and they are warned that they would need to reboot after this operation, I'm not sure how I would defend the change. Thanks, Petr > -- > Ondrej Mosnacek > Senior Software Engineer, Linux Security - SELinux kernel > Red Hat, Inc.
On Thu, Sep 15, 2022 at 6:29 PM Petr Lautrbach <plautrba@redhat.com> wrote: > Ondrej Mosnacek <omosnace@redhat.com> writes: > > > On Thu, Sep 15, 2022 at 2:45 PM Petr Lautrbach <plautrba@redhat.com> wrote: > >> `fixfiles -M relabel` temporary bind mounts filestems before relabeling > >> but it leaves a directory mounted in /tmp/tmp.XXXX when a user hits > >> CTRL-C. It means that if the user run `fixfiles -M relabel` again and > >> answered Y to clean out /tmp directory, it would remove all data from > >> mounted fs. > >> > >> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2125355 > >> > >> Signed-off-by: Petr Lautrbach <plautrba@redhat.com> > >> --- > >> policycoreutils/scripts/fixfiles | 11 +++++++++++ > >> 1 file changed, 11 insertions(+) > >> > >> diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles > >> index c72ca0eb9d61..6811921970f2 100755 > >> --- a/policycoreutils/scripts/fixfiles > >> +++ b/policycoreutils/scripts/fixfiles > >> @@ -207,6 +207,15 @@ rpm -q --qf '[%{FILESTATES} %{FILENAMES}\n]' "$1" | grep '^0 ' | cut -f2- -d ' ' > >> [ ${PIPESTATUS[0]} != 0 ] && echo "$1 not found" >/dev/stderr > >> } > >> > >> +# unmount tmp bind mount before exit > >> +umount_TMP_MOUNT() { > >> + if [ -n "$TMP_MOUNT" ]; then > >> + umount "${TMP_MOUNT}${m}" || exit 130 > >> + rm -rf "${TMP_MOUNT}" || echo "Error cleaning up." > >> + fi > >> + exit 130 > >> +} > >> + > >> # > >> # restore > >> # if called with -n will only check file context > >> @@ -251,6 +260,7 @@ case "$RESTORE_MODE" in > >> else > >> # we bind mount so we can fix the labels of files that have already been > >> # mounted over > >> + trap umount_TMP_MOUNT SIGINT > >> for m in `echo $FILESYSTEMSRW`; do > >> TMP_MOUNT="$(mktemp -d)" > >> test -z ${TMP_MOUNT+x} && echo "Unable to find temporary directory!" && exit 1 > >> @@ -261,6 +271,7 @@ case "$RESTORE_MODE" in > >> umount "${TMP_MOUNT}${m}" || exit 1 > >> rm -rf "${TMP_MOUNT}" || echo "Error cleaning up." > >> done; > >> + trap SIGINT > >> fi > >> else > >> echo >&2 "fixfiles: No suitable file systems found" > >> -- > >> 2.37.3 > >> > > > > And what if the fixfiles process is terminated via SIGTERM or even > > SIGKILL? > > Good point. > > > Or a power failure occurs at the wrong time? > > After power failure and reboot there's no bind mountpoints from fixfiles > left. Indeed, sorry for the brainfart. > > What if some > > other process leaves behind a bind mount / other mount in /tmp? > > I don't know. But it's up to users to decide. If they are not sure, they > should answer 'No' to /tmp clean up. I disagree. We shouldn't offer the user an easy way of shooting themselves in the foot for no good reason. > > > > My suggestion would be to: > > 1) Change the trap from SIGINT to EXIT. That will cover both SIGTERM and SIGINT. > > I'll send updated patch with this. > > > 2) Additionally modify fullrelabel() to not traverse across mounts > > when doing the removing (+ possibly exit with an error whenever a > > mountpoint is encountered - OR - try to unmount the mounts instead of > > removing their contents). > > Given that I don't know what was the original motivation for the cleaing > code and users are asked whether they want to clean up /tmp and they are > warned that they would need to reboot after this operation, I'm not sure > how I would defend the change. The reboot will not fix the deletion of all files caused by a leftover mount. If you don't want to harden the removal, then at least please add a big warning to the prompt that the operation will also attempt to delete files inside mounts mounted under /tmp. (But that's pretty silly, when you could instead just add -xdev and avoid the problem altogether...) I would say the motivation is simply to force the re-creation of all temporary files in /tmp so that any changes in type transition rules are applied. It is illogical to remove also contents of mounts, as these will likely use different labeling rules (depending on the superblock or in case of a bind mount the original path). There is just no way that deleting across mounts would be anything but a horrible idea in this case. -- Ondrej Mosnacek Senior Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
Ondrej Mosnacek <omosnace@redhat.com> writes: > On Thu, Sep 15, 2022 at 6:29 PM Petr Lautrbach <plautrba@redhat.com> wrote: >> Ondrej Mosnacek <omosnace@redhat.com> writes: >> >> > On Thu, Sep 15, 2022 at 2:45 PM Petr Lautrbach <plautrba@redhat.com> wrote: >> >> `fixfiles -M relabel` temporary bind mounts filestems before relabeling >> >> but it leaves a directory mounted in /tmp/tmp.XXXX when a user hits >> >> CTRL-C. It means that if the user run `fixfiles -M relabel` again and >> >> answered Y to clean out /tmp directory, it would remove all data from >> >> mounted fs. >> >> >> >> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2125355 >> >> >> >> Signed-off-by: Petr Lautrbach <plautrba@redhat.com> >> >> --- >> >> policycoreutils/scripts/fixfiles | 11 +++++++++++ >> >> 1 file changed, 11 insertions(+) >> >> >> >> diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles >> >> index c72ca0eb9d61..6811921970f2 100755 >> >> --- a/policycoreutils/scripts/fixfiles >> >> +++ b/policycoreutils/scripts/fixfiles >> >> @@ -207,6 +207,15 @@ rpm -q --qf '[%{FILESTATES} %{FILENAMES}\n]' "$1" | grep '^0 ' | cut -f2- -d ' ' >> >> [ ${PIPESTATUS[0]} != 0 ] && echo "$1 not found" >/dev/stderr >> >> } >> >> >> >> +# unmount tmp bind mount before exit >> >> +umount_TMP_MOUNT() { >> >> + if [ -n "$TMP_MOUNT" ]; then >> >> + umount "${TMP_MOUNT}${m}" || exit 130 >> >> + rm -rf "${TMP_MOUNT}" || echo "Error cleaning up." >> >> + fi >> >> + exit 130 >> >> +} >> >> + >> >> # >> >> # restore >> >> # if called with -n will only check file context >> >> @@ -251,6 +260,7 @@ case "$RESTORE_MODE" in >> >> else >> >> # we bind mount so we can fix the labels of files that have already been >> >> # mounted over >> >> + trap umount_TMP_MOUNT SIGINT >> >> for m in `echo $FILESYSTEMSRW`; do >> >> TMP_MOUNT="$(mktemp -d)" >> >> test -z ${TMP_MOUNT+x} && echo "Unable to find temporary directory!" && exit 1 >> >> @@ -261,6 +271,7 @@ case "$RESTORE_MODE" in >> >> umount "${TMP_MOUNT}${m}" || exit 1 >> >> rm -rf "${TMP_MOUNT}" || echo "Error cleaning up." >> >> done; >> >> + trap SIGINT >> >> fi >> >> else >> >> echo >&2 "fixfiles: No suitable file systems found" >> >> -- >> >> 2.37.3 >> >> >> > >> > And what if the fixfiles process is terminated via SIGTERM or even >> > SIGKILL? >> >> Good point. >> >> > Or a power failure occurs at the wrong time? >> >> After power failure and reboot there's no bind mountpoints from fixfiles >> left. > > Indeed, sorry for the brainfart. > >> > What if some >> > other process leaves behind a bind mount / other mount in /tmp? >> >> I don't know. But it's up to users to decide. If they are not sure, they >> should answer 'No' to /tmp clean up. > > I disagree. We shouldn't offer the user an easy way of shooting > themselves in the foot for no good reason. > >> >> >> > My suggestion would be to: >> > 1) Change the trap from SIGINT to EXIT. That will cover both SIGTERM and SIGINT. >> >> I'll send updated patch with this. >> >> > 2) Additionally modify fullrelabel() to not traverse across mounts >> > when doing the removing (+ possibly exit with an error whenever a >> > mountpoint is encountered - OR - try to unmount the mounts instead of >> > removing their contents). >> >> Given that I don't know what was the original motivation for the cleaing >> code and users are asked whether they want to clean up /tmp and they are >> warned that they would need to reboot after this operation, I'm not sure >> how I would defend the change. > > The reboot will not fix the deletion of all files caused by a leftover > mount. If you don't want to harden the removal, then at least please > add a big warning to the prompt that the operation will also attempt > to delete files inside mounts mounted under /tmp. (But that's pretty > silly, when you could instead just add -xdev and avoid the problem > altogether...) There's prompt which says: "this command can remove all files in /tmp" so users are already warned. > I would say the motivation is simply to force the re-creation of all > temporary files in /tmp so that any changes in type transition rules > are applied. It is illogical to remove also contents of mounts, as > these will likely use different labeling rules (depending on the > superblock or in case of a bind mount the original path). There is > just no way that deleting across mounts would be anything but a > horrible idea in this case. > My problem is that when users run fixfiles twice, it could remove files from / Your problem is that when users use /tmp as a place for mountpoints, running fixfiles (even once) could remove data from their mount points inside /tmp. They are just different issues. Petr
diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles index c72ca0eb9d61..6811921970f2 100755 --- a/policycoreutils/scripts/fixfiles +++ b/policycoreutils/scripts/fixfiles @@ -207,6 +207,15 @@ rpm -q --qf '[%{FILESTATES} %{FILENAMES}\n]' "$1" | grep '^0 ' | cut -f2- -d ' ' [ ${PIPESTATUS[0]} != 0 ] && echo "$1 not found" >/dev/stderr } +# unmount tmp bind mount before exit +umount_TMP_MOUNT() { + if [ -n "$TMP_MOUNT" ]; then + umount "${TMP_MOUNT}${m}" || exit 130 + rm -rf "${TMP_MOUNT}" || echo "Error cleaning up." + fi + exit 130 +} + # # restore # if called with -n will only check file context @@ -251,6 +260,7 @@ case "$RESTORE_MODE" in else # we bind mount so we can fix the labels of files that have already been # mounted over + trap umount_TMP_MOUNT SIGINT for m in `echo $FILESYSTEMSRW`; do TMP_MOUNT="$(mktemp -d)" test -z ${TMP_MOUNT+x} && echo "Unable to find temporary directory!" && exit 1 @@ -261,6 +271,7 @@ case "$RESTORE_MODE" in umount "${TMP_MOUNT}${m}" || exit 1 rm -rf "${TMP_MOUNT}" || echo "Error cleaning up." done; + trap SIGINT fi else echo >&2 "fixfiles: No suitable file systems found"
`fixfiles -M relabel` temporary bind mounts filestems before relabeling but it leaves a directory mounted in /tmp/tmp.XXXX when a user hits CTRL-C. It means that if the user run `fixfiles -M relabel` again and answered Y to clean out /tmp directory, it would remove all data from mounted fs. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2125355 Signed-off-by: Petr Lautrbach <plautrba@redhat.com> --- policycoreutils/scripts/fixfiles | 11 +++++++++++ 1 file changed, 11 insertions(+)