Implement seccomp notify support

[mauriciovasquezbernal]

Add support for notify seccomp actions. Seccomp profiles can now specify the SCMP_ACT_NOTIFY action that notifies a user space application (seccomp agent) when such syscall is performed. The seccomp agent can, in turn, decide to perform the syscall on behalf of the container and set the syscall return values or even inject file descriptors to the container. This, for example, allows for unprivileged containers to run some privileged syscalls that the seccomp agent performs on its behalf.

The format and more details can be found in the OCI runtime specification, in the seccomp section. If you are a developer and want more information, check out the example agent in runc, the linux kernel documentation, libseccomp-golang and Christian Brauner's blog post. To use this feature you need Linux >= 5.7 and libseccomp >= 2.5.0.

[kolyshkin]

CI failure is a known (but rare) flake, see #2425

=== RUN   TestExecInTTY
    execin_test.go:349: unexpected carriage-return in output
--- FAIL: TestExecInTTY (0.15s)

(restarted the job)

[vbatts]

opencontainers/runtime-spec#1074 merged now

[rata]

Hi! I'm a coworker of @mauriciovasquezbernal, I'll coordinate with him to fix the conflicts in this PR and move it to a regular PR (not draft) when ready! :)

[rata]

Hi!

I’ve rebased the PR and fixed all the conflicts, all tests should be green too.

The OCI runtime-spec change this patch implements was recently merged and I see that the spec change is already merged in runc too. Should we mark this PR for 1.0 milestone? Currently it is in the post 1.0 milestone.

I’m keeping the PR as a draft for now, as the libseccomp-golang PR is not yet merged and we need it for this PR (not only for tests, we use some consts in core runc too). However, changes on the libseccomp PR will have minimal (most likely none) consequences to this code. So, we keep it as draft until that PR is merged, but please review whenever you can :)

[rata]

Updated, fixed a conflict in go.sum too

[rata]

Rebased on top of latest master

[kolyshkin]

Perhaps it makes sense to consolidate this code since it's repeated twice. Something like

seccompFd, err := recvSeccompFd(childPid, sync.Fd)

maybe?

[rata]

Yeap, good call. I'm moving only these bits to the new function and not the if p.config.Config.Seccomp.ListenerPath == "" part as we need p for that and it would be called with p being different structs. Also, IMHO, semantically seems better to keep both things separate

[kolyshkin]

1. \n should be removed.
2. Please to use %w for error.

[kolyshkin]

3. I think net.Dial already includes path/address in the error message, as well as the error (dial), so there's no need to wrap it.

[rata]

Cool, returning err then

[alban]

I think net.Dial already includes path/address in the error message, as well as the error (dial), so there's no need to wrap it.

Correct:

$ cat main.go 
package main

import (
	"fmt"
	"net"
)

func main() {
	_, err := net.Dial("unix", "/fdsjkfhdjk")
	if err != nil {
		fmt.Printf("%s\n", err)
	}
}
$ go run main.go 
dial unix /fdsjkfhdjk: connect: no such file or directory

[kolyshkin]

still LGTM :)

[kolyshkin]

@AkihiroSuda this needs your still-LGTM after a rebase.

@cyphar PTAL

[rata]

@AkihiroSuda if it helps, the rebase were to solve very simple conflicts (the makefile has a new target now, needed to adjust for that, some bats helpers conflicts but trivial, and things like that)

[cyphar]

Looked through it and it all seems very reasonable -- kolyshkin has probably reviewed this several times more thoroughly than I have. 😉

I only have one question about the seemingly arbitrary write restriction.

[cyphar]

Is there a reason for this restriction? I can imagine how it might be unsafe (or unwise) to do this on paper, but is it really necessary to disallow this within runc?

[rata]

Yes, basically it will hang for ever if we allow it. We need the write syscall to write the seccomp fd plain number over the pipe to the parent (we write to the pipe here), then the parent gets the fd with pifd_getfd and sends it over the unix socket to the seccomp agent.

If the write syscall has the notify action, we can't never send a msg to the parent, which will mean the seccomp agent won't get the seccomp fd and the write will be blocked forever.

However read/close (functions that we also use to sync with parent via the pipe) are not restricted as it is possible to send the seccomp fd to the parent, the seccomp agent will get the seccomp fd and the first few notifications it will get if read/close are handled are for the ones done in syncParentSeccomp() (and quite trivially allow to proceed syscalls on fds that are not known by the agent, for example).

Please note the parent is able to proceed, while we are blocked in read/close, because the parent is another process and not subject to the seccomp filter.

I will add a comment to explain this in the code, if that sounds good to you @cyphar

[rata]

@cyphar Added a comment explaining this here

[cyphar]

Ah of course. Yeah that's a bit sad :/ -- but we can always figure out some workaround for this in the future. (I wonder how the LXC folks worked around this problem -- though I think they don't have a similar write-based synchronisation model since they're written in C which means you don't need to do so much busy work.)

[rata]

@cyphar I've asked Christian Brauner in the past. It seems they use clone3 sharing the fds table and use a fixed number for the seccomp notify fd. Then, they get it with pidfd_getfd().

Not sure how LXC uses a fixed fd number for seccomp notify, though.

[cyphar]

You could just pick an arbitrarily high fd and use dup3 to clone it to the right fd number. It might be interesting to look into whether we could do it with pidfd_getfd though since we'd need to have a fallback for quite a while, it'd probably complicate the code a bit too much...

[rata]

Oh, right, that was easy. Yeah, for runc I think is simpler to use the pipe, that is the main sync mechanism used tree-wide. We are using pidfd_getfd here, though: we sync with the parent to let them know when the fd is ready and send the fd plain number, then the parent gets it with pidfd_getfd

[cyphar]

Ah, I forgot that we are requiring Linux 5.7 or newer for the feature -- pidfd_getfd isn't available for the earlier kernels with seccomp notify support.

[rata]

And 5.7 is also enforced by libseccomp-golang, that only supports notification with tsync (requires libseccomp api level 6). This is as the vast majority of go programs will need tsync too. Here is the conversation we had with upstream about this when writing the patches for libseccomp-golang

Before 5.7, notify and tsync where mutually exclusive (torvalds/linux@7a0df7f). From Linux 5.7 introduced a new API to enable seccomp notification with tsync together in this commit torvalds/linux@5189149.

[rata]

Rebased to fix conflicts (a simple conflict with 64358c4 that moved some code we are touching) and added a comment to explain the write restriction @cyphar commented on (see #2682 (comment)).

@kolyshkin @AkihiroSuda @dqminh can you please re-LGTM this? :)

@cyphar thanks for the comment! I guess this is ready to merge? Or is there anything else I can do?

[kolyshkin]

(I wish github had a way to see the difference between old and new commits, e.g. similar to what gerrit does)

Trusting the changes are mostly cosmetical, still LGTM

[cyphar]

LGTM, thanks for working on this for so long!

[rata]

@cyphar @kolyshkin @AkihiroSuda qminh @vbatts thanks for your time and reviews! :)

[rata]

@kolyshkin I've updated the release notes for this. I realized I forgot to mention the seccomp agent example in the runc source code! Updated to mention that now.