gantry / gantry5

:rocket: Next Generation Template / Theme Framework
http://gantry.org
1.04k stars 204 forks source link

inappropriate folder permissions #2363

Closed mandville closed 6 years ago

mandville commented 6 years ago

disappointing coding and possible security risk

if (is_dir($folder)) {
    return;
}

$success = @mkdir($folder, 0777, true);

if (!$success) {
    // Double check if the folder exists
mahagr commented 6 years ago

It's not a security risk, see PHP documentation on how the permissions are set in PHP.

mandville commented 6 years ago

so you will gladly set your folders to 777? so it is disappointing coding then?

its not a security risk

..Depending on the way your server is configured

mahagr commented 6 years ago

Every major library does the same as they expect your server to be secure and properly configured.

Symfony: https://github.com/symfony/filesystem/blob/master/Filesystem.php#L94

Twig: https://github.com/twigphp/Twig/blob/2.x/lib/Twig/Cache/Filesystem.php#L52

Doctrine: https://github.com/doctrine/cache/blob/master/lib/Doctrine/Common/Cache/FileCache.php#L217

Wordpress: https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/functions.php#L1626

Even PHP does the same: http://php.net/manual/en/function.mkdir.php

mandville commented 6 years ago

expect your server to be secure and properly configured

and the police expect everyone to observe the law.

a decent set up server would not need such a messy work around. If you open the folder permissions, then close them. simple good coding practice.

the code should be made idiot proof, not carrying a disclaimer to say that the users server may not be worthy of using this code. if it will not be fixed, just say so

and to complete your link list. https://www.anchor.com.au/blog/2012/09/the-chmod-777-trap-how-and-why-to-avoid-it/ http://laurent.bachelier.name/2010/07/chmod-777-is-evil/ https://magazine.joomla.org/issues/issue-jan-2011/item/353-777-the-number-of-the-beast

mahagr commented 6 years ago

But PHP isn't doing chmod 0777. Just look into the filesystem and you should see that. The articles are old and they are talking about doing chmod 0777 from FTP/SSH, not from PHP script.

The only CMS/PHP library that I'm aware of not using mkdir($folder, 0777); is Joomla which defaults to 0755.

This is a common misunderstanding on how PHP handles file permissions on file/folder creation. You shouldn't be using 0777 permissions for the folders -- but PHP will never allow you to do that -- unless your code specifically changes the PHP settings to override that behaviour (or if you use chmod() function).

It's a Won't Fix because of there isn't any security vulnerability here.

RaspberryLime commented 6 years ago

For the sake of such a trivial fix (replacing 777 with 755) what exactly is the problem with changing the code?

If a user’s server can’t cope with 755 for some reason isn’t it better to highlight that as a problem, and get the host to fix their server or the customer to move hosts, rather than actually potentially introducing a security problem with your software by using 777?

Regardless of what other vendors are doing, why not lead the way?

mandville commented 6 years ago

With all due respect, the Gantry code in at least 4 different places is creating a folder with 777 folder permissions .

You shouldn't be using 0777 permissions for the folders but that is exactly what the code you provide is forcing!

the articles are to show the dangers of 777 and yes they are over 5 years old but still relevant. if you are denying any security issue on an industry aware "dont do this because" situation https://www.google.com/search?q=are+777+folders+dangerous there is a multitude of scripts NOT relying on dodgy coding to create folders

newkind commented 6 years ago

Please note that 0777 via PHP is different than 777 via FTP/SFTP. When you set 777 via FTP/SFTP it's 777, but when you use 0777 via PHP it has to consider servers umask value.

https://secure.php.net/manual/en/function.mkdir.php

Note that you probably want to specify the mode as an octal number, which means it should have a leading zero. The mode is also modified by the current umask, which you can change using umask().

https://www.cyberciti.biz/tips/understanding-linux-unix-umask-value-usage.html

By default most Linux distro set it to 0022 (022) or 0002 (002).

The default umask for the root user is 022 result into default directory permissions are 755 and default file permissions are 644

Most servers have umask value set to 022 by default.

This means that by doing 0777 via PHP on server with umask set to 022 what you get is actually 755.

mandville commented 6 years ago

You can not guarantee the unmask value. if the rest of the script (gantry) allows the server to handle folder creation without using 0777 or similar then [insert thought logic here]

mahagr commented 6 years ago

Most of Gantry files are generated by Twig. If you are so worried about this issue, you should open an issue in there, though they have already responded to this one:

https://github.com/twigphp/Twig/issues/2353

As they said, all the distributions are using secure settings and there is a reason why they use the default 0777 mask.

RaspberryLime commented 6 years ago

Most, but not all?

Can’t your code check to see what the server value is and adjust accordingly if it’s not what you’re currently assuming it to be?

mahagr commented 6 years ago

Joomla is using 0777 mask in multiple places as well. I guess you won't now claim that Joomla is vulnerable?

mahagr commented 6 years ago

https://github.com/joomla-framework/image/blob/master/src/Image.php#L392 https://github.com/joomla-framework/filesystem/blob/master/src/Folder.php#L212

So yeah, everyone uses mkdir with 0777 mask, including latest Joomla versions. :) In fact Joomla 4 will do that much more than 3.x.

