Fix handling of file capabilities. #11704
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes the handling of file capabilities by changing how file capability data is retrieved, parsed, and applied across various components, while ensuring compatibility with Linux behavior. Key changes include:
- Refactoring test cases and variable usage in the iptables and e2e tests to use a unified runtime variable and combinatorial test loops.
- Updating capability extraction functions and replacing deprecated bits-based operations with new auth-based capability set methods.
- Introducing a new function to handle privileged root users and updating related build file dependencies.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/iptables/nftables_test.sh | Updated test script to use the environment variable RUNTIME instead of a positional runtime argument. |
| test/e2e/exec_test.go | Refactored TestFileCap with nested loops for combinatorial testing and added new tests for non-zero root ID and effective flag handling. |
| runsc/specutils/specutils.go | Replaced AllCapabilitiesUint64 with AllCapabilitiesSet and removed the bits dependency. |
| runsc/container/container_test.go | Updated capability mask conversion for CAP_NET_RAW using the new auth API. |
| pkg/sentry/loader/loader.go | Improved error handling for file capability retrieval by checking for EOPNOTSUPP. |
| pkg/sentry/kernel/kernel.go | Modified file capability application logic to integrate with new helper functions. |
| pkg/sentry/kernel/auth/capability_set*.go | Added Add and Clear methods on CapabilitySet; updated CapsFromVfsCaps to return an extra boolean and handle EPERM correctly. |
| pkg/sentry/kernel/auth/BUILD | Adjusted visibility and dependency updates in the authentication package build. |
| Makefile | Revised the test invocation command for nftables tests. |
| # The image passed to docker run uses iptables-nft by default, so the above | ||
| # rules can't be simply scraped and passed to gVisor. We test that those rules | ||
| # are correctly translated to iptables-legacy rules. | ||
| net_name="nftables-test-net-$(shuf -i 0-99999999 -n 1)" | ||
| docker network create "$net_name" | ||
| trap "docker network rm \"$net_name\"" EXIT | ||
| got=$(docker run --network="$net_name" --rm --runtime "$runtime" --privileged gvisor.dev/images/iptables iptables-legacy -t nat -S) | ||
| got=$(docker run --network="$net_name" --rm --runtime "$RUNTIME" --privileged gvisor.dev/images/iptables iptables-legacy -t nat -S) |
There was a problem hiding this comment.
[nitpick] Consider adding a comment explaining the change from using a positional 'runtime' argument to the environment variable 'RUNTIME' for clarity in the test setup.
Copilot uses AI. Check for mistakes.
| for _, success := range []bool{true, false} { | ||
| for _, useTmpfs := range []bool{true, false} { | ||
| for _, rootUser := range []bool{true, false} { | ||
| tcName := "success" |
There was a problem hiding this comment.
[nitpick] Consider adding brief inline comments to clarify how the test case names are constructed from the nested boolean loops to improve readability.
Copilot uses AI. Check for mistakes.
| (CapabilitySet(capData.Inheritable()) & creds.InheritableCaps) | ||
| // by applying the file capability that is specified by capData. It also | ||
| // returns a boolean indicating whether the file capability is applied. | ||
| func CapsFromVfsCaps(vfsCaps linux.VfsNsCapData, creds *Credentials) (*Credentials, bool, error) { |
There was a problem hiding this comment.
Update the function comment to explicitly document the meaning of the returned boolean, indicating whether the file capabilities were applied.
Copilot uses AI. Check for mistakes.
45b428f to
5e5f515
Compare
All the following changes are compatible with what Linux does. - Do not attempt to apply file capabilities when it is not present. - If the rootID specified in v3 file capability struct does not own the current userns, then file capabilities are not applied. - While applying file capabilities, EPERM is only returned if VFS_CAP_FLAGS_EFFECTIVE is set. - Re-raise capabilities of root user as per capabilities(7). Compare security/commoncap.c:handle_privileged_root(). PiperOrigin-RevId: 761725759
5e5f515 to
a178e09
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix handling of file capabilities.
All the following changes are compatible with what Linux does.
current userns, then file capabilities are not applied.
VFS_CAP_FLAGS_EFFECTIVE is set.
security/commoncap.c:handle_privileged_root().