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

runtime: TestSUID prompts for password #60690

Closed
jrick opened this issue Jun 8, 2023 · 19 comments
Closed

runtime: TestSUID prompts for password #60690

jrick opened this issue Jun 8, 2023 · 19 comments
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-OpenBSD Security Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone

Comments

@jrick
Copy link
Contributor

jrick commented Jun 8, 2023

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

$ go version
go version go1.20.5 openbsd/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/jrick/.cache/go-build"
GOENV="/home/jrick/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="openbsd"
GOINSECURE=""
GOMODCACHE="/home/jrick/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="openbsd"
GOPATH="/home/jrick/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/jrick/src/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/jrick/src/go/pkg/tool/openbsd_amd64"
GOVCS=""
GOVERSION="go1.20.5"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
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 -fno-caret-diagnostics -Qunused-arguments -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build84902=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Built latest Go 1.20.5 from source, running bash all.bash. I saw a password prompt during the tests, and narrowed it down to the runtime test TestSUID.

What did you expect to see?

Passing tests as an unprivileged user. If privilege escalation is not possible, the test should be skipped.

What did you see instead?

$ go test -v runtime
[...]
=== RUN   TestSUID
    crash_test.go:138: running go build -o /tmp/go-build3479369129/testsuid.exe 
Password:

The test eventually times out after the default 10m deadline.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jun 8, 2023
@dr2chase
Copy link
Contributor

dr2chase commented Jun 8, 2023

Which version of openbsd is this? The test seems to pass in our builders, which seem to be 7.2.

@jrick
Copy link
Contributor Author

jrick commented Jun 8, 2023

$ sysctl kern.version 
kern.version=OpenBSD 7.3-current (GENERIC.MP) #1215: Sun Jun  4 00:31:12 MDT 2023
    deraadt@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP

@dr2chase
Copy link
Contributor

dr2chase commented Jun 8, 2023

@rolandshoemaker

@dr2chase dr2chase added Security NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 8, 2023
@bcmills
Copy link
Contributor

bcmills commented Jun 8, 2023

(attn @golang/openbsd)

@jrick
Copy link
Contributor Author

jrick commented Jun 8, 2023

The test seems to pass in our builders, which seem to be 7.2.

If I had to guess, the builder user might not be a member of the wheel group like I am, so the su prompt errors and the test gets skipped as designed.

@rolandshoemaker
Copy link
Member

We use exec.CommandContext in the test, which maps stdin to /dev/null, and should cause things (like sudo and su) which attempt to read from stdin to just fail (which is what happens everywhere else, as far as I can tell).

Seemingly OpenBSD su is not doing this, and for some reason we aren't hitting the timeout on the context. For the former, we might just want to remap stdin to a reader which just returns a single null byte or something, which should (hopefully) always cause su/sudo to fail, for the later I have no clue what is happening, might be an exec issue?

@bcmills
Copy link
Contributor

bcmills commented Jun 8, 2023

On various platforms we have had trouble with prompts that bypass stdin by either opening a GUI or reaching down into the TTY directly. I suspect that is what is happening here.

See previously:

@jrick
Copy link
Contributor Author

jrick commented Jun 8, 2023

OpenBSD su uses the BSD Authentication system

@jrick
Copy link
Contributor Author

jrick commented Jun 8, 2023

$ pstree -p 24724 
-+= 00001 root /sbin/init
 \-+= 89008 root /usr/X11R6/bin/xenodm
   \-+= 94607 root xenodm: :0 (xenodm)
     \-+= 29809 jrick /bin/sh /etc/X11/xenodm/Xsession
       \-+- 95231 jrick /bin/sh /home/jrick/.xsession
         \-+- 85790 jrick xfce4-session
           \-+- 18863 jrick xfce4-terminal --geometry=80x24 --role=xfce4-terminal-1668188145-154447547 --show-menubar --show-borders --hide-toolbar --act
             \-+= 78874 jrick ksh
               \-+- 24724 root su
                 \--- 90481 root passwd -v wheel=yes -v invokinguser=jrick -v login=yes -s login -- root daemon (login_passwd)
$ fstat -p 90481
USER     CMD          PID   FD MOUNT        INUM  MODE         R/W    SZ|DV
root     login_passwd 90481 text /usr      3732486  -r-sr-xr-x     r     9392
root     login_passwd 90481   wd /home     4177920  drwxr-xr-x     r     2048
root     login_passwd 90481    0 pipe 0x0 state: E
root     login_passwd 90481    1 /          104184  crw--w----    rw    ttyp3
root     login_passwd 90481    2 /          104184  crw--w----    rw    ttyp3
root     login_passwd 90481    3* unix stream 0x0
root     login_passwd 90481    4 /          104849  crw-rw-rw-   rwp      tty

@jrick
Copy link
Contributor Author

jrick commented Jun 8, 2023

https://github.com/openbsd/src/blob/master/libexec/login_passwd/login_passwd.c is the source for login_passwd that is being spawned to handle the authentication task.

@jrick
Copy link
Contributor Author

jrick commented Jun 8, 2023

Looks like su is being killed after the 5s timeout, but the login_passwd process is being orphaned and inherited by init.

