libct: don't close syncPipe

[kolyshkin]

Recent CVE-2024-21626 fix (commit f2f1621) broke a recently added test (commit 0bc4732, #4173) because if exec fails, runc is unable to communicate the error back to the parent.

Let's not close syncPipe.

This fixes the following test failure:

# bats tests/integration/exec.bats 
...
 ✗ RUNC_DMZ=legacy runc exec [execve error]
   (in test file tests/integration/exec.bats, line 340)
     `[ ${#lines[@]} -eq 1 ]' failed
   runc spec (status=0):
   
   runc run -d --console-socket /tmp/bats-run-77CvQx/runc.JurXM2/tty/sock test_busybox (status=0):
   
   runc exec -t test_busybox /run.sh (status=255):
   writing sync procError: write sync: bad file descriptor
   exec /run.sh: no such file or directory

[rata]

Is this fd created with the close on exec option? I guess so, but it will be nice to verify it :)

[kolyshkin]

Yes it is:

runc/libcontainer/sync_unix.go

Line 77 in 0212048

fds, err := unix.Socketpair(unix.AF_LOCAL, unix.SOCK_SEQPACKET|unix.SOCK_CLOEXEC, 0)

[rata]

Perfect!

Sorry, something changed and while I can answer now, I can't mark this as resolved.

[rata]

Shall we try to check if the fd in question has the closeOnExec flag set before leaving it open?

[kolyshkin]

First, having close on exec flag is not a panacea.
Second, the one we're excepting is not a file or directory, so no CVE-2024-21626 style exploits are possible.

Maybe we can check that excepted fd is not referring to a file or directory...

[kolyshkin]

Something like this? Or is this check totally redundant? WDYT @rata?

diff --git a/libcontainer/utils/utils_unix.go b/libcontainer/utils/utils_unix.go
index 87055358..4daa40d4 100644
--- a/libcontainer/utils/utils_unix.go
+++ b/libcontainer/utils/utils_unix.go
@@ -130,6 +130,18 @@ func runtime_IsPollDescriptor(fd uintptr) bool //nolint:revive
 func UnsafeCloseFrom(minFd int, except ...int) error {
        // We cannot use close_range(2) even if it is available, because we must
        // not close some file descriptors.
+
+       // Currently we only allow sockets to be excepted.
+       for _, fd := range except {
+               var st unix.Stat_t
+               err := unix.Fstat(fd, &st)
+               if err != nil {
+                       return os.NewSyscallError("fstat", err)
+               }
+               if st.Mode&unix.S_IFMT != unix.S_IFSOCK {
+                       return fmt.Errorf("excepted fd %d is not a socket", fd)
+               }
+       }
        return fdRangeFrom(minFd, func(fd int) {
                for _, ex := range except {
                        if fd == ex {

[rata]

1. What do you mean with "close on exec is not a panacea"? That we don't know if this is a socket, dirfd or whatever, so checking for close on exec can vary?
2. Can't fstat() be costly?
3. Why wouldn't we check that even the socket is close on exec? We don't want to leak socket fds either, right? Sure, the implications might be different of leaking a socket, but why would we want to leak a socket?

But yeah, if it is because of 1, and it isn't easy to do a type assertion or something and check the close on exec, I think we can spare the fstat() call, this diff you pasted and just keep the code as it is.

I would add a big note to the function documentation, saying the fds excepted must be manually verified to be closed on exec, though.

[kolyshkin]

1. What do you mean with "close on exec is not a panacea"?

What I mean by it is, in addition to having O_CLOEXEC set, we also have to explicitly close file descriptors before calling execve. See "Attacks 3a and 3b" in GHSA-xr7r-f8xq-vfvv (in short, something like process.args=/proc/self/fd/7/../../../bin/bash can be used by an attacker).

So, checking the O_CLOEXEC won't be enough here, this is why I'm checking that it's a socket (i.e. not a file or directory), as one can't use "attacks 3a/3b" with a socket.

2. Can't fstat() be costly?

Indeed it can (and it can also fail for some reason). That is why I don't like the checking code. We already know it's a socket when we call this function.

3. Why wouldn't we check that even the socket is close on exec?

Because close on exec is not a panacea (see above).

I would add a big note to the function documentation, saying the fds excepted must be manually verified to be closed on exec, though.

I agree about a note (not about close on exec) -- will add.

[rata]

Oh, right, great point. If we go forward with this, I think a comment is all that is needed (explaining which kind of fds is safe to leave open).

However, the other PR to just close the fd seems better IMHO. We don't risk anything, is simpler, we still enforce all fds are closed...

[lifubang]

We have discussed this topic in a private fork, how about close it explicitly using _ = l.pipe.Close() like what had done in standard_init_linux.go? One benifit is that we can have the same implementation on both create and exec.

[sctb512]

I also found the logpipe is closed when I enable debug. Perhaps some test cases are missing?

DEBU[0000]libcontainer/sync.go:127 libcontainer.doReadSync() reading sync
DEBU[0000] child process in init()
DEBU[0000] setns_init: about to exec
DEBU[0000]libcontainer/sync.go:131 libcontainer.doReadSync() sync pipe closed
Failed to write to log, write logpipe: file already closed
writing sync procError: write sync: bad file descriptor
exec /run.sh: no such file or directory
DEBU[0000]signals.go:92 main.(*signalHandler).forward() process exited

[kolyshkin]

@sctb512 please don't hijack this -- this PR is about a totally different issue than the one you have. If you feel there's a bug, open a bug report (or a PR to fix it). If you have a question -- we have /~https://github.com/opencontainers/runc/discussions

[kolyshkin]

Closing in favor of #4192 which is simpler.