Thanks so much, @emitorino, appreciate it! I tested the live version with autostart and it’s working great.
I’m coming back with one more auto-connect request that will hopefully be my last one before we release our stable version of strict confinement (thanks for your patience!). Could I please add an auto-connect request for a system-files interface called runtime-singleton-slack? It will look like this in the .yml:
This was an odd one - sharing details in case another Electron-based snap is affected. The strict confinement snap has an occasional crash on log-in (~50% of the time).
The root cause: we set TMPDIR to $XDG_RUNTIME_DIR (/run/user/$uid/snap.slack) in order to give libappindicator read access to our tray icon resources. However, it looks like $XDG_RUNTIME_DIR sometimes does not have the correct read permissions: when Electron tries to open a temporary singleton socket in $XDG_RUNTIME_DIR, snappy-debug will show the error below. This lack of read permission is what causes the crash:
= AppArmor = Time: Feb 16 19:29:24 Log: apparmor="DENIED" operation="file_perm"
profile="snap.slack.slack" name="/run/user/1000/snap.slack/scoped_dirlCcQck/SingletonSocket" pid=8247 comm="slack" requested_mask="r" denied_mask="r" fsuid=1000 ouid=0
File: /run/user/1000/snap.slack/scoped_dirlCcQck/SingletonSocket (read)
Suggestions:
* adjust program to use $SNAP_DATA
* adjust program to use /run/shm/snap.$SNAP_NAME.*
* adjust program to use /run/snap.$SNAP_NAME.*
* adjust snap to use snap layouts (https://forum.snapcraft.io/t/snap-layouts/7207)
SettingTMPDIR back to the SNAP default (/tmp/snap.slack) breaks the tray icon, because the resources can’t be read.
Setting the TMPDIR to something like /run/shm/snap.$SNAP_NAME or /run/snap.$SNAP_NAME, or any folder within $SNAP results in similar permissions issues.
Using a layout to map /run/user to $SNAP/tmp/ also didn’t seem to work.
I tried to set the runtime-singleton-slack read filepath to something more specific, like /run/user/*/snap.slack. But it looks like ‘*’ and any Regex are reserved characters for AppArmor.
Long-term, I’d like to move us away from libappindicator - then we wouldn’t reset the $TMPDIR variable at all. Unfortunately that won’t be a small task, and I’d love to get the strict snap out to customers sooner rather than later.
Would it be possible to grant this system-files auto-connect request for our launch? If you and/or the security team has any alternative suggestions that could address the bug, I’m open to them!
+1 from me on this request (creating an assertion to access snap.slack singleton) as a temporary measure until this can be resolved or implemented in a different manner.
This is the issue since the apparmor rule uses an owner match. The file is owned by root but the process is running as non-root (at the time of the denial). This suggests that the setuid sandbox is in use, but I don’t see where a request was made for that for the slack snap. Is slack plugging browser-support with allow-sandbox: true? If not, how is this socket being created with root permissions? (perhaps creative use of hooks?)
@alexmurray and/or @emitorino - the path here is /run/user/1000/snap.slack/scoped_dirlCcQck/SingletonSocket. Since we’re talking about a subdirectory in /run/user/<uid> and the system is supposed to create this directory with 0700 permissions, and DAC is evaluated before AppArmor, perhaps it makes sense to loosen the default template to remove owner from the owner /run/user/[0-9]*/snap.@{SNAP_INSTANCE_NAME}/** mrwklix, rule and trust that DAC has it covered? Alternatively, since this appears to be happening with a snap that has allow-sandbox: true with browser-support, perhaps that interface could add /run/user/[0-9]*/snap.@{SNAP_INSTANCE_NAME}/** rwk, when allow-sandbox: true?
Thanks @jdstrand! To give a bit more information - we’re currently disabling the setuid sandbox by passing --no-sandbox into the launch command, and I think allow-sandbox: false is what’s set for us (we’re aren’t explicitly setting it to true). It’s entirely possible that this is a bug on Electron’s side that I’ll happily dig into as well. I’m investigating it on our end to see if we’re not passing the correct permissions from the first process to the second process, but I think that may be a more long-term fix.
Sorry, for the second part of your message - if the owner permissions are changed, is there anything that Slack would need to change in our .yml? We’re ready to launch the new snap to our stable channel outside of this bug, so just wanted to make sure I did any due diligent needed.
If you have disabled the sandbox (and you’re right that it will default to allow-sandbox: false) then I’m puzzled how the socket is owned by root since a non-root process isn’t going to be able to create a root-owned socket (the ouid (object uid) in the apparmor denial indicates it is owned by root). It would be good to get to the bottom of that.
The second part was for members of the security team to consider a change in the policy that would not require you to need this use of system-files. While I understand why you requested it, in terms of the snap ecosystem, I don’t care for the addition because your snap now would have read access to everything under /run/user, which violates snap isolation and would not be appropriate long term (I understand the slack snap already has been granted use of classic confinement so this is more of a ‘principle’ thing).
I suspect answering the question on why it is being created as root in the first place will shed light on the solution (maybe it is something you can fix yourself or maybe it is something that can be fixed in policy). If it’s decided that a policy change to browser-support or similar is warranted, it might be possible to temporarily grant your snap the runtime-singleton-slack access until a version of snapd is out that has the policy change.
Thank you!! We’re very excited to have the strict snap out (if you have any feedback or issues, please let us know, would love to hear what you think! )
So sorry to post again, all - we’re aiming for this Wednesday (March 9) as our strict snap stable release day, and I think we’ve exhausted debugging options from our side (setting the browser-sandbox to true confoundingly still causes this issue to appear). Would it be possible to explore the changes @jdstrand mentioned above, to get the snap unblocked in the snap store for a Wednesday release with the crash permissions issue fixed? Happy to make any changes needed to the interface. Thanks so much!
Hi @khammond - I am still confused as to how the socket becomes owned by root (unless your snap still has the setuid chrome-sandbox enabled or a root daemon). However, given that it currently has classic confinement (ie no confinement) even to grant the overly broad permission to read /run/user as a strictly confined snap is a good security win. So I think the best course of action is to grant this via system-files for now and then we can work with you to see if we can possibly get it working without this in the future. However, to fit the current naming conventions, can you please rename the system-files instance to run-user? +1 from me for system-files named run-user for read access to /run/user. Can other @reviewers please vote?
+2 votes for, 0 votes against, granted system-files instance named run-user for read permission to /run/user for slack - @khammond can you please update the slack snap to use this name instead of runtime-singleton-slack and the snap should pass the automated review.
Hi all: I just wanted to let you know that we pushed the strict confinement snap to our latest/stable channel. Thank you all so much again for all of the help you provided! I really appreciated all of the responses and guidance as we went through this process.
I’ll be monitoring the forums here for user reports as more users update to the new snap, but let me know if there’s anything else that we on the Slack team can do
That is excellent news @khammond - thanks for all your dedication to get slack to be strictly confined - please let us know if you do encounter any issues and we will try help resolve them ASAP with you.
)