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

profiles: loupe: harden and disable apparmor #6333

Merged
merged 1 commit into from
May 12, 2024

Conversation

kmk3
Copy link
Collaborator

@kmk3 kmk3 commented May 8, 2024

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 9a0db13 ("profiles: add loupe", 2024-04-30) /
PR #6327.

[1] #6327 (review)
[2] #6333 (comment)

@SkewedZeppelin
Copy link
Collaborator

SkewedZeppelin commented May 8, 2024

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

@kmk3
Copy link
Collaborator Author

kmk3 commented May 8, 2024

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
Copy link
Collaborator

SkewedZeppelin commented May 8, 2024

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
Copy link
Collaborator

glitsj16 commented May 8, 2024

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.

Copy link
Collaborator

@glitsj16 glitsj16 left a comment

Choose a reason for hiding this comment

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

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 9a0db13 ("profiles: add loupe", 2024-04-30) /
PR netblue30#6327.

[1] netblue30#6327 (review)
[2] netblue30#6333 (comment)
@kmk3 kmk3 changed the title profiles: loupe: add warning and disable in firecfg profiles: loupe: harden and disable apparmor May 8, 2024
@kmk3
Copy link
Collaborator Author

kmk3 commented May 8, 2024

@glitsj16

Thanks for the testing!

Updated the PR with most of your changes.

Now the profile seems pretty good.

@SkewedZeppelin Thoughts?

@SkewedZeppelin
Copy link
Collaborator

@kmk3
go for it
@glitsj16
thank you

@kmk3 kmk3 mentioned this pull request May 8, 2024
@rusty-snake
Copy link
Collaborator

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.

Copy link
Collaborator

@glitsj16 glitsj16 left a comment

Choose a reason for hiding this comment

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

LGTM

@kmk3 kmk3 merged commit 6c91074 into netblue30:master May 12, 2024
3 checks passed
@kmk3 kmk3 deleted the loupe-warn-disable branch May 12, 2024 17:45
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

4 participants