brianteeman commented 6 years ago

Really?

mahagr commented 6 years ago

@brianteeman Yes, and it is not a vulnerability. Basically they are claiming that Symfony, Twig, Joomla, WP, Zend, Doctrine and about every PHP code out there is vulnerable without understanding how the mkdir() method works. The code is only using bad permissions if user makes a mistake of changing PHP configuration where he should not or if the server has already been compromized.

I'm saying this only because of someone reported this into vel as a vulnerability when it is user error:

https://vel.joomla.org/vel-blog/2185-gantry-package-5-4-26-other

brianteeman commented 6 years ago

Where does joomla set anything with 777? Certainly not at the two places you mention.

Even if you think it is down to a user error that it is possible no code should be trying to set anything at 777 and relying on the user.

mahagr commented 6 years ago

@brianteeman Writing mkdir($folder); is same as writing mkdir($folder, 0777);. The second parameter uses umask() which turns the permissions to whatever was configured in PHP, usually something like 0755, 0775, 0700 or 0770.

Joomla 3.8 uses the above code in multiple places, including the updater component and Joomla Framework.

In fact, forcing 0755 mask for the directory is considered bad practice, which is why libraries like Joomla Framework, Zend, Symfony, Doctrine and others never assume folder permissions in their code. The code does not rely on the user of the code, but it does assume that the server is properly configured and well maintained.

brianteeman commented 6 years ago

If you look at the framework code you will see $oldmask = umask(0); mkdir($path, $perms); umask($oldmask); which does allow you to correctly set a specific permission and overcome any bad server configuration or bad code

mahagr commented 6 years ago

@brianteeman Actually it does the opposite. It allows you to create world writable folders no matter what the settings in your server are. So it moves the responsibility of using the correct permissions from the server maintainer to the programmer (or a setting in admin, if the programmer decides to do that). That also explains why the method does not use 0777 mask -- it is not the mask, it is the real permissions, thus making the method dangerous to use as every other library uses the mask except Joomla.

So basically you can use mkdir($dir, 0777); safely in PHP EXCEPT when you are using Folder::mkdir() from Joomla Framework. To be honest, I disagree with this decision, mostly because of it is not how mkdir() works in PHP itself. That said, it is still not a vulnerability, not in our code, not in Twig code nor in Joomla.

brianteeman commented 6 years ago

0777 is not a mask

022 etc is a mask

brianteeman commented 6 years ago

The responsibility should be that of the programmer and not the site maintainers

mahagr commented 6 years ago

Yeah, I said mask in wrong place as I couldn't guess any other word to describe it (I'm not native in English). What I meant is that the method in PHP gets filtered by the mask, but Joomla implementation seems not to do it. There's nothing wrong with either approach, except that Joomla implementation may cause bugs in code if users assume it works like PHP.

