Merge pull request opencontainers#1570 from cyphar/close-statedirfd-hole

init: switch away from stateDirFd entirely

Author: crosbymichael

libcontainer/container_linux.go

@@ -350,6 +350,23 @@ func (c *linuxContainer) deleteExecFifo() {
 	os.Remove(fifoName)
 }
 
+// includeExecFifo opens the container's execfifo as a pathfd, so that the
+// container cannot access the statedir (and the FIFO itself remains
+// un-opened). It then adds the FifoFd to the given exec.Cmd as an inherited
+// fd, with _LIBCONTAINER_FIFOFD set to its fd number.
+func (c *linuxContainer) includeExecFifo(cmd *exec.Cmd) error {
+	fifoName := filepath.Join(c.root, execFifoFilename)
+	fifoFd, err := unix.Open(fifoName, unix.O_PATH|unix.O_CLOEXEC, 0)
+	if err != nil {
+		return err
+	}
+
+	cmd.ExtraFiles = append(cmd.ExtraFiles, os.NewFile(uintptr(fifoFd), fifoName))
+	cmd.Env = append(cmd.Env,
+		fmt.Sprintf("_LIBCONTAINER_FIFOFD=%d", stdioFdCount+len(cmd.ExtraFiles)-1))
+	return nil
+}
+
 func (c *linuxContainer) newParentProcess(p *Process, doInit bool) (parentProcess, error) {
 	parentPipe, childPipe, err := utils.NewSockPair("init")
 	if err != nil {
@@ -363,18 +380,15 @@ func (c *linuxContainer) newParentProcess(p *Process, doInit bool) (parentProces
 		return c.newSetnsProcess(p, cmd, parentPipe, childPipe)
 	}
 
-	// We only set up rootDir if we're not doing a `runc exec`. The reason for
-	// this is to avoid cases where a racing, unprivileged process inside the
-	// container can get access to the statedir file descriptor (which would
-	// allow for container rootfs escape).
-	rootDir, err := os.Open(c.root)
-	if err != nil {
-		return nil, err
+	// We only set up fifoFd if we're not doing a `runc exec`. The historic
+	// reason for this is that previously we would pass a dirfd that allowed
+	// for container rootfs escape (and not doing it in `runc exec` avoided
+	// that problem), but we no longer do that. However, there's no need to do
+	// this for `runc exec` so we just keep it this way to be safe.
+	if err := c.includeExecFifo(cmd); err != nil {
+		return nil, newSystemErrorWithCause(err, "including execfifo in cmd.Exec setup")
 	}
-	cmd.ExtraFiles = append(cmd.ExtraFiles, rootDir)
-	cmd.Env = append(cmd.Env,
-		fmt.Sprintf("_LIBCONTAINER_STATEDIR=%d", stdioFdCount+len(cmd.ExtraFiles)-1))
-	return c.newInitProcess(p, cmd, parentPipe, childPipe, rootDir)
+	return c.newInitProcess(p, cmd, parentPipe, childPipe)
 }
 
 func (c *linuxContainer) commandTemplate(p *Process, childPipe *os.File) (*exec.Cmd, error) {
@@ -406,7 +420,7 @@ func (c *linuxContainer) commandTemplate(p *Process, childPipe *os.File) (*exec.
 	return cmd, nil
 }
 
-func (c *linuxContainer) newInitProcess(p *Process, cmd *exec.Cmd, parentPipe, childPipe, rootDir *os.File) (*initProcess, error) {
+func (c *linuxContainer) newInitProcess(p *Process, cmd *exec.Cmd, parentPipe, childPipe *os.File) (*initProcess, error) {
 	cmd.Env = append(cmd.Env, "_LIBCONTAINER_INITTYPE="+string(initStandard))
 	nsMaps := make(map[configs.NamespaceType]string)
 	for _, ns := range c.config.Namespaces {
@@ -429,7 +443,6 @@ func (c *linuxContainer) newInitProcess(p *Process, cmd *exec.Cmd, parentPipe, c
 		process:       p,
 		bootstrapData: data,
 		sharePidns:    sharePidns,
-		rootDir:       rootDir,
 	}, nil
 }
 

libcontainer/factory_linux.go

@@ -233,10 +233,10 @@ func (l *LinuxFactory) Type() string {
 // This is a low level implementation detail of the reexec and should not be consumed externally
 func (l *LinuxFactory) StartInitialization() (err error) {
 	var (
-		pipefd, rootfd int
+		pipefd, fifofd int
 		consoleSocket  *os.File
 		envInitPipe    = os.Getenv("_LIBCONTAINER_INITPIPE")
-		envStateDir    = os.Getenv("_LIBCONTAINER_STATEDIR")
+		envFifoFd      = os.Getenv("_LIBCONTAINER_FIFOFD")
 		envConsole     = os.Getenv("_LIBCONTAINER_CONSOLE")
 	)
 
@@ -252,11 +252,11 @@ func (l *LinuxFactory) StartInitialization() (err error) {
 	)
 	defer pipe.Close()
 
-	// Only init processes have STATEDIR.
-	rootfd = -1
+	// Only init processes have FIFOFD.
+	fifofd = -1
 	if it == initStandard {
-		if rootfd, err = strconv.Atoi(envStateDir); err != nil {
-			return fmt.Errorf("unable to convert _LIBCONTAINER_STATEDIR=%s to int: %s", envStateDir, err)
+		if fifofd, err = strconv.Atoi(envFifoFd); err != nil {
+			return fmt.Errorf("unable to convert _LIBCONTAINER_FIFOFD=%s to int: %s", envFifoFd, err)
 		}
 	}
 
@@ -291,7 +291,7 @@ func (l *LinuxFactory) StartInitialization() (err error) {
 		}
 	}()
 
-	i, err := newContainerInit(it, pipe, consoleSocket, rootfd)
+	i, err := newContainerInit(it, pipe, consoleSocket, fifofd)
 	if err != nil {
 		return err
 	}

libcontainer/init_linux.go

@@ -68,7 +68,7 @@ type initer interface {
 	Init() error
 }
 
-func newContainerInit(t initType, pipe *os.File, consoleSocket *os.File, stateDirFD int) (initer, error) {
+func newContainerInit(t initType, pipe *os.File, consoleSocket *os.File, fifoFd int) (initer, error) {
 	var config *initConfig
 	if err := json.NewDecoder(pipe).Decode(&config); err != nil {
 		return nil, err
@@ -89,7 +89,7 @@ func newContainerInit(t initType, pipe *os.File, consoleSocket *os.File, stateDi
 			consoleSocket: consoleSocket,
 			parentPid:     unix.Getppid(),
 			config:        config,
-			stateDirFD:    stateDirFD,
+			fifoFd:        fifoFd,
 		}, nil
 	}
 	return nil, fmt.Errorf("unknown init type %q", t)

libcontainer/process_linux.go

@@ -203,7 +203,6 @@ type initProcess struct {
 	process       *Process
 	bootstrapData io.Reader
 	sharePidns    bool
-	rootDir       *os.File
 }
 
 func (p *initProcess) pid() int {
@@ -258,7 +257,6 @@ func (p *initProcess) start() error {
 	err := p.cmd.Start()
 	p.process.ops = p
 	p.childPipe.Close()
-	p.rootDir.Close()
 	if err != nil {
 		p.process.ops = nil
 		return newSystemErrorWithCause(err, "starting init process command")

libcontainer/standard_init_linux.go

@@ -22,7 +22,7 @@ type linuxStandardInit struct {
 	pipe          *os.File
 	consoleSocket *os.File
 	parentPid     int
-	stateDirFD    int
+	fifoFd        int
 	config        *initConfig
 }
 
@@ -164,9 +164,11 @@ func (l *linuxStandardInit) Init() error {
 	}
 	// close the pipe to signal that we have completed our init.
 	l.pipe.Close()
-	// wait for the fifo to be opened on the other side before
-	// exec'ing the users process.
-	fd, err := unix.Openat(l.stateDirFD, execFifoFilename, os.O_WRONLY|unix.O_CLOEXEC, 0)
+	// Wait for the FIFO to be opened on the other side before exec-ing the
+	// user process. We open it through /proc/self/fd/$fd, because the fd that
+	// was given to us was an O_PATH fd to the fifo itself. Linux allows us to
+	// re-open an O_PATH fd through /proc.
+	fd, err := unix.Open(fmt.Sprintf("/proc/self/fd/%d", l.fifoFd), unix.O_WRONLY|unix.O_CLOEXEC, 0)
 	if err != nil {
 		return newSystemErrorWithCause(err, "openat exec fifo")
 	}
@@ -180,7 +182,7 @@ func (l *linuxStandardInit) Init() error {
 	}
 	// close the statedir fd before exec because the kernel resets dumpable in the wrong order
 	// https://github.com/torvalds/linux/blob/v4.9/fs/exec.c#L1290-L1318
-	unix.Close(l.stateDirFD)
+	unix.Close(l.fifoFd)
 	if err := syscall.Exec(name, l.config.Args[0:], os.Environ()); err != nil {
 		return newSystemErrorWithCause(err, "exec user process")
 	}