Closed jrick closed 1 year ago
Which version of openbsd is this? The test seems to pass in our builders, which seem to be 7.2.
$ 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
@rolandshoemaker
(attn @golang/openbsd)
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.
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?
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:
OpenBSD su
uses the BSD Authentication system
$ 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
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.
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)
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.
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.
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
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?
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.
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, " ")))
}
@jrick, want to send a CL for your above patch?
Change https://go.dev/cl/502575 mentions this issue: runtime: Use doas -n in TestSUID on OpenBSD
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat 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 theruntime
testTestSUID
.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?
The test eventually times out after the default 10m deadline.