I disagree with you on this -- as a programmer I shouldn't make assumptions on how the server has been set up. PHP makes sure you always use the right permissions as much as bash or FTP makes sure that if you copy files to the server, they are by default using the correct permissions. All servers come with working and secure permissions -- it is the site maintainers responsibility not to mess up with the settings they do not understand. If I change umask from bash to make all the files world writable, do you blame bash from allowing you to do that? There may be a legit reason why you would want to make something like that.

Let's take an example: you have a server with separate user and web server accounts. Most PHP applications you can make that to work by carefully configuring your server to use 775 permissions for folders and 664 for the files, but that is not possible in Joomla as it blocks the write access for the group. This makes Gantry to properly work in this kind of setups with the exception of Joomla.

Also your claim that it is not site maintainers responsibility is wrong. While by default they do not need to know about this as all servers come with secure default settings, they do need to take the responsibility not to change the default masks, not only in PHP but also in their bash etc shells they are using. It doesn't help to have 0755 permissions for newly created folders if every folder in your installation already uses 0777 permissions.

As I said many times before: this is not a security issue as no servers come configured with unsafe folder/file masks. There is a valid reason why you would want to change those masks, which means that the PHP programmer should not make any assumptions on the permissions of the new files/folders unless they are for temporary files or there's another specific reason to use more strict permissions. Also, there is no added benefit of not using the default parameter as it does not improve security in any imageable way. I am not talking about Joomla method here -- if you're using Folder::mkdir(), you need to be very careful of what permissions you use in your code, but that is not true with the builtin mkdir(). I believe that Joomla is how it is because of historic reasons and if @mbabker or anyone else would make the methods from scratch, they would do them as they are elsewhere in the Framework -- and that is like everyone else has been doing for ages in the PHP world -- using the default mask of the mkdir() method.

RaspberryLime commented 6 years ago

“It doesn't help to have 0755 permissions for newly created folders if every folder in your installation already uses 0777 permissions.”

How do you know that all the other folders are 777? You don’t. You’re making assumptions. And the fewer 777 folders there are, the better.

“the PHP programmer should not make any assumptions on the permissions of the new files/folders unless they are for temporary files or there's another specific reason to use more strict permissions”

Unless they’re for temporary files? No, those need to have the correct permissions too, especially when the issue is with folders which are 777. The content of the folders may be temporary but the folder is not. (Yes the folder gets deleted and recreated whenever the cache is updated, so it’s temporary in that sense, but the fact there’s a folder there with the same name all the time means it’s not temporary for all intents and purposes.)

How about you stop making assumptions and make your code bulletproof no matter what the server configuration is?

mahagr commented 6 years ago

@RaspberryLime There is no point in discussing this further. Just try to create folders with 0777 permissions and you'll see that it will never do that.

There are also no known vulnerabilities on this in any PHP library out there and even Joomla is using the same code as I am in multiple places (in its updater and Image class).

Are you saying here that the latest Joomla 3.8 is vulnerable as well? Because of if you think that Gantry or Twig are vulnerable, it means that Joomla is too.

fcoulter commented 6 years ago

Please explain how this happens then: https://forum.joomla.org/viewtopic.php?f=714&t=965475&p=3539833#p3539827

mahagr commented 6 years ago

@fcoulter It happens in a fresh Joomla installation without any extensions installed. Please do not blame extensions without first testing the issue.

mbabker commented 6 years ago

I believe that Joomla is how it is because of historic reasons and if @mbabker or anyone else would make the methods from scratch, they would do them as they are elsewhere in the Framework -- and that is like everyone else has been doing for ages in the PHP world -- using the default mask of the mkdir() method.

