Skip to content

libct: don't close syncPipe #4183

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

Closed
kolyshkin wants to merge 1 commit into from
Closed

libct: don't close syncPipe #4183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion libcontainer/setns_init_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ func (l *linuxSetnsInit) Init() error {
// that all O_CLOEXEC file descriptors have already been closed and thus
// the second execve(2) from runc-dmz cannot access internal file
// descriptors from runc.
if err := utils.UnsafeCloseFrom(l.config.PassedFilesCount + 3); err != nil {
if err := utils.UnsafeCloseFrom(l.config.PassedFilesCount+3, int(l.pipe.File().Fd())); err != nil {
Copy link
Member

@rata rata Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@kolyshkin kolyshkin Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

@rata rata Feb 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect!

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

return err
}
return system.Exec(name, l.config.Args, os.Environ())
Expand Down
2 changes: 1 addition & 1 deletion libcontainer/standard_init_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ func (l *linuxStandardInit) Init() error {
// that all O_CLOEXEC file descriptors have already been closed and thus
// the second execve(2) from runc-dmz cannot access internal file
// descriptors from runc.
if err := utils.UnsafeCloseFrom(l.config.PassedFilesCount + 3); err != nil {
if err := utils.UnsafeCloseFrom(l.config.PassedFilesCount+3, int(l.pipe.File().Fd())); err != nil {
return err
}
return system.Exec(name, l.config.Args, os.Environ())
Expand Down
9 changes: 7 additions & 2 deletions libcontainer/utils/utils_unix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,17 +120,22 @@ func runtime_IsPollDescriptor(fd uintptr) bool //nolint:revive

// UnsafeCloseFrom closes all file descriptors greater or equal to minFd in the
// current process, except for those critical to Go's runtime (such as the
// netpoll management descriptors).
// netpoll management descriptors), and the explicitly specified ones.
//
// NOTE: That this function is incredibly dangerous to use in most Go code, as
// closing file descriptors from underneath *os.File handles can lead to very
// bad behaviour (the closed file descriptor can be re-used and then any
// *os.File operations would apply to the wrong file). This function is only
// intended to be called from the last stage of runc init.
func UnsafeCloseFrom(minFd int) error {
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.
return fdRangeFrom(minFd, func(fd int) {
for _, ex := range except {
if fd == ex {
Copy link
Member

@rata rata Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@kolyshkin kolyshkin Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

@kolyshkin kolyshkin Feb 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Member

@rata rata Feb 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  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.

Copy link
Contributor Author

@kolyshkin kolyshkin Feb 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  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.

  1. 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.

  1. 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.

Copy link
Member

@rata rata Feb 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

return
}
}
if runtime_IsPollDescriptor(uintptr(fd)) {
// These are the Go runtimes internal netpoll file descriptors.
// These file descriptors are operated on deep in the Go scheduler,
Expand Down