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
}