The only way I would do that is if I wrote a wrapping method (like Joomla\Filesystem\Folder::create() is) that did not allow the developer to specify permissions at all (basically omit that method's $mode argument). As long as the developer can specify permissions, what we have in that method is IMO the most safe and correct way to ensure that the path is created with the permissions specified by the developer (even if it is 0777). Because if you don't deal with the umask first, then what you specify for the $mode argument of mkdir() is NOT going to be what the final permissions are. In the default settings, this is OK (because the default configuration for an AMP stack results in directories being created at 0755 or 0775, never 0777), but ONLY if you are using the default settings and not specifying another value.

Yes, the code allows developers to shoot themselves in the foot. Yes, it can result in unsafe permissions for created paths in a production environment. No, it is not our responsibility as the API owner to if ($mode === 0777) { throw new FilesystemException('What you are doing is very unsafe.); }.

Long and short, just grepping any PHP code base for "777" is wrong. You really need to understand the context behind the code instead of just seeing that string and posting "this is wrong". Especially when the default value of the optional $mode argument of the in-question mkdir() PHP function is 0777, which as pointed out is safe on correctly configured systems because of the umask.

fcoulter commented 6 years ago

This is a Joomla installation with gantry installed. All the folders with elevated permissions appear to be generated by gantry.

I have never come across a problem with folder permissions using Joomla, either in a fresh installation or with other extensions.

And I am not blaming anyone, just asking a question, which you have avoided answering. It is not my job to test your extension, that is yours.

mahagr commented 6 years ago

Thank you @mbabker, I knew what your response would be and I really appreciate your insight.

@fcoulter As I just said, I did reproduce this "issue" in a clean Joomla installation by downloading the latest Joomla version from joomla.org site without installing Gantry. All Joomla cache folders were world writable.

mahagr commented 6 years ago

This is the line in Joomla code that allows the folder to be world writable:

https://github.com/joomla/joomla-cms/blob/3.8.12/libraries/src/Cache/Storage/FileStorage.php#L488

Again, this is not a vulnerability, it is likely either 1) server configuration error 2) user error 3) hacked site 4) enter your favorite claim here

mahagr commented 6 years ago

Changing the Joomla code to

            @mkdir($dir, 0755) && file_put_contents($indexFile, '<!DOCTYPE html><title></title>');

"fixed" the "issue" for me.

@mbabker, please say me that this is not a bug and you won't fix it. :)

fcoulter commented 6 years ago

The problem is that most users have no idea whether they are using a correctly configured system or not, and have no way of changing it if they are not. Most will be on shared servers with no control over the configuration.

Is there not a case for making a bit more effort to check for this?

mbabker commented 6 years ago

