Merge pull request #2130 from cyphar/apparmor-verify-procfs

*: verify operations on /proc/... are on procfs

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,14 +32,18 @@ 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
 }
 
 // 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

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
 }

vendor.conf

@@ -6,7 +6,7 @@ github.com/opencontainers/runtime-spec  29686dbc5559d93fb1ef402eeda3e35c38d75af4
 # Core libcontainer functionality.
 github.com/checkpoint-restore/go-criu   17b0214f6c48980c45dc47ecb0cfd6d9e02df723 # v3.11
 github.com/mrunalp/fileutils            7d4729fb36185a7c1719923406c9d40e54fb93c7
-github.com/opencontainers/selinux       3a1f366feb7aecbf7a0e71ac4cea88b31597de9e # v1.2.2
+github.com/opencontainers/selinux       5215b1806f52b1fcc2070a8826c542c9d33cd3cf # v1.3.0 (+ CVE-2019-16884)
 github.com/seccomp/libseccomp-golang    689e3c1541a84461afc49c1c87352a6cedf72e9c # v0.9.1
 github.com/sirupsen/logrus              8bdbc7bcc01dcbb8ec23dc8a28e332258d25251f # v1.4.1
 github.com/syndtr/gocapability          d98352740cb2c55f81556b63d4a1ec64c5a319c2