joomla / joomla-cms

Home of the Joomla! Content Management System
https://www.joomla.org
GNU General Public License v2.0
4.73k stars 3.65k forks source link

Joomla\Filesystem\File::delete method chmod issue #23504

Open sven420 opened 5 years ago

sven420 commented 5 years ago

When Path::canChmod($file) returns false method throws an exception as if deletion is not possible but some servers have such setup that chmod is not but file deletion is possible.

Because of this I got errors when trying to update extensions but after commenting out this and line below that is trying to chmod file to 0777 update was success.

In my opinion unlink($file) should be executed even if chmod is not possible and this is a bug that can cause issues for some users.

mbabker commented 5 years ago

In my opinion unlink($file) should be executed even if chmod is not possible and this is a bug that can cause issues for some users.

-1, the entire point of attempting to chmod the resource is to make sure it is writable as a delete action is a write action. Blindly executing chmod() without ensuring the resource is in a writable state or executing unlink() on a read only resource is also prone to errors, removing sanity checks is not a valid bug fix here.

The fact you are having issues with the chmod() function points to either PHP being too restricted (i.e. the function is in the disabled functions list), filesystem permissions oddities, or the umask being an invalid value causing chmod(0777) to put the file into a read-only state.

sven420 commented 5 years ago

I see your point but we are not always in the situation where we have control over PHP or server configuration and Joomla should work in those cases also.

I don't think chmod is good sanity check here because executing unlink in try catch block would produce exception in case file isn't writeable.

What if chmod passes but unlink fails for some reason, if chmod check is definite proof that file is writable another try catch block for unlink is not necessary.

I still think that unlink try catch should be executed even if chmod part fails, there is no reason not to try it in method named delete.

mbabker commented 5 years ago

I still think that unlink try catch should be executed even if chmod part fails, there is no reason not to try it in method named delete.

If anything is going to change about the code, this should be the workflow. It should continue to be full of sanity checks, but potentially gets around whatever issue you might be running into. At the end of the day, the API MUST ensure it can actually work with the file given otherwise everyone's better off just hardcoding the use of unlink() into their code.

sven420 commented 5 years ago

I agree this is the proper way to do this.

rotech1 commented 5 years ago

Recently have similar issues with an extension update ( and un-install, too! ) see https://forum.joomla.org/viewtopic.php?f=728&t=968897


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/23504.

mbabker commented 5 years ago

For the record nothing significant has changed in Joomla\CMS\Filesystem\File (previously JFile) nor Joomla\Filesystem\File in quite some time. There were exactly zero changes in those classes between 3.9.0 and 3.9.1 (or for that fact any other parts of the code using methods from those classes) so if something has changed between those two releases it has nothing to do with the filesystem API.

Without a consistent environment to debug the root cause of the issue, at best all we're getting in this thread is "this code is blocking me from doing something" and that doesn't establish why it's preventing you from doing something.

rotech1 commented 5 years ago

It really looks like 3.9.2 has some kind of filesystem bug - see also https://www.kunena.org/forum/k5-1-support/156312-j3-9-2-breaks-attachment-uploads


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/23504.

mbabker commented 5 years ago

You can't have a new bug in an API that had zero changes in 3 releases.

rotech1 commented 5 years ago

Well, some very file-system-like problems suddenly appear after the J3.9.2 update - what else could be causing these behaviors?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/23504.

mbabker commented 5 years ago

Without an environment to recreate those issues I couldn't tell you. I can tell you though that the filesystem APIs have not been changed in quite some time (independently auditable; https://github.com/joomla/joomla-cms/commits/3.8.13/libraries/joomla/filesystem shows all changes to the libraries/joomla/filesystem folder from 3.8.13 and earlier and https://github.com/joomla/joomla-cms/commits/3.9.2/libraries/src/Filesystem for changes after the classes were namespaced during 3.9's development). So whatever issue you are seeing that points to an error coming from the filesystem API is a symptom of something else.

Again, the first problem is identifying an environment that can actually create the problem. Because if we are going strictly from my own hosting, Joomla's hosting, or my personal development space, I could say this is not a core bug and close it right now as a local environment issue and leave it to you to debug. If there is some sort of core issue though, it's better to troubleshoot and identify it instead of blindly making changes to files that have gone untouched in a substantial amount of time.

mahagr commented 5 years ago

@mbabker FYI

We are also getting reports where updating/removing extensions fail in Joomla 3.9 but not in Joomla 3.8. I am trying to get more information about the environments as I have no idea what could cause it to happen.

PHP documentation says:

Note: When safe mode is enabled, PHP checks whether the files or directories you are about to operate on have the same UID (owner) as the script that is being executed. In addition, you cannot set the SUID, SGID and sticky bits.

Maybe safe mode is what causes all the issues? Because of the above paragraph in the docs, I don't believe that chmod() is a reliable way to figure out if the file can be deleted or not.

mbabker commented 5 years ago

If someone wants to start blindly changing code by all means go for it. All this thread has is a bunch of "this is broken and needs fixing" comments but there is still no real issue identified, just a bunch of hypotheticals. That's still not a good way to troubleshoot and fix potential code issues in methods which have not changed but at this point I'm done arguing for proper debugging and troubleshooting of issues.

mahagr commented 5 years ago

@mbabker I've never proposed blindly to change the code, I guess that we're all just trying to narrow down what could cause the issue to happen -- and why you won't get the same issue with older versions of Joomla.

I just received a response from one user and it looks like he had libraries folder with UID root, which is obviously a permission issue in his end. I have no idea how older Joomla versions do not fail in this case, maybe older Joomla releases just silently fail to update the files?

For a long time, I have suspected that Joomla has a bug in the installer, which sometimes causes files not to be updated even if installation succeeds without an error. Deleting the files manually and installing the same version again always helped before and I believe the same is true now.

Maybe someone finally found the issue and fixed it?

I will let you know if something new comes out on this.

brianteeman commented 5 years ago

Safe mode was removed in php 5.4.0 so if that is the problem there is an obvious and easy answer

rytechsites commented 5 years ago

We manage over 100 joomla sites, we are on: Php 7.0.7

We have found for SOME tools (not all)

If we are on release 3.9.2, and we try to upgrade either of these, we end up getting file errors on the 'uninstall of files', it is as if the files don't uninstall properly.

If we manually install joomla 3.8.13, and attempt to run these upgrades everything works perfectly well.

Please note, on my 'privately hosted sites' on siteground I have NOT had this issue at all.

Our servers are 'local' at our university, and they are managed here 100%. Initially we tried to change file permissions, etc, but that didn't work.

Our best workaround was to go back to 3.8.13...

Is there anything else that I could give to help you see the recreation of this issue?

I'm guessing it is a combination of our file system, php and joomla 3.9.2...something...

thanks, Laura

billtomczak commented 5 years ago

@rytechsites I can confirm that we've identified this as the source of problems we've been seeing with our stable of Joomlashack extensions. Updates to library extensions are handled differently in Joomla. First they are uninstalled, then installed. If the uninstall fails - as it does under certain specific conditions - the entire install/upgrade procedure fails leaving the filesystem and/or database corrupted. Untangling the mess is sometimes pretty painful.

So far we've found that it seems to become an issue when an installed site is moved from one server/host to another. But not always, not reliably.

We're in the process of redoing our whole build/install/deployment chain and it will include workarounds for this problem. In the meantime, all we've been able to do is respond to users when it rears its ugly little head.

I'm not entirely convinced it's a problem as such in Joomla code. Definitely some weird quirk of file permission reporting on some servers under some (unspecified/unknown) circumstances.

rytechsites commented 5 years ago

Thank you very much!!! I agree, I think it is a strange combination of 'things', because unfortunately it doesn't happen 'all of the time'.

If you need us to run any kind of testing in our environment, because THERE it does happen 100% of the time with specific extensions, please let me know.

thanks, Laura

rytechsites commented 5 years ago

I just realized something... when we 'install our upgrades', we copy our files from our 'production environment' over to our 'development' environment...install all upgrades, then 'move everything back to production'...

which means we do move the files from one host to another...

-- Laura

brianteeman commented 5 years ago

We're in the process of redoing our whole build/install/deployment chain and it will include workarounds for this problem. In the meantime, all we've been able to do is respond to users when it rears its ugly little head.

That's why problems never get fixed. People like yourself with the skills to fix it never do. You just keep it to yourselves.

And of course you don't even need to work around the problem because @mbabker already fixed it https://github.com/joomla/joomla-cms/pull/23303

mahagr commented 5 years ago

And of course you don't even need to work around the problem because @mbabker already fixed it #23303

Well, not quite true. The fix for the library installer is already present in J3.9.2 and it doesn't have any effect on this chmod issue. That said, I'm really glad to see the library installer fix!

But in this case, it is better to fail with a nasty error message than to silently ignore files which cannot be updated. It is far better to have the installer to fail than to end up having a wrong set of files.

@rytechsites If you copy files from one server to another, make sure that the file owner/group are correct for all files after doing that. The installer won't be able to copy/replace the files if the files are owned by one user while the installer (web server) runs as some other user. :) The only reason why chmod() would fail, is that the user doesn't have the permissions needed to do the operation.

rytechsites commented 5 years ago

@mahagr thank you for your notes. We will verify, but as far as I know our 'process' of moving the files from one server to another works as it should.

This was a complicated issue, and I appreciate all involved that are taking the time an anyway they can to assist.

joomla-cms-bot commented 5 years ago

Set to "closed" on behalf of @franz-wohlkoenig by The JTracker Application at issues.joomla.org/joomla-cms/23504

ghost commented 5 years ago

closed as Issue seems resolved. If not please reopen.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/23504.

jobst commented 5 years ago

Joomla Version: 3.9.4 PHP: 7.1.24 Gantry5 installed: 5.4.24 Gantry5 upgrading to: 5.4.28

I have the problem, too, but I know that one of your assumptions is incorrect and I will get to that.

If anything is going to change about the code, this should be the workflow. It should continue to be full of sanity checks, but potentially gets around whatever issue you might be running into. At the end of the day, the API MUST ensure it can actually work with the file given otherwise everyone's better off just hardcoding the use of unlink() into their code.

Agreed.

* Check file actually exists, if not then return true (because it should never be considered an error to attempt to delete a non-existing filesystem resource, you wanted it gone and it's not there)

Agreed

* Check file is writable, if not then execute `chmod($file, 0777);` and fail if `chmod()` cannot be performed

I am not sure how you do the test whether the file is write able, but in MY case I am 100% sure that the file is write able by the user running the server. However, CHMOD is the problem, let me explain.

Assume "Apache" is the user running the webserver, for the sake of this example I named it the same as the type of server I use (Apache). Editor is the user allowed to edit and make changes and not, it's NOT root. Also "OTHER" is NOT allowed to see the files, this is the correct way to do this for many reasons (goes beyond the scope of what we want to do here).

 -rw-rw---- Editor Apache 3601 Aug 10 2018 EventListener.php

then the Apache Server ALLOWED to do delete, edit and move the file BUT the user of the server is NOT ALLOWED to do a FULL chmod as this goes fully against the LINUX/UNIX security philosophy. In the example below I have su'ed into a shell AS the user running the server (Apache). I have then echo'ed (well its an append) something to the file, then displayed whether it was appened. At the end I did a CHMOD, this WILL and MUST fail on ANY UNIX/LINUX - and it should IMHO.

[Apache@MACHINE /WebsiteRoot/libraries/gantry5/classes/Gantry/Admin] $>ls -al
total 56
drwxrwx---. 3 Editor Apache 4096 Mar 28 11:54 .
drwxrwx---. 6 Editor Apache 4096 Aug 10  2018 ..
drwxrwx---. 4 Editor Apache 4096 Aug 10  2018 Controller
-rw-rw----. 1 Editor Apache 7847 Mar 28 11:54 EventListener.php
-rw-rw----. 1 Editor Apache 3601 Aug 10  2018 Page.php
-rw-rw----. 1 Editor Apache 4839 Aug 10  2018 Particles.php
-rw-rw----. 1 Editor Apache 5170 Aug 10  2018 Router.php
-rw-rw----. 1 Editor Apache 3487 Aug 10  2018 Styles.php
-rw-rw----. 1 Editor Apache 4479 Aug 10  2018 ThemeList.php
-rw-rw----. 1 Editor Apache 3952 Aug 10  2018 Theme.php
[Apache@MACHINE /WebsiteRoot/libraries/gantry5/classes/Gantry/Admin] $>echo "// a comment " > EventListener.php
[Apache@MACHINE /WebsiteRoot/libraries/gantry5/classes/Gantry/Admin] $>tail -n 4 EventListener.php
        CacheHelper::cleanMenu();
    }
}
// a comment
[Apache@MACHINE /WebsiteRoot/libraries/gantry5/classes/Gantry/Admin] $>chmod 0777 EventListener.php
chmod: changing permissions of `EventListener.php': Operation not permitted
[Apache@MACHINE /WebsiteRoot/libraries/gantry5/classes/Gantry/Admin] $>

So you can see Apache is able to write to the file but cannot CHMOD. Apache should not be able to change the file to 0777, if it could it would be ludicrous.

Change your check to "touch" the file or "move" the file to a temp file then reverse it but do NOT use CHMOD rather tell the user the operation was not successful.

Also I am not sure how you test whether the file is write able, clearly in my case it's 100% write able. You do not need full 0666 permissions, this would allow OTHERS to see what's in the files and in many cases files contain information that should not be seen by the public.

I know many people say you should have 0664 on files, that is not needed. I am not the only system administrator that will tell you exactly this. The user running the server needs to be able to write and this is 100% sufficient:

 -rw-rw---- Editor Apache 3601 Aug 10 2018 EventListener.php
jobst commented 5 years ago

closed as Issue seems resolved. If not please reopen.

If I knew how I would ;-)

orlitzky commented 5 years ago

This also completely breaks every Joomla installation that uses filesystem ACLs to grant read/write access. Using "can I chmod" as a test is fucking stupid, and has no basis in anything at all for being correlated with "can I delete this file."

Here's the patch to fix it:

- if (!Path::canChmod($file))
+ if (!Path::canChmod($file) && false)
jobst commented 5 years ago

This also completely breaks every Joomla installation that uses filesystem ACLs to grant read/write access. Using "can I chmod" as a test is fucking stupid, and has no basis in anything at all for being correlated with "can I delete this file."

I did not want to use "fucking stupid" in my explanation, but I totally agree! It is as stupid as people saying you need to chmod every file to 0644 - which makes it WOLRD readable. If you have different editors for virtual hosts they all can see each others files which in many cases is a security risk!

JimDee commented 5 years ago

Just leaving a comment in case anyone comes here with this issue on a LiquidWeb CloudSites platform. They currently use their own permissions on things that aren't the standards recommended by J! devs. From time to time on that platform, you'll run into this issue and others that reek of weird permissions. The fix, in this case, is to simply get on w/ their tech support (as it's a managed platform) and say "hey, can you update the permissions on my domain". They get enough requests these days that they know what's needed & run a command or two... then all works well (including the fix for this issue, on this particular platform).


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/23504.

HLeithner commented 5 years ago

So I think we need a solution and failing because we can't change the permissions makes no sense to me. I'm also not sure what happens if the filesystem has extended permissions.

mbabker commented 5 years ago

Before making the assertion that "changing permissions should not cause delete to fail", you need to understand filesystem permissions. On a Linux filesystem, a delete action requires write permissions. So a file in a readonly state (400 or 444) cannot be deleted, you must execute chmod +w /path/to/file to get it into a write state (600, 644, or 664 depending on your user and group permissions) before you can delete the file. There is also a comment in the File::delete() method that notes of a Windows concern:

https://github.com/joomla/joomla-cms/blob/a81ada410a5bf6b700a79d432fc5926146ac9f94/libraries/src/Filesystem/File.php#L209-L210

https://github.com/joomla/joomla-cms/blob/a81ada410a5bf6b700a79d432fc5926146ac9f94/libraries/vendor/joomla/filesystem/src/File.php#L129-L130

This concern needs to be validated and either confirmed to still be an issue on whatever Windows operating systems Joomla supports (there isn't a declaration of such anywhere, but there is a stated support of IIS 7.0 which was shipped with Windows Vista and Windows Server 2008, which to me implies those are the oldest Windows OS builds that need to be confirmed at a minimum for this problem) or if it is confirmed to no longer be an issue the comment removed from the code as it is misleading.

I laid out 3 months ago exactly what steps can be taken to lessen the impact of whatever problem people are running into in https://github.com/joomla/joomla-cms/issues/23504#issuecomment-453206318 but it seems people would rather drop F-bombs and call those contributing to Joomla stupid than actually work toward a reasonable solution.

But, I'm in a particularly good mood today, so here's my last useful contribution to this issue, you all have fun deciding what to do with it. For S&G, I changed the delete method to this:

public static function delete($file)
{
    $files = (array) $file;

    foreach ($files as $file)
    {
        $file     = Path::clean($file);
        $filename = basename($file);

        // If the file does not exist, there is no need to execute the rest of this code
        if (!file_exists($file))
        {
            continue;
        }

        // Try to delete, if we succeed then skip going into the error handling code below
        if (@unlink($file))
        {
            continue;
        }

        $unlinkError        = error_get_last();
        $unlinkErrorMessage = !empty($unlinkError['message']) ? $unlinkError['message'] : 'Unknown Error';

        // If the file is already writable, bail out now
        if (strpos(Path::getPermissions($file), 'w') !== false)
        {
            throw new FilesystemException(
                sprintf('%s: Failed deleting %s with error "%s"', __METHOD__, $filename, $unlinkErrorMessage)
            );
        }

        // We're going to try and make the file writable to delete it, first make sure we can do this
        if (!Path::canChmod($file))
        {
            throw new FilesystemException(
                sprintf('%s: Failed deleting %s with error "%s"', __METHOD__, $filename, $unlinkErrorMessage)
            );
        }

        /*
         * Try making the file writable first. If it's read-only, it can't be deleted on Windows,
         * even if the parent folder is writable
         */
        if (!Path::setPermissions($file, '0777'))
        {
            throw new FilesystemException(
                sprintf('%s: Failed deleting unwritable file %s', __METHOD__, $filename)
            );
        }

        // One last try to delete
        if (!@unlink($file))
        {
            $unlinkError        = error_get_last();
            $unlinkErrorMessage = !empty($unlinkError['message']) ? $unlinkError['message'] : 'Unknown Error';

            throw new FilesystemException(
                sprintf('%s: Failed deleting %s with error "%s"', __METHOD__, $filename, $unlinkErrorMessage)
            );
        }
    }

    return true;
}

With this added test case:

public function testDeleteForReadOnlyFile()
{
    $this->skipIfUnableToChmod();

    $name = 'tempFile';
    $data = 'Lorem ipsum dolor sit amet';

    if (!File::write($this->testPath . '/' . $name, $data))
    {
        $this->markTestSkipped('The test file could not be created.');
    }

    $this->assertTrue(
        Path::setPermissions($this->testPath . '/' . $name, '0400')
    );

    $this->assertTrue(
        File::delete($this->testPath . '/' . $name),
        'The file was not deleted.'
    );
}

None of the error handling code ever actually gets triggered. I then changed the setPermissions call to Path::setPermissions($this->testPath, '0400', '0511') to put the folder and all of its files into a readonly mode. Then that test case started failing. Dumping the result of Path::getPermissions($this->testPath . '/' . $name) after the unlink() call, I found that file had already been chmod to 777 by PHP itself (PHP 7.2.10, Homebrew PHP install on MacOS 10.14.3; I only have PHP 7.1 and 7.2 at my disposal so don't go looking for PHP 5.3 results from me) and yet I continued to receive a unlink(/private/var/folders/b1/db5lfb3s37d1qbwy17r6s_y40000gn/T/1555256492.1502.463511382/tempFile): Permission denied error message. So even with the file writable, the delete operation failed (as expected) because the folder is read-only.

So, if someone wants to confirm behaviors across operating systems and PHP versions, it needs to be determined how PHP is behaving and a plan formulated on how to change things, probably in CMS 4.0 and Framework 2.0, accordingly. I would seriously suggest making liberal use of error_get_last() in the filesystem API and expanding the Exception subclass it throws to include the PHP error message as a separate property (similar to query errors, otherwise you're easily creating a path disclosure problem because the CMS doesn't do adequate error handling) no matter what though because the fact that the filesystem API absorbs errors and doesn't do anything useful to help people debug problems is quite sad.

Good luck.

brianteeman commented 5 years ago

on how to change things, probably in CMS 4.

If that is the case then we only need to be concerned with php 7+

HLeithner commented 5 years ago

thx @mbabker that function is looks much better, at least as concept. I'm not sure who far we should go trying to delete a file. As you mention under posix file system (confirmed for linux) its not possible to delete a file if the folder is write only (even if you are the owner). So in this case we have to change the write permissions for the folder.

if we do the magic for a windows version we should do the same for posix filesystem folder permissions.

btw. I would set the poermissions back to the original after deleting a file successful (folder) and before we thru an exception.

jobst commented 5 years ago

@mbaker the last suggestion will not work on a unix based system, you cannot chmod 0777, it will refuse to do this - as it should it goes 100% against every security principles of unix.

Below in he code section the file EventListener.php is RW (read and write) for the user running the webserver (Apache). It still fails on my servers upgrading. I have never ever had any problems before with any Joomla installation for any website on any of my servers until the last upgrade, now it is broken.

To show the problem I logged into the server as the user Apache (user running the server) and did a CHMOD 0777 on THAT file and you can see it it does NOT work (neither should it work as it would be an immense security issue).

First you see I appended some stuff to the file which shows the file allows write/append, then I did a CHMOD on that file which failed - you see it is write able but you should not be able to change the permission TO WORLD can read/write.

` [Apache@MACHINE /WebsiteRoot/libraries/gantry5/classes/Gantry/Admin] $>ls -al total 56 drwxrwx---. 3 Editor Apache 4096 Mar 28 11:54 . drwxrwx---. 6 Editor Apache 4096 Aug 10 2018 .. drwxrwx---. 4 Editor Apache 4096 Aug 10 2018 Controller -rw-rw----. 1 Editor Apache 7847 Mar 28 11:54 EventListener.php -rw-rw----. 1 Editor Apache 3601 Aug 10 2018 Page.php -rw-rw----. 1 Editor Apache 4839 Aug 10 2018 Particles.php -rw-rw----. 1 Editor Apache 5170 Aug 10 2018 Router.php -rw-rw----. 1 Editor Apache 3487 Aug 10 2018 Styles.php -rw-rw----. 1 Editor Apache 4479 Aug 10 2018 ThemeList.php -rw-rw----. 1 Editor Apache 3952 Aug 10 2018 Theme.php

