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

proposal: runtime: add a way to check if fd is used by go runtime #67639

Closed
kolyshkin opened this issue May 24, 2024 · 16 comments
Closed

proposal: runtime: add a way to check if fd is used by go runtime #67639

kolyshkin opened this issue May 24, 2024 · 16 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Proposal
Milestone

Comments

@kolyshkin
Copy link
Contributor

Proposal Details

While working on fixing CVE-2024-21626, we had to implement a function that closes all file descriptors above a specific one (similar to what close_range(2) does). Once we did that, we found out that closing go's internal poll fds results in subsequent panic from go runtime, so we had to exclude those.

The only way possible way to do so was to use internal/poll.IsPollDescriptor (via go:linkname kludge). You can see the code doing this here (initially added by this commit).

We'd like this functionality as a part of public Go API. Perhaps something like this:

package runtime

// IsInternalDescriptor reports whether fd is a
// descriptor being used by Go runtime internally.
func IsInternalDescriptor(fd uintptr) bool

Cc @cyphar

@gopherbot gopherbot added this to the Proposal milestone May 24, 2024
@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals May 24, 2024
@ianlancetaylor ianlancetaylor added the compiler/runtime Issues related to the Go compiler and/or runtime. label May 24, 2024
@kolyshkin
Copy link
Contributor Author

Adding a bit of context here as the CVE text referenced above is quite long.

There exists a container-specific (chroot-specific) attack vector, for which having CLOEXEC flag for an open (directory) FD is not a remediation. Those FDs had to be explicitly closed before spawning a child.

@kolyshkin
Copy link
Contributor Author

In addition to the above, Go 1.23 adds pidfd support for Linux, so there will be another class of file descriptors used internally by Go runtime -- pidfds. Those are probably also need to be reported.

@cyphar
Copy link

cyphar commented Aug 21, 2024

@kolyshkin Are the pidfds used internally or are they all just associated with things Go program users need? If they're not used by the Go runtime internally, we can just close_range them like everything else AFAICS?

@rsc
Copy link
Contributor

rsc commented Aug 28, 2024

Can this be explained better without reference to the very large CVE text?
What fd's exactly need to be covered?
If you close the epoll fd then yes it breaks epoll.
But if you close the cached /dev/random fd that will break crypto/rand.
Don't you need to know about that fd too?
And what about other fds that are cached by other packages?
I don't understand why "used by runtime" is a meaningful line.
What about other packages with cached fds?
Don't we want to not break those too?

@kolyshkin
Copy link
Contributor Author

@kolyshkin Are the pidfds used internally or are they all just associated with things Go program users need?