If you want to check server config (because it's not the easiest thing to do), here's a one-liner (I would put this in a standalone umask.php file so the setting is not influenced by anything else and you should in theory see what the server's config is):

<?php
echo 'Current umask is: ' . decoct(umask(0022)) . PHP_EOL;

(Note this will also reset the umask to the "safe" 0022, because there is no way of getting the umask without resetting it)

If it says the current mask is 22, you're good; mkdir() will create directories with 0755 permissions. If it says the current mask is 2, you're OK; mkdir() will create directories with 0775 permissions. If it says anything else, IMO you have a problem and need to check that.

If what you're getting from that snippet is acceptable yet the cache directories are still created with 0777 permissions, that tells me that something is calling umask(0) and you need to scan your install to find it.

mbabker commented 6 years ago

Is there not a case for making a bit more effort to check for this?

Any check is going to be arbitrary at best. Like I said, I consider what we do in Joomla\Filesystem\Folder::create() to be safe because it eliminates the potential of influence by a misconfigured umask, at the expense of allowing a developer to call Joomla\Filesystem\Folder::create('/whatever/my/new/directory/is', 0777); and actually get a 0777 permission set on a directory (but they're calling it that way, it's not our responsibility to tell them no).

Changing the Joomla code to [snip] "fixed" the "issue" for me.

If it can be guaranteed 100% of the time every time that calling mkdir($path, 0755); does NOT create side effects ever, I might be willing to entertain a patch. Without that assurance though, I'd be very hesitant to arbitrarily change every mkdir() call.

mahagr commented 6 years ago

@fcoulter I appreciate that you have taken this issue seriously, I really do. I have taken it seriously, too, and used half a day trying to figure out what is going on. I wish that someone had pointed me to the Joomla forum earlier as I could figure out the reason right after reading the first post, where user said that ALL folders in /cache were using 0777 and later that enabling Joomla caching caused the issue to come back. On that point I was able to reproduce the issue in 5 minutes and later found the line which is reason for the behavior.

I just wish that people stop pointing fingers on extensions when the issue has nothing to do with them.

mahagr commented 6 years ago

@mbabker I can guarantee that the "fix" causes a lot more issues than it fixes, I have already shown an example earlier in this issue how it breaks existing sites in this comment: https://github.com/gantry/gantry5/issues/2363#issuecomment-421944160

So by fixing this one site (eg. fixing the symptoms and not the cause) you guarantee that you will break multiple well-configured sites. Not only that -- the fix would also cause hacked sites to stay undetected and people thinking that their sites are good when they are sending a lot of spam messages or doing something even worse.

fcoulter commented 6 years ago

Presumably instead of using mkdir you could use Joomla\Filesystem\Folder::create('/whatever/my/new/directory/is', 0755); ? That should guarantee the folder permissions as it does deal with the umask

Is there a reason why this is not acceptable?

(Looking at the PHP documentation and the discussion following, there are some issues with umask() http://php.net/manual/en/function.umask.php so it sounds like there is never going to be a perfect solution.)

But still using Joomla\Filesystem\Folder::create should stop 0777 folders being inadvertently created.

mbabker commented 6 years ago

Is there a reason why this is not acceptable?

In some contexts, there is an unacceptable performance hit with using that abstraction over mkdir() directly, so blindly changing all calls isn't acceptable either (actually it is explicitly documented in one class that Joomla core avoids this for the mentioned performance issue, especially as the Joomla\CMS\Filesystem classes still support the FTP layer). For everything outside of the Joomla CMS environment, that means introducing a rather large external dependency to a package to get what could in essence be a 5-line helper method. So, dependency management comes into play there (remember Gantry is not a Joomla only product).

But still using Joomla\Filesystem\Folder::create should stop 0777 folders being inadvertently created.

It does stop 0777 directories from being inadvertently created by setting sane defaults and doing what we can in avoiding umask related issues, it does not stop developers from explicitly creating 0777 directories. That's the extent we can sanely go without imposing arbitrary restrictions for the sake of arbitrary restrictions.

mahagr commented 6 years ago

First, I'm really sorry if I've sounded blunt on this issue, I've not had enough sleep and rest during the weekend because of being in training and moving my office at the same time for the past few days. I was trying my best to explain the issue, but sometimes things are hard to explain and it does not help to do it in a foreign language and to be tired while you're doing that.

@mbabker Is 100% correct in his reply -- extra layer would just make caching too slow to be useful in Joomla. I bet that turning cache off would then speed up your site. And even if Joomla did use its Folder methods, Gantry is supposed to work in WordPress and Grav as well and having Joomla Framework as an extra dependency would have been too much.

What comes to using 0755 everywhere (in this context), it is considered a bad practice for multiple reasons. 1) It forces you to use specific server setup and restricts administrators choices on how they want to set up their sites -- it also causes compatibility issues on existing setups. 2) It is really easy to miss the permission somewhere in your code (Joomla updater), or you happen to use some library (including Joomla Framework), which does not follow the same practice, not to mention potential hazards if you install a new software to your site. 3) It hides the real security issues (both in the code and in the server setup) and makes you feel a false sense of security. It does not prevent attacks from being successful, but it may prevent automated tools from locating the bad code, bad configuration or hacked site.

There are other valid points, but I'm way too tired to think about them.

I think that at this point is appropriate to say something from my background even if I usually do not want to do that. I used to be part of Joomla Security Group, fixing few vulnerabilities in Joomla, though unfortunately I just do not have the time anymore -- and I barely had it back then. Before that, I was a trained security expert coding in Linux kernel. I was doing a firewall and intrusion prevention system for routers, from the router at your office to IBM supercomputers. I am not perfect, and I do make mistakes (not to mention that it has been a while), but I generally know what I am talking about when it comes to security.

