os.RemoveAll: openFdAt function without O_CLOEXEC and cause fd escape to child process

[fuweid]

What version of Go are you using (go version)?

$ go version
go version go1.12.7 linux/amd64

Does this issue reproduce with the latest release?

This is concurrent issue. When parent process has goroutine to remove the folder, other goroutine try to exec child process. But the os.RemoveAll calles openFdAt function which open file without O_CLOEXEC. The opened file at parent process will escape to child process.

https://github.com/golang/go/blob/release-branch.go1.12/src/os/removeall_at.go#L156-L178

func openFdAt(dirfd int, name string) (*File, error) {
	var r int
	for {
		var e error
		r, e = unix.Openat(dirfd, name, O_RDONLY, 0)
		if e == nil {
			break
		}

		// See comment in openFileNolog.
		if runtime.GOOS == "darwin" && e == syscall.EINTR {
			continue
		}

		return nil, e
	}

	if !supportsCloseOnExec {
		syscall.CloseOnExec(r)
	}

	return newFile(uintptr(r), name, kindOpenFile), nil
}

unix.Openat works without O_CLOEXEC.

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/root/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/root/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build585800205=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I use the following script to reproduce issue instead of complex concurrent one.

package main

import (
        "fmt"
        "io/ioutil"
        "os"
        "os/exec"

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

func main() {
        dir, err := ioutil.TempDir("", "fd-escape")
        if err != nil {
                panic(err)
        }
        f, err := os.Open(dir)
        if err != nil {
                panic(err)
        }

        // copy from os.RemoveAll go 1.12 openFdAt without O_CLOEXEC
        if _, err := unix.Openat(int(f.Fd()), "/tmp", os.O_RDONLY, 0); err != nil {
                panic(err)
        }

        cmd := exec.Command("sleep", "10")
        if err := cmd.Start(); err != nil {
                panic(err)
        }

        fds, err := ioutil.ReadDir(fmt.Sprintf("/proc/%d/fd", cmd.Process.Pid))
        if err != nil {
                panic(err)
        }

        for _, fd := range fds {
                fname, err := os.Readlink(fmt.Sprintf("/proc/%d/fd/%s", cmd.Process.Pid, fd.Name()))
                if err != nil {
                        panic(err)
                }
                fmt.Println(fd.Name(), " --> ", fname)
        }
        cmd.Wait()
}

root@ubuntu-xenial ~/w/testing# go run parent.go
0  -->  /dev/null
1  -->  /dev/null
2  -->  /dev/null
5  -->  /tmp

When I add the O_CLOEXEC into unix.Openat, the /tmp will be gone.

// copy from os.RemoveAll go 1.12 openFdAt with O_CLOEXEC
if _, err := unix.Openat(int(f.Fd()), "/tmp", os.O_RDONLY|syscall.O_CLOEXEC, 0); err != nil {
                panic(err)
}

root@ubuntu-xenial ~/w/testing# go run parent.go
0  -->  /dev/null
1  -->  /dev/null
2  -->  /dev/null

What did you expect to see?

child process should not have any opened fd from parent.

I check go1.10, go.11 code base and found that the RemoveAll use os.Open with O_CLOEXEC. I think go1.12 might miss this part for openat.

What did you see instead?

fd escape to child - leaking

@yyb196 @Ace-Tang @rudyfly

[yyb196]

I think it's a bug of go-1.12, child process will accidentally inherit fd opened by parent process doing os.RemoveAll("/tmp/xxx")

[gopherbot]

Change https://golang.org/cl/188537 mentions this issue: os: enable the close-on-exec flag for openFdAt

[fuweid]

Hi @oiooj @ianlancetaylor Thanks for quick response! Could we please make patch to 1.12 release? I think it is like kind of security issue and it impact the behaviour of children process. WDYT?

[oiooj]

Yes, I think it should be backport to 1.12. Hi, @gopherbot please open backport to 1.12

[gopherbot]

Backport issue(s) opened: #33424 (for 1.12).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

[fuweid]

@oiooj Thanks!

[gopherbot]

Change https://golang.org/cl/188538 mentions this issue: [release-branch.go1.12] os: enable the close-on-exec flag for openFdAt

[fuweid]

Hi @oiooj Is there any schedule to release v1.12.8?

[oiooj]

From Brad:

They're released approximately monthly:
https://golang.org/doc/devel/release.html#go1.12.minor
So the next one is due soon. It's been almost a month.

I think it's very soon, maybe in a week.

[fuweid]

@oiooj Thanks!