Re: [PATCH V2] runc: fix CVE-2019-16884

[Bruce Ashfield <bruce.ashfield@gmail.com>]

sorry for the delay, this is now merged.

I'll have versions bumps for the runc recipes completed soon, but
there's no reason to not merge this while I finish that effort.

Cheers,

Bruce

In message: [meta-virtualization] [PATCH V2] runc: fix CVE-2019-16884
on 06/11/2019 Chen Qi wrote:

> Signed-off-by: Chen Qi <Qi.Chen@windriver.com>
> ---
>  ...nly-allow-proc-mount-if-it-is-procfs.patch | 201 ++++++++++++++++++
>  recipes-containers/runc/runc-docker_git.bb    |   1 +
>  .../runc/runc-opencontainers_git.bb           |   1 +
>  3 files changed, 203 insertions(+)
>  create mode 100644 recipes-containers/runc/files/0001-Only-allow-proc-mount-if-it-is-procfs.patch
> 
> diff --git a/recipes-containers/runc/files/0001-Only-allow-proc-mount-if-it-is-procfs.patch b/recipes-containers/runc/files/0001-Only-allow-proc-mount-if-it-is-procfs.patch
> new file mode 100644
> index 0000000..5aca99e
> --- /dev/null
> +++ b/recipes-containers/runc/files/0001-Only-allow-proc-mount-if-it-is-procfs.patch
> @@ -0,0 +1,201 @@
> +From d75b05441772417a0828465a9483f16287937724 Mon Sep 17 00:00:00 2001
> +From: Michael Crosby <crosbymichael@gmail.com>
> +Date: Mon, 23 Sep 2019 16:45:45 -0400
> +Subject: [PATCH] Only allow proc mount if it is procfs
> +
> +Fixes #2128
> +
> +This allows proc to be bind mounted for host and rootless namespace usecases but
> +it removes the ability to mount over the top of proc with a directory.
> +
> +```bash
> +> sudo docker run --rm  apparmor
> +docker: Error response from daemon: OCI runtime create failed:
> +container_linux.go:346: starting container process caused "process_linux.go:449:
> +container init caused \"rootfs_linux.go:58: mounting
> +\\\"/var/lib/docker/volumes/aae28ea068c33d60e64d1a75916cf3ec2dc3634f97571854c9ed30c8401460c1/_data\\\"
> +to rootfs
> +\\\"/var/lib/docker/overlay2/a6be5ae911bf19f8eecb23a295dec85be9a8ee8da66e9fb55b47c841d1e381b7/merged\\\"
> +at \\\"/proc\\\" caused
> +\\\"\\\\\\\"/var/lib/docker/overlay2/a6be5ae911bf19f8eecb23a295dec85be9a8ee8da66e9fb55b47c841d1e381b7/merged/proc\\\\\\\"
> +cannot be mounted because it is not of type proc\\\"\"": unknown.
> +
> +> sudo docker run --rm -v /proc:/proc apparmor
> +
> +docker-default (enforce)        root     18989  0.9  0.0   1288     4 ?
> +Ss   16:47   0:00 sleep 20
> +```
> +
> +Signed-off-by: Michael Crosby <crosbymichael@gmail.com>
> +
> +Upstream-Status: Backport [https://github.com/opencontainers/runc/pull/2129/commits/331692baa7afdf6c186f8667cb0e6362ea0802b3]
> +
> +CVE: CVE-2019-16884
> +
> +Signed-off-by: Chen Qi <Qi.Chen@windriver.com>
> +---
> + libcontainer/container_linux.go   |  4 +--
> + libcontainer/rootfs_linux.go      | 50 +++++++++++++++++++++++--------
> + libcontainer/rootfs_linux_test.go |  8 ++---
> + 3 files changed, 43 insertions(+), 19 deletions(-)
> +
> +diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go
> +index 7e58e5e0..d51e35df 100644
> +--- a/src/import/libcontainer/container_linux.go
> ++++ b/src/import/libcontainer/container_linux.go
> +@@ -19,7 +19,7 @@ import (
> + 	"syscall" // only for SysProcAttr and Signal
> + 	"time"
> + 
> +-	"github.com/cyphar/filepath-securejoin"
> ++	securejoin "github.com/cyphar/filepath-securejoin"
> + 	"github.com/opencontainers/runc/libcontainer/cgroups"
> + 	"github.com/opencontainers/runc/libcontainer/configs"
> + 	"github.com/opencontainers/runc/libcontainer/intelrdt"
> +@@ -1160,7 +1160,7 @@ func (c *linuxContainer) makeCriuRestoreMountpoints(m *configs.Mount) error {
> + 		if err != nil {
> + 			return err
> + 		}
> +-		if err := checkMountDestination(c.config.Rootfs, dest); err != nil {
> ++		if err := checkProcMount(c.config.Rootfs, dest, ""); err != nil {
> + 			return err
> + 		}
> + 		m.Destination = dest
> +diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go
> +index f13b226e..5650b0ac 100644
> +--- a/src/import/libcontainer/rootfs_linux.go
> ++++ b/src/import/libcontainer/rootfs_linux.go
> +@@ -13,7 +13,7 @@ import (
> + 	"strings"
> + 	"time"
> + 
> +-	"github.com/cyphar/filepath-securejoin"
> ++	securejoin "github.com/cyphar/filepath-securejoin"
> + 	"github.com/mrunalp/fileutils"
> + 	"github.com/opencontainers/runc/libcontainer/cgroups"
> + 	"github.com/opencontainers/runc/libcontainer/configs"
> +@@ -197,7 +197,7 @@ func prepareBindMount(m *configs.Mount, rootfs string) error {
> + 	if dest, err = securejoin.SecureJoin(rootfs, m.Destination); err != nil {
> + 		return err
> + 	}
> +-	if err := checkMountDestination(rootfs, dest); err != nil {
> ++	if err := checkProcMount(rootfs, dest, m.Source); err != nil {
> + 		return err
> + 	}
> + 	// update the mount with the correct dest after symlinks are resolved.
> +@@ -388,7 +388,7 @@ func mountToRootfs(m *configs.Mount, rootfs, mountLabel string, enableCgroupns b
> + 		if dest, err = securejoin.SecureJoin(rootfs, m.Destination); err != nil {
> + 			return err
> + 		}
> +-		if err := checkMountDestination(rootfs, dest); err != nil {
> ++		if err := checkProcMount(rootfs, dest, m.Source); err != nil {
> + 			return err
> + 		}
> + 		// update the mount with the correct dest after symlinks are resolved.
> +@@ -435,12 +435,12 @@ func getCgroupMounts(m *configs.Mount) ([]*configs.Mount, error) {
> + 	return binds, nil
> + }
> + 
> +-// checkMountDestination checks to ensure that the mount destination is not over the top of /proc.
> ++// checkProcMount checks to ensure that the mount destination is not over the top of /proc.
> + // dest is required to be an abs path and have any symlinks resolved before calling this function.
> +-func checkMountDestination(rootfs, dest string) error {
> +-	invalidDestinations := []string{
> +-		"/proc",
> +-	}
> ++//
> ++// if source is nil, don't stat the filesystem.  This is used for restore of a checkpoint.
> ++func checkProcMount(rootfs, dest, source string) error {
> ++	const procPath = "/proc"
> + 	// White list, it should be sub directories of invalid destinations
> + 	validDestinations := []string{
> + 		// These entries can be bind mounted by files emulated by fuse,
> +@@ -463,16 +463,40 @@ func checkMountDestination(rootfs, dest string) error {
> + 			return nil
> + 		}
> + 	}
> +-	for _, invalid := range invalidDestinations {
> +-		path, err := filepath.Rel(filepath.Join(rootfs, invalid), dest)
> ++	path, err := filepath.Rel(filepath.Join(rootfs, procPath), dest)
> ++	if err != nil {
> ++		return err
> ++	}
> ++	// pass if the mount path is located outside of /proc
> ++	if strings.HasPrefix(path, "..") {
> ++		return nil
> ++	}
> ++	if path == "." {
> ++		// an empty source is pasted on restore
> ++		if source == "" {
> ++			return nil
> ++		}
> ++		// only allow a mount on-top of proc if it's source is "proc"
> ++		isproc, err := isProc(source)
> + 		if err != nil {
> + 			return err
> + 		}
> +-		if path != "." && !strings.HasPrefix(path, "..") {
> +-			return fmt.Errorf("%q cannot be mounted because it is located inside %q", dest, invalid)
> ++		// pass if the mount is happening on top of /proc and the source of
> ++		// the mount is a proc filesystem
> ++		if isproc {
> ++			return nil
> + 		}
> ++		return fmt.Errorf("%q cannot be mounted because it is not of type proc", dest)
> + 	}
> +-	return nil
> ++	return fmt.Errorf("%q cannot be mounted because it is inside /proc", dest)
> ++}
> ++
> ++func isProc(path string) (bool, error) {
> ++	var s unix.Statfs_t
> ++	if err := unix.Statfs(path, &s); err != nil {
> ++		return false, err
> ++	}
> ++	return s.Type == unix.PROC_SUPER_MAGIC, nil
> + }
> + 
> + func setupDevSymlinks(rootfs string) error {
> +diff --git a/libcontainer/rootfs_linux_test.go b/libcontainer/rootfs_linux_test.go
> +index d755984b..1bfe7c66 100644
> +--- a/src/import/libcontainer/rootfs_linux_test.go
> ++++ b/src/import/libcontainer/rootfs_linux_test.go
> +@@ -10,7 +10,7 @@ import (
> + 
> + func TestCheckMountDestOnProc(t *testing.T) {
> + 	dest := "/rootfs/proc/sys"
> +-	err := checkMountDestination("/rootfs", dest)
> ++	err := checkProcMount("/rootfs", dest, "")
> + 	if err == nil {
> + 		t.Fatal("destination inside proc should return an error")
> + 	}
> +@@ -18,7 +18,7 @@ func TestCheckMountDestOnProc(t *testing.T) {
> + 
> + func TestCheckMountDestOnProcChroot(t *testing.T) {
> + 	dest := "/rootfs/proc/"
> +-	err := checkMountDestination("/rootfs", dest)
> ++	err := checkProcMount("/rootfs", dest, "/proc")
> + 	if err != nil {
> + 		t.Fatal("destination inside proc when using chroot should not return an error")
> + 	}
> +@@ -26,7 +26,7 @@ func TestCheckMountDestOnProcChroot(t *testing.T) {
> + 
> + func TestCheckMountDestInSys(t *testing.T) {
> + 	dest := "/rootfs//sys/fs/cgroup"
> +-	err := checkMountDestination("/rootfs", dest)
> ++	err := checkProcMount("/rootfs", dest, "")
> + 	if err != nil {
> + 		t.Fatal("destination inside /sys should not return an error")
> + 	}
> +@@ -34,7 +34,7 @@ func TestCheckMountDestInSys(t *testing.T) {
> + 
> + func TestCheckMountDestFalsePositive(t *testing.T) {
> + 	dest := "/rootfs/sysfiles/fs/cgroup"
> +-	err := checkMountDestination("/rootfs", dest)
> ++	err := checkProcMount("/rootfs", dest, "")
> + 	if err != nil {
> + 		t.Fatal(err)
> + 	}
> +-- 
> +2.17.1
> +
> diff --git a/recipes-containers/runc/runc-docker_git.bb b/recipes-containers/runc/runc-docker_git.bb
> index c9f460b..8d810d0 100644
> --- a/recipes-containers/runc/runc-docker_git.bb
> +++ b/recipes-containers/runc/runc-docker_git.bb
> @@ -7,6 +7,7 @@ SRC_URI = "git://github.com/opencontainers/runc;nobranch=1;name=runc-docker \
>             file://0001-runc-Add-console-socket-dev-null.patch \
>             file://0001-Makefile-respect-GOBUILDFLAGS-for-runc-and-remove-re.patch \
>             file://0001-runc-docker-SIGUSR1-daemonize.patch \
> +           file://0001-Only-allow-proc-mount-if-it-is-procfs.patch \
>            "
>  
>  RUNC_VERSION = "1.0.0-rc8"
> diff --git a/recipes-containers/runc/runc-opencontainers_git.bb b/recipes-containers/runc/runc-opencontainers_git.bb
> index 361bc94..3a7e7aa 100644
> --- a/recipes-containers/runc/runc-opencontainers_git.bb
> +++ b/recipes-containers/runc/runc-opencontainers_git.bb
> @@ -4,5 +4,6 @@ SRCREV = "652297c7c7e6c94e8d064ad5916c32891a6fd388"
>  SRC_URI = " \
>      git://github.com/opencontainers/runc;branch=master \
>      file://0001-Makefile-respect-GOBUILDFLAGS-for-runc-and-remove-re.patch \
> +    file://0001-Only-allow-proc-mount-if-it-is-procfs.patch \
>      "
>  RUNC_VERSION = "1.0.0-rc8"
> -- 
> 2.17.1
> 
> -- 
> _______________________________________________
> meta-virtualization mailing list
> meta-virtualization@yoctoproject.org
> https://lists.yoctoproject.org/listinfo/meta-virtualization