On Thu, Sep 15, 2022 at 6:29 PM Petr Lautrbach <plautrba@xxxxxxxxxx> wrote: > Ondrej Mosnacek <omosnace@xxxxxxxxxx> writes: > > > On Thu, Sep 15, 2022 at 2:45 PM Petr Lautrbach <plautrba@xxxxxxxxxx> 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@xxxxxxxxxx> > >> --- > >> 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.
