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

Not possible to give groups share and write to a whole group folder and delete permissions to only specific subfolders without security problem #1757

Open
D3nnis3n opened this issue Nov 17, 2021 · 21 comments
Labels
1. to develop Issues that are ready for development bug feature: acl Items related to the groupfolders ACL or "Advanced Permissions" feature: trashbin Items related to the trashbin feature

Comments

@D3nnis3n
Copy link

D3nnis3n commented Nov 17, 2021

If i gave the group "users" rights to only share and write to a group folder and then want to give a user in that group via advanced permissions the right to delete in a specific subfolder like /a/b/c, then the user can still not delete (and hence not move or rename, as that for some weird reason seems to be tied to delete) any files in there, as the group rights with no deleting overwrite the specific user right with yes for deleting.

So my only option is to allow deleting in general for that whole groupfolder to the group and then prohibit the group from deleting everywhere via advanced permissions and then add on top the permission for that specific user. That on the other hand, makes all users of that group able to delete any files out of the trashbin, even those deleted in folders they do not have delete access to and even then when they do not have delete access on any folder. Extremely big security issue.

Steps to reproduce

Create groupfolder "test" and group "test"
Give test access to that group folder and make it have write and share, but not delete permissions
Via advanced permissions give user "test" the permission to delete for subfolder /a/b/c
Try to delete something in that folder -> Be presented with "permission denied"

Workaround:

Create groupfolder "test" and group "test"
Give test access to that group folder and make it have write, share and delete permissions.
Via advanced permissions take the delete permission from group test on the main group folder with inheritance.
Via advanced permissions give user "test" the permission to delete for subfolder /a/b/c
Try to delete something in that folder -> Works for user test, but not for any other users in group test, as intended.
BUT: All users of group test can now delete stuff from the trashbin, even if they have no delete permissions ANYWHERE.
=> Big security issue resulting, as people can now delete stuff they should absolutely not be able to, leading to possible permanent data loss

Expected behaviour

Being able to give the group users rights to share and write to a group folder plus deletion permissions in specific subfolders only without them being able to delete everything in the trash bin, even stuff they do not have access to, which will result in accidential data loss when many unexperienced users use the software.

Actual behaviour

Not being able to do that, but either people being unable to delete files or being able to delete everything out of the trashbin, even stuff they should have no access to delete.

Server configuration

Operating system: Ubuntu 20.04 LTS
Web server: Apache 2.4.41
Database: MariaDB 10.3.31
PHP version: 8.0.12
Nextcloud version: 22.2.3
Group folders version:
Updated from an older Nextcloud/ownCloud or fresh install: Fresh install
Where did you install Nextcloud from: Official page "Download for server"
Are you using external storage, if yes which one: No
Are you using encryption: No
Are you using an external user-backend, if yes which one: No

Client configuration

Browser: Google Chrome, Edge, Firefox
Operating system: Windows 11

Logs

Shouldn't be relevant for this issue. If specific ones are needed I provide them on demand.

@D3nnis3n D3nnis3n added 0. Needs triage Issues that need to be triaged bug labels Nov 17, 2021
@fschrempf
Copy link
Contributor

Create groupfolder "test" and group "test"
Give test access to that group folder and make it have write and share, but not delete permissions
Via advanced permissions give user "test" the permission to delete for subfolder /a/b/c
Try to delete something in that folder -> Be presented with "permission denied"

This is expected behavior. See #1085.

BUT: All users of group test can now delete stuff from the trashbin, even if they have no delete permissions ANYWHERE.
=> Big security issue resulting, as people can now delete stuff they should absolutely not be able to, leading to possible permanent data loss

You mean that the "test" group can delete files from the trashbin that weren't deleted from the "/a/b/c" sub folder, but from other places? And this doesn't happen if you don't give the "test" group delete permissions in "/a/b/c" via ACL?

@D3nnis3n
Copy link
Author

D3nnis3n commented Nov 18, 2021

You mean that the "test" group can delete files from the trashbin that weren't deleted from the "/a/b/c" sub folder, but from other places? And this doesn't happen if you don't give the "test" group delete permissions in "/a/b/c" via ACL?

