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

seccomp: allow specifying a custom profile with --privileged #47500

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AkihiroSuda
Copy link
Member

- What I did

Fix #47499

- How I did it

Updated daemon/seccomp_linux.go

- How to verify it

$ cat <<EOF >foo.json
{
 "defaultAction": "SCMP_ACT_ALLOW",
 "syscalls": [ { "names": [ "connect" ], "action": "SCMP_ACT_ERRNO" } ]
}
EOF

$ docker run --security-opt seccomp=foo.json --privileged busybox wget -O- http://1.1.1.1/
Connecting to 1.1.1.1 (1.1.1.1:80)
wget: can't connect to remote host (1.1.1.1): Operation not permitted

"Operation not permitted" is the expected error here.

- Description for the changelog

seccomp: allow specifying a custom profile with `--privileged`

- A picture of a cute animal (not mandatory but encouraged)
🐧

@AkihiroSuda AkihiroSuda added this to the 26.0.0 milestone Mar 5, 2024
@AkihiroSuda AkihiroSuda force-pushed the fix-47499 branch 2 times, most recently from 64eb594 to 46119ea Compare March 5, 2024 15:47
Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

I'm not sure this is correct or incorrect yet. As of late we have been discussing how --privileged and LSMs should combine (see the revival of #38075); so I'm registering my -1 for now until we decide how these options should (or should not) combine overall.

@AkihiroSuda
Copy link
Member Author

To clarify the background, I'm proposing this for capturing socket syscalls with SCMP_ACT_NOTIFY to accelerate rootless containers: https://github.com/rootless-containers/bypass4netns

This is not an attempt to harden privileged containers beyond as it is.

Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

Overall LGTM and I think this is correct design.

But we should verify that all the individually controllable options also bundled into --privileged behave the same way. Atm. it looks like they do but it has not been verified and ideally should have individual test cases.

It would also be nice if there was a clear security scope definition(if there isn't already) , for example https://github.com/moby/buildkit/blob/master/PROJECT.md#security-boundary that would make it clear that --privileged is still considered insecure for security, no matter if you also use a different flag as well.

@vvoland vvoland modified the milestones: 26.0.0, 27.0.0 Mar 14, 2024
@thaJeztah
Copy link
Member

@neersighted do you have a strong objection against this one? Overall, I think it's fine to do this, and it seems like a more logical option than silently discarding the user's preference (we should either error or accept, not "discard").

We should make sure that we're clear in the docs what the expectation is though; @dvdksn has a PR pending in the CLI;

I think that one is "good enough" for now, but we should probably outline that even if we accept options like this, a --privileged container won't be able to provide full guarantees, as the reduced security could allow a compromised container to "undo" some of the (security) options that were set. (e.g., escaping the container's filesystem, and modify the container's config).

@neersighted
Copy link
Member

Ah, circling back, we talked about this in a recent maintainers' call, and my takeaway is we probably should allow it, but we'd like to figure out what the semantics for all security-opt combinations with --privileged (ideally making them as consistent as possible) are and add some decent tests here, as the current semantics are quite unclear from the code.

I don't necessarily think we need to block everything on reaching that ideal state, but given there is more and more demand re: combining or decomposing --privileged, I think we need to make this happen sooner than later/spend some serious effort on it (cc @klueska).

}
for _, tc := range testCases {
cID := container.Run(ctx, t, apiClient, tc.ops...)
res, err := container.Exec(ctx, apiClient, cID, []string{"wget", "-O-", "http://1.1.1.1"})
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should be connecting to a local service?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why?

Copy link
Member

Choose a reason for hiding this comment

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

One of the tests is going to hit it, why make it hit an external endpoint?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds complicated, maybe the test should be modified to avoid socket syscalls ( if it is necessary to remove the dependency on 1.1.1.1)

Copy link
Member

Choose a reason for hiding this comment

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

If you have another syscall in mind that seems ok.
The container could also setup netcat or something on localhost.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe the profile could block socket and we don't need to do anything special?

`--privileged --security-opt seccomp=<CUSTOM.json>` was ignoring
`<CUSTOM.json>`.

Fix issue 47499

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--privileged --security-opt seccomp=<CUSTOM.json> ignores <CUSTOM.json>
6 participants