It is used internally by os, even if specified by a user (the latter case is being fixed by https://go.dev/cl/607695

Can this be explained better without reference to the very large CVE text?

Sorry for being brief before, please see below for answers. The summary is: we need a way to close all file descriptors
above a specific number (very similar to what close_range(2) does on Linux), excluding those that are owned by Go runtime. Note that having CLOEXEC bit set on those FDs is not enough -- we have a peculiar requirement to explicitly close them before the exec happens.

What fd's exactly need to be covered?

Those that are owned (were opened and used internally) by the go runtime, [and are essential for go runtime to function properly]. (1)

(I'm not entirely sure if the square bracketed part should be part of the definition or not).

If you close the epoll fd then yes it breaks epoll.

I'd like to add it breaks it badly (runtime panics).

But if you close the cached /dev/random fd that will break crypto/rand.
Don't you need to know about that fd too?
And what about other fds that are cached by other packages?
I don't understand why "used by runtime" is a meaningful line.

Yes, I just realized that I made a mistake above suggesting that we include pidfd into the same category, crossing the line between "fds owned by go runtime" (as in pollfd) and "fds owned by go standard library" (as in a pidfds used by os, or /dev/random fd used by crypto/rand).

I think we only need ones from the first category only (as defined in (1) above). If we'll also need those owned by packages from the standard library, I guess those packages can add a way to identify if a certain fd is owned by them. (2)

What about other packages with cached fds?
Don't we want to not break those too?

I guess, similar to (2) above, those package could have similar interfaces / ways to figure out if an fd is owned by them.

@rsc rsc moved this from Incoming to Active in Proposals Aug 29, 2024
@rsc
Copy link
Contributor

rsc commented Aug 29, 2024

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@ianlancetaylor
Copy link
Member

@kolyshkin Can you explain why there is a distinction between descriptors owned by the Go runtime, and descriptors owned by other parts of the Go standard library, and, for that matter, descriptors owned by other packages that may be part of the program? When does your program need to close all the descriptors? Why is it OK to close a descriptor held by some package but not OK to close a descriptor held by the runtime? Thanks.

@kolyshkin
Copy link
Contributor Author

Can you explain why there is a distinction between descriptors owned by the Go runtime, and descriptors owned by other parts of the Go standard library, and, for that matter, descriptors owned by other packages that may be part of the program? When does your program need to close all the descriptors? Why is it OK to close a descriptor held by some package but not OK to close a descriptor held by the runtime? Thanks.

I think that for some scenarios it might be beneficial to know if an fd is used by some package. In our use case (runc), though, we're closing all fds above a specific one right before calling exec(2) and thus are not really interested if any go packages will stop working. Essentially, we just need runtime to not panic.

@rsc
Copy link
Contributor

rsc commented Sep 4, 2024

It sounds like you have a loop over fds and want to close all of them except the runtime epoll descriptor. Assuming your program does not have other epoll descriptors, or assuming that the security problem causing you to pre-close fds is itself not about some other epoll descriptor, then it seems like it would be enough to just not close any epoll descriptors in that loop.

Is there some way to ask Linux "is this an epoll descriptor?" and then just skip those?

@mateusz834
Copy link
Member

mateusz834 commented Sep 4, 2024

I wonder whether this could be detected via an error code from epoll api, so creating a fd and using these two errors:

man epoll_ctl(2):

EINVAL epfd is not an epoll file descriptor, or fd is the same as
epfd, or the requested operation op is not supported by
this interface.

ENOENT op was EPOLL_CTL_MOD or EPOLL_CTL_DEL, and fd is not
registered with this epoll instance

So while trying to remove a fd (created only for epoll detection), i guess we would get EINVAL when the fd is not an epoll, and ENOENT when it is an epoll.

@cyphar
Copy link

cyphar commented Sep 4, 2024

So, this "close all fds" code is run right before doing execve so for our particular usecase what we need is to know which file descriptors the Go runtime uses internally (if we close the epoll descriptors, you get crashes -- which is bad). For our particular usecase, it doesn't matter if any other long-lived file descriptors in the process get closed -- the process is about to exec anyway. The main thing we're concerned with is avoiding crashes (which a naive implementation that closes the Go runtime epoll descriptors causes).

You might be thinking "why not just use O_CLOEXEC?". Well, we actually do use O_CLOEXEC but it turns out there are attacks you can do against container runtimes where O_CLOEXEC is not sufficient (in short, because the file descriptors are open during exec you can reference them as the target of the exec and thus keep the file descriptor open, leading to severe container breakouts as in CVE-2024-21626). To protect against this, we need to close as many file descriptors as possible before execve so an attacker can't pin any useful file descriptors.

I appreciate this is a very niche thing, but we currently need //go:linkname to do this (and we are on the Wall of Shame ™️ as a result) and so finding a supported way of doing this would be nice. Another alternative would be for us to just skip all epoll descriptors we see (as suggested by @mateusz834, and one of our draft patches did do this). However, it's possible that future Go versions will have some other long-lived file descriptor we cannot close without crashing the program, and so having this be a supported thing would be preferable.

@ianlancetaylor
Copy link
Member

As discussed above, arbitrary Go packages can hold file descriptors open. That doesn't matter for your use case, because you presumably only have a single goroutine running and you only care about descriptors used by the runtime package that are not under your control. So this is a very very special purpose use case. Only people with this exact use case are going to care about descriptors used by the runtime and not by other packages.

At the same time, it's hard to imagine that the runtime will ever use any other descriptors. It's possible, of course. Many things are possible. But it seems very unlikely.

So we have the combination of two unlikely cases. That is not a good argument for new API that needs to be documented and maintained forever. If there is some other workaround, we should use that.

@maxatome
Copy link

maxatome commented Sep 9, 2024

@kolyshkin you probably wants something different.
When you close all fds, you get a panic from the go runtime: OK.
But, if I understand well, at this stage, you no longer need the go runtime anymore because once you have closed all the fds you will immediately call exec.
So what you need is a way to tell the runtime to shutdown, so you can close all the fds you find?

@rsc
Copy link
Contributor

rsc commented Sep 11, 2024

Another alternative would be for us to just skip all epoll descriptors we see (as suggested by @mateusz834, and one of our draft patches did do this). However, it's possible that future Go versions will have some other long-lived file descriptor we cannot close without crashing the program, and so having this be a supported thing would be preferable.

If skipping all the epolls works, then I would strongly suggest doing that. If this hypothetical about a different long-lived file descriptor comes to pass, we can always revisit then. It doesn't seem worth introducing this concept before we need it.

@rsc
Copy link
Contributor

rsc commented Sep 11, 2024

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Decline in Proposals Sep 11, 2024
@gopherbot gopherbot moved this from Likely Decline to Declined in Proposals Sep 18, 2024
@gopherbot
Copy link
Contributor

gopherbot commented Sep 18, 2024

No change in consensus, so declined.
— aclements for the proposal review group

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Proposal
Projects
Status: Declined
Development

No branches or pull requests

7 participants