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

Add new condition ALLOW_TRAY #4510

Merged
merged 1 commit into from
Oct 9, 2021

Conversation

rusty-snake
Copy link
Collaborator

No description provided.

@kmk3
Copy link
Collaborator

kmk3 commented Sep 9, 2021

diff --git a/etc/firejail.config b/etc/firejail.config
index 2e355586b..5111bb769 100644
--- a/etc/firejail.config
+++ b/etc/firejail.config
@@ -2,6 +2,9 @@
 # keyword-argument pairs, one per line. Most features are enabled by default.
 # Use 'yes' or 'no' as configuration values.
 
+# Allow programs to display a tray icon
+# allow-tray no
+
 # Enable AppArmor functionality, default enabled.
 # apparmor yes

From the PR date, I'd guess that this is related to issues like #4509:

@zarandya commented on Sep 4:

The nextcloud client profile is broken: nextcloud crashes at startup as it is
trying to use OpenGL for its gui. Also, it has no access to the system tray.

I added this to nextcloud.local to fix it:

ignore no3d
dbus-user.talk org.freedesktop.StatusNotifierItem
dbus-user.talk org.kde.StatusNotifierWatcher

@zarandya commented on Sep 4:

Is enough to add this?

ignore no3d
dbus-user.talk org.kde.StatusNotifierWatcher

It indeed is.

So is this intended to just allow interacting with the system tray through
dbus? If so, wouldn't it suffice to add some dbus filters like the above in a
new allow-system-tray.inc file?

@rusty-snake
Copy link
Collaborator Author

So is this intended to just allow interacting with the system tray through
dbus?

Yes

If so, wouldn't it suffice to add some dbus filters like the above in a
new allow-system-tray.inc file?

It's a different idea. For tray¹ icons to work you need dbus-user.talk org.kde.StatusNotifierWatcher and, depending on the implementation, dbus-user.own org.kde.*. Both are unsafe and you don't want them allowed by default. On the other hand some users might heavily use tray icons. Therefore the Idea is to have a option in firejail.config which enables tray icons globally (profiles require ?ALLOW_TRAY: dbus-user.talk org.kde.StatusNotifierWatcher) instead of creating a lot .locals. This is much easier for novice users, especially if it is integrated into firejail-welcome.sh.

¹ There are many tray icon standards but this is the most common one.

General: I have already argued against allow-*.inc includes for D-Bus.

@kmk3
Copy link
Collaborator

kmk3 commented Sep 11, 2021

@rusty-snake commented on Sep 10:

So is this intended to just allow interacting with the system tray through
dbus?

Yes

If so, wouldn't it suffice to add some dbus filters like the above in a new
allow-system-tray.inc file?

It's a different idea. For tray¹ icons to work you need dbus-user.talk org.kde.StatusNotifierWatcher and, depending on the implementation,
dbus-user.own org.kde.*. Both are unsafe and you don't want them allowed by
default. On the other hand some users might heavily use tray icons. Therefore
the Idea is to have a option in firejail.config which enables tray icons
globally (profiles require ?ALLOW_TRAY: dbus-user.talk org.kde.StatusNotifierWatcher) instead of creating a lot .locals. This is
much easier for novice users, especially if it is integrated into
firejail-welcome.sh.

?ALLOW_TRAY: dbus-user.talk org.kde.StatusNotifierWatcher

That makes sense; +1 for the condition.

It wasn't clear to me how this was intended to be implemented, so my first
thought was that the filters were going to be hardcoded in firejail.

¹ There are many tray icon standards but this is the most common one.

Indeed; link for reference: #4053.

@kmk3
Copy link
Collaborator

kmk3 commented Sep 11, 2021

@rusty-snake commented on Sep 10:

General: I have already argued against allow-*.inc includes for D-Bus.

(Continued on #4524)

@netblue30 netblue30 merged commit a28ca71 into netblue30:master Oct 9, 2021
@netblue30
Copy link
Owner

merged!

@rusty-snake rusty-snake deleted the allow-tray-condition branch October 9, 2021 12:53
rusty-snake added a commit to rusty-snake/firejail that referenced this pull request Oct 9, 2021
netblue30 added a commit that referenced this pull request Oct 10, 2021
kmk3 added a commit that referenced this pull request Dec 11, 2021
@kmk3 kmk3 added this to Done (on RELNOTES) in Release 0.9.68 Jan 27, 2022
@kmk3 kmk3 added the enhancement New feature request label Jan 27, 2022
kmk3 added a commit to kmk3/firejail that referenced this pull request Feb 15, 2022
According to netblue30#4053, `dbus-user.talk org.kde.StatusNotifierWatcher` is
unsafe and allows escaping the sandbox, but it is required by multiple
programs for tray functionality.  Users may not be aware of this (for
example, see netblue30#4508), so add a warning about it.

Note: allow-tray was added on commit c86cae2 ("Add new condition
ALLOW_TRAY", 2021-09-04) / PR netblue30#4510.
kmk3 added a commit to kmk3/firejail that referenced this pull request Feb 16, 2022
According to netblue30#4053, there is currently no safe (in the sense of not
allowing to escape the sandbox) implementation of
`org.kde.StatusNotifierWatcher`, but it is required by multiple programs
for tray functionality.  Users may not be aware of this (for example,
see netblue30#4508), so add a warning about it.

Note: allow-tray was added on commit c86cae2 ("Add new condition
ALLOW_TRAY", 2021-09-04) / PR netblue30#4510.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature request
Projects
No open projects
Release 0.9.68
  
Done (on RELNOTES)
Development

Successfully merging this pull request may close these issues.

None yet

4 participants