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

Groupfolders for which a user has no reading-rights (Advanced Permissions) can still be copied and read out! #1692

Closed
Dejagenkidama opened this issue Oct 11, 2021 · 6 comments · Fixed by nextcloud/server#29362
Assignees
Labels
3. to review Items that need to be reviewed bug high high priority security Items that address a security vulnerability

Comments

@Dejagenkidama
Copy link

Dejagenkidama commented Oct 11, 2021

Example:

Employee list / NC users:
MA1 site manager
MA2 accountant
MA3 project employee Lisa
MA4 project employee Hans

NC folder structure (group folder):

Group folder company location Berlin
...├── Administration
..........├── accounting
...├── projects
..........├── Project A
..........├── Project B

The following should be set via “Advanced Permissions”:
Write and read rights to all folders should have: MA1 site manager
Write and read rights to accounting folders should have: only MA2 accountant
Write and read rights to project folder A should have: only MA3 project employee Lisa
Write and read rights to project folder B should have: only MA4 project employee Hans

All of this is easy to set using the “Advanced Permissions” in the “Groupfolder”.
However…
For the root folder “Group folder company location Berlin”, all NC-users must have at least reading rights, otherwise you won’t see a folder at all…

“MA4 Projektmitarbeiter Hans” does not initially see the “Administration-Accounting” folder.
However, if he copies the complete root directory “Group folder company location Berlin” and inserts it into another of his own folders, all directories and their contents are visible to him.

Can this copying of the “invisible” folder or in general be prevented somehow?
How would you solve this problem?

Thank’s for the Tipps. Matthias

@kesselb
Copy link
Contributor

kesselb commented Oct 15, 2021

cc @nextcloud/security

@fschrempf fschrempf added 0. Needs triage Issues that need to be triaged bug high high priority security Items that address a security vulnerability labels Oct 15, 2021
@fschrempf
Copy link
Contributor

@Dejagenkidama Can you please provide information about your NC version and Groupfolders app version?

@Tetrachloromethane250
Copy link

Hello,

I have the same issue with Nextcloud version 20.0.13 and GroupFolders version 8.2.3.
Working through with the built in updater (on "stable" channel) and testing shows the issue is still there on 21.0.5/9.0.3 and on 22.2.0/10.0.0, so it doesn't seem to be limited to any one version (I haven't tested with any older versions than 20.0.13).

In each case downloading the folder with the ACLs works correctly, leaving out sub-folders that do not have read permission. Copying a sub-folder with further permissions nested inside also has the same issue of ignoring ACLs.

@CarlSchwan
Copy link
Member

I could reproduce on master

@CarlSchwan CarlSchwan self-assigned this Oct 21, 2021
@CarlSchwan CarlSchwan added 1. to develop Issues that are ready for development and removed 0. Needs triage Issues that need to be triaged labels Oct 21, 2021
@CarlSchwan
Copy link
Member

I was hoping that this would be an easy fix by just extending the groupfolder ACLStorageWrapper::copy/copyFromStorafe but unfortunately, this is only used when copying stuff inside a group folder and not the other way around :/

CarlSchwan added a commit to nextcloud/server that referenced this issue Oct 21, 2021
Using advanced ACL, it is possible that an user has access to a
directory but not to a subdirectory, so the copying use
Common::copyFromStorage instead of Local::copyFromStorage.

Fix nextcloud/groupfolders#1692

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@CarlSchwan
Copy link
Member

Hi, I just created nextcloud/server#29362, it would be nice if someone here could also test it and see if this correctly fix the problem and doesn't create new ones :)

@CarlSchwan CarlSchwan added 3. to review Items that need to be reviewed and removed 1. to develop Issues that are ready for development labels Oct 21, 2021
CarlSchwan added a commit to nextcloud/server that referenced this issue Oct 21, 2021
Using advanced ACL, it is possible that an user has access to a
directory but not to a subdirectory, so the copying use
Common::copyFromStorage instead of Local::copyFromStorage.

Fix nextcloud/groupfolders#1692

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
backportbot-nextcloud bot pushed a commit to nextcloud/server that referenced this issue Oct 21, 2021
Using advanced ACL, it is possible that an user has access to a
directory but not to a subdirectory, so the copying use
Common::copyFromStorage instead of Local::copyFromStorage.

Fix nextcloud/groupfolders#1692

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
backportbot-nextcloud bot pushed a commit to nextcloud/server that referenced this issue Oct 21, 2021
Using advanced ACL, it is possible that an user has access to a
directory but not to a subdirectory, so the copying use
Common::copyFromStorage instead of Local::copyFromStorage.

Fix nextcloud/groupfolders#1692

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
backportbot-nextcloud bot pushed a commit to nextcloud/server that referenced this issue Oct 21, 2021
Using advanced ACL, it is possible that an user has access to a
directory but not to a subdirectory, so the copying use
Common::copyFromStorage instead of Local::copyFromStorage.

Fix nextcloud/groupfolders#1692

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
backportbot-nextcloud bot pushed a commit to nextcloud/server that referenced this issue Nov 2, 2021
Using advanced ACL, it is possible that an user has access to a
directory but not to a subdirectory, so the copying use
Common::copyFromStorage instead of Local::copyFromStorage.

Fix nextcloud/groupfolders#1692

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@AndyScherzinger AndyScherzinger removed the security Items that address a security vulnerability label Nov 3, 2021
@fschrempf fschrempf added the security Items that address a security vulnerability label Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Items that need to be reviewed bug high high priority security Items that address a security vulnerability
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants