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

ProtectHome does not really protect home with ostree #3211

Closed
ruihe774 opened this issue Mar 11, 2024 · 6 comments · Fixed by #3230
Closed

ProtectHome does not really protect home with ostree #3211

ruihe774 opened this issue Mar 11, 2024 · 6 comments · Fixed by #3230
Labels
area/sysroot Issues related to OstreeSysroot difficulty/medium medium complexity/difficutly issue triaged This issue has been evaluated and is valid

Comments

@ruihe774
Copy link
Contributor

ProtectHome=yes in systemd (see man:systemd.exec) hides user directories to "ensure they cannot get access to private user data". However, this setting does not really protect home with ostree. Services can still access user directories from /sysroot/ostree/deploy/fedora/var/home/ with ProtectHome=yes. An example is as follows.

Suppose we have a secret file to protect:

echo "my secret" > /home/test/secret

And a malicious service:

[Unit]
Description=Secret thief

[Service]
Type=oneshot
ProtectHome=true
ExecStart=/usr/bin/cat /sysroot/ostree/deploy/fedora/var/home/test/secret

[Install]
WantedBy=default.target

After it is started, the service can still steal the content of secret.

To enforce the protection as expected, one has to also add something like InaccessiblePaths=/sysroot, which I think is counterintuitive and does not compatible with environment without ostree.

I don't know if I shall post this issue here or to systemd. Please let me know if this is more relevant to systemd.

Maybe related: #2082

@cgwalters
Copy link
Member

More closely related is #907

I would say we should aim to switch to having the /sysroot directory be mode 0700 to start. You can of course just make this change in any systems you're using; the main fallout right now is it breaks unprivileged read-only direct use of the ostree CLI for reading system state; but many or even most use cases may not care about that. (e.g. things using rpm-ostree already have a DBus intermediate API)


This is a valid issue however do be aware that there's a really giant difference between using ProtectHome as a safeguard mainly against accidental damage versus protecting against malicious (or fully compromised) code running as a systemd unit. In your example of just using ProtectHome on its own...well, the unit can just unmount the masking mounts and regain access...so it needs to be combined with other things.

systemd offers a whole lot of isolation knobs but the most reliable and complete in my opinion starts with DynamicUser=yes which will also reliably fix access to user home directories, no matter where they're mounted. I am sure upstream systemd will agree; note the description for ProtectSystem=strict says "This setting cannot ensure protection in all cases.". The other flow of course is to start with a podman-style isolated container (especially with using user namespaces there).

@cgwalters cgwalters added difficulty/medium medium complexity/difficutly issue triaged This issue has been evaluated and is valid area/sysroot Issues related to OstreeSysroot labels Mar 11, 2024
@ruihe774
Copy link
Contributor Author

systemd offers a whole lot of isolation knobs but the most reliable and complete in my opinion starts with DynamicUser=yes which will also reliably fix access to user home directories, no matter where they're mounted.

Yes, we can use more strict hardening if we know it will not break our services. But as a matter of fact many distro just start with a permissive set of systemd hardening features. (e.g., Fedora and OpenSUSE). ProtectHome is usually turned on but DynamicUser is not since it will break too many things. With ostree, such systemd security hardening come from distro will not work as expected, leaving environment with ostree relatively more vulnerable than normal setup. I pointed out #2082 to show that even within ostree development we could fail to realize the ineffectiveness of ProtectHome.

@cgwalters
Copy link
Member

Longer term what we should probably do is have /sysroot actually be part of a separate mount namespace, held by a dedicated process.

pointed out #2082 to show that even within ostree development we could fail to realize the ineffectiveness of ProtectHome.

It's not ineffective; it still helps against misconfiguration. A service has to go rather "out of its way" to access files in /sysroot - nothing is going to do that by default. In contrast it's not uncommon for code that becomes part of a systemd service to look in /home for something via misconfiguration, and ProtectHome is still effective against that.

Or if you're arguing it's ineffective, then using only ProtectHome=true (as in your example) is equally ineffective given it's easy to circumvent in an infinite variety of ways, from changing the mount namespaces to loading kernel modules to...

@ruihe774
Copy link
Contributor Author

I would say we should aim to switch to having the /sysroot directory be mode 0700 to start. You can of course just make this change in any systems you're using; the main fallout right now is it breaks unprivileged read-only direct use of the ostree CLI for reading system state; but many or even most use cases may not care about that. (e.g. things using rpm-ostree already have a DBus intermediate API)

I've just tested that and 0700 make rpm-ostree fail with "Permission denied". I have no idea why...

