Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

syscall.Faccessat and os.LookPath regression in Go 1.20 #58552

Closed
kolyshkin opened this issue Feb 16, 2023 · 7 comments
Closed

syscall.Faccessat and os.LookPath regression in Go 1.20 #58552

kolyshkin opened this issue Feb 16, 2023 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@kolyshkin
Copy link
Contributor

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

$ go version
1.20

Does this issue reproduce with the latest release?

Yes, I believe so (go 1.20.1).

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/kir/.cache/go-build"
GOENV="/home/kir/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/kir/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/kir/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/kir/sdk/go1.20"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/kir/sdk/go1.20/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.20"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
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 -fdebug-prefix-map=/tmp/go-build536264762=/tmp/go-build -gno-record-gcc-switches"

What did you do?

CL 416115 added using faccessat2(2) from syscall.Faccessat on Linux
(which is the only true way to implement AT_EACCESS flag handing),
if available. If not available, it uses some heuristics to mimic the
kernel behavior, mostly taken from glibc (see CL 126415).

Next, CL 414824 added using the above call (via unix.Eaccess) to
exec.LookPath in order to check if the binary can really be executed.

As a result, in a very specific scenario, described below,
syscall.Faccessat (and thus exec.LookPath) mistakenly tells that the
binary can not be executed, while in reality it can be. This makes
it a regression in Go 1.20, compared to earlier versions.

This scenario involves all these conditions:

  • no faccessat2 support available (i.e. either Linux kernel < 5.8,
    or a seccomp set up to disable faccessat2);
  • the current user is not root (i.e. geteuid() != 0);
  • CAP_DAC_OVERRIDE capability is set for the current process;
  • the file to be executed does not have executable permission
    bit set for either the current EUID or EGID;
  • the file to be executed have at least one executable bit set.

Unfortunately, this set of conditions was observed in the wild -- a
container run as a non-root user with the binary file owned by root with
executable permission set for a user only [1]. Essentially it means it
is not as rare as it may seem.

Now, CAP_DAC_OVERRIDE essentially makes the kernel bypass most of the
checks, so execve(2) and friends work the same was as for root user,
i.e. if at least one executable bit it set, the permission to execute
is granted (see generic_permission() function in the Linux kernel).

What did you expect to see?

No regressions in exec.LookPath() behavior.

What did you see instead?

See above.

@gopherbot
Copy link

Change https://go.dev/cl/468735 mentions this issue: syscall: Faccessat: check for CAP_DAC_OVERRIDE on Linux

@kolyshkin
Copy link
Contributor Author

It will probably be impossible to create a test case for this one, given the existing CI infra, but I will provide a test case demonstrating the issue soon.

@kolyshkin
Copy link
Contributor Author

OK here's a repro:

  1. exec_test.go:
package main

