Skip to content

syscall.Faccessat and os.LookPath regression in Go 1.20 [1.20 backport] #58624

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.

Sign up for GitHub

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

Jump to bottom
Closed
gopherbot opened this issue Feb 21, 2023 · 3 comments
Closed

syscall.Faccessat and os.LookPath regression in Go 1.20 [1.20 backport] #58624

gopherbot opened this issue Feb 21, 2023 · 3 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Milestone
Go1.20.2

Comments

@gopherbot
Copy link
Contributor

@kolyshkin requested issue #58552 to be considered for backport to the next 1.20 minor release.

@gopherbot Please backport to Go 1.20.
@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Feb 21, 2023
@gopherbot gopherbot mentioned this issue Feb 21, 2023
Closed
@gopherbot gopherbot added this to the Go1.20.2 milestone Feb 21, 2023
@gopherbot
Copy link
Contributor Author

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

@kolyshkin
Copy link
Contributor

Please consider this backport -- it is a serious regression in Go 1.20, preventing a binary from being executed in a certain scenario (see details in the original issue).

@kolyshkin kolyshkin mentioned this issue Feb 21, 2023
Closed
@liggitt liggitt mentioned this issue Feb 21, 2023
Closed
34 tasks
@thanm thanm added the CherryPickApproved Used during the release process for point releases label Feb 22, 2023
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Feb 22, 2023
@gopherbot
Copy link
Contributor Author

Closed by merging aef8a8c to release-branch.go1.20.

gopherbot pushed a commit that referenced this issue Feb 28, 2023
@kolyshkin @gopherbot
[release-branch.go1.20] syscall: Faccessat: check for CAP_DAC_OVERRID…
aef8a8c 
…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 <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
(cherry picked from commit 031401a)
Reviewed-on: https://go-review.googlesource.com/c/go/+/469956
Auto-Submit: Dmitri Shuralyov <[email protected]>
Run-TryBot: Than McIntosh <[email protected]>
Reviewed-by: Than McIntosh <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
romaindoumenc pushed a commit to TroutSoftware/go that referenced this issue Mar 3, 2023
@kolyshkin @romaindoumenc
[release-branch.go1.20] syscall: Faccessat: check for CAP_DAC_OVERRID…
53d24d2 
…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 <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
(cherry picked from commit 031401a)
Reviewed-on: https://go-review.googlesource.com/c/go/+/469956
Auto-Submit: Dmitri Shuralyov <[email protected]>
Run-TryBot: Than McIntosh <[email protected]>
Reviewed-by: Than McIntosh <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
kolyshkin added a commit to kolyshkin/runc that referenced this issue Mar 8, 2023
@kolyshkin
Bump go 1.20.2
5f88f5f 
Go 1.20.2 has an important fix to an issue described in [1].
Switch from using Go 1.19 from Dockerfile (used for release binaries and
some CI) as well as from cirrus-ci.

Note in cirrus-ci we have to specify the patch number explicitly, as
otherwise go 1.20 will be used, while in Dockerfile saying 1.20 means
to use 1.20.x (which is 1.20.2 now).

[1] golang/go#58624

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin kolyshkin mentioned this issue Mar 8, 2023
Closed
kolyshkin added a commit to kolyshkin/runc that referenced this issue Mar 9, 2023
@kolyshkin
Bump go 1.20.2
1139330 
Go 1.20.2 has an important fix to an issue described in [1].
Switch from using Go 1.19 from Dockerfile (used for release binaries and
some CI) as well as from cirrus-ci.

Note in cirrus-ci we have to specify the patch number explicitly, as
otherwise go 1.20 will be used, while in Dockerfile saying 1.20 means
to use 1.20.x (which is 1.20.2 now).

[1] golang/go#58624

Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin added a commit to kolyshkin/runc that referenced this issue Mar 16, 2023
@kolyshkin
Bump go 1.20.2
7e183d9 
Go 1.20.2 has an important fix to an issue described in [1].
Switch from using Go 1.19 from Dockerfile (used for release binaries and
some CI) as well as from cirrus-ci.

Note in cirrus-ci we have to specify the patch number explicitly, as
otherwise go 1.20 will be used, while in Dockerfile saying 1.20 means
to use 1.20.x (which is 1.20.2 now).

[1] golang/go#58624

Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin added a commit to kolyshkin/runc that referenced this issue Mar 27, 2023
@kolyshkin
Dockefile: bump go go 1.20
99a337f 
Go 1.20.2 has an important fix to an issue described in [1].

Switch from using Go 1.19 from Dockerfile, which is used for release
binaries and some CI.

[1] golang/go#58624

Signed-off-by: Kir Kolyshkin <[email protected]>
@pacoxu pacoxu mentioned this issue Feb 5, 2024
Closed
@golang golang locked and limited conversation to collaborators Feb 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Projects
None yet
Milestone
Go1.20.2
Development

No branches or pull requests
3 participants
@kolyshkin @gopherbot @thanm