@fcoulter There is never a perfect solution, most of what we do is full of compromises. It is still important to pick up the lesser bad whenever possible and in this case, it is better to follow well-known practices for multiple reasons which were brought up in this issue. I know it is frustrating as a moderator or support person to deal with issues like this where there is an "obvious" solution to prevent it. But you cannot fix the real issues by hiding them. What if the site uses 0777 permissions because of it got hacked? Or because of someone made a mistake? Would it be better automated tools to detect the issue than allowing the site to stay compromised because it is more convenient to hide the issue? I am not saying that this is what happened, but at least now we know that there is something wrong with the site.

mahagr commented 6 years ago

The discussion continues in Joomla forum and so far we have tracked down the issue to the server. Another site in the server was hacked not a long time ago which means that there is a possibility that the server has been compromised. Another possibility is that there is a local PHP settings file, such as .htaccess with bad configuration settings in it.

PS. The issue is also not restricted to Gantry cache but affects the whole /cache folder, including all the files created by Joomla itself.

fcoulter commented 6 years ago

extra layer would just make caching too slow to be useful

It is surely not necessary to add an extra layer just to be a bit more careful. How about mbabker's suggestion of checking the umask before creating the directory?

It forces you to use specific server setup and restricts administrators choices on how they want to set up their sites

Then offer site administrators some choice on this, eg as a configurable option. And part of my point is that those responsible for server setup and the site administrator may be completely different people. The latter may have little control over the former, but they should at least be able to have some control over what their software is doing on their behalf.

It is really easy to miss the permission somewhere in your code (Joomla updater), or you happen to use some library (including Joomla Framework), which does not follow the same practice, not to mention potential hazards if you install a new software to your site.

I don't understand that point.

It hides the real security issues (both in the code and in the server setup) and makes you feel a false sense of security. It does not prevent attacks from being successful, but it may prevent automated tools from locating the bad code, bad configuration or hacked site.

I simply do not buy this as an argument for not trying to do the right thing.

mbabker commented 6 years ago

In performance critical code, any extra step impacts processing time.

It forces you to use specific server setup and restricts administrators choices on how they want to set up their sites

Then offer site administrators some choice on this, eg as a configurable option.

There should never be an application level setting to define a umask or default user permissions for the filesystem. A running PHP application should be respecting the server configuration, not bypassing it or defining its own overruling configuration.

It hides the real security issues (both in the code and in the server setup) and makes you feel a false sense of security. It does not prevent attacks from being successful, but it may prevent automated tools from locating the bad code, bad configuration or hacked site.

I simply do not buy this as an argument for not trying to do the right thing.

Application level overrides of server configuration masks a higher level issue. It's not about someone choosing to "do the right thing". There are a lot of things that Joomla, WordPress, and Grav could do to "do the right thing" in some people's eyes, but the reality is some of those choices would be an override of a server level configuration directive (right or wrong) and some server administrators would consider the application code to be doing the wrong thing at that point.

mahagr commented 6 years ago

Thanks Michael, though I think I already proved my point why it would have been a bad idea to make these changes. I am almost sure what the end result will be -- I have seen this too many times before.

What @fcoulter is proposing is "security via obscurity", which is usually used by large corporations when they are trying to hide their mistakes. It usually fails because hackers will take advantage of the fact that nobody can find the real bugs/issues because they are so hard to detect.

In this case us "fixing" the issue would have prevented both the site owner and the server maintainer from seeing potentially compromised and at least misconfigured site. In both cases, there is a major security risk, which should never be ignored.

What comes to other comments, Michael already responded to them and I agree with him 101%.

fcoulter commented 6 years ago

What @fcoulter is proposing is "security via obscurity", which is usually used by large corporations when they are trying to hide their mistakes. It usually fails because hackers will take advantage of the fact that nobody can find the real bugs/issues because they are so hard to detect.

I am most certainly not proposing security by obscurity. I am simply suggesting that we should not be making a bad thing worse.

I normally hate argument by analogy, but I am going to do it anyway. It is like saying "it is possible that your house is made of highly combustible material. Let's find out by setting fire to it. Then we will know if there is an issue."

