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.

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

syscall: StartProcess returns Errno(0) on Linux if SYS_CAPGET or SYS_CAPSET fails #57208

Closed
bcmills opened this issue Dec 9, 2022 · 4 comments
Closed

syscall: StartProcess returns Errno(0) on Linux if SYS_CAPGET or SYS_CAPSET fails #57208

bcmills opened this issue Dec 9, 2022 · 4 comments
Assignees
@bcmills
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Linux
Milestone
Go1.20

Comments

@bcmills
Copy link
Contributor

bcmills commented Dec 9, 2022

On Linux, if the CLONE_NEWUSER flag is not set we write any requested UID or GID mappings in the parent process in forkAndExecInChild after the child process has already been forked.

If that fails, we write an error to the child via a pipe, which we expect the child to read and finally write back on a different pipe, which the parent reads after forkAndExecInChild returns.

Somewhere in that sequence, the actual errno value is being dropped and the parent is reading errno 0 from the pipe instead of the actual error code. I have identified three likely shadowing bugs in forkAndExecInChild1:
https://cs.opensource.google/search?q=%22err1%20:%3D%22%20filepath:syscall%2Fexec_linux.go&ss=go%2Fgo
@bcmills bcmills added OS-Linux NeedsFix The path to resolution is known, but the work has not been done. labels Dec 9, 2022
@bcmills bcmills added this to the Go1.21 milestone Dec 9, 2022
@bcmills bcmills self-assigned this Dec 9, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Dec 9, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/456516 mentions this issue: syscall: clean up variable naming and expand comments in forkAndExecInChild

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/456515 mentions this issue: syscall: fix shadowing bugs in forkAndExecInChild1

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/456375 mentions this issue: syscall: avoid making assumptions about syscall permissions

@bcmills bcmills changed the title syscall: StartProcess returns Errno(0) on Linux if the parent process fails to write syscall: StartProcess returns Errno(0) on Linux if SYS_CAPGET or SYS_CAPSET fails Dec 9, 2022
@bcmills
Copy link
Contributor Author

bcmills commented Dec 9, 2022

The SYS_CAPGET and SYS_CAPSET calls were added in CL 156577 for #23152.

@bcmills bcmills modified the milestones: Go1.21, Go1.20 Dec 12, 2022
gopherbot pushed a commit that referenced this issue Jan 25, 2023
@gopherbot
syscall: clean up variable declarations in forkAndExecInChild
e216ee7 
The various forkAndExecInChild implementations have comments
explaining that they pre-declare variables to force allocations
to occur before forking, but then later use ":=" declarations
for additional variables.

To make it clearer that those ":=" declarations do not allocate,
we move their declarations up to the predeclared blocks.

For #57208.

Change-Id: Ie8cb577fa7180b51b64d6dc398169053fdf8ea97
Reviewed-on: https://go-review.googlesource.com/c/go/+/456516
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Jan 27, 2023
@gopherbot
syscall: avoid making assumptions about syscall permissions
0926714 
We currently check for at least three different permission bits before
running tests that require root permissions: we look for UID 0, lack
of an LXC container, and lack of a Docker container, and probe a
number of distro-specific files in /proc and /sys.

The sheer number of these checks suggests that we have probably missed
at least one. Per Alan J. Perlis, “If you have a procedure with ten
parameters, you probably missed some.” (And, indeed, we definitely
have: a Debian patch¹ adds one more environment check!)

CL 58170 added some of these container checks, but “decided to go this
way instead of just skipping os.IsPermission errors because many of
those tests were specifically written to check false positive
permission errors.” However, we can't in general distinguish between a
false-positive error and a real one caused by a container: if one is
making a change to the syscall package, they should run the tests with
-v and check for unexpected skips.

Notably:

- TestUnshare already skips itself if the command fails with an error
  ending in the string "operation not permitted", which could be caused
  by a variety of possible bugs.

- The Unshare tests added in CL 38471 will fail with a permission
  error if CLONE_NEWNS is not supported, but it seems to me that if
  CLONE_NEWNS is supported — sufficient to start the process! — then
  Unmount must also be supported, and the test can at least check that
  the two are consistent.

- The AmbientCaps tests should fail to start the subprocess with
  EINVAL or similar (not produce bogus output) if the kernel does not
  support ambient caps for any reason, which we can then detect.
  (If the subprocess fails in the way the test is concerned about, it
  will exit with status 2, not fail to start in the first place.)

By executing the system calls and checking for permission errors,
this change exposed an existing bug for AmbientCaps (filed as #57208),
which was detected by the linux-arm-aws builder.

For #57208.
Updates #21379.
Updates #14693.

¹https://sources.debian.org/patches/golang-1.19/1.19.3-1/0006-skip-userns-test-in-schroot-as-well.patch/

Change-Id: I9b167661fa1bb823168c8b50d8bbbf9643e49f76
Reviewed-on: https://go-review.googlesource.com/c/go/+/456375
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Alexander Morozov <lk4d4math@gmail.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
@golang golang locked and limited conversation to collaborators Dec 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@bcmills bcmills

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Linux
Projects
None yet
Milestone
Go1.20
Development

No branches or pull requests
2 participants
@bcmills @gopherbot