From d463f6485b809b5ea738f84e05ff5b456058a184 Mon Sep 17 00:00:00 2001
From: Aleksa Sarai <asarai@suse.de>
Date: Fri, 27 Sep 2019 12:01:07 +1000
Subject: [PATCH] *: verify that operations on /proc/... are on procfs

This is an additional mitigation for CVE-2019-16884. The primary problem
is that Docker can be coerced into bind-mounting a file system on top of
/proc (resulting in label-related writes to /proc no longer happening).

While we are working on mitigations against permitting the mounts, this
helps avoid our code from being tricked into writing to non-procfs
files. This is not a perfect solution (after all, there might be a
bind-mount of a different procfs file over the target) but in order to
exploit that you would need to be able to tweak a config.json pretty
specifically (which thankfully Docker doesn't allow).

Specifically this stops AppArmor from not labeling a process silently
due to /proc/self/attr/... being incorrectly set, and stops any
accidental fd leaks because /proc/self/fd/... is not real.

Signed-off-by: Aleksa Sarai <asarai@suse.de>
---
 libcontainer/apparmor/apparmor.go | 10 +++++--
 libcontainer/utils/utils_unix.go  | 44 ++++++++++++++++++++++++-------
 2 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/libcontainer/apparmor/apparmor.go b/libcontainer/apparmor/apparmor.go
index 7fff0627f..debfc1e48 100644
--- a/libcontainer/apparmor/apparmor.go
+++ b/libcontainer/apparmor/apparmor.go
@@ -6,6 +6,8 @@ import (
 	"fmt"
 	"io/ioutil"
 	"os"
+
+	"github.com/opencontainers/runc/libcontainer/utils"
 )
 
 // IsEnabled returns true if apparmor is enabled for the host.
@@ -19,7 +21,7 @@ func IsEnabled() bool {
 	return false
 }
 
-func setprocattr(attr, value string) error {
+func setProcAttr(attr, value string) error {
 	// Under AppArmor you can only change your own attr, so use /proc/self/
 	// instead of /proc/<tid>/ like libapparmor does
 	path := fmt.Sprintf("/proc/self/attr/%s", attr)
@@ -30,6 +32,10 @@ func setprocattr(attr, value string) error {
 	}
 	defer f.Close()
 
+	if err := utils.EnsureProcHandle(f); err != nil {
+		return err
+	}
+
 	_, err = fmt.Fprintf(f, "%s", value)
 	return err
 }
@@ -37,7 +43,7 @@ func setprocattr(attr, value string) error {
 // changeOnExec reimplements aa_change_onexec from libapparmor in Go
 func changeOnExec(name string) error {
 	value := "exec " + name
-	if err := setprocattr("exec", value); err != nil {
+	if err := setProcAttr("exec", value); err != nil {
 		return fmt.Errorf("apparmor failed to apply profile: %s", err)
 	}
 	return nil
diff --git a/libcontainer/utils/utils_unix.go b/libcontainer/utils/utils_unix.go
index c96088988..1576f2d4a 100644
--- a/libcontainer/utils/utils_unix.go
+++ b/libcontainer/utils/utils_unix.go
@@ -3,33 +3,57 @@
 package utils
 
 import (
-	"io/ioutil"
+	"fmt"
 	"os"
 	"strconv"
 
 	"golang.org/x/sys/unix"
 )
 
+// EnsureProcHandle returns whether or not the given file handle is on procfs.
+func EnsureProcHandle(fh *os.File) error {
+	var buf unix.Statfs_t
+	if err := unix.Fstatfs(int(fh.Fd()), &buf); err != nil {
+		return fmt.Errorf("ensure %s is on procfs: %v", fh.Name(), err)
+	}
+	if buf.Type != unix.PROC_SUPER_MAGIC {
+		return fmt.Errorf("%s is not on procfs", fh.Name())
+	}
+	return nil
+}
+
+// CloseExecFrom applies O_CLOEXEC to all file descriptors currently open for
+// the process (except for those below the given fd value).
 func CloseExecFrom(minFd int) error {
-	fdList, err := ioutil.ReadDir("/proc/self/fd")
+	fdDir, err := os.Open("/proc/self/fd")
+	if err != nil {
+		return err
+	}
+	defer fdDir.Close()
+
+	if err := EnsureProcHandle(fdDir); err != nil {
+		return err
+	}
+
+	fdList, err := fdDir.Readdirnames(-1)
 	if err != nil {
 		return err
 	}
-	for _, fi := range fdList {
-		fd, err := strconv.Atoi(fi.Name())
+	for _, fdStr := range fdList {
+		fd, err := strconv.Atoi(fdStr)
+		// Ignore non-numeric file names.
 		if err != nil {
-			// ignore non-numeric file names
 			continue
 		}
-
+		// Ignore descriptors lower than our specified minimum.
 		if fd < minFd {
-			// ignore descriptors lower than our specified minimum
 			continue
 		}
-
-		// intentionally ignore errors from unix.CloseOnExec
+		// Intentionally ignore errors from unix.CloseOnExec -- 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).
 		unix.CloseOnExec(fd)
-		// 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)
 	}
 	return nil
 }