$ pstree -p 78472 
-+= 00001 root /sbin/init
 \-+= 89008 root /usr/X11R6/bin/xenodm
   \-+= 94607 root xenodm: :0 (xenodm)
     \-+= 29809 jrick /bin/sh /etc/X11/xenodm/Xsession
       \-+- 95231 jrick /bin/sh /home/jrick/.xsession
         \-+- 85790 jrick xfce4-session
           \-+- 18863 jrick xfce4-terminal --geometry=80x24 --role=xfce4-terminal-1668188145-154447547 --show-menubar --show-borders --hide-toolbar --ac
             \-+= 18939 jrick ksh
               \-+= 50319 jrick go test -v -run TestSUID runtime
                 \-+- 03873 jrick /tmp/go-build3204115679/b001/runtime.test -test.testlogfile=/tmp/go-build3204115679/b001/testlog.txt -test.paniconexit
                   \-+- 78472 root su root -c chmod 0777 /tmp/go-build2860340479
                     \--- 47006 root passwd -v wheel=yes -v invokinguser=jrick -v login=yes -s login -- root daemon (login_passwd)
$ pstree -p 78472 
$ pstree -p 47006 
-+= 00001 root /sbin/init
 \--- 47006 root passwd -v wheel=yes -v invokinguser=jrick -v login=yes -s login -- root daemon (login_passwd)

@jrick
Copy link
Contributor Author

jrick commented Jun 8, 2023

I'm not sure the exact reasons why, but replacing cmd.CombinedOutput() with a call to cmd.Run() allows the test to become skipped properly. Of course, this will no longer capture the stdout/stderr. Setting either Stdout or Stderr to a new(bytes.Buffer) or even io.Discard reintroduces the hang. Something about that authentication process is mucking up reading from the output pipes after su is killed.

@rolandshoemaker
Copy link
Member

This is extremely useful context, thanks!

CombinedOutput actually sets the Stdout and Stderr to buffers, which I think overrides the default pipe behavior of Run, which might be triggering this issue (although that sounds extremely weird on the face to me), but who knows.

I'll take a look at this tomorrow and see if I can come up with a more robust solution that what we have now.

@jrick
Copy link
Contributor Author

jrick commented Jun 9, 2023

Interestingly, no hang with this diff. It did leave the login_passwd process running in the background until I typed another character at the shell, though.

$ got diff
diff /home/jrick/src/go
commit - e827d41c0a2ea392c117a790cdfed0022e419424
path + /home/jrick/src/go
blob - 1d304113d6abf73ae4b5807ffde3f1551f50098f
file + src/runtime/security_test.go
--- src/runtime/security_test.go
+++ src/runtime/security_test.go
@@ -30,8 +30,23 @@ func privesc(command string, args ...string) error {
 	} else {
 		cmd = exec.CommandContext(ctx, "su", highPrivUser, "-c", fmt.Sprintf("%s %s", command, strings.Join(args, " ")))
 	}
-	_, err := cmd.CombinedOutput()
-	return err
+
+	stdoutPipe, err := cmd.StdoutPipe()
+	if err != nil {
+		return err
+	}
+	stderrPipe, err := cmd.StderrPipe()
+	if err != nil {
+		return err
+	}
+	go io.Copy(io.Discard, stdoutPipe)
+	go io.Copy(io.Discard, stderrPipe)
+
+	err = cmd.Run()
+	if err != nil {
+		return err
+	}
+	return nil
 }
 
 const highPrivUser = "root"
$ go test -count=1 -v -run TestSUID runtime 
=== RUN   TestSUID
    crash_test.go:138: running go build -o /tmp/go-build1121432718/testsuid.exe 
Password:    security_test.go:108: unable to set permissions on "/tmp/go-build1121432718", likely no passwordless sudo/su: signal: killed
--- SKIP: TestSUID (5.14s)
PASS
ok  	runtime	5.149s

@bcmills
Copy link
Contributor

bcmills commented Jun 9, 2023

That almost certainly doesn't fix the process leak — it just masks it (because the child process inherits raw FDs instead of the parent process starting copying goroutines).

It looks like at least Linux and OpenBSD sudo support a -n flag to explicitly suppress the prompt — does that work here?

@jrick
Copy link
Contributor Author

jrick commented Jun 9, 2023

sudo is no longer part of the OpenBSD base system. it was replaced by doas. but doas is not generally usable either, it requires manually creating a /etc/doas.conf to enable. an empty config file is enough to enable basic support, and most automated privilege escalation is going to be using it anyways over su. it might make sense to defer to doas similarly to how sudo is preferred for darwin.

@jrick
Copy link
Contributor Author

jrick commented Jun 9, 2023

this works, no hang and it doesn't have to wait the 5s timeout either.

diff /home/jrick/src/go
commit - e827d41c0a2ea392c117a790cdfed0022e419424
path + /home/jrick/src/go
blob - 1d304113d6abf73ae4b5807ffde3f1551f50098f
file + src/runtime/security_test.go
--- src/runtime/security_test.go
+++ src/runtime/security_test.go
@@ -27,6 +27,8 @@ func privesc(command string, args ...string) error {
 	var cmd *exec.Cmd
 	if runtime.GOOS == "darwin" {
 		cmd = exec.CommandContext(ctx, "sudo", append([]string{"-n", command}, args...)...)
+	} else if runtime.GOOS == "openbsd" {
+		cmd = exec.CommandContext(ctx, "doas", append([]string{"-n", command}, args...)...)
 	} else {
 		cmd = exec.CommandContext(ctx, "su", highPrivUser, "-c", fmt.Sprintf("%s %s", command, strings.Join(args, " ")))
 	}

@bcmills
Copy link
Contributor

bcmills commented Jun 12, 2023

@jrick, want to send a CL for your above patch?

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/502575 mentions this issue: runtime: Use doas -n in TestSUID on OpenBSD

@dmitshur dmitshur added this to the Go1.21 milestone Jun 12, 2023
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 12, 2023
@golang golang locked and limited conversation to collaborators Jun 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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-OpenBSD Security Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants