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

Update Systemd security settings #345

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rahulsundaram
Copy link

@rahulsundaram rahulsundaram commented Feb 28, 2024

Based on the discussions in https://src.fedoraproject.org/rpms/dbus-broker/pull-request/11, I am proposing this change here. As part of https://fedoraproject.org/wiki/Changes/SystemdSecurityHardening which has been approved for Fedora 40, I am working on updating Systemd services to add additional hardening settings, please review this PR and let me know if you have any feedback

https://www.freedesktop.org/software/systemd/man/latest/systemd.exec.html has detailed information on each of these settings including the version of Systemd where they were introduced.

@dvdhrm
Copy link
Member

dvdhrm commented Mar 1, 2024

Thanks for opening this PR! I think it looks good. I have to look into each individual key again, but on first glance this is all good.

dbus-broker-launch uses NSS services to resolve user-names (e.g., getpwnam(3)). These will pull in nss-plugins into the running process. Most modern plugins will communicate with a standalone service, so they will likely work fine under these restrictions. However, I have not fully audited this.

I think I am fine with trying to apply these, but we need to document that this will affect nss-plugins, and we need to make sure to allow opening any UDS-sockets, since that is what most of these use for communication.

@rahulsundaram rahulsundaram changed the title Update dbus-broker.service.in Update Systemd security settings Mar 1, 2024
@rahulsundaram
Copy link
Author

Thanks for the review. I cleaned up the commit message a bit. Let me know if you would want to merge this as is or if there is an ask from me here on running any tests etc.

src/units/system/dbus-broker.service.in Show resolved Hide resolved
src/units/system/dbus-broker.service.in Outdated Show resolved Hide resolved
src/units/system/dbus-broker.service.in Outdated Show resolved Hide resolved
src/units/system/dbus-broker.service.in Outdated Show resolved Hide resolved
@rahulsundaram
Copy link
Author

I have made changes requested but also added similar settings to the user service. While it is not part of the scope of change proposal I am working on, it seems reasonable to knock it out while we are here. If you want to split it out or deal with later, let me know

@@ -13,6 +13,23 @@ Sockets=dbus.socket
ExecStart=@bindir@/dbus-broker-launch --scope user
ExecReload=@bindir@/busctl --user call org.freedesktop.DBus /org/freedesktop/DBus org.freedesktop.DBus ReloadConfig
Slice=session.slice
LockPersonality=yes
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure to test this with the latest systemd version, as most of these in user units imply PrivateUsers=yes to work

@dvdhrm
Copy link
Member

dvdhrm commented Apr 16, 2024

I am very hesitant to merge this, given that I have not tested this and no runtime tests for the individual keys are included in this PR. This can easily lead to errors based on nss-modules on specific client machines, or other non-standard configuration setups.

Where do the individual keys of this PR come from? Did you verify they can be safely applied to dbus-broker? What is the rationale behind these keys? How did you evaluate which keys to apply, and which don't?

To merge this, I would have to survey each individual key, evaluate whether they can be safely applied to dbus-broker and document how this can possibly affect user-configurations. I currently do not see this happening anytime soon.

If you want to move this forward, can you elaborate on the individual keys why you think they are safe, possibly split this PR to merge less controversial keys first, or even providing tests for common setups to show that they do not break.

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.

3 participants