append a comment at the end of the file and then show the last four lines

[Apache@MACHINE /WebsiteRoot/libraries/gantry5/classes/Gantry/Admin] $>echo "// a comment " > EventListener.php [Apache@MACHINE /WebsiteRoot/libraries/gantry5/classes/Gantry/Admin] $>tail -n 4 EventListener.php CacheHelper::cleanMenu(); } } // a comment

CHMOD the file to EVERYONE can read write

[Apache@MACHINE /WebsiteRoot/libraries/gantry5/classes/Gantry/Admin] $>chmod 0777 EventListener.php chmod: changing permissions of EventListener.php': Operation not permitted [Apache@MACHINE /WebsiteRoot/libraries/gantry5/classes/Gantry/Admin] $>

You cannot do something the operating system simply does not allow - a user cannot change a file to be world read/write, this would be a security flaw in itself.

Also there might be a much better approach - in case you are worried not being able to re-do something. Before you do the REAL copy/delete you go through the same file tree and CHECK whether EVERY file that needs to be writeable IS writeable, if NOT stop the process. Call it PRE-CHECK - if it fails tell the user which file if stuffed, this way you do not need to reverse. So when you do the real run later, you will NOT fail.

But just do not use CHMOD, you cannot tell UNIX what you want it to do, so work around it.