In a real world environment you simply cannot automatically assume that the umask is correctly set and say that it is not your problem if it is not. Even if the server is perfectly configured, as you helpfully point out, all it takes is somewhere in the code there is a umask(0), and whoever wrote it forgot to restore the value after they did whatever they were going to do, or made a logical error that meant the code restoring it is not executed, or something else went wrong that prevented that. Most Joomla sites will be hugely complex systems, with maybe dozens of extensions, each with hundreds or thousands of lines of code. Not all of it will be perfect.

If you want to be free to use

$success = @mkdir($folder, 0777, true);

which is a potentially dangerous operation, then it seems to me that you have some responsibility to at least check whether this is actually problematic. I don't suggest that you need to do this every time, but perhaps a check in your extensions control panel? Just a thought. Then if there is an issue with the umask and the site administrator chooses to ignore it you have done your due diligence, and anything that happens is entirely their own fault.

I do not think that other industries would be able to get away with taking such little responsibility. A car manufacturer would not be able to say "our brakes work fine when you drive in good conditions, it's not our fault if you took them out in bad weather. Oh sorry, didn't we tell you?" Sorry, another argument by analogy, but actually I think it is pertinent.

I have the greatest respect for your expertise, and Michael Babker's, but I think that sometimes expertise can narrow your range of thinking, and I think that might be what is happening here.

mahagr commented 6 years ago

@fcoulter I thought that Joomla already had that check in admin (for 0666/0777 files/folders)...?

But yes, it is a really, really good point and Joomla (and other CMSes) should really have that check in them to make sure that the server has been properly configured. If you have been following the forum topic at all, Joomla cache files have the exact same problem and by looking at the code, also images folders seem to have similar permission issues on badly configured/hacked servers.

I think we should create an issue into Joomla itself to solve this file permission issue in the best possible way. To me, the best possible solution for this is to have a similar logic to Grav, where there's a plugin which runs through a fixed set of tests and the result gets cached so it doesn't have a performance penalty. This plugin could run from admin only and display a notification message just like when you have too old PHP version (or if you're still using Joomla 2.5).

I do not think that this is something that should be fixed in extension level, mostly because of many extensions use composer and you cannot assume that every extension developer should know the internals of the libraries they use. Gantry is not the only extension which uses external dependencies and I bet almost every site out there has an extension which is creating files in a way or another. Heck -- Joomla itself has world writable cache files, you don't need to install Gantry to get them.

@fcoulter I fully agree with you that we should not get away of the responsibility to warn users if their sites have well known and common issue. What I am against is fixing the issue by hiding it under the carpet. If there is an issue, there should be a huge RED warning about it instead of trying to fix it with workarounds which may cause much more trouble in the long term. But even no warning is better than masking the issue so it cannot be found and fixed. This is because there are other tools which allow hosting companies to find potential issues in the sites. By hiding the issue you make no favour to anyone, even though it does help greatly on support.

Based on your latest comment this responsibility belongs to Joomla, Grav and WordPress, not for the extensions/plugins which can be installed on them. And yes, Joomla itself creates world writable cache files. :)

I am really happy that we finally stopped being defensive and have constructive proposals on how to deal with badly configured servers.

Please try if you can reproduce the issue with a plain Joomla installation without installing any 3rd party extensions. Here is example on how to test it in Linux:

https://www.siteground.com/kb/how_to_change_the_default_umask_in_apache/

Or you can just add umask(0); to index.php if you feel lazy -- there's no difference on behavior.

fcoulter commented 6 years ago

I fully accept that you can reproduce the issue on a plain Joomla installation. I have not disputed this.

I still believe that there is a place for a check at the extension level as well, for several reasons:-

  1. It is the extension that will actually be doing the potentially unsafe operation, therefore I think that some responsibility lies with the extension developer
  2. At the moment Joomla etc do not show these warnings
  3. If the umask issue is produced through faulty code it might not affect the entire site, eg some plugin code may or may not be executed depending on circumstances, eg an extension-specific plugin
  4. It never does any harm to repeat warnings.

