Skip to content

Support for dropping inheritable capabilities #8356

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

Jump to bottom
Merged
merged 1 commit into from
Sep 20, 2023
Merged

Support for dropping inheritable capabilities #8356

merged 1 commit into from
Sep 20, 2023

Conversation

dmcgowan
Copy link
Member

@dmcgowan dmcgowan commented Apr 5, 2023

The inheritable capabilities set should never be larger than the bounding capabilities set. When a base spec is provided with non-empty inheritable capabilities today, the container will fail to run when those capabilities are explicitly dropped. This issue is only seen when a base spec with inheritable capabilities is explicitly provided in the configuration file, the default spec already removed inheritable capabilities in GHSA-c9cp-9c75-9v8c. Since the container is unable to start with this setup, there is no security concern. It may be worth a discussion whether more should be done to warn users who may have set this in their configuration before the GHSA, whether through logging or our documentation.

The error seen when dropping the bounding capabilities to less than the inheritable capabilities is

failed to create containerd task: failed to create shim task: OCI runtime create failed: container_linux.go:380: starting container process caused: apply caps: operation not permitted: unknown

@dmcgowan dmcgowan added cherry-pick/1.6.x Change to be cherry picked to release/1.6 branch cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch labels Apr 5, 2023
@dmcgowan
Copy link
Member Author

dmcgowan commented Apr 5, 2023

Marking this as a cherry-pick candidate since it is a regression

@thaJeztah
Copy link
Member

/cc @corhere @cpuguy83 @rumpl (in case we need changes in moby as well in some form (I'm AFK, can't check right now))

@cpuguy83
Copy link
Member

cpuguy83 commented Apr 5, 2023

Moby doesn't let users supply a base OCI spec to use so I don't think we'd need any changes.

@thaJeztah
Copy link
Member

Thanks Brian!

@samuelkarp
Copy link
Member

It may be worth a discussion whether more should be done to warn users who may have set this in their configuration before the GHSA, whether through logging or our documentation.

I think docs would be a good place to start. @cji, do you have thoughts on whether a warning in the log would be useful or just noisy?

samuelkarp
samuelkarp approved these changes Apr 5, 2023
View reviewed changes
Copy link
Member

@samuelkarp samuelkarp left a comment

Choose a reason for hiding this comment

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

LGTM with one nit

oci/spec_opts.go Outdated
@@ -943,6 +943,11 @@ func WithCapabilities(caps []string) SpecOpts {
s.Process.Capabilities.Bounding = caps
s.Process.Capabilities.Effective = caps
s.Process.Capabilities.Permitted = caps
if caps == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if caps == nil {
if len(caps) == 0 {

I see that filterCaps does effectively drop everything if len(caps)==0 anyway, but it looks a little more explicit in WithCapabilities this way.

@dmcgowan
Support for dropping inheritable capabilities
d0527e2 
Signed-off-by: Derek McGowan <derek@mcg.dev>
@dmcgowan dmcgowan force-pushed the drop-inheritable-capabilities branch from 8b4384f to d0527e2 Compare April 6, 2023 00:47
@cji
Copy link
Contributor

cji commented Apr 6, 2023

It may be worth a discussion whether more should be done to warn users who may have set this in their configuration before the GHSA, whether through logging or our documentation.

I think docs would be a good place to start. @cji, do you have thoughts on whether a warning in the log would be useful or just noisy?

I do like the idea of a warning since if I understand this change correctly, instead of failing to start a container with this configuration, it will now properly limit the bounding set silently. Giving a warning to the user that may not otherwise realize the capabilities their container has aren't what they expected sounds useful for any debugging they might end up having to do.

@samuelkarp
Copy link
Member

I do like the idea of a warning since if I understand this change correctly, instead of failing to start a container with this configuration, it will now properly limit the bounding set silently.

It'll properly limit the inheritable set. Note that this only applies if inheritable caps have been set, either explicitly or through an overridden base spec (like the CRI plugin supports).

Giving a warning to the user that may not otherwise realize the capabilities their container has aren't what they expected sounds useful for any debugging they might end up having to do.

I'm wondering too: should we warn for all containers (which could be quite noisy), or only warn at startup if an overridden base_runtime_spec in the CRI plugin has inheritable caps specified? I'd lean toward the second since it'll reduce noise.

@cji
Copy link
Contributor

cji commented Apr 7, 2023

I'm wondering too: should we warn for all containers (which could be quite noisy), or only warn at startup if an overridden base_runtime_spec in the CRI plugin has inheritable caps specified? I'd lean toward the second since it'll reduce noise.

+1 on the second approach as well.

@dmcgowan
Copy link
Member Author

dmcgowan commented Apr 7, 2023

Created #8364 to track. That change should not be a part of this PR since this PR is focused on the bug fix.

@samuelkarp
Copy link
Member

That change should not be a part of this PR since this PR is focused on the bug fix.

Agreed, that's why I approved this one already.

@dmcgowan dmcgowan requested a review from fuweid September 20, 2023 00:29
fuweid
fuweid approved these changes Sep 20, 2023
View reviewed changes
Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

@fuweid fuweid merged commit 782ad19 into containerd:main Sep 20, 2023
@dmcgowan dmcgowan deleted the drop-inheritable-capabilities branch April 20, 2024 00:45
This was referenced Jul 16, 2024
Merged
Merged
@samuelkarp samuelkarp added cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch and removed cherry-pick/1.6.x Change to be cherry picked to release/1.6 branch cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch labels Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samuelkarp samuelkarp samuelkarp approved these changes

@fuweid fuweid fuweid approved these changes

Assignees
No one assigned
Labels
cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch
Projects
Pull Request Review
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dmcgowan @thaJeztah @cpuguy83 @samuelkarp @cji @fuweid