nextcloud / server

☁️ Nextcloud server, a safe home for all your data
https://nextcloud.com
GNU Affero General Public License v3.0
26.89k stars 4.01k forks source link

Created files/directories on local storage ignore web server umask and therefore have incorrect permissions set #29041

Closed tjwood100 closed 1 year ago

tjwood100 commented 3 years ago

How to use GitHub

Steps to reproduce

  1. Set Apache umask to 0007 in /etc/apache2/envvars or equivalent
  2. Create a file or directory on local storage

Expected behaviour

Created files/directories should observe Apache's umask, i.e. if Apache's umask is 0007 and if I have a directory with permissions drwxrwsr-x me mygroup

And I create a file within that directory in nextcloud, it should have the permissions -rw-rw---- www-data mygroup

and I as member of mygroup should be able to write to the file.

My use case is that I access files in local storage externally as well as via Nextcloud. It should be possible for me (as a member of the group the files are created under) to locally edit a file or write to a directory created via Nextcloud.

Actual behaviour

Apache's umask is ignored and the file is created with rw-r--r-- permissions therefore I can't write to it from outside of nextcloud.

Note that as well as not being group writeable, they are also other-readable, which could be unexpected.

This issue didn't happen in earlier versions of nextcloud - it appears to have been introduced by this commit https://github.com/nextcloud/server/commit/e5dc1a8085226492b6d323142381fd163451c06d which forces umask to be 022. Also this replaced an earlier commit https://github.com/nextcloud/server/commit/d182043e83a277a7f95e4d689b1e96d7b1b9a6fb which forcibly set permissions on created directories to 755.

Server configuration

Operating system: Ubuntu 20.04.3

Web server: Apache

Database: mariadb

PHP version: 7.4.3

Nextcloud version: Nextcloud 22.2.0

Updated from an older Nextcloud/ownCloud or fresh install: Fresh install

Where did you install Nextcloud from: tarball

Signing status:

Signing status No errors have been found.

List of activated apps:

App list Enabled: - accessibility: 1.8.0 - activity: 2.15.0 - bruteforcesettings: 2.2.0 - circles: 22.1.1 - cloud_federation_api: 1.5.0 - comments: 1.12.0 - contactsinteraction: 1.3.0 - dashboard: 7.2.0 - dav: 1.19.0 - federatedfilesharing: 1.12.0 - federation: 1.12.0 - files: 1.17.0 - files_external: 1.13.0 - files_pdfviewer: 2.3.0 - files_rightclick: 1.1.0 - files_sharing: 1.14.0 - files_trashbin: 1.12.0 - files_versions: 1.15.0 - files_videoplayer: 1.11.0 - firstrunwizard: 2.11.0 - logreader: 2.7.0 - lookup_server_connector: 1.10.0 - nextcloud_announcements: 1.11.0 - notifications: 2.10.1 - oauth2: 1.10.0 - password_policy: 1.12.0 - photos: 1.4.0 - privacy: 1.6.0 - provisioning_api: 1.12.0 - recommendations: 1.1.0 - serverinfo: 1.12.0 - settings: 1.4.0 - sharebymail: 1.12.0 - support: 1.5.0 - survey_client: 1.10.0 - systemtags: 1.12.0 - text: 3.3.0 - theming: 1.13.0 - twofactor_backupcodes: 1.11.0 - updatenotification: 1.12.0 - user_status: 1.2.0 - viewer: 1.6.0 - weather_status: 1.2.0 - workflowengine: 2.4.0 Disabled: - admin_audit - encryption - user_ldap

Nextcloud configuration:

Config report 'dbtype' => 'mysql', 'version' => '22.2.0.2',

the rest is not relevant

Are you using external storage, if yes which one: yes - local

Are you using encryption: no but not relevant

Are you using an external user-backend, if yes which one: no

Client configuration

Browser: not relevant

Operating system: not relevant

Logs

not relevant

tjwood100 commented 3 years ago

I have verified that commenting out the ten lines that call umask() in Local.php fixes this issue, so it was definitely a regression introduced by https://github.com/nextcloud/server/commit/e5dc1a8085226492b6d323142381fd163451c06d (and it looks like https://github.com/nextcloud/server/commit/d182043e83a277a7f95e4d689b1e96d7b1b9a6fb was a previous attempt that would have done similar). It's not clear what issue either of these were trying to fix - they aren't linked to bug reports as far as I can tell. (The commit message on the earlier of these says "this works around any umask that might be set and limiting the folder permissions", but it's overlooked that these changes themselves are limiting the folder/file permissions, which is a problem in this case)

tjwood100 commented 3 years ago

Judging by the tests added in commit https://github.com/nextcloud/server/commit/e5dc1a8085226492b6d323142381fd163451c06d it seems that the intent of the change is to make sure that if Nextcloud creates a file or directory, it is then able to write it. That seems like a reasonable aim. But it seems that explicitly setting the umask to 022 is a very crude way of doing that, which inadvertently changes the group and other permissions.

A better pattern might be something like:

$oldMask = umask();
umask($oldMask & 077); // clear owner mask, but keep group and others parts of mask intact
$result = // do whatever
umask($oldMask);
Flowdalic commented 2 years ago

I'd also like to add that it took me a while to figure out why nextcloud would not respect setting UMask in the corresponding systemd unit override. The culprit here is PR #25280. Besides they approach already mentioned, this change should not invoked umask() with cleared 'other' bits, as this allows for world-readable files. Sure, you can further protect access to nextcloud files by -x'ing the data directory, but when it comes to permissions and security, a defensive approach is always preferable.

bgottschall commented 2 years ago

How about putting a configuration parameter in the config file for such an important part of a cloud solution like setting file and folder permissions? I don't like that Nextcloud is forcing those permission when I want to have control who has access to my files. I'm not opposed to setting the defaults to umask(027), but giving the opportunity to change them accordingly would solve this issue.

MartinBrugnara commented 2 years ago

@bgottschall see if pr #32723 covers it.

bgottschall commented 2 years ago

@MartinBrugnara if one is pedantic it does not fix this issue as nextcloud would still ignore the umask given to it from the OS or the starting service. Normally umasks are configured where the process is started. Your pr gives one at least the opportunity to configure it within nextcloud. I don't really understand why the default umask needs to be always hardcoded when it is given from the system itself. I assume setting the default umask in the nextcloud config to umask() wouldn't work with some poeple since it is not a constant?! Maybe that discussion should be placed in your pr. Feel free to copy it over if you like.

MartinBrugnara commented 2 years ago

@bgottschall I agree it does not fix the problem in general, it's just one step in the right direction.

As you said, my PR just does what you suggested in the comment, it exposes the umask setting via config file instead of hard-coding it.

I don't really understand why the default umask needs to be always hardcoded when it is given from the system itself. I assume setting the default umask in the nextcloud config to umask() wouldn't work with some poeple since it is not a constant?!

I didn't replicate the specific issue but the commit message from #25280 (451c06d) suggests that third-party scripts, extraneous from Nextcloud but running on the same workers, may tamper with umask and thus break folder/files creation. Note that in a well thought deployment this should never happen.

Nevertheless we may want to ensure that no matter what the ENV configuration is for umask, Nextcloud creates the files with minimal set of permissions to guarantee the correct execution. This could be achieved by the selecting proper max-umask e min-perm and then play with the bits as @tjwood100 suggested. At this point we could deal also with tampered umask values.

I am not sure what the correct solution is for all scenarios, only that having umask hard-coded was not working for my installation... so I tried to help =)

EDIT: we could also just put the "override umask" action behind a feature-flag and document it properly,

bgottschall commented 2 years ago

@MartinBrugnara I absolutely appreciate your pr as it also solves my problems. In fact I had another pr pending which got obsolete because of yours. Though I did only remove the fixed umask out of the local storage functions since I never understood what it was useful for. I never understood the commit message of #25280 fixing 'other php stuff', because its not the responsibility of the nextcloud code to alleviate any potential wrong doing of 'other php stuff'. So after I studied this problem myself, I never got why one would want to fix or even change the umask that is given from the system. If the umask is wrong the system is configured wrong and not nextcloud. If the umask is changed by other stuff, the other stuff is responsible or should be fixed. I never got any explanation why things were done like they were done and the author of #25280 @icewind1991 is quite active on github, was supposed to review my pr and stayed silent. Maybe he can bring some light into this issue.

flan7 commented 2 years ago

I am having difficulties with this issue, is it still hard coded? Odd behavior.

bgottschall commented 2 years ago

@toqov have you configured the umask in the nextcloud config? It is still hard coded basically. Changing it the default way like everyone would do it on a linux server doesn't work with nextcloud anymore, but you can now configure it in the nextcloud config.

'localstorage.umask' => 0000

flan7 commented 1 year ago

@toqov have you configured the umask in the nextcloud config? It is still hard coded basically. Changing it the default way like everyone would do it on a linux server doesn't work with nextcloud anymore, but you can now configure it in the nextcloud config.

'localstorage.umask' => 0000

Even with setting the umask this way to 0002, nextcloud is still saving files as -rw-r--r--

This seems to cause permission errors with my unraid NFS share where my data dir is located.

MartinBrugnara commented 1 year ago

@toqov please verify which version you are running, afaik https://github.com/nextcloud/server/pull/32723 has been merged for NC25 but has not yet been backported for <= 24.

You could wait a few days for the release of 25 or manually apply the patch to your installation, or, even better, help the project complete the backports ;)

Shrizt commented 1 year ago

Judging by the tests added in commit e5dc1a8 it seems that the intent of the change is to make sure that if Nextcloud creates a file or directory, it is then able to write it. That seems like a reasonable aim. But it seems that explicitly setting the umask to 022 is a very crude way of doing that, which inadvertently changes the group and other permissions.

Finally Issue resolved in version 25+ Just add to config.php 'localstorage.umask' => 002, it will make new files group writable. or whatever umask you need. It is now not hardcoded (code var load added in www/nextcloud/lib/private/Files/Storage/Local.php )

alexsmartens commented 1 year ago

Would it be reasonable to allow changing permissions for creating new files/directories in Admin Settings? I installed Nextcloud server via snap. All snap packages are installed on a read-only files system so that I cannot edit neither config.php nor Local.php.

szaimen commented 1 year ago

Hi, please update to 24.0.9 or better 25.0.3 and report back if it fixes the issue. Thank you!

My goal is to add a label like e.g. 25-feedback to this ticket of an up-to-date major Nextcloud version where the bug could be reproduced. However this is not going to work without your help. So thanks for all your effort!

If you don't manage to reproduce the issue in time and the issue gets closed but you can reproduce the issue afterwards, feel free to create a new bug report with up-to-date information by following this link: https://github.com/nextcloud/server/issues/new?assignees=&labels=bug%2C0.+Needs+triage&template=BUG_REPORT.yml&title=%5BBug%5D%3A+

alexsmartens commented 8 months ago

Still facing the same issue when copying files through the browser. Any chance someone could help?

Installation info:

snap list nextcloud
Name       Version      Rev    Tracking       Publisher   Notes
nextcloud  27.1.4snap1  39212  latest/stable  nextcloud✓  -

https://github.com/nextcloud/server/assets/26261714/be087575-b725-436d-ba2d-5bec5253110f

joshtrichards commented 8 months ago

@alexsmartens A better place to post your query at this point would probably be the Nextcloud Help Forum - https://help.nextcloud.com

I installed Nextcloud server via snap. All snap packages are installed on a read-only files system so that I cannot edit neither config.php nor Local.php

https://github.com/nextcloud-snap/nextcloud-snap?tab=readme-ov-file#configuration

alexsmartens commented 8 months ago

In case anyone is facing the same issue, this is what helped me:


sudo snap disable nextcloud
sudo chown -R root:root /path/to/nextcloud_data_directory
sudo chmod -R 0770 /path/to/nextcloud_data_directory
sudo snap enable nextcloud
sudo snap run nextcloud.occ files:scan --all