Merge pull request #2512 from Pennyzct/FC_mount_noexec

Jailer: checking whether chrootBasedir is mounted `noexec`

virtcontainers/container.go

@@ -481,7 +481,7 @@ func (c *Container) shareFiles(m Mount, idx int, hostSharedDir, guestSharedDir s
 	} else {
 		// These mounts are created in the shared dir
 		mountDest := filepath.Join(hostSharedDir, c.sandbox.id, filename)
-		if err := bindMount(c.ctx, m.Source, mountDest, false); err != nil {
+		if err := bindMount(c.ctx, m.Source, mountDest, false, "private"); err != nil {
 			return "", false, err
 		}
 		// Save HostPath mount value into the mount list of the container.

virtcontainers/container_test.go

@@ -174,15 +174,15 @@ func TestUnmountHostMountsRemoveBindHostPath(t *testing.T) {
 			ctx:    context.Background(),
 		}
 
-		if err := bindMount(c.ctx, src, hostPath, false); err != nil {
+		if err := bindMount(c.ctx, src, hostPath, false, "private"); err != nil {
 			t.Fatal(err)
 		}
 		defer syscall.Unmount(hostPath, 0)
-		if err := bindMount(c.ctx, src, nonEmptyHostpath, false); err != nil {
+		if err := bindMount(c.ctx, src, nonEmptyHostpath, false, "private"); err != nil {
 			t.Fatal(err)
 		}
 		defer syscall.Unmount(nonEmptyHostpath, 0)
-		if err := bindMount(c.ctx, src, devPath, false); err != nil {
+		if err := bindMount(c.ctx, src, devPath, false, "private"); err != nil {
 			t.Fatal(err)
 		}
 		defer syscall.Unmount(devPath, 0)

virtcontainers/fc.go

@@ -180,46 +180,6 @@ func (fc *firecracker) trace(name string) (opentracing.Span, context.Context) {
 	return span, ctx
 }
 
-// bindMount bind mounts a source in to a destination. This will
-// do some bookkeeping:
-// * evaluate all symlinks
-// * ensure the source exists
-// * recursively create the destination
-func (fc *firecracker) bindMount(ctx context.Context, source, destination string, readonly bool) error {
-	span, _ := trace(ctx, "bindMount")
-	defer span.Finish()
-
-	if source == "" {
-		return fmt.Errorf("source must be specified")
-	}
-	if destination == "" {
-		return fmt.Errorf("destination must be specified")
-	}
-
-	absSource, err := filepath.EvalSymlinks(source)
-	if err != nil {
-		return fmt.Errorf("Could not resolve symlink for source %v", source)
-	}
-
-	if err := ensureDestinationExists(absSource, destination); err != nil {
-		return fmt.Errorf("Could not create destination mount point %v: %v", destination, err)
-	}
-
-	fc.Logger().WithFields(logrus.Fields{"src": absSource, "dst": destination}).Debug("Bind mounting resource")
-
-	if err := syscall.Mount(absSource, destination, "bind", syscall.MS_BIND|syscall.MS_SLAVE, ""); err != nil {
-		return fmt.Errorf("Could not bind mount %v to %v: %v", absSource, destination, err)
-	}
-
-	// For readonly bind mounts, we need to remount with the readonly flag.
-	// This is needed as only very recent versions of libmount/util-linux support "bind,ro"
-	if readonly {
-		return syscall.Mount(absSource, destination, "bind", uintptr(syscall.MS_BIND|syscall.MS_SLAVE|syscall.MS_REMOUNT|syscall.MS_RDONLY), "")
-	}
-
-	return nil
-}
-
 // For firecracker this call only sets the internal structure up.
 // The sandbox will be created and started through startSandbox().
 func (fc *firecracker) createSandbox(ctx context.Context, id string, networkNS NetworkNamespace, hypervisorConfig *HypervisorConfig, stateful bool) error {
@@ -371,19 +331,7 @@ func (fc *firecracker) fcInit(timeout int) error {
 	span, _ := fc.trace("fcInit")
 	defer span.Finish()
 
-	// Fetch sandbox network to be able to access it from the sandbox structure.
-	err := os.MkdirAll(fc.jailerRoot, DirMode)
-	if err != nil {
-		return err
-	}
-	defer func() {
-		if err != nil {
-			if err := os.RemoveAll(fc.vmPath); err != nil {
-				fc.Logger().WithError(err).Error("Fail to clean up vm directory")
-			}
-		}
-	}()
-
+	var err error
 	//FC version set and check
 	if fc.info.Version, err = fc.getVersionNumber(); err != nil {
 		return err
@@ -537,13 +485,31 @@ func (fc *firecracker) createJailedDrive(name string) (string, error) {
 	return r, nil
 }
 
+// when running with jailer, firecracker binary will firstly be copied into fc.jailerRoot,
+// and then being executed there. Therefore we need to ensure fc.JailerRoot has exec permissions.
+func (fc *firecracker) fcRemountJailerRootWithExec() error {
+	if err := bindMount(context.Background(), fc.jailerRoot, fc.jailerRoot, false, "shared"); err != nil {
+		fc.Logger().WithField("JailerRoot", fc.jailerRoot).Errorf("bindMount failed: %v", err)
+		return err
+	}
+
+	// /run is normally mounted with rw, nosuid(MS_NOSUID), relatime(MS_RELATIME), noexec(MS_NOEXEC).
+	// we re-mount jailerRoot to deliberately leave out MS_NOEXEC.
+	if err := remount(context.Background(), syscall.MS_NOSUID|syscall.MS_RELATIME, fc.jailerRoot); err != nil {
+		fc.Logger().WithField("JailerRoot", fc.jailerRoot).Errorf("Re-mount failed: %v", err)
+		return err
+	}
+
+	return nil
+}
+
 func (fc *firecracker) fcJailResource(src, dst string) (string, error) {
 	if src == "" || dst == "" {
 		return "", fmt.Errorf("fcJailResource: invalid jail locations: src:%v, dst:%v",
 			src, dst)
 	}
 	jailedLocation := filepath.Join(fc.jailerRoot, dst)
-	if err := fc.bindMount(context.Background(), src, jailedLocation, false); err != nil {
+	if err := bindMount(context.Background(), src, jailedLocation, false, "slave"); err != nil {
 		fc.Logger().WithField("bindMount failed", err).Error()
 		return "", err
 	}
@@ -685,8 +651,23 @@ func (fc *firecracker) fcListenToFifo(fifoName string) (string, error) {
 }
 
 func (fc *firecracker) fcInitConfiguration() error {
+	err := os.MkdirAll(fc.jailerRoot, DirMode)
+	if err != nil {
+		return err
+	}
+	defer func() {
+		if err != nil {
+			if err := os.RemoveAll(fc.vmPath); err != nil {
+				fc.Logger().WithError(err).Error("Fail to clean up vm directory")
+			}
+		}
+	}()
+
 	if fc.config.JailerPath != "" {
 		fc.jailed = true
+		if err := fc.fcRemountJailerRootWithExec(); err != nil {
+			return nil
+		}
 	}
 
 	fc.fcSetVMBaseConfig(int64(fc.config.MemorySize),
@@ -841,6 +822,12 @@ func (fc *firecracker) cleanupJail() {
 	fc.umountResource(fcLogFifo)
 	fc.umountResource(fcMetricsFifo)
 	fc.umountResource(defaultFcConfig)
+	// if running with jailer, we also need to umount fc.jailerRoot
+	if fc.config.JailerPath != "" {
+		if err := syscall.Unmount(fc.jailerRoot, syscall.MNT_DETACH); err != nil {
+			fc.Logger().WithField("JailerRoot", fc.jailerRoot).WithError(err).Error("Failed to umount")
+		}
+	}
 
 	fc.Logger().WithField("cleaningJail", fc.vmPath).Info()
 	if err := os.RemoveAll(fc.vmPath); err != nil {

virtcontainers/mount.go

@@ -31,6 +31,13 @@ var rootfsDir = "rootfs"
 
 var systemMountPrefixes = []string{"/proc", "/sys"}
 
+var propagationTypes = map[string]uintptr{
+	"shared":  syscall.MS_SHARED,
+	"private": syscall.MS_PRIVATE,
+	"slave":   syscall.MS_SLAVE,
+	"ubind":   syscall.MS_UNBINDABLE,
+}
+
 func isSystemMount(m string) bool {
 	for _, p := range systemMountPrefixes {
 		if m == p || strings.HasPrefix(m, p+"/") {
@@ -260,7 +267,8 @@ const mountPerm = os.FileMode(0755)
 // * evaluate all symlinks
 // * ensure the source exists
 // * recursively create the destination
-func bindMount(ctx context.Context, source, destination string, readonly bool) error {
+// pgtypes stands for propagation types, which are shared, private, slave, and ubind.
+func bindMount(ctx context.Context, source, destination string, readonly bool, pgtypes string) error {
 	span, _ := trace(ctx, "bindMount")
 	defer span.Finish()
 
@@ -284,8 +292,12 @@ func bindMount(ctx context.Context, source, destination string, readonly bool) e
 		return fmt.Errorf("Could not bind mount %v to %v: %v", absSource, destination, err)
 	}
 
-	if err := syscall.Mount("none", destination, "", syscall.MS_PRIVATE, ""); err != nil {
-		return fmt.Errorf("Could not make mount point %v private: %v", destination, err)
+	if pgtype, exist := propagationTypes[pgtypes]; exist {
+		if err := syscall.Mount("none", destination, "", pgtype, ""); err != nil {
+			return fmt.Errorf("Could not make mount point %v %s: %v", destination, pgtypes, err)
+		}
+	} else {
+		return fmt.Errorf("Wrong propagation type %s", pgtypes)
 	}
 
 	// For readonly bind mounts, we need to remount with the readonly flag.
@@ -297,6 +309,24 @@ func bindMount(ctx context.Context, source, destination string, readonly bool) e
 	return nil
 }
 
+// An existing mount may be remounted by specifying `MS_REMOUNT` in
+// mountflags.
+// This allows you to change the mountflags of an existing mount.
+// The mountflags should match the values used in the original mount() call,
+// except for those parameters that you are trying to change.
+func remount(ctx context.Context, mountflags uintptr, src string) error {
+	absSrc, err := filepath.EvalSymlinks(src)
+	if err != nil {
+		return fmt.Errorf("Could not resolve symlink for %s", src)
+	}
+
+	if err := syscall.Mount(absSrc, absSrc, "", syscall.MS_REMOUNT|mountflags, ""); err != nil {
+		return fmt.Errorf("remount %s failed: %v", absSrc, err)
+	}
+
+	return nil
+}
+
 // bindMountContainerRootfs bind mounts a container rootfs into a 9pfs shared
 // directory between the guest and the host.
 func bindMountContainerRootfs(ctx context.Context, sharedDir, sandboxID, cID, cRootFs string, readonly bool) error {
@@ -305,7 +335,7 @@ func bindMountContainerRootfs(ctx context.Context, sharedDir, sandboxID, cID, cR
 
 	rootfsDest := filepath.Join(sharedDir, sandboxID, cID, rootfsDir)
 
-	return bindMount(ctx, cRootFs, rootfsDest, readonly)
+	return bindMount(ctx, cRootFs, rootfsDest, readonly, "private")
 }
 
 // Mount describes a container mount.

virtcontainers/mount_test.go

@@ -189,7 +189,7 @@ func TestGetDeviceForPathBindMount(t *testing.T) {
 
 	defer os.Remove(dest)
 
-	err = bindMount(context.Background(), source, dest, false)
+	err = bindMount(context.Background(), source, dest, false, "private")
 	assert.NoError(err)
 
 	defer syscall.Unmount(dest, syscall.MNT_DETACH)
@@ -283,6 +283,107 @@ func TestIsEphemeralStorage(t *testing.T) {
 	assert.False(isHostEmptyDir)
 }
 
+func TestBindMountInvalidSourceSymlink(t *testing.T) {
+	source := filepath.Join(testDir, "fooFile")
+	os.Remove(source)
+
+	err := bindMount(context.Background(), source, "", false, "private")
+	assert.Error(t, err)
+}
+
+func TestBindMountFailingMount(t *testing.T) {
+	source := filepath.Join(testDir, "fooLink")
+	fakeSource := filepath.Join(testDir, "fooFile")
+	os.Remove(source)
+	os.Remove(fakeSource)
+	assert := assert.New(t)
+
+	_, err := os.OpenFile(fakeSource, os.O_CREATE, mountPerm)
+	assert.NoError(err)
+
+	err = os.Symlink(fakeSource, source)
+	assert.NoError(err)
+
+	err = bindMount(context.Background(), source, "", false, "private")
+	assert.Error(err)
+}
+
+func TestBindMountSuccessful(t *testing.T) {
+	assert := assert.New(t)
+	if tc.NotValid(ktu.NeedRoot()) {
+		t.Skip(testDisabledAsNonRoot)
+	}
+
+	source := filepath.Join(testDir, "fooDirSrc")
+	dest := filepath.Join(testDir, "fooDirDest")
+	syscall.Unmount(dest, 0)
+	os.Remove(source)
+	os.Remove(dest)
+
+	err := os.MkdirAll(source, mountPerm)
+	assert.NoError(err)
+
+	err = os.MkdirAll(dest, mountPerm)
+	assert.NoError(err)
+
+	err = bindMount(context.Background(), source, dest, false, "private")
+	assert.NoError(err)
+
+	syscall.Unmount(dest, 0)
+}
+
+func TestBindMountReadonlySuccessful(t *testing.T) {
+	assert := assert.New(t)
+	if tc.NotValid(ktu.NeedRoot()) {
+		t.Skip(testDisabledAsNonRoot)
+	}
+
+	source := filepath.Join(testDir, "fooDirSrc")
+	dest := filepath.Join(testDir, "fooDirDest")
+	syscall.Unmount(dest, 0)
+	os.Remove(source)
+	os.Remove(dest)
+
+	err := os.MkdirAll(source, mountPerm)
+	assert.NoError(err)
+
+	err = os.MkdirAll(dest, mountPerm)
+	assert.NoError(err)
+
+	err = bindMount(context.Background(), source, dest, true, "private")
+	assert.NoError(err)
+
+	defer syscall.Unmount(dest, 0)
+
+	// should not be able to create file in read-only mount
+	destFile := filepath.Join(dest, "foo")
+	_, err = os.OpenFile(destFile, os.O_CREATE, mountPerm)
+	assert.Error(err)
+}
+
+func TestBindMountInvalidPgtypes(t *testing.T) {
+	assert := assert.New(t)
+	if tc.NotValid(ktu.NeedRoot()) {
+		t.Skip(testDisabledAsNonRoot)
+	}
+
+	source := filepath.Join(testDir, "fooDirSrc")
+	dest := filepath.Join(testDir, "fooDirDest")
+	syscall.Unmount(dest, 0)
+	os.Remove(source)
+	os.Remove(dest)
+
+	err := os.MkdirAll(source, mountPerm)
+	assert.NoError(err)
+
+	err = os.MkdirAll(dest, mountPerm)
+	assert.NoError(err)
+
+	err = bindMount(context.Background(), source, dest, false, "foo")
+	expectedErr := fmt.Sprintf("Wrong propagation type %s", "foo")
+	assert.EqualError(err, expectedErr)
+}
+
 // TestBindUnmountContainerRootfsENOENTNotError tests that if a file
 // or directory attempting to be unmounted doesn't exist, then it
 // is not considered an error

virtcontainers/syscall_test.go

@@ -7,94 +7,13 @@
 package virtcontainers
 
 import (
-	"context"
 	"os"
 	"path/filepath"
-	"syscall"
 	"testing"
 
-	ktu "github.com/kata-containers/runtime/pkg/katatestutils"
 	"github.com/stretchr/testify/assert"
 )
 
-func TestBindMountInvalidSourceSymlink(t *testing.T) {
-	source := filepath.Join(testDir, "fooFile")
-	os.Remove(source)
-
-	err := bindMount(context.Background(), source, "", false)
-	assert.Error(t, err)
-}
-
-func TestBindMountFailingMount(t *testing.T) {
-	source := filepath.Join(testDir, "fooLink")
-	fakeSource := filepath.Join(testDir, "fooFile")
-	os.Remove(source)
-	os.Remove(fakeSource)
-	assert := assert.New(t)
-
-	_, err := os.OpenFile(fakeSource, os.O_CREATE, mountPerm)
-	assert.NoError(err)
-
-	err = os.Symlink(fakeSource, source)
-	assert.NoError(err)
-
-	err = bindMount(context.Background(), source, "", false)
-	assert.Error(err)
-}
-
-func TestBindMountSuccessful(t *testing.T) {
-	assert := assert.New(t)
-	if tc.NotValid(ktu.NeedRoot()) {
-		t.Skip(testDisabledAsNonRoot)
-	}
-
-	source := filepath.Join(testDir, "fooDirSrc")
-	dest := filepath.Join(testDir, "fooDirDest")
-	syscall.Unmount(dest, 0)
-	os.Remove(source)
-	os.Remove(dest)
-
-	err := os.MkdirAll(source, mountPerm)
-	assert.NoError(err)
-
-	err = os.MkdirAll(dest, mountPerm)
-	assert.NoError(err)
-
-	err = bindMount(context.Background(), source, dest, false)
-	assert.NoError(err)
-
-	syscall.Unmount(dest, 0)
-}
-
-func TestBindMountReadonlySuccessful(t *testing.T) {
-	assert := assert.New(t)
-	if tc.NotValid(ktu.NeedRoot()) {
-		t.Skip(testDisabledAsNonRoot)
-	}
-
-	source := filepath.Join(testDir, "fooDirSrc")
-	dest := filepath.Join(testDir, "fooDirDest")
-	syscall.Unmount(dest, 0)
-	os.Remove(source)
-	os.Remove(dest)
-
-	err := os.MkdirAll(source, mountPerm)
-	assert.NoError(err)
-
-	err = os.MkdirAll(dest, mountPerm)
-	assert.NoError(err)
-
-	err = bindMount(context.Background(), source, dest, true)
-	assert.NoError(err)
-
-	defer syscall.Unmount(dest, 0)
-
-	// should not be able to create file in read-only mount
-	destFile := filepath.Join(dest, "foo")
-	_, err = os.OpenFile(destFile, os.O_CREATE, mountPerm)
-	assert.Error(err)
-}
-
 func TestEnsureDestinationExistsNonExistingSource(t *testing.T) {
 	err := ensureDestinationExists("", "")
 	assert.Error(t, err)
@@ -107,8 +26,9 @@ func TestEnsureDestinationExistsWrongParentDir(t *testing.T) {
 	os.Remove(dest)
 	assert := assert.New(t)
 
-	_, err := os.OpenFile(source, os.O_CREATE, mountPerm)
+	file, err := os.OpenFile(source, os.O_CREATE, mountPerm)
 	assert.NoError(err)
+	defer file.Close()
 
 	err = ensureDestinationExists(source, dest)
 	assert.Error(err)
@@ -123,20 +43,22 @@ func TestEnsureDestinationExistsSuccessfulSrcDir(t *testing.T) {
 
 	err := os.MkdirAll(source, mountPerm)
 	assert.NoError(err)
+	defer os.Remove(source)
 
 	err = ensureDestinationExists(source, dest)
 	assert.NoError(err)
 }
 
 func TestEnsureDestinationExistsSuccessfulSrcFile(t *testing.T) {
-	source := filepath.Join(testDir, "fooDirSrc")
-	dest := filepath.Join(testDir, "fooDirDest")
+	source := filepath.Join(testDir, "fooFileSrc")
+	dest := filepath.Join(testDir, "fooFileDest")
 	os.Remove(source)
 	os.Remove(dest)
 	assert := assert.New(t)
 
-	_, err := os.OpenFile(source, os.O_CREATE, mountPerm)
+	file, err := os.OpenFile(source, os.O_CREATE, mountPerm)
 	assert.NoError(err)
+	defer file.Close()
 
 	err = ensureDestinationExists(source, dest)
 	assert.NoError(err)