sailfishos / sailjail-permissions

Other
5 stars 17 forks source link

[permissions] Add permission for /home/.shared directory. JB#54619 OMP#OS-7158 #85

Closed TheNeuroUnicorn closed 1 year ago

TheNeuroUnicorn commented 3 years ago

Contributes to https://github.com/sailfishos/firejail/pull/13. Some applications like an antivirus and etc want to get access to some shared directory between all users to store big blobs of data in it, /home/.shared is good one. Permission SharedStorage will make directory inside by /OrganizationName/ApplicationName.

TheNeuroUnicorn commented 3 years ago

mkdir and whitelist rules themselves look good to me but see also my comment about directory permissions in the other PR.

@Tomin1 So app launched by user1 and(or) user2 wont be able to access the shared folder by only allowing this folder in sanboxing? How can we probably allow it for r\w if firejail is setting directories to 700 for default?

Tomin1 commented 3 years ago

mkdir and whitelist rules themselves look good to me but see also my comment about directory permissions in the other PR.

@Tomin1 So app launched by user1 and(or) user2 wont be able to access the shared folder by only allowing this folder in sanboxing?

Yes, that's what I'm saying. If the app is first launched by one user and then later by another user, that another user can't access the data. Firejail allows only the owning user to access the created directories.

How can we probably allow it for r\w if firejail is setting directories to 700 for default?

Either the directories should be created in beforehand so that firejail doesn't touch them at all (and there is no need for mkdir rule), or that firejail is somehow modified to set the permissions we need for this case. For the latter option I thought about adding mkdir-shared or similar rule that would be like mkdir but it would also set the permissions to something lax (it probably has to be 777). However it's not small effort to add another rule to firejail, even if in this case you can mostly reuse what mkdir already has. I think firejail doesn't have anything appropriate for this purpose already and I don't have better suggestions to give at the moment.

LaakkonenJussi commented 3 years ago

mkdir and whitelist rules themselves look good to me but see also my comment about directory permissions in the other PR.

@Tomin1 So app launched by user1 and(or) user2 wont be able to access the shared folder by only allowing this folder in sanboxing?

Yes, that's what I'm saying. If the app is first launched by one user and then later by another user, that another user can't access the data. Firejail allows only the owning user to access the created directories.

How can we probably allow it for r\w if firejail is setting directories to 700 for default?

Either the directories should be created in beforehand so that firejail doesn't touch them at all (and there is no need for mkdir rule), or that firejail is somehow modified to set the permissions we need for this case. For the latter option I thought about adding mkdir-shared or similar rule that would be like mkdir but it would also set the permissions to something lax (it probably has to be 777). However it's not small effort to add another rule to firejail, even if in this case you can mostly reuse what mkdir already has. I think firejail doesn't have anything appropriate for this purpose already and I don't have better suggestions to give at the moment.

Just an idea referring to comment of @Tomin1 - perhaps we could use mount to bind the directory inside the sandbox with proper permissions? Granted, that dir may need to be created before it can be used, apps could even die if that main dir does not exist yet.

So:

  1. Have a shared dir defined and created with certain group permissions somewhere, e.g., /home/.shared
  2. Have a private-mount-like rule in firejail: --shareddir=SHAREDPATH,groupname/gid that uses the SHAREDPATH/groupname to create a subdir with the gid used for permissions. The user running the firejail must be in that same group, so it would be needed to be added to OS as well.
  3. When the rule is set, lets say, --shareddir=/home/.shared,datashare firejail would a) create /home/.shared/groupname for datashare:datashare 700 (?) and b) bind mount /home/.shared/groupname -> sandbox $HOME/Shared with username:datashare permissions.

Does this make any sense? Checked the code that the other mounts are done already so this might work. But I may be missing something still. I guess this describes better the one I started in https://github.com/sailfishos/firejail/pull/13

Lost some details during my vacation related to firejail that this comment may be off. But in theory this would enable the use of common share, and even with different groups to have different sharing options. But then again, creating the groups each time a new app starts may be an overkill. Or there could be a more generic group and the second parameter just can act as an optional parameter and the full path is only madnatory to which shared data is put.

pvuorela commented 1 year ago

Conflicts.

I'm not either necessarily being the rationale here. Either such antivirus should be run with a lot more higher privileges to check every user's things, or then if it's run separately for each user the instances shouldn't be trusting data in some shared directory.

For other purposes this might be better as more visible directory. And then again if it's really shared, should it be allowed by default?

I'd lean towards just closing.

pvuorela commented 1 year ago

I interpret thumbs up as agreeing to closing and no objections either, and still conflicting.

Thus closing now.