@travier
Copy link
Member

travier commented Apr 11, 2024

An option would be to have /sysroot/ostree/deploy/ (and not the entire sysroot) made 0700 as that should protect most files (but not from root).

Another related topic is accessing SetUID binaries from previous deployment to exploit patched vulnerabilities.

@cgwalters
Copy link
Member

An option would be to have /sysroot/ostree/deploy/ (and not the entire sysroot) made 0700 as that should protect most files (but not from root).

Yes. We can recommend that. It's just right now it breaks e.g.

$ rpm-ostree db diff a6d38801e7f93cf3c897dd58fce1665a2c22aa90e452977aefbf32d7cb0be624 3a48d126f22f97149fadc925ad551afd5af175d37abe58bac7cc2abc22d2fb97
error: loading sysroot: Parsing deployment /ostree/boot.1/default/f757112d872eaa23891e9e80f366072676e94dd841e7db1bcd750f44d5ceecf5/0 in stateroot 'default': opendir(ostree/boot.1/default/f757112d872eaa23891e9e80f366072676e94dd841e7db1bcd750f44d5ceecf5/0): Permission denied

But, that's fixable by having even the database diffing go over the daemon. Or maybe we have the daemon give the client file descriptors to just the rpm database dirs.

travier added a commit to travier/rpm-ostree that referenced this issue Apr 12, 2024
The fix in coreos#4911 for CVE-2024-2905 only fixes the permissions for the
current deployment and not for previous deployments for existing
installations.

The affected files can still be read from the previous deployment by
looking at them in `/sysroot/ostree/deploy/...`.

Extend the unit to fix all deployments on the system and update the
logic to only update files that exists.

See: coreos#4911
See: GHSA-2m76-cwhg-7wv6
See: ostreedev/ostree#3211
travier added a commit to travier/rpm-ostree that referenced this issue Apr 12, 2024
The fix in coreos#4911 for CVE-2024-2905 only fixes the permissions for the
current deployment and not for previous deployments for existing
installations.

The affected files can still be read from the previous deployment by
looking at them in `/sysroot/ostree/deploy/...`.

Extend the logic to fix all deployments on the system and update the
logic to only update files that exists.

See: coreos#4911
See: GHSA-2m76-cwhg-7wv6
See: ostreedev/ostree#3211
cgwalters added a commit to cgwalters/rpm-ostree that referenced this issue Apr 12, 2024
It was pointed out that in the previous change here we missed
the fact that the previous deployments were accessible.

- Move the logic into Rust, adding unit tests
- Change the code to iterate over all deployments
- Add an integration test too

Note: A likely future enhancement here will be to finally
deny unprivileged access to non-default roots; cc
ostreedev/ostree#3211
cgwalters added a commit to cgwalters/rpm-ostree that referenced this issue Apr 12, 2024
It was pointed out that in the previous change here we missed
the fact that the previous deployments were accessible.

- Move the logic into Rust, adding unit tests
- Change the code to iterate over all deployments
- Add an integration test too

Note: A likely future enhancement here will be to finally
deny unprivileged access to non-default roots; cc
ostreedev/ostree#3211
cgwalters added a commit to cgwalters/rpm-ostree that referenced this issue Apr 12, 2024
It was pointed out that in the previous change here we missed
the fact that the previous deployments were accessible.

- Move the logic into Rust, adding unit tests
- Change the code to iterate over all deployments
- Add an integration test too

Note: A likely future enhancement here will be to finally
deny unprivileged access to non-default roots; cc
ostreedev/ostree#3211
cgwalters added a commit to cgwalters/ostree that referenced this issue Apr 12, 2024
We want to start switching things so that the toplevel `/ostree`
repository is mode 0700, to close off unprivileged code
from being able to access it.  Previous deployment roots
may have setuid binaries, etc.  The `/var/lib/containers/storage`
directory is mode 0700 for this reason I believe.

Closes: ostreedev#3211
lukewarmtemp pushed a commit to lukewarmtemp/rpm-ostree that referenced this issue Apr 18, 2024
It was pointed out that in the previous change here we missed
the fact that the previous deployments were accessible.

- Move the logic into Rust, adding unit tests
- Change the code to iterate over all deployments
- Add an integration test too

Note: A likely future enhancement here will be to finally
deny unprivileged access to non-default roots; cc
ostreedev/ostree#3211
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/sysroot Issues related to OstreeSysroot difficulty/medium medium complexity/difficutly issue triaged This issue has been evaluated and is valid
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants