Should dir_mode of default_permissions (from config.php) override sugar_mkdir calls?

[pgorod]

This is another possible issue from my analysis of permissions problems in SuiteCRM. I'm not sure it's an issue, so I phrased it as question; but it's worth considering it carefully - and I think it's a bug.

In calls to sugar_mkdir, an optional parameter is available to specify dir_mode permissions of the new directory. Here are the calls throughout our code:

ModuleInstall/ModuleInstaller.php:2052:        sugar_mkdir( $dest );

ModuleInstall/ModuleScanner.php:212:      'sugar_mkdir',

data/Relationships/RelationshipFactory.php:202:        sugar_mkdir(dirname($this->getCacheFile()), null, true);

install/installSystemCheck.php:244:               if ((is_dir($dirname)) || @sugar_mkdir($dirname,0755, true)) // set permissions to restrictive - use make_writable to change in a standard way to the required permissions

install/performSetup.php:487:    sugar_mkdir('custom/modules/Home', 0775);
install/performSetup.php:491:    sugar_mkdir('custom/include', 0775);

install/install_utils.php:2141:    if ((is_dir($dirname)) || @sugar_mkdir($dirname,0755)) {

modules/ModuleBuilder/MB/MBPackage.php:596:       sugar_mkdir($path . $trimmedPath, NULL, true);

modules/MailMerge/Merge.php:155:        sugar_mkdir($dataDir);

modules/MailMerge/MailMerge.php:153:              sugar_mkdir($this->mm_data_dir);
modules/MailMerge/MailMerge.php:154:              sugar_mkdir($this->mm_data_dir.'/Temp/');

modules/Configurator/controller.php:183:                sugar_mkdir('custom/modules/Home', 0775);
modules/Configurator/controller.php:187:                sugar_mkdir('custom/include', 0775);

modules/Administration/Common.php:55:    return sugar_mkdir("custom/include/language", null, true);
modules/Administration/Common.php:69:   return sugar_mkdir("custom/modules/$module/language", null, true);

modules/EmailTemplates/EmailTemplateFormBase.php:206:       sugar_mkdir('public', 0777);

modules/EmailTemplates/EmailTemplate.php:897:      sugar_mkdir('public', 0777);

modules/AOS_PDF_Templates/templateParser.php:75:        sugar_mkdir('public', 0777);

jssource/minify_utils.php:225:     if(function_exists('sugar_mkdir')){
jssource/minify_utils.php:226:           sugar_mkdir($prefix_process_path.'/'.$bu_dir);

include/dir_inc.php:50:     sugar_mkdir( $dest );
include/dir_inc.php:119:             if(!sugar_mkdir($mkPath)) return false;

include/DetailView/DetailView2.php:98:          sugar_mkdir('modules/'.$this->module.'/metadata');

include/SugarCharts/SugarChart.php:724:      sugar_mkdir($dir, null, true);

include/EditView/EditView2.php:136:          sugar_mkdir('modules/'.$this->module.'/metadata');

include/utils/file_utils.php:70:        sugar_mkdir($dir, 0775);
include/utils/file_utils.php:77:            sugar_mkdir($dir, 0775);
include/utils/file_utils.php:109:    if( sugar_mkdir( $temp_dir ) ){
include/utils/file_utils.php:179:        sugar_mkdir($dir, 0755);
include/utils/file_utils.php:186:            sugar_mkdir($dir, 0755);

include/utils/sugar_file_utils.php:46: * sugar_mkdir
include/utils/sugar_file_utils.php:62: function sugar_mkdir($pathname, $mode = null, $recursive = false, $context = '')

custom/backup/custom/modules/Administration/Reschedule_repair.php:84:       sugar_mkdir($dir, 0755);

As you can see, there are plenty of calls making use of that parameter (specifying 0755, 0775, etc).

The problem is that the function itself, found in sugar_file_utils.php, is not using that, it's doing this:

function sugar_mkdir($pathname, $mode = null, $recursive = false, $context = '')
{
    $mode = get_mode('dir_mode', $mode);
    if (sugar_is_dir($pathname, $mode)) {
        return true;
    }
    if (empty($mode)) {
        $mode = 0777;
    }
...

That first line, the call to get_mode (defined below, in the same file), on non-Windows systems will simply override the $mode value with whatever it finds in the config.php file.

So all those values passed into the function are not taking effect. I don't think this was the intended behaviour...

Possible change

I think it should be written like this:

function sugar_mkdir($pathname, $mode = null, $recursive = false, $context = '')
{
    if (!isset($mode)) {
         $mode = get_mode('dir_mode', $mode);
    }
    if (sugar_is_dir($pathname, $mode)) {
        return true;
    }
    if (empty($mode)) {
        $mode = 0777;
    }
...

Note that sugar_chmod() uses exactly this approach to the $mode parameter.

So, which should override which?

1. Should config.php always be used regardless of the values specified in each call? Then we should remove those values from the function calls, they're misleading...
2. Or should we check the parameter with isset and only use the config.php values when the parameter is absent?

What semantics do we want for the default_permissions in config.php? Are they settings that override everything, or are they just the base values to use when nothing else is specified?

Impact

In the current situation, with the default values of default_permissions specified in config.php, many directories that developers clearly intended to be universally writable will be created as 02770, and that final 0 will cause problems for some systems... Then this leads people to use very generous permissions in config.php, unnecessarily weakening their systems' security.

[chris001]

Default permissions is probably meant to be the permissions when there's not a permission requested by the calling code. Only use the default permissions when the permissions mode parameter is absent.

[pgorod]

Three more points of suspicion in this file:

1. Comments don't match the code in 4 or 5 functions, they keep saying default_permissions overrides $mode, when the code does the opposite.

2. sugar_mkdir() can be used recursively, however when doing the call for recursion PHP's mkdir() is called, shouldn't we call sugar_mkdir() again? Otherwise the permissions will be different for subdirs...

3. sugar_mkdir() has a parameter called context which does nothing except get passed down to PHP's mkdir(). This optional parameter is not used in any of SuiteCRM's calls to to this function.

It's amazing how parts of the code that are so sensitive for robustness and security are written so carelessly! :(

But leaving that aside... can I go ahead and do a PR with the changes for this file? Can someone else (especially from SalesAgility) corroborate me and Chris on this issue?

[chris001]

1. Interesting. The definition of default is, what you use when no information is provided. By this definition, mode should override default_permissions dir_mode.
2. Right, unless it would be possible to call mkdir() recursively and produce the same effect as calling sugar_mkdir() recursively.
3. Obviously it either didn't undergo a code review, or they left it in for future planned use, to signal to the lower layers, where the call is coming from.

[pgorod]

The issue with the recursiveness is a bit more complicated than I initially thought, but anyway my intuition turned out to be right. It's true that PHP's mkdir isn't called recursively because it doesn't need to be, it handles the several subdirectories inside the function.

But since the detailed notes on that function confirm that only the final directory (lowest level on the tree) gets the permissions specified on the mode parameter, for our purposes we would have to really create a recursive sugar_mkdir to create subdirs one at a time and chmod them one at a time.

This involves working through the path which might present difficulties with Windows vs Linux paths. Anyway after some time on StackOverflow I believe this kind of iteration would be a good solution:

function chmod_r($path) {
    $dir = new DirectoryIterator($path);
    foreach ($dir as $item) {
        chmod($item->getPathname(), 0777);
        if ($item->isDir() && !$item->isDot()) {
            chmod_r($item->getPathname());
        }
    }
}

That DirectoryIterator object seems to handle everything and we don't have to get into the whole forward-slash/backward-slash thing.

Regarding Windows, currently most of these functions don't actually do anything on Windows, most code is inside a if (!is_windows()) conditional block. This can't be right, Windows also has permissions, this is probably just assuming everything is open there. Unless, of course, it isn't...

[pgorod]

@adamjakab what's up with this deactivated test file?

I'm attempting a rewrite of sugar_file_utils.php to make it more robust (it's full of bugs that can be found just be reading the code carefully) and I suppose I could be doing my work with those tests at hand, and perhaps improve them. For example, recursive sugar_mkdir isn't working properly on all directory levels, but the tests aren't catching that yet.

I don't know anything about the automatic tests here, but

• does it have to be using VFS? Or could there be another simpler approach?
• is there a way to test automatically also on Windows?

Thanks!

[pgorod]

@adamjakab I don't think you caught my mention 19 days ago, maybe you were too busy with other things... can you please shed some light on my questions on those tests? Thanks!

[chris001]

VFS is a virtual file system, used to mock the real file system for phpunit tests, to protect you from hosing your machine's actual file system.

[pgorod]

Ok, so the questions are 1) can we get it to work and more importantly 2) do these tests make sense on a different file system? I mean, if I want to be picky about how permissions are applied and interpreted, how umask affects the created files, how sticky bits from directories are applied, and really get down to detail, then these tests will only be valid for VFS, right? Of course, if I run them in say, Ext3 on Ubuntu, they will only be valid for that. But at least a more mainstream filesystem would be nice.

I guess the automatic tests have to be rougher, higher level stuff, and I'll have to make picky tests just for my development and manually test then in Linux and Windows...

[chris001]

These should answer your questions https://github.com/mikey179/vfsStream-examples/tree/master/src/part02 https://github.com/mikey179/vfsStream

$ composer require mikey179/vfsStream

[pgorod]

Thanks Chris, you seem to know the answer to everything! : - )

I'm afraid I'm always biting off more than I can chew, I'm not going to work on these tests, I don't know anything about this. I'll just test locally and try to contribute code because I want to fix the creation of directories which is broken (and I know where and how).

I can edit the automatic tests later, after somebody gets vfsStream working. Thanks anyway.

[pgorod]

Now that I finally finished the PR regarding cron running as root (that was a long time coming), I intend to work on this one. I have no doubt these things put together will make a very noticeable improvement in SuiteCRM robustness.

Here, my current intention is to rewrite and refactor all the directory creation code which is currently a bloody mess. No wonder we get errors. There are now two functions to create directories:

1. sugar_mkdir which accepts a mode parameter, but ignores it; accepts a recursive parameter but doesn't really "recurse", just calls the php mkdir which can create multiple dir levels, but doesn't set permissions on all of them; accepts a context parameter, but it's never used.
2. mkdir_recursive which is in fact recursive (well - not strictly, it doesn't call itself; but it iterates several directory levels). At the final level it calls sugar_mkdir. It does not have a mode parameter.

My plan is to deprecate both these functions, and add a single new one suite_mkdir that is essentially the code in mkdir_recursive, but with a mode parameter. Then it calls php's mkdir directly, one level at a time, applying the requested permissions on all levels it needs to create. Then I refactor about 100 instances of calls to both old functions.

If anybody has objections I'd love to hear them before I start working on this... : - )