import (
	"os"
	"os/exec"
	"testing"

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

func checkPrereqs(t *testing.T) {
	// 1. No faccessat2 support available (i.e. either Linux kernel < 5.8,
	// or a seccomp set up to disable faccessat2).
	err := unix.Faccessat2(unix.AT_FDCWD, ".", unix.X_OK, unix.AT_EACCESS)
	if err == nil {
		t.Skip("prereq failed: need no faccessat2(2)")
	} else {
		t.Logf("Faccess2: %v (good, this is expected)", err)
	}

	// 2. The current user is not root (i.e. geteuid() != 0).
	if os.Geteuid() == 0 {
		t.Skip("prereq failed: need not be root")
	}

	// 3. Effective CAP_DAC_OVERRIDE capability is set for the current process.
	hdr := unix.CapUserHeader{Version: unix.LINUX_CAPABILITY_VERSION_3}
	data := [2]unix.CapUserData{}
	err = unix.Capget(&hdr, &data[0])
	if err != nil {
		t.Fatalf("capget failed: %v", err)
	}
	if data[0].Effective&(1<<unix.CAP_DAC_OVERRIDE) == 0 {
		t.Skip("prepreq failed: need CAP_DAC_OVERRIDE set")
	}
}

func TestLookPath(t *testing.T) {
	checkPrereqs(t)

	const (
		binary     = "binary"
		binaryPath = "./" + binary
		contents   = "#!/bin/sh\necho 123\n"

		// 4. The file to be executed does not have executable permission
		//    bit set for either the current EUID or EGID;
		// 5. The file to be executed have at least one executable bit set.
		perms = 0o667 // rw-rw-rwx, i.e. no x permission for user or group
	)

	err := os.WriteFile(binary, []byte(contents), perms)
	if err != nil {
		t.Fatal(err)
	}
	defer os.Remove(binary)

	// Sanity check: we should be able to run the binary.
	err = exec.Command(binaryPath).Run()
	if err != nil {
		t.Fatal(err)
	}

	// Go 1.20 and 1.20.1 fail here, see https://go.dev/issue/58552.
	_, err = exec.LookPath(binaryPath)
	if err != nil {
		t.Fatalf("exec.LookPath: %v", err)
	}
}
  1. Test environment
[kir@localhost cap-dac-override]$ uname -a
Linux localhost.localdomain 3.10.0-1160.83.1.el7.x86_64 #1 SMP Wed Jan 25 16:41:43 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
[kir@localhost cap-dac-override]$ cat /etc/centos-release
CentOS Linux release 7.9.2009 (Core)

NOTE that this won't work on CentOS 8, despite kernel being older than 5.8, they have faccessat2(2) syscall backported.

  1. A runner script, run.sh:
#!/bin/sh

set -eu

for GO in go1.19.6 go1.20.1; do
	$GO test -c -o tst.$GO .
	sudo setcap cap_dac_override=ep ./tst.$GO
	./tst.$GO -test.v || true
done
  1. Test run on various Go versions:
[kir@localhost cap-dac-override]$ bash -x run.sh
+ set -eu
+ for GO in go1.19.6 go1.20.1
+ go1.19.6 test -c -o tst.go1.19.6 .
+ sudo setcap cap_dac_override=ep ./tst.go1.19.6
+ ./tst.go1.19.6 -test.v
=== RUN   TestLookPath
    exec_test.go:18: Faccess2: function not implemented (good, this is expected)
--- PASS: TestLookPath (0.00s)
PASS
+ for GO in go1.19.6 go1.20.1
+ go1.20.1 test -c -o tst.go1.20.1 .
+ sudo setcap cap_dac_override=ep ./tst.go1.20.1
+ ./tst.go1.20.1 -test.v
=== RUN   TestLookPath
    exec_test.go:18: Faccess2: function not implemented (good, this is expected)
    exec_test.go:67: exec.LookPath: exec: "./binary": permission denied
--- FAIL: TestLookPath (0.00s)
FAIL
+ true

@kolyshkin
Copy link
Contributor Author

I have patched go1.20 (in ~/sdk) in place with the v2 patch from https://go-review.googlesource.com/c/go/+/468735 and the test above is passing now (go1.20 is the patched version):

[kir@localhost cap-dac-override]$ ./tst.go1.20 -test.v
=== RUN   TestLookPath
    exec_test.go:18: Faccess2: function not implemented (good, this is expected)
--- PASS: TestLookPath (0.01s)
PASS

gopherbot pushed a commit to golang/sys that referenced this issue Feb 17, 2023
CL 126516 added support for flags argument, implemented in the same way
as glibc does (it tries to guess what the kernel would do).

CL 246537 added using faccess2(2) Linux syscall which supports the flags
directly. For older kernels, though, the syscall is not available, and
the code uses glibc-like fallback.

There is one very specific scenario in which the fallback fails.
The scenario involves all these conditions:
 - no faccessat2 support available (i.e. either Linux kernel < 5.8,
   or a seccomp set up to disable faccessat2);
 - the current user is not root (i.e. geteuid() != 0);
 - CAP_DAC_OVERRIDE capability is set for the current process;
 - the file to be executed does not have executable permission
   bit set for either the current EUID or EGID;
 - the file to be executed have at least one executable bit set.

Unfortunately, this set of conditions was observed in the wild -- a
container run as a non-root user with the binary file owned by root with
executable permission set for a user only [1]. Essentially it means it
is not as rare as it may seem.

Now, CAP_DAC_OVERRIDE essentially makes the kernel bypass most of the
checks, so execve(2) and friends work the same was as for root user,
i.e. if at least one executable bit it set, the permission to execute
is granted (see generic_permission() function in the Linux kernel).

Modify the code to check for CAP_DAC_OVERRIDE and mimic the kernel
behavior for permission checks.

This is essentially the same fix as CL 468735 for Go syscall package.

Tested on CentOS 7 with the repro similar to the one from [2].

[1] opencontainers/runc#3715
[2] golang/go#58552 (comment)

Change-Id: I726b6acab6a6e6d0358ef98e6a582b405c347614
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Reviewed-on: https://go-review.googlesource.com/c/sys/+/468877
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
@kolyshkin
Copy link
Contributor Author

@gopherbot Please backport to Go 1.20.

@gopherbot
Copy link

Backport issue(s) opened: #58624 (for 1.20).

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

@gopherbot
Copy link

Change https://go.dev/cl/469956 mentions this issue: [release-branch.go1.20] syscall: Faccessat: check for CAP_DAC_OVERRIDE on Linux

johanbrandhorst pushed a commit to Pryz/go that referenced this issue Feb 22, 2023
CL 416115 added using faccessat2(2) from syscall.Faccessat on Linux
(which is the only true way to implement AT_EACCESS flag handing),
if available. If not available, it uses some heuristics to mimic the
kernel behavior, mostly taken from glibc (see CL 126415).

Next, CL 414824 added using the above call (via unix.Eaccess) to
exec.LookPath in order to check if the binary can really be executed.

As a result, in a very specific scenario, described below,
syscall.Faccessat (and thus exec.LookPath) mistakenly tells that the
binary can not be executed, while in reality it can be. This makes
this bug a regression in Go 1.20.

This scenario involves all these conditions:
 - no faccessat2 support available (i.e. either Linux kernel < 5.8,
   or a seccomp set up to disable faccessat2);
 - the current user is not root (i.e. geteuid() != 0);
 - CAP_DAC_OVERRIDE capability is set for the current process;
 - the file to be executed does not have executable permission
   bit set for either the current EUID or EGID;
 - the file to be executed have at least one executable bit set.

Unfortunately, this set of conditions was observed in the wild -- a
container run as a non-root user with the binary file owned by root with
executable permission set for a user only [1]. Essentially it means it
is not as rare as it may seem.

Now, CAP_DAC_OVERRIDE essentially makes the kernel bypass most of the
checks, so execve(2) and friends work the same was as for root user,
i.e. if at least one executable bit it set, the permission to execute
is granted (see generic_permission() function in the Linux kernel).

Modify the code to check for CAP_DAC_OVERRIDE and mimic the kernel
behavior for permission checks.

[1] opencontainers/runc#3715

Fixes golang#58552.

Change-Id: I82a7e757ab3fd3d0193690a65c3b48fee46ff067
Reviewed-on: https://go-review.googlesource.com/c/go/+/468735
Reviewed-by: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 27, 2023
@dmitshur dmitshur added this to the Go1.21 milestone Feb 27, 2023
gopherbot pushed a commit that referenced this issue Feb 28, 2023
…E on Linux

CL 416115 added using faccessat2(2) from syscall.Faccessat on Linux
(which is the only true way to implement AT_EACCESS flag handing),
if available. If not available, it uses some heuristics to mimic the
kernel behavior, mostly taken from glibc (see CL 126415).

Next, CL 414824 added using the above call (via unix.Eaccess) to
exec.LookPath in order to check if the binary can really be executed.

As a result, in a very specific scenario, described below,
syscall.Faccessat (and thus exec.LookPath) mistakenly tells that the
binary can not be executed, while in reality it can be. This makes
this bug a regression in Go 1.20.

This scenario involves all these conditions:
 - no faccessat2 support available (i.e. either Linux kernel < 5.8,
   or a seccomp set up to disable faccessat2);
 - the current user is not root (i.e. geteuid() != 0);
 - CAP_DAC_OVERRIDE capability is set for the current process;
 - the file to be executed does not have executable permission
   bit set for either the current EUID or EGID;
 - the file to be executed have at least one executable bit set.

Unfortunately, this set of conditions was observed in the wild -- a
container run as a non-root user with the binary file owned by root with
executable permission set for a user only [1]. Essentially it means it
is not as rare as it may seem.

Now, CAP_DAC_OVERRIDE essentially makes the kernel bypass most of the
checks, so execve(2) and friends work the same was as for root user,
i.e. if at least one executable bit it set, the permission to execute
is granted (see generic_permission() function in the Linux kernel).

Modify the code to check for CAP_DAC_OVERRIDE and mimic the kernel
behavior for permission checks.

[1] opencontainers/runc#3715

For #58552.
Fixes #58624.

Change-Id: I82a7e757ab3fd3d0193690a65c3b48fee46ff067
Reviewed-on: https://go-review.googlesource.com/c/go/+/468735
Reviewed-by: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
(cherry picked from commit 031401a)
Reviewed-on: https://go-review.googlesource.com/c/go/+/469956
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Than McIntosh <thanm@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
romaindoumenc pushed a commit to TroutSoftware/go that referenced this issue Mar 3, 2023
…E on Linux

CL 416115 added using faccessat2(2) from syscall.Faccessat on Linux
(which is the only true way to implement AT_EACCESS flag handing),
if available. If not available, it uses some heuristics to mimic the
kernel behavior, mostly taken from glibc (see CL 126415).

Next, CL 414824 added using the above call (via unix.Eaccess) to
exec.LookPath in order to check if the binary can really be executed.

As a result, in a very specific scenario, described below,
syscall.Faccessat (and thus exec.LookPath) mistakenly tells that the
binary can not be executed, while in reality it can be. This makes
this bug a regression in Go 1.20.

This scenario involves all these conditions:
 - no faccessat2 support available (i.e. either Linux kernel < 5.8,
   or a seccomp set up to disable faccessat2);
 - the current user is not root (i.e. geteuid() != 0);
 - CAP_DAC_OVERRIDE capability is set for the current process;
 - the file to be executed does not have executable permission
   bit set for either the current EUID or EGID;
 - the file to be executed have at least one executable bit set.

Unfortunately, this set of conditions was observed in the wild -- a
container run as a non-root user with the binary file owned by root with
executable permission set for a user only [1]. Essentially it means it
is not as rare as it may seem.

Now, CAP_DAC_OVERRIDE essentially makes the kernel bypass most of the
checks, so execve(2) and friends work the same was as for root user,
i.e. if at least one executable bit it set, the permission to execute
is granted (see generic_permission() function in the Linux kernel).

Modify the code to check for CAP_DAC_OVERRIDE and mimic the kernel
behavior for permission checks.

[1] opencontainers/runc#3715

For golang#58552.
Fixes golang#58624.

Change-Id: I82a7e757ab3fd3d0193690a65c3b48fee46ff067
Reviewed-on: https://go-review.googlesource.com/c/go/+/468735
Reviewed-by: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
(cherry picked from commit 031401a)
Reviewed-on: https://go-review.googlesource.com/c/go/+/469956
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Than McIntosh <thanm@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
@golang golang locked and limited conversation to collaborators Feb 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants