35e3b4
From d463f6485b809b5ea738f84e05ff5b456058a184 Mon Sep 17 00:00:00 2001
35e3b4
From: Aleksa Sarai <asarai@suse.de>
35e3b4
Date: Fri, 27 Sep 2019 12:01:07 +1000
35e3b4
Subject: [PATCH] *: verify that operations on /proc/... are on procfs
35e3b4
35e3b4
This is an additional mitigation for CVE-2019-16884. The primary problem
35e3b4
is that Docker can be coerced into bind-mounting a file system on top of
35e3b4
/proc (resulting in label-related writes to /proc no longer happening).
35e3b4
35e3b4
While we are working on mitigations against permitting the mounts, this
35e3b4
helps avoid our code from being tricked into writing to non-procfs
35e3b4
files. This is not a perfect solution (after all, there might be a
35e3b4
bind-mount of a different procfs file over the target) but in order to
35e3b4
exploit that you would need to be able to tweak a config.json pretty
35e3b4
specifically (which thankfully Docker doesn't allow).
35e3b4
35e3b4
Specifically this stops AppArmor from not labeling a process silently
35e3b4
due to /proc/self/attr/... being incorrectly set, and stops any
35e3b4
accidental fd leaks because /proc/self/fd/... is not real.
35e3b4
35e3b4
Signed-off-by: Aleksa Sarai <asarai@suse.de>
35e3b4
---
35e3b4
 libcontainer/apparmor/apparmor.go | 10 +++++--
35e3b4
 libcontainer/utils/utils_unix.go  | 44 ++++++++++++++++++++++++-------
35e3b4
 2 files changed, 42 insertions(+), 12 deletions(-)
35e3b4
35e3b4
diff --git a/libcontainer/apparmor/apparmor.go b/libcontainer/apparmor/apparmor.go
35e3b4
index 7fff0627f..debfc1e48 100644
35e3b4
--- a/libcontainer/apparmor/apparmor.go
35e3b4
+++ b/libcontainer/apparmor/apparmor.go
35e3b4
@@ -6,6 +6,8 @@ import (
35e3b4
 	"fmt"
35e3b4
 	"io/ioutil"
35e3b4
 	"os"
35e3b4
+
35e3b4
+	"github.com/opencontainers/runc/libcontainer/utils"
35e3b4
 )
35e3b4
 
35e3b4
 // IsEnabled returns true if apparmor is enabled for the host.
35e3b4
@@ -19,7 +21,7 @@ func IsEnabled() bool {
35e3b4
 	return false
35e3b4
 }
35e3b4
 
35e3b4
-func setprocattr(attr, value string) error {
35e3b4
+func setProcAttr(attr, value string) error {
35e3b4
 	// Under AppArmor you can only change your own attr, so use /proc/self/
35e3b4
 	// instead of /proc/<tid>/ like libapparmor does
35e3b4
 	path := fmt.Sprintf("/proc/self/attr/%s", attr)
35e3b4
@@ -30,6 +32,10 @@ func setprocattr(attr, value string) error {
35e3b4
 	}
35e3b4
 	defer f.Close()
35e3b4
 
35e3b4
+	if err := utils.EnsureProcHandle(f); err != nil {
35e3b4
+		return err
35e3b4
+	}
35e3b4
+
35e3b4
 	_, err = fmt.Fprintf(f, "%s", value)
35e3b4
 	return err
35e3b4
 }
35e3b4
@@ -37,7 +43,7 @@ func setprocattr(attr, value string) error {
35e3b4
 // changeOnExec reimplements aa_change_onexec from libapparmor in Go
35e3b4
 func changeOnExec(name string) error {
35e3b4
 	value := "exec " + name
35e3b4
-	if err := setprocattr("exec", value); err != nil {
35e3b4
+	if err := setProcAttr("exec", value); err != nil {
35e3b4
 		return fmt.Errorf("apparmor failed to apply profile: %s", err)
35e3b4
 	}
35e3b4
 	return nil
35e3b4
diff --git a/libcontainer/utils/utils_unix.go b/libcontainer/utils/utils_unix.go
35e3b4
index c96088988..1576f2d4a 100644
35e3b4
--- a/libcontainer/utils/utils_unix.go
35e3b4
+++ b/libcontainer/utils/utils_unix.go
35e3b4
@@ -3,33 +3,57 @@
35e3b4
 package utils
35e3b4
 
35e3b4
 import (
35e3b4
-	"io/ioutil"
35e3b4
+	"fmt"
35e3b4
 	"os"
35e3b4
 	"strconv"
35e3b4
 
35e3b4
 	"golang.org/x/sys/unix"
35e3b4
 )
35e3b4
 
35e3b4
+// EnsureProcHandle returns whether or not the given file handle is on procfs.
35e3b4
+func EnsureProcHandle(fh *os.File) error {
35e3b4
+	var buf unix.Statfs_t
35e3b4
+	if err := unix.Fstatfs(int(fh.Fd()), &buf); err != nil {
35e3b4
+		return fmt.Errorf("ensure %s is on procfs: %v", fh.Name(), err)
35e3b4
+	}
35e3b4
+	if buf.Type != unix.PROC_SUPER_MAGIC {
35e3b4
+		return fmt.Errorf("%s is not on procfs", fh.Name())
35e3b4
+	}
35e3b4
+	return nil
35e3b4
+}
35e3b4
+
35e3b4
+// CloseExecFrom applies O_CLOEXEC to all file descriptors currently open for
35e3b4
+// the process (except for those below the given fd value).
35e3b4
 func CloseExecFrom(minFd int) error {
35e3b4
-	fdList, err := ioutil.ReadDir("/proc/self/fd")
35e3b4
+	fdDir, err := os.Open("/proc/self/fd")
35e3b4
+	if err != nil {
35e3b4
+		return err
35e3b4
+	}
35e3b4
+	defer fdDir.Close()
35e3b4
+
35e3b4
+	if err := EnsureProcHandle(fdDir); err != nil {
35e3b4
+		return err
35e3b4
+	}
35e3b4
+
35e3b4
+	fdList, err := fdDir.Readdirnames(-1)
35e3b4
 	if err != nil {
35e3b4
 		return err
35e3b4
 	}
35e3b4
-	for _, fi := range fdList {
35e3b4
-		fd, err := strconv.Atoi(fi.Name())
35e3b4
+	for _, fdStr := range fdList {
35e3b4
+		fd, err := strconv.Atoi(fdStr)
35e3b4
+		// Ignore non-numeric file names.
35e3b4
 		if err != nil {
35e3b4
-			// ignore non-numeric file names
35e3b4
 			continue
35e3b4
 		}
35e3b4
-
35e3b4
+		// Ignore descriptors lower than our specified minimum.
35e3b4
 		if fd < minFd {
35e3b4
-			// ignore descriptors lower than our specified minimum
35e3b4
 			continue
35e3b4
 		}
35e3b4
-
35e3b4
-		// intentionally ignore errors from unix.CloseOnExec
35e3b4
+		// Intentionally ignore errors from unix.CloseOnExec -- the cases where
35e3b4
+		// this might fail are basically file descriptors that have already
35e3b4
+		// been closed (including and especially the one that was created when
35e3b4
+		// ioutil.ReadDir did the "opendir" syscall).
35e3b4
 		unix.CloseOnExec(fd)
35e3b4
-		// the cases where this might fail are basically file descriptors that have already been closed (including and especially the one that was created when ioutil.ReadDir did the "opendir" syscall)
35e3b4
 	}
35e3b4
 	return nil
35e3b4
 }