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 #719

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

Conversation

rahulsundaram
Copy link

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. If you have a list of directories that this service must be able to write to, I would like to add it an allow list and turn on ProtectSystem fyi.

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.

@rahulsundaram rahulsundaram force-pushed the systemd_sec branch 2 times, most recently from 7eade14 to 9905e40 Compare March 8, 2024 13:40
@ximion
Copy link
Collaborator

ximion commented Mar 8, 2024

We can not do this, at least not by default for everything. Especially not ProtectSystem.
PackageKit needs full access to ready every aspect of the system and write to the complete system, as its very purpose is to make modifications to it.
Even a private temp directory is a bit risky, although that may have a bigger chance for success.

Still, I think adding an exception to Fedora's policy for PK would make more sense, as it literally can not function with any hardening enabled.

@rahulsundaram
Copy link
Author

rahulsundaram commented Mar 9, 2024

Still, I think adding an exception to Fedora's policy for PK would make more sense, as it literally can not function with any hardening enabled.

To be clear, there isn't a Fedora policy that mandates any of this. The change proposal is explicitly about enabling settings on a service by service basis. If PackageKit can only accept some subset of these things that's perfectly fine. If none of this will work at all, we can close it out

@ximion
Copy link
Collaborator

ximion commented Mar 9, 2024

I think we can potentially allow things like ProtectHome=yes, however for the remaining stuff I am very concerned that there will be unexpected side effects with scripts running for the various backends during package installation that will break if we suddenly hide certain information from them. I am especially concerned about the kernel tunables and module protections, as some packages will definitely want to touch those during the update process, as well as any IPC restrictions.

I also do not see much of a point in restricting access to the system for a daemon which runs as root and literally has the ability to replace the current shell or install any suid-root binary into /usr/bin by design.
We also can not accept this upstream and enable it until we are sure all backends will not break with these options, which is a huge endeavour as any distribution not only has their own packages to deal with, but also 3rd-party sources and proprietary vendor packages that are hard to test.

@rahulsundaram
Copy link
Author

I have turned off the settings you were concerned about. Let me know if you want to consider the rest. If you don't see value in adding any restrictions upstream, I can close it out as well and mark this PR as entirely rejected.

@ximion
Copy link
Collaborator

ximion commented Mar 9, 2024

I am mostly concerned that I can't estimate the impact of this given how many backends PK has and how much stuff packages do, while at the same time not seeing the benefit of guarding some things for a daemon that can literally replace your entire OS by design.
@hughsie, what do you think?

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.

None yet

2 participants