netblue30 / firejail

Linux namespaces and seccomp-bpf sandbox
https://firejail.wordpress.com
GNU General Public License v2.0
5.83k stars 568 forks source link

profiles: loupe: harden and disable apparmor #6333

Closed kmk3 closed 6 months ago

kmk3 commented 6 months ago

The profile currently does not include disable-common nor makes ${HOME} read-only, so the program can simply write to ~/.bashrc directly[1].

disable-common.inc was commented due to it apparently breaking bwrap. As discovered by @glitsj16, it seems that allowing the bwrap binary is enough to make it work (and that apparmor breaks loupe)[2].

So disable apparmor, allow bwrap and include disable-common.inc, plus other hardening by @glitsj16.

This amends commit 9a0db13e1 ("profiles: add loupe", 2024-04-30) / PR #6327.

[1] https://github.com/netblue30/firejail/pull/6327#pullrequestreview-2033860865 [2] https://github.com/netblue30/firejail/pull/6333#issuecomment-2099805480

SkewedZeppelin commented 6 months ago

I don't see how a permissive profile is worse than no profile. That is defeatist. It should stay enabled.

kmk3 commented 6 months ago

I don't see how a permissive profile is worse than no profile. That is defeatist. It should stay enabled.

The usual distinction between a more vs less permissive profile is whether it uses whitelisting in the user home or not.

I can't think of any profile for a common GUI program that allows a hole as big as writing to ~/.bashrc. If this is not the case for other such programs, users may reasonably assume that if one of them runs under firejail by default, then its profile provides at least a modicum of protection of the most critical files. If it doesn't, then running it under firejail by default would likely give the user a false sense of security.

Also, imagine if we ended up with a significant number of profiles that do next to nothing for security. Then users would have to check each and every profile to see if it does the bare minimum. That is, the burden for vetting profiles would be shifted onto users, which would make firejail and firecfg rather pointless except for power users IMO.

SkewedZeppelin commented 6 months ago

but loupe is already special cased as it already aggressively sandboxes each decoded image into its own sandbox

and again, I didn't use disable-common because it seems to directly conflict with it invoking bwrap

glitsj16 commented 6 months ago

After actually installing and testing loupe I think we can accomodate both sides of the arguments/security considerations. Let me explain by going over my observations.

[1] apparmor breaks loupe:

...
bwrap: Failed to make / slave: Permission denied

We need to fix this.

[2] We can include disable-common.inc without breaking things when combining it with noblacklist ${PATH}/bwrap.

[3] Additional hardenings I've tested succesfully: private-bin bwrap,loupe dbus-user none dbus-system none

[4] Adding read-only ${HOME} works fine for image viewing. It might cripple loupe's 'Move to trash' and 'Set as background' functionalities (didn't test those to be honest). Personally I don't regard those to be important enough to not restrict image viewers sandboxes.

Here's my hardened loupe.profile.

Hope this helps to clear up some of the pain-points in both PR's.

kmk3 commented 6 months ago

@glitsj16

Thanks for the testing!

Updated the PR with most of your changes.

Now the profile seems pretty good.

@SkewedZeppelin Thoughts?

SkewedZeppelin commented 6 months ago

@kmk3 go for it @glitsj16 thank you

rusty-snake commented 6 months ago

I can't think of any profile for a common GUI program that allows a hole as big as writing to ~/.bashrc.

Unfiltered access to the session-bus is a hole as big as writing to ~/.bashrc IMHO. Even worse, you do not have to wait.