mbabker commented 5 years ago

Actually, yes, you can chmod a resource to 777 if you have the permissions to do so. Additionally, with many PHP filesystem operations, the umask (default 0022) is used and that converts a 0777 permission to 0755 for directories and 0644 for files (default config normally, it's actually a little harder to get PHP to set something to 777 than you may realize). And, as I mentioned before, in the scenario where the directory is readonly, the call to unlink() resulted in PHP setting the file to 0777 and failing to delete it because the folder is not writable. So no, chmod($file, 0777); in and of itself is not this super magical security flaw.

Below in he code section the file EventListener.php is RW (read and write) for the user running the webserver (Apache). It still fails on my servers upgrading. I have never ever had any problems before with any Joomla installation for any website on any of my servers until the last upgrade, now it is broken.

Please provide a reproduction scenario that can be created against the core Joomla API. The use of Gantry in your scenarios does not provide a valid means for core to add test cases to its unit testing library to prevent regressions, and I am not going to spend the minimal time I do feel interested in looking at GitHub digging into third party libraries.

Again, I'm done with this issue. I have provided all of the insights that I can, I have provided all of the potential courses of action that can address whatever problem people are running into, even without the ability to create the problem myself. It's on the rest of the "core team" to do something now because I just don't care enough to go any further here.

mahagr commented 5 years ago

Just to clarify: having 0777 in any other method than chmod() is totally safe because of umask() which is being applied to the permissions (read the PHP documentation!). And yes, chmod(0777) (which isn't using the umask, so the file will actually be using 0777) before delete is also safe because of only the user with proper permissions to change the settings can delete the file. It just ensures that the file can be deleted in case if something prevented writes on that file.

PS: The operation from the command line isn't permitted as your user does not have permissions to do the chmod action at all. Try using 0755 (or 0750) and it will fail too. You need to change your user to be able to change those permissions. Linux does not prevent you from changing your own files to world writable, you can try it and it'll just work.

jobst commented 5 years ago

@mahagr let me explain again.

I have never ever had problems with upgrades until the last update, it always worked on all of my Joomla websites. I also know security back and forth and how to apply it and that chmod will fail in many cases as you already see as I am NOT the only one complaining.

If you want to run webserver securely making sure you limit hacking in every possible way you can (its never 100% but close) one of the BIG security things: you do not have the USER running the web server be the OWNER of the tree - this is a no no, the OWNER of the tree is the EDITOR, the user running the web server is the GROUP.

Say you have a large of virtual hosts, each host has it's own editor, e.g. drwxr-x---. 3 /var/www/website1/htdocs/ Editor1 Apache drwxr-x---. 3 /var/www/website2/htdocs/ Editor2 Apache drwxr-x---. 3 /var/www/website3/htdocs/ Editor3 Apache You prevent Editor1 have access to website2 and website3 in case there are very important information in that tree. Apache (the user running the webserver) has access

By the way one of my favorite CRM's have learned this exact lesson, they PRE-CHECK every file they need to write before they actually do the UPGRADE and then tell the user "cannot write this file", then you have to fix it and restart the PRE-CHECK until everything is fine - ONLY then they do the real thing. For years they had that exact the problem they could not reverse the changes made because of failures.

PS: You do not seem to understand Unix very well, it will not work, CHMOD IS THE PROBLEM! There is NO other way in UNIX to give permission to change the file other than put it into sudoers. But that EXACTLY defeats the purpose of keeping a web server safe.

HLeithner commented 5 years ago

The only reason why we chmod is a IIS 7 issue, for normal POSIX systems this has no effect because if we can change permissions on a file doesn't not mean that we can delete it.

At least in theory see https://github.com/joomla/joomla-cms/issues/23228

Maybe @jobst and @mahagr want to understand that we also support other systems then linux. Michael showed a solution that works, setting it to 777 makes no sense on linux but as the comment says it seams to be needed for windows.

jobst commented 5 years ago

@HLeithner never said I do not understand you are supporting other systems, that is totally fine and all part of being a CMS ... it's just chmod has problems on Linux and that's all I am pointing out!

mahagr commented 5 years ago

I've been using Linux as my main operating system since 1996 and for 7 years my main job was programming in Linux kernel space in a security company (Firewall/IPS), so I do know POSIX pretty well. I also used to be a security expert and I do know the dangers of using world-writable permissions. I'm not saying that using 0777 is not bad, I'm saying that the code isn't doing that (with the exception of deleting files).

But on the contrary, I can say that using 0755 inside PHP code is BAD, because of it will prevent setups like yours from working because of Apache group isn't allowed to modify or delete the files. Using 0777 (with umask!) works and is safe in every single properly configured setup, 0755 does not because of it will not allow Apache to modify the files. And this is true primarily in Linux.

EDIT: Again, the only place where you should not use 0777 in PHP is chmod() command, which does not use umask. In there you should write chmod(0777 ~ umask()) for folders and without x for files.

mahagr commented 5 years ago

Just something I noticed from this:

drwxr-x---. 3 /var/www/website1/htdocs/ Editor1 Apache

Unless you are running your web server as Editor1, Joomla is not allowed to modify any of the files in your site. This means that you need to do Joomla updates manually by copying the files by hand and running manually the upgrade scripts.

Another note on the upgrades starting to suddenly fail: there was a bug in Joomla where read-only files were silently skipped by the installer -- installation process passed, but some files were left behind without an update. While not optimal, now the installer at least fails if some of the files cannot be changed, which is a great improvement in my eyes and saves a lot of time for everyone when things do not work after the upgrade. Not that it cannot be improved, I do agree that pre-check before installation may be a good idea.

ReLater commented 5 years ago

@ mbabker has provided some code in https://github.com/joomla/joomla-cms/issues/23504#issuecomment-483007077 .

Now my question to the experts here is: Why don't you provide "better" code with detailed and reproducable testing instructions and code examples that apply to Joomla core?

Anybody can test it then.

orlitzky commented 5 years ago

Before making the assertion that "changing permissions should not cause delete to fail", you need to understand filesystem permissions. On a Linux filesystem, a delete action requires write permissions. So a file in a readonly state (400 or 444) cannot be deleted

No:

$ mkdir permissions-test
$ touch permissions-test/example.txt
$ sudo chown root:root permissions-test/example.txt
$ sudo chmod 400 permissions-test/example.txt
$ rm -f permissions-test/example.txt
jobst commented 5 years ago

Just something I noticed from this:

drwxr-x---. 3 /var/www/website1/htdocs/ Editor1 Apache

My bad, I used this as an example. I made this up (my tree is somewhere else) and forgot to change the group write. Sorry.

miojamo commented 4 years ago

This also completely breaks every Joomla installation that uses filesystem ACLs to grant read/write access. Using "can I chmod" as a test is fucking stupid, and has no basis in anything at all for being correlated with "can I delete this file."

Here's the patch to fix it:

- if (!Path::canChmod($file))
+ if (!Path::canChmod($file) && false)

I have this issue under Windows Ubuntu 18.04 WSL, mounted directory with all permissions set to 777 by default by WSL. This patch worked great! There is no other solution so far. Thanks.

kris-sum commented 4 years ago

I have this issue under Windows Ubuntu 18.04 WSL, mounted directory with all permissions set to 777 by default by WSL. This patch worked great! There is no other solution so far. Thanks.

^ THIS! It seems that depending on how your WSL filesystem is mounted, the chmod will fail. Been scratching my head over this for a couple of hours!

miojamo commented 4 years ago

@kris-sum Do you have any other solution for the problem? I've tried to reinstall Ubuntu, but issue persist Also asked here with no answer: https://askubuntu.com/questions/1203423/windows-10-and-ubuntu-18-04-wsl-folders-permissions-problem

billtomczak commented 4 years ago

Just an FYI on this issue in case it might be helpful to others still struggling with this issue

We have been using Cloudways servers for our staging and demo sites and can now reliably reproduce this problem. The way they've organized their system, if you use the 'Reset Permissions' option for a site on a server, ownership is given to the master/root user of the server. The only fix is to call their support team and have them set ownership back to the site user. They are aware of the issue and claim they will fix it someday.

Whenever, for whatever reason, the right (wrong?) file(s) are owned by the master user, we'll start having this problem. A quick call to Cloudways support to fix all the file/directory ownership for a site solves the problem.

orlitzky commented 4 years ago

Just patch out the problematic line. It's wrong and never should have been added. This isn't a problem with your systems, it's a problem with joomla.

OctavianC commented 4 years ago

Just ran into this exact issue on a customer's server. $file is 0777 and can be deleted, but canChmod() fails for some reason and this prevents the installation from continuing. Simply commenting that line out resolves the issue.

mbabker commented 4 years ago

I find it funny I wrote proof of concept code a year ago that could have theoretically addressed this issue, and yet here we are, a year later, issue still open, and people still instructing to core hack to bypass the issue.

Can someone else please provide some code? Maybe someone will act on it if it didn't come from me. That seems to have worked a few times in regards to getting pull requests actioned.