golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.26k stars 17.57k forks source link

os/exec: Can't use os.Command (with ExtraFiles) that uses /proc/self/fd/<exe fd> as the executable #66654

Open ClydeByrdIII opened 6 months ago

ClydeByrdIII commented 6 months ago

Go version

go 1.21.8 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/user/.cache/go-build'
GOENV='/home/user/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/user/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/user/go'
GOPRIVATE=''
GOPROXY=''
GOROOT='/home/user/go/v1.21.8'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/user/go/v1.21.8/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.8'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK='/home/user/project/go.work'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build3303884229=/tmp/go-build -gno-record-gcc-switches'

What did you do?

The following program attempts to Fork+Exec a binary (/usr/local/bin/play-sandbox) via an open file descriptor (FD) in the current process and have the child inherit additional FDs and print the usage information (-h / --help)

go playground link https://go.dev/play/p/MXQGzqeVJK0

package main

import (
    "fmt"
    "os"
    "os/exec"

    "golang.org/x/sys/unix"
)

func main() {
    // open up a binary as O_PATH (more or less just a pointer to the file in the filesystem with no r/w support)
    binPtr, err := os.OpenFile("/usr/local/bin/play-sandbox", unix.O_PATH, 0)
    if err != nil {
        panic(err)
    }
    defer binPtr.Close()
    // refer to the binary via the processes FD table entry
    saved := fmt.Sprintf("/proc/self/fd/%d", int(binPtr.Fd()))

    cmd := exec.Command(saved, "-h")
    // direct child's output to parents stdout
    cmd.Stderr = os.Stdout
    cmd.Stdout = os.Stdout

    // add one or more files for the child to inherit (and thus cause potential fd stomping)
    cmd.ExtraFiles = append(cmd.ExtraFiles, os.Stdout)

    fmt.Printf("Running cmd: %v\n", cmd)
    err = cmd.Run()
    if err != nil {
        panic(err)
    }
}

What did you see happen?

Running cmd: /proc/self/fd/3 -h
panic: fork/exec /proc/self/fd/3: permission denied

goroutine 1 [running]:
main.main()
    /tmp/sandbox3277445564/prog.go:52 +0x2d3

Program exited.

What did you expect to see?

Running cmd: /proc/self/fd/4 -h
Usage of /proc/self/fd/4:
  -dev
        run in dev mode (show help messages)
  -listen string
        HTTP server listen address. Only applicable when --mode=server (default ":80")
  -mode string
        Whether to run in "server" mode or "contained" mode. The contained mode is used internally by the server mode. (default "server")
  -untrusted-container string
        container image name that hosts the untrusted binary under gvisor (default "gcr.io/golang-org/playground-sandbox-gvisor:latest")
  -workers int
        number of parallel gvisor containers to pre-spin up & let run concurrently (default 8)

Program exited.

The problem can be circumvented by simply pushing the exe FD in the parent process to a FD integer high enough that it won't get clobbered by the file descriptors in cmd.ExtraFiles.

e.g situation: 4 FDs are intended to be inherited by the child after exec [stdin(0), stdout(1), stderr(2), stdin_dup(4)] and the fd pointing to the exe is 3. Problem: exe(3) get's clobbered by stdin_dup (4) in the child before exec.

Workaround: By making the fd of exe, point to higher then len(ProcAttr.Files) (4 in this case), the exe isn't clobbered (and if CLO_EXEC is set, it'll also be closed in the exec'ed child. Which is my desired end-state)

https://go.dev/play/p/gF5zi4g2fTq


I wasn't sure whether to mark this as a syscall issue or not; I originally ran in to this using libcap's cap.Launcher and was able to reproduce with the equivalent os/exec's Command setup. Both of them use syscall.ForkExec, which is where I believe the FD stomping occurs based on some strace + print statements.

I see in exec_linux.go that there's special handling to avoid stomping on needed FDs like pipes. Perhaps some special casing for /proc/self/fd/\<fd> as argv[0] could be done too? Not sure, no expert here.

dmitshur commented 6 months ago

CC @bradfitz, @ianlancetaylor.

ianlancetaylor commented 6 months ago

That seems like a very unusual special case. The only way I see to fix it is to recognize the use of /proc/self/fd or /proc/MYPID/fd, dup the file to a large number, make sure that O_CLOEXEC is set, and exec that. As you've discovered the program doing this can do that anyhow. Right now that seems preferable to me: force the program that wants to handle this very unusual case to handle it itself.

ClydeByrdIII commented 6 months ago

That seems like a very unusual special case.

I agree; It's definitely unusual, although perhaps not unexpected in Linux. It's an expected use case of the O_PATH flag used with open(2)

One use of O_PATH for regular files is to provide the equivalent of POSIX.1's O_EXEC functionality. This permits us to open a file for which we have execute permission but not read permission, and then execute that file, with steps something like the following:

              char buf[PATH_MAX];
              fd = open("some_prog", O_PATH);
              snprintf(buf, PATH_MAX, "/proc/self/fd/%d", fd);
              execl(buf, "some_prog", (char *) NULL);

or similar reasons for fexecve(3)

The idea behind fexecve() is to allow the caller to verify (checksum) the contents of an executable before executing it. Simply opening the file, checksumming the contents, and then doing an execve(2) would not suffice, since, between the two steps, the filename, or a directory prefix of the pathname, could have been exchanged (by, for example, modifying the target of a symbolic link).

My use-case just happens to also incorporate inheriting IPC primitives with the parent and child that lead me to this issue.

The only way I see to fix it is to recognize the use of /proc/self/fd or /proc/MYPID/fd, dup the file to a large number, make sure that O_CLOEXEC is set, and exec that.

I feared as such.

As you've discovered the program doing this can do that anyhow. Right now that seems preferable to me: force the program that wants to handle this very unusual case to handle it itself.

Understandable; one would hope the issue would be more readily presentable though. It had a non-ideal amount of digging needed to figure out it was occurring in golang source code and not the higher-level ForkExec wrappers.

There's a non-zero possibility of someone using this pattern and invoking an unintended binary rather than getting various ERRNO (EPERM, ENXIO, etc) errors that I found.

If not an in-place fix, would you folks be open to any (or all) of the below:

As I have workarounds to my specific situation, I won't go crazy, if nothing changes, but just figured I'd raise the awareness.

Thanks for the quick look and response!

ianlancetaylor commented 6 months ago

Documentation would certainly be fine.

espadolini commented 3 months ago

Can't you also execute /proc/<parent pid>/fd/<n>, and ensure that the file isn't closed until the new process has been started (or finishes, even)?

ClydeByrdIII commented 3 months ago

It wasn’t necessary to repro the issue, so I omitted this detail, but my use case also changes the UID/GID of the process which happens before the exec, so the child process won’t have the ability to read /proc/<parent pid>/fd/<n> in my case, but otherwise if the child has permissions to read that location yeah that should work.