Yes that's what I mean. And no, they can do that with no delete permissions in /a/b/c as well.

You say that this is expected behaviour, but how can I achieve then that some people can delete files in a specific subfolder, without being able to delete any file out of the trashbin they could not delete into it to begin with? That's just not possible? For me this 'expected behaviour' makes no sense, as it severely limits usage of the whole system, for me it is practically unusable.

Our usecase is that we got a group folder for volunteers of a charitable NGO which should be able to view and edit all files in the NGO group folder, but only the departement leads of each department should be able to delete files in their respective subfolders. (Due to law regulations on keeping documents, the leads know what they can delete 'final' and what not) The problem is, if I do that the second way, as the first one doesn't work (and is expected to, as you say), then every single volunteer can just delete anything out of the trashbin, even though he is not supposed to have any deletion rights at all. If I do not give the group deletion rights, noone can delete anything, even if explicitly authorized as I would - seemingly in opposite to what is supposed to happen - naturally expect.

If I add a second group for the leaders to delete, then they can delete stuff in everyones subfolders, not only their own, which is also not what we want. Basically, if what I expected to work would actually work, this would not be a problem at all.

Is there any other way how this can be achieved or can this be suggested as a feature or something else?

@fschrempf
Copy link
Contributor

With "expected behavior" I only meant your "first way". Your use-case sounds valid and it should work as you describe it in your workaround. I guess there is a bug with the trashbin handling and ACL.

@CarlSchwan As you have been recently working on the trashbin, could you have a quick look and maybe confirm, that this is a valid bug.

@fschrempf fschrempf added 1. to develop Issues that are ready for development feature: acl Items related to the groupfolders ACL or "Advanced Permissions" feature: trashbin Items related to the trashbin feature and removed 0. Needs triage Issues that need to be triaged labels Nov 18, 2021
@D3nnis3n
Copy link
Author

D3nnis3n commented Nov 18, 2021

I mean I get if that is just what you want for your software, we're just trying to have a pretty simple organisation to make sure that we do not loose data we legally need to keep. Working with lots of unpaid volunteers always is a difficulty and they already invest so much of their free time, I would not really want to force everyone to get instructed first to not touch the trash bin at all.

Especially as the trash bin also shows their own deleted stuff of their private user folders, so they don't even know if a file is from the group folder or not and might accidentially delete something they thought is from their own user folder. I mean, it had already happened ... (And thats the biggest problem)

@fschrempf
Copy link
Contributor

I mean I get if that is just what you want for your software, ...

I don't know what you mean by that. I totally see your point and the trouble this is causing.

It looks like there is a bug and hopefully there's someone in the community who can confirm this bug and maybe even provide a fix. I'm just volunteering here to do some issue triaging at the moment.

@D3nnis3n
Copy link
Author

D3nnis3n commented Nov 18, 2021

Yeh, I see that might have come off wrong. Basically I wanted to say that I'm of course fine if this turns out to be intended in the end, given we are very grateful there are people that work on such a software at all and even provide them for people like us to make our processes so much easier, and all of that basically for free. Given we know very well how volunteer work can be, we are always extremely happy to see how many people in the software community are so helpful and provide stuff others can use.

So just a thanks for all your work!

And if this actually turns out to be a bug that can be fixed, that would be great.

@CarlSchwan
Copy link
Member

It sounds like #1739 that I fixed 10 days ago

@fschrempf
Copy link
Contributor

@CarlSchwan Ah, I had something like that in the back of my mind. Thanks for the reminder!

@D3nnis3n The mentioned fix is not yet released, but it looks like it should fix your issue. I will keep this issue open until you have tested and reported back.

@D3nnis3n
Copy link
Author

Oh, yeh, this looks like it could be it. Will test once I can :)

@fschrempf fschrempf self-assigned this Nov 19, 2021
@fschrempf fschrempf added needs info and removed 1. to develop Issues that are ready for development labels Nov 21, 2021
@fschrempf
Copy link
Contributor

