From f8d84ecf92b0954e53115b015e68307664066602 Mon Sep 17 00:00:00 2001 From: Rodrigo Campos Date: Mon, 23 Sep 2024 16:15:10 +0200 Subject: [PATCH] core/mount: Prevent accidental removal of rootfs files Using os.RemoveAll() is quite risky, as if the unmount failed and we can delete files from the container rootfs. In fact, we were doing just that. Let's use os.Remove() to make sure we only deleted empty dirs. Big kudos to @mbaynton for reporting this issue with lot of details, nailing it down to containerd lines of code and showing all the log lines to understand the big picture. Fixes: #10704 Signed-off-by: Rodrigo Campos --- core/mount/mount_linux.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/core/mount/mount_linux.go b/core/mount/mount_linux.go index ec5417ed7..cd3b354cf 100644 --- a/core/mount/mount_linux.go +++ b/core/mount/mount_linux.go @@ -256,9 +256,16 @@ func doPrepareIDMappedOverlay(lowerDirs []string, usernsFd int) (tmpLowerDirs [] if err := unix.Unmount(lowerDir, unix.MNT_DETACH); err != nil { log.L.WithError(err).Warnf("failed to unmount temp lowerdir %s", lowerDir) } + // Using os.Remove() so if it's not empty, we don't delete files in the + // rootfs. + if err := os.Remove(lowerDir); err != nil { + log.L.WithError(err).Warnf("failed to remove temporary overlay lowerdir's") + } } - if terr := os.RemoveAll(filepath.Clean(filepath.Join(tmpLowerDirs[0], ".."))); terr != nil { - log.L.WithError(terr).Warnf("failed to remove temporary overlay lowerdir's") + + // This dir should be empty now. Otherwise, we don't do anything. + if err := os.Remove(filepath.Join(tmpLowerDirs[0], "..")); err != nil { + log.L.WithError(err).Infof("failed to remove temporary overlay dir") } } for i, lowerDir := range lowerDirs {