Update mctpd and example configs to minimise necessary permissions#163
Open
SafetyInObscurity wants to merge 2 commits into
Open
Update mctpd and example configs to minimise necessary permissions#163SafetyInObscurity wants to merge 2 commits into
SafetyInObscurity wants to merge 2 commits into
Conversation
SafetyInObscurity
force-pushed
the
noroot
branch
3 times, most recently
from
5ff82b7 to
43bfb81
Compare
SafetyInObscurity
force-pushed
the
noroot
branch
4 times, most recently
from
aac0693 to
8e0724f
Compare
Allow mctpd, which should be an unprivileged user, to run the daemon with minimum capabilities. mctpd needs CAP_NET_BIND_SERVICE and CAP_NET_ADMIN, so those have been added to AmbientCapabilities in the service config. The mctpd user has been allowed to own the au.com.codeconstruct.MCTP1 object as well as root. root also retains the right to own the object, meaning the config remains valid if the daemon is run as root. Unprivileged users can only access properties and are prohibited from configuring the network. Root and users in the mctp-admin group are permitted to interact with any method the daemon provides. For some setups, it may be useful to create a group with permissions to use the RegisterTypeSupport or RegisterVDMTypeSupport methods so that programs may register support without being given permission to modify the network characteristics. Signed-off-by: James Lee <james@codeconstruct.com.au>
SafetyInObscurity
force-pushed
the
noroot
branch
8 times, most recently
from
9f33051 to
df9fb86
Compare
jk-ozlabs
reviewed
Jul 8, 2026
jk-ozlabs
left a comment
jk-ozlabs
left a comment
Member
There was a problem hiding this comment.
Looks good! A couple of minor comments inline, and a bit of an open-ended query.
| ] + test_ops_sources + netlink_sources + util_sources, | ||
| include_directories: include_directories('src'), | ||
| dependencies: [libsystemd, toml_dep], | ||
| dependencies: [libsystemd, toml_dep, libcap], |
Member
There was a problem hiding this comment.
it might be nice to prevent the test-mctpd process from attempting to alter capabilities - its ops->bind() doesn't really do a bind(), as it's a mock implementation, and so it shouldn't have any capabilities in the first place. We could do the caps change as a separate op, but that seems pretty heavy-handed.
Any ideas to handle that?
[I am assuming that the absence of CAP_NET_BIND_SERVICE in the first place should not be an issue for the drop.]
Author
There was a problem hiding this comment.
I don't think there should be any trouble from trying to set the capabilities to their current value.
If you really wanted to prevent it from trying to change capabilities during testing, there should be ways to have MCTPD_MANAGE_CAPABILITIES set to 0 for the test.
Is there anything specific you're worried about?
Added the set_cap() function to mctpd, allowing the process to drop CAP_NET_BIND_SERVICE when it is no longer needed. This requires libcap on the target device. If libcap is missing drop_bind_cap() will be a stub, and mctpd will emit a warning when it starts. Because mctpd requires additional capabilities and may recieve hostile inputs, dropping the capability when it is no longer needed is useful to increase safety. libcap has been added as a possible dependency to the meson build and mcpd now uses the MCTPD_MANAGING_CAPABILITIES macro to determine if libcap is available. mctpd.md has been updated to reflect the new capability management. Signed-off-by: James Lee <james@codeconstruct.com.au>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Updated mctpd to manage its capabilities, dropping
CAP_NET_BIND_SERVICEwhen no longer needed. The example configs have been updated to run mctpd as an unprivileged user with only necessary capabilities added while also allowing it to continue to be run as root if desired.The ability to interact with mctpd via the system D-bus has also been restricted to root and a special
mctp-adminrole, though properties can still be viewed by unprivileged users.