I have never suggested hiding problems, and I find it slightly irritating that you keep attributing that to me. I am simply suggesting that if there is a problem we don't make things worse than they need to be. I think that is a good rule for life in general as well.

I also favour constructive proposals.

mbabker commented 6 years ago

If you want to be free to use

$success = @mkdir($folder, 0777, true);

which is a potentially dangerous operation, then it seems to me that you have some responsibility to at least check whether this is actually problematic.

mkdir() by default, by your argument, is a "potentially dangerous operation". Its $mode argument defaults to 0777, and I'm going to assume (because this predates me being around the language) its done that way in part because the language developers expected developers using the function to be running a correctly configured environment where umask would correctly bring down 0777 to the "normal" 0755). This is why I said before you can't just scan a code base for "777" and say there's a security issue, there is a lot more context that has to be taken into consideration, and you have to take into consideration PHP internals such as default values which may be omitted from the call (the only reason you're seeing it here is because the $recursive argument is being set to true, and its default is false).

And yes, Joomla itself creates world writable cache files. :)

Just to be clear here, it is not that Joomla is actively trying to make 0777 cache directories. That comes from external influence, i.e. server misconfiguration that shared hosts will never own up to. Same can be said for most any PHP library or application. Actually, if you want an example of irresponsible use of umask manipulation to purposefully create 0777 directories, here you go.

fcoulter commented 6 years ago

Actually, if you want an example of irresponsible use of umask manipulation to purposefully create 0777 directories, here you go.

!

mkdir() by default, by your argument, is a "potentially dangerous operation".

Yes I mean that any operation that impacts the file system is going to have some potential risk attached (under normal circumstances that should mean not an actual one).

RaspberryLime commented 6 years ago

If there isn’t currently a check done for a sane value of umask during Joomla installation, then can one be added in? With a massive warning and perhaps some copy-and-pastable text for a shared host user to send to their host in order to try and get it fixed?

Then have a check run when an admin logs in to the backend. Perhaps cached for 12 hours so there’s no real hit on performance. If issues are found then again display a massive warning with copy-and-pastable text, above where the notice about updates to Joomla currently go.

Remember that there are a lot of shared host users, with non technical users using one-click installers provided by their host, and also not-technical-enough users installing Joomla the “traditional” way. Providing these users especially with something which explains the situation, and which they can send to their host, I think would be useful. The one-click installation users are perhaps the most vulnerable to being hacked so just having a check during installation itself isn’t enough. (Have never used an installer so don’t know how they handle errors, are they different for each host?)

I still think that extensions should do checks too, but having the two checks above would at least identify server-level configuration issues.

mahagr commented 6 years ago

@fcoulter I know that I had a hidden provocation there, sorry about it. I think I may still be slightly offended by some of the comments on this issue. Please forgive me! I know that you didn't mean to hide issues, you want to solve them just as much as I do -- and so do everyone else who are participating in this issue. But sometimes the best intentions may cause more issues than they solve, especially if you miss some crucial detail. This is why I have been so strict on the issue, people were forcing me to solve the issue by making it even worse. And by worse, I mean as seen by a seasoned developer who has been solving security issues in the past.

@mbabker Yes, I should have said that Joomla allows you to create world writable folders in the context of this issue -- by someone setting umask() to 0. I was assuming that everyone reads every message, but yeah... I'm trying to be more careful in the future. I am just too tired, I didn't really have much sleep last night. :(

Regarding that WordPress code, WOW. That said, I am not happy with this Joomla code either: https://github.com/joomla-framework/filesystem/blob/master/src/Folder.php#L208-L220 But based on our previous discussion, I believe we agree on why it is so bad. Is there any way to make the method safe in J4.0 without affecting the most common use cases? I think there is, but I want to hear your 2 cents on it... If we want to change how the method work, it is either now or never.

@RaspberryLime Great response, I think that is the way to go! Do we already have an issue in Joomla for this one? Who wants to create it?

Now I need a nap. :)