@D3nnis3n FYI, if you run the latest version of Groupfolders (11.1.0) with NC 23, you should have the mentioned fix included. If you are still on NC 22, you still have to wait for a release (see #1833).

@fschrempf
Copy link
Contributor

The mentioned change was released for NC21 onwards. Let's assume this is fixed. Please reopen or create a new issue if the issue persists.

@D3nnis3n
Copy link
Author

Sorry, I didn't get to test this earlier.
Unfortunately it seems to me that noone can delete anything out of group folders anymore, I just get "Error when removing from trashbin" with this log entry:

"reqId":"YhN7lY0cmHHs5a2CdWQ0HAAAAAc","level":4,"time":"2022-02-21T11:46:29+00:00","remoteAddr":"2a02:810d:600:6256:ccad:63bc:495:f35d","user":"Dennis","app":"webdav","method":"DELETE","url":"/remote.php/dav/trashbin/Dennis/trash/test.md.d1645443821","message":"Failed to remove item from trashbin","userAgent":"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/97.0.4692.99 Safari/537.36","version":"23.0.2.1","exception":{"Exception":"Exception","Message":"Failed to remove item from trashbin","Code":0,"Trace":[{"file":"/var/www/vhosts/localhost/subdomains/cloud/apps/files_trashbin/lib/Trash/TrashManager.php","line":68,"function":"removeItem","class":"OCA\\GroupFolders\\Trash\\TrashBackend","type":"->"},{"file":"/var/www/vhosts/localhost/subdomains/cloud/apps/files_trashbin/lib/Sabre/AbstractTrash.php","line":89,"function":"removeItem","class":"OCA\\Files_Trashbin\\Trash\\TrashManager","type":"->"},{"file":"/var/www/vhosts/localhost/subdomains/cloud/3rdparty/sabre/dav/lib/DAV/Tree.php","line":179,"function":"delete","class":"OCA\\Files_Trashbin\\Sabre\\AbstractTrash","type":"->"},{"file":"/var/www/vhosts/localhost/subdomains/cloud/3rdparty/sabre/dav/lib/DAV/CorePlugin.php","line":281,"function":"delete","class":"Sabre\\DAV\\Tree","type":"->"},{"file":"/var/www/vhosts/localhost/subdomains/cloud/3rdparty/sabre/event/lib/WildcardEmitterTrait.php","line":89,"function":"httpDelete","class":"Sabre\\DAV\\CorePlugin","type":"->"},{"file":"/var/www/vhosts/localhost/subdomains/cloud/3rdparty/sabre/dav/lib/DAV/Server.php","line":472,"function":"emit","class":"Sabre\\DAV\\Server","type":"->"},{"file":"/var/www/vhosts/localhost/subdomains/cloud/3rdparty/sabre/dav/lib/DAV/Server.php","line":253,"function":"invokeMethod","class":"Sabre\\DAV\\Server","type":"->"},{"file":"/var/www/vhosts/localhost/subdomains/cloud/3rdparty/sabre/dav/lib/DAV/Server.php","line":321,"function":"start","class":"Sabre\\DAV\\Server","type":"->"},{"file":"/var/www/vhosts/localhost/subdomains/cloud/apps/dav/lib/Server.php","line":339,"function":"exec","class":"Sabre\\DAV\\Server","type":"->"},{"file":"/var/www/vhosts/localhost/subdomains/cloud/apps/dav/appinfo/v2/remote.php","line":35,"function":"exec","class":"OCA\\DAV\\Server","type":"->"},{"file":"/var/www/vhosts/localhost/subdomains/cloud/remote.php","line":166,"args":["/var/www/vhosts/localhost/subdomains/cloud/apps/dav/appinfo/v2/remote.php"],"function":"require_once"}],"File":"/var/www/vhosts/localhost/subdomains/cloud/apps/groupfolders/lib/Trash/TrashBackend.php","Line":177,"CustomMessage":"--"}}

@CarlSchwan
Copy link
Member

Let's reopen this for now

@CarlSchwan CarlSchwan reopened this Feb 21, 2022
@D3nnis3n
Copy link
Author

D3nnis3n commented Feb 21, 2022

I found something extremely odd:

  1. Setup my workaround solution.
  2. Create a file in the folder where your group has full delete permssions, but a second group has those revoked via ACL directly on that folder (override).
  3. Delete the file you just created.
  4. Check your file system group folder trashbin (FTP) and see that the file deleted is now in there.
  5. Delete the file in the trashbin on Web UI of Nextcloud. Everything works.

Now, do it this way:

  1. Setup my workaround solution.
  2. Create a file in the folder where your group has full delete permssions, but a second group has those revoked via ACL directly on that folder (override).
  3. Delete that file.
  4. Check your file system group folder trashbin and see that the file deleted is now in there.
  5. Log into an account that is part of the group that has delete permissions removed for that folder you deleted the file out of.
  6. Check your trashbin, find the deleted file and delete the file permanently in the trashbin. Correctly see the error "Can't delete from trashbin". This is what I expected.
  7. Check your file system group folder trashbin and see that the file deleted is now NO LONGER in there.
  8. Log back to the account that has permission to delete files from the folder. See that the file is still shown as in the trash bin. Try to permanently delete it again. Now also see the error "Can't delete from tashbin". This is very unexpected.

This means:
The files that I cannot delete out of the trashbin are files that are no longer in the trashbin on filesystem level, but still shown in the trashbin on Nextcloud web. That means that people that should not be able to delete a file from trashbin do correctly see the error they can't, but the file still gets removed from the filesystem. Given it now is no longer on the filesystem, someone that has access to delete it cannot either, as it doesn't exist anymore in filesystem.

That's at least how it appears to me. Weird stuff.

@D3nnis3n
Copy link
Author

D3nnis3n commented Feb 21, 2022

Let me try a full reproduction case again, I see the above might be totally confusing.

You need:
Group A & Group B
Group Folder Z
User G in Group A
User F in Group B

Add Group A & Group B to Group Folder Z with all three permissions set, like this:
image

Set the permissions of Group B on the Group Folder Z via the "Share" menu to "Delete" prohibited.
Like this:
image

  1. Login with user G.
  2. Create a file in Group Folder Z.
  3. Delete the file in Group Folder Z
  4. Check your filesystem to see that the deleted file is now correctly in the folder trashbin.
  5. Go to the trashbin in Nextcloud and permanently delete the file.
  6. Success.

Now do this:

  1. Login with user G.
  2. Create a file in Group Folder Z.
  3. Delete the file in Group Folder Z
  4. Check your filesystem to see that the deleted file is now correctly in the folder trashbin.
  5. Logout and login with user F.
  6. Go to the trashbin in Nextcloud and try to permanently delete the file. See an error.

7. Check your filesystem to see that the deleted file is now removed from the folder trashbin.

  1. Logout and login with user G.

9. Go to the trashbin in Nextcloud, see the file is still shown there and try to permanently delete the file again. Now see an error here as well.

Sorry, I'm really not good in this :( Hope this is somewhat understandable.

@D3nnis3n
Copy link
Author

I'm using NextCloud 23.0.2 and Group Folders 11.1.2 as of the system information.

@CarlSchwan
Copy link
Member

Thanks for the detailed reproduction steps, I definitely won't have to look at it this week but hopefully get some time for it next week

@D3nnis3n
Copy link
Author

No problem. I can reliably reproduce this over and over. I can also confirm that all files that I cannot remove from my trashbin are files that are no longer in the trashbin folder on the filesystem, e.g. probably have been tried to delete by someone that should have no permission. Means, my trashbin shown in NextCloud is no longer consistent with what's actually on the file system. This will be fun to clean up.

@D3nnis3n
Copy link
Author

Is there any new information on this issue, could you reproduce it? :)

@D3nnis3n
Copy link
Author

Daring to ask about it a few months later again :D

@fschrempf fschrempf removed their assignment Jul 29, 2022
@fschrempf fschrempf added 1. to develop Issues that are ready for development and removed needs info labels Sep 1, 2023
@D3nnis3n
Copy link
Author

D3nnis3n commented Mar 5, 2024

Maybe it's fine to ask after 18 months if you were at least able to repro? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1. to develop Issues that are ready for development bug feature: acl Items related to the groupfolders ACL or "Advanced Permissions" feature: trashbin Items related to the trashbin feature
Projects
None yet
Development

No branches or pull requests

3 participants