From 5bf32312131af572334eaf5a3ef5ccaf9e08a19f Mon Sep 17 00:00:00 2001 From: Peng Tao Date: Wed, 19 Feb 2020 04:46:02 -0800 Subject: [PATCH 1/2] vc: do not follow symlink when umounting contanier host path So that if a guest changes it, we do not end up propergating the error. Fixes: #2474 Signed-off-by: Peng Tao --- virtcontainers/container.go | 2 +- virtcontainers/mount.go | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/virtcontainers/container.go b/virtcontainers/container.go index 2a9f2269ee..def5938ecb 100644 --- a/virtcontainers/container.go +++ b/virtcontainers/container.go @@ -590,7 +590,7 @@ func (c *Container) unmountHostMounts() error { span, _ := c.trace("unmount") span.SetTag("host-path", m.HostPath) - if err := syscall.Unmount(m.HostPath, syscall.MNT_DETACH); err != nil { + if err := syscall.Unmount(m.HostPath, syscall.MNT_DETACH|UmountNoFollow); err != nil { c.Logger().WithFields(logrus.Fields{ "host-path": m.HostPath, "error": err, diff --git a/virtcontainers/mount.go b/virtcontainers/mount.go index 2a77285ece..a8f036f59e 100644 --- a/virtcontainers/mount.go +++ b/virtcontainers/mount.go @@ -24,6 +24,9 @@ import ( // IPC is used. const DefaultShmSize = 65536 * 1024 +// Sadly golang/sys doesn't have UmountNoFollow although it's there since Linux 2.6.34 +const UmountNoFollow = 0x8 + var rootfsDir = "rootfs" var systemMountPrefixes = []string{"/proc", "/sys"} @@ -333,7 +336,7 @@ func bindUnmountContainerRootfs(ctx context.Context, sharedDir, sandboxID, cID s defer span.Finish() rootfsDest := filepath.Join(sharedDir, sandboxID, cID, rootfsDir) - err := syscall.Unmount(rootfsDest, syscall.MNT_DETACH) + err := syscall.Unmount(rootfsDest, syscall.MNT_DETACH|UmountNoFollow) if err == syscall.ENOENT { logrus.Warnf("%s: %s", err, rootfsDest) return nil From 5f9a77cccc82fdfcb3f01423d1fc7b2ec51d9271 Mon Sep 17 00:00:00 2001 From: Peng Tao Date: Wed, 19 Feb 2020 06:40:58 -0800 Subject: [PATCH 2/2] vc: validate container path when cleaning up A malicious can trick us with a crafted container rootfs symlink and make runtime umount other mountpoints. Make sure we do not walk through symlinks when umounting. Signed-off-by: Peng Tao --- virtcontainers/mount.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/virtcontainers/mount.go b/virtcontainers/mount.go index a8f036f59e..9987a9b177 100644 --- a/virtcontainers/mount.go +++ b/virtcontainers/mount.go @@ -331,11 +331,24 @@ type Mount struct { BlockDeviceID string } +func isSymlink(path string) bool { + stat, err := os.Stat(path) + if err != nil { + return false + } + return stat.Mode()&os.ModeSymlink != 0 +} + func bindUnmountContainerRootfs(ctx context.Context, sharedDir, sandboxID, cID string) error { span, _ := trace(ctx, "bindUnmountContainerRootfs") defer span.Finish() rootfsDest := filepath.Join(sharedDir, sandboxID, cID, rootfsDir) + if isSymlink(filepath.Join(sharedDir, sandboxID, cID)) || isSymlink(rootfsDest) { + logrus.Warnf("container dir %s is a symlink, malicious guest?", cID) + return nil + } + err := syscall.Unmount(rootfsDest, syscall.MNT_DETACH|UmountNoFollow) if err == syscall.ENOENT { logrus.Warnf("%s: %s", err, rootfsDest) @@ -350,6 +363,10 @@ func bindUnmountAllRootfs(ctx context.Context, sharedDir string, sandbox *Sandbo var errors *merr.Error for _, c := range sandbox.containers { + if isSymlink(filepath.Join(sharedDir, sandbox.id, c.id)) { + logrus.Warnf("container dir %s is a symlink, malicious guest?", c.id) + continue + } c.unmountHostMounts() if c.state.Fstype == "" { // even if error found, don't break out of loop until all mounts attempted