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

capability: Apply: deny for another process #174

Merged
merged 1 commit into from
Oct 30, 2024

Conversation

kolyshkin
Copy link
Collaborator

@kolyshkin kolyshkin commented Oct 22, 2024

Current version of capsV3.Apply has a major problem: if you do something like this:

	c, err := capability.NewPid(pid)
	...
	c.Set(capability.AMBIENT, capability.CAP_CHOWN)
	c.Apply(capability.AMBIENT)

then the ambient capability will be applied to the current process, rather than the process identified by pid. Same issue for BOUNDS.

For CAPS the situation is slightly different: capset(2) man page says:

EPERM The caller attempted to use capset() to modify the capabilities
of a thread other than itself, but lacked sufficient privilege.
For kernels supporting VFS capabilities, this is never permitted.

Here kernels supporting VFS capabilities means most kernels >= v2.6.24, and all kernels >= v2.6.33. Since Go 1.18+ only supports Linux >= v2.6.32, this pretty much means "all kernels".

Meaning, Apply(CAPS) with non-zero pid will try capset(2) and return EPERM.

Let's return an error early if pid is set in Apply, and add a test case.

Fixes: #168
Closes: #171

Current version of capsV3.Apply has a major problem: if you do something
like this:

	c, err := capability.NewPid(pid)
	...
	c.Set(capability.AMBIENT, capability.CAP_CHOWN)
	c.Apply(capability.AMBIENT)

then the ambient capability will be applied to the current process,
rather than the process identified by pid. Same issue for BOUNDS.

For CAPS the situation is slightly different: capset(2) man page says:

> EPERM	The caller attempted to use capset() to modify the capabilities
> 	of a thread other than itself, but lacked sufficient privilege.
> 	For kernels supporting VFS capabilities, this is never permitted.

Here "kernels supporting VFS capabilities" means most kernels >= v2.6.24,
and all kernels >= v2.6.33. Since Go 1.18+ only supports Linux >= v2.6.32,
this pretty much means "all kernels".

Meaning, Apply(CAPS) with non-zero pid will try capset and return EPERM.

Let's return an error early if pid is set in Apply, and add a test case.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin kolyshkin changed the title capabilities: Apply: deny for another process capability: Apply: deny for another process Oct 22, 2024
@lifubang
Copy link
Contributor

Meaning, Apply(CAPS) with non-zero pid will try capset(2) and return EPERM.

But they also said:
That is, on
kernels that have VFS capabilities support, when calling
capset(), the only permitted values for hdrp->pid are 0 or,
equivalently, the value returned by gettid(2).

but lacked sufficient privilege.

It means this is permitted If we have a sufficient privilege?

@kolyshkin
Copy link
Collaborator Author

Meaning, Apply(CAPS) with non-zero pid will try capset(2) and return EPERM.

But they also said: That is, on kernels that have VFS capabilities support, when calling capset(), the only permitted values for hdrp->pid are 0 or, equivalently, the value returned by gettid(2).

Theoretically, we should also allow settings CAPS when pid == os.Gettid(). Practically, since using a TID has the exactly same result as using 0 (in other words, there is no additional functionality when one uses a TID), it is easier for the library to just require 0 here and not handle the TID special case.

but lacked sufficient privilege.

It means this is permitted If we have a sufficient privilege?

It says "For kernels supporting VFS capabilities, this is never permitted". This means that we never have a sufficient privilege. You can read the whole man page at e.g. https://man7.org/linux/man-pages/man2/capset.2.html (the other good source of information is https://man7.org/linux/man-pages/man7/capabilities.7.html).

@kolyshkin
Copy link
Collaborator Author

@lifubang thank you for review, let me know what you think.

@kolyshkin
Copy link
Collaborator Author

Merging this as we need to move forward

@kolyshkin kolyshkin merged commit f261e83 into moby:main Oct 30, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] capability: some errors related to pid
2 participants