Up to date with #106.

Problem/Motivation

The MTimeProtectedFileStorage protects against "rogue" upload scripts that take filenames from $_GET. Unlike so many threats this is being used in the wild to hack sites and the problem is that often these scripts are standalone , not need Drupal, just sit there forgotten. Now with Drupal 8 including these files; you can't even protect by putting the filedir outside of the docroot cos an attacker could rely on Drupal *including* generated PHP files. So I somewhat expect MTimeProtectedFileStorage being used in a lot of setups, not just shared. At the same time, we want to use the php storage for many things -- joachim suggested dumping the module implements cache into this, I suggested regenerating entity classes on field CRUD and using the container directly. So this will again add to the widespread usage of php storage. We need this to work satisfactory to many, often contradictory requirements. Currently, however when the webserver runs under a different user than the command line user, you can't use drush with these files, it's impossible to read these from an editor/debugger and so on.

Proposed resolution

We need to relax permissions while still remaining secure. We decided that the non-listability of the directory is not important against rogue upload scripts as you can't list the directory easily (like .htaccess disabling directory listing) and so you need some arbitrary PHP code to obtain the filename. And much good the filename does to you: if you just write it the mtime will be higher than the directory mtime and that's invalid. Deleting and writing a new one changes the directory mtime and so that's invalid too.

Remaining tasks

Review and commit.

To Review,

In short: install from GUI, drush cc, check php directory empty; clean up; install with drush; clean cache from UI; check php dir empty. Detailed:

Please post your results, even if you only get part way through testing. :) Thanks!

  1. You need an environment where the web server user (like www-data or http) is different from the user who runs drush from command line. Most Linux environments are set up this way, most Mac OS and Windows setups are not. If you use MAMP Pro, you can meet this user/environment requirement via the GUI which under the General menu item allows you change your Apache preferences to use 'www/mysql' instead of 'youruser/youruser'. After making this change you'll need to restart Apache, and then you can verify the user/status for the Apache process by running 'ps aux | grep apache'. Additionally, in php.ini you may need need to increase the memory_limit to at least 128M and max_execution_time to 60 setting in MAMP's php.ini file (you can edit this in MAMP Pro via File > Edit Template > PHP > 5.3.20.php.ini)
  2. You need drush from github master (do a 'git remote -v' in your drush directory to see if it is using github. If it is, 'git pull --rebase'. If it is not coming from github, remove it and clone from github.)
  3. Patch drush (from github) with https://drupal.org/files/drush-do_not_test.patch this actually removes a currently necessary hack from drush.
    1. curl -O https://drupal.org/files/drush-do_not_test.patch
      (or use wget instead of curl -O)
    2. git checkout -b somedrushbranchname
    3. git apply --index drush-do_not_test.patch
    4. git commit -m "cm permissions drush patch to remove hack"

    In order to show the drupal patch works, we need to verify the problem exists first (without the patch). Then later redo the steps with the patch.

  4. Make sure you have the latest drupal from git. 'git pull --rebase' in your drupal docroot.
    and clear out previous install:
        sudo rm -r sites; 
        git checkout sites; git status;
        cp sites/default/default.settings.php sites/default/settings.php;  mysql -uroot -proot -e  "DROP DATABASE drupal";
    1. Do not patch drupal (so you should see an error in step 7).
    2. Patch drupal. patch (from #98 or more recent. copy the address into paste buffer and use it in the curl or wget.)
      1. curl -O https://drupal.org/files/1908440_102.patch
      2. git checkout -b cm-perm
      3. git apply --index 1908440_102.patch
      4. git commit -m "cm permissions patch 102"

    ---- now we can test -----

  5. Now install from the UI (that uses user www-data or http). Note the core/INSTALL.txt has instructions for setting permissions on sites/default and settings.php. It says to reset permissions when done, but we will be removing sites and getting it back out from git when we are done and then reinstall. Now try drush cc -y all (that uses your command line user).

    (without patch/4.1) Should see error: ...
    (with patch/4.2) With the drupal patch (from #98 or more recent) , no errors should occur.

  6. Then clean up (remove files directory, clean settings.php) try to reinstall with drush si (if it works; there are reports when it doesn't in drush issue queue.) Now try to clear cache from the UI.
    (without patch/4.1)) Should see error: ...
    (with patch/4.2)) With the drupal patch (from #98 or more recent) , no errors should occur.
  7. Cache clear should remove the sites/default/files/php directory.

Note: this means to review, install has been done 4 times. without the patch, in the ui. without the patch, with drush si, with the patch, in the ui. with the patch, with drush si. Between each of these installs, the old site needs to be wiped out and start clean. See the steps in 4, but key is removing the sites default (files, php dirs and the settings.php) and also dropping the db.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

clemens.tolboom’s picture

Assigned: clemens.tolboom » Unassigned
FileSize
3.15 KB

The attached patch makes it possible to work again by doing some directory tweaking like

$ chgrp www-data files # _www on mac
$ chmod g+s files
$ umask 0002

[edit]Note:
The user running drush is a member of the same group as webserver

clemens is member of group www-data
[/edit]

Status: Active » Needs work

The last submitted patch, cm-commandline-umask-1908440-1.patch, failed testing.

clemens.tolboom’s picture

Assigned: Unassigned » clemens.tolboom
Status: Needs work » Needs review
FileSize
5.26 KB

I changed the test to reflect the same attributes and respect the current UMASK.

clemens.tolboom’s picture

Assigned: Unassigned » clemens.tolboom

Are other scripts like Running Tests Through command-line failing too?

clemens.tolboom’s picture

The code I want to change originated from #1675260: Implement PHP reading/writing secured against "leaky" script. Which had a long discussion about the security constraints.

The twig code I'm refering to is

core/vendor/twig/twig/lib/Twig/Environment.php:                @chmod($file, 0666 & ~umask());

Doing a grep on core as a whole

$ grep -r umask *
core/lib/Drupal/Component/Archiver/ArchiveTar.php:                // make file executable, obey umask
core/lib/Drupal/Component/Archiver/ArchiveTar.php:                $mode = fileperms($v_header['filename']) | (~umask() & 0111);
core/vendor/symfony/class-loader/Symfony/Component/ClassLoader/ClassCollectionLoader.php:            @chmod($file, 0666 & ~umask());
core/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/File/File.php:        @chmod($target, 0666 & ~umask());
core/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/File/UploadedFile.php:                @chmod($target, 0666 & ~umask());
core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/CacheWarmer/CacheWarmer.php:            @chmod($file, 0666 & ~umask());
core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpCache/Store.php:        @chmod($path, 0666 & ~umask());
core/vendor/twig/twig/CHANGELOG: * fixed chmod mode to apply the umask correctly
core/vendor/twig/twig/CHANGELOG: * fixed default cache umask
core/vendor/twig/twig/lib/Twig/Environment.php:                @chmod($file, 0666 & ~umask());
grep: sites/default/files/php/service_container/service_container_prod_.php: Permission denied

The patch from #3 mimics this umask use cases. I'm not sure the patch is the proper solution but I'm quite disturbed drush is not operating as expected on D8 anymore :(

clemens.tolboom’s picture

effulgentsia’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Component/PhpStorage/FileStorage.php
@@ -109,7 +109,7 @@ class FileStorage implements PhpStorageInterface {
-      chmod($path, 0700);
+      chmod($path, 0777 & ~umask());

I think adding the & ~umask() everywhere is possibly a good idea, but changing 700 to 777 might be risky. Would be good for chx to comment here. An alternative idea is if we change getFullPath() to include the current system user id in the hash? So that command line scripts and the web server use different compiled PHP files, each one with the correct ownership.

effulgentsia’s picture

Status: Needs work » Needs review

Didn't intend to change status.

clemens.tolboom’s picture

@effulgentsia I discussed the umask() with a SysOp and he said its better to set it to 0777 instead of 077x as you will always end up on discussing what the current SysOp thinks how his system should be configured.

Adding the current user-id does not work without loosening up the permissions as directories are also created by the current user.

And yes we need someone like @chx to review this patch :)

I learned this week my 'dev user' is member of the apache group so I don't care for the x in 077x ;-)

I'm not sure what http://httpd.apache.org/docs/2.2/mod/mod_userdir.html needs to operate properly in this context :(

Bottom line is we need a secure Drupal AND a great Drupal 8 command-line experience.

(As a side note:
I did a wrongfully thought https://github.com/fabpot/Twig/pull/991 and re-reading http://php.net/manual/en/function.mkdir.php discovered

mode is ignored on Windows.

Is umask ignored on windows at all? I'm not sure what that means to our current implementation.
)

sun’s picture

Two issues here:

1) The 0700 is supposed to explicitly disallow group + world access, and only grant owner access. The files written by PhpStorage are not public files.

2) For directories and files otherwise created by Drupal, we have two chmod variables in settings.php already.

3) Yes, Windows does not care for the group and world bits, and the owner bit is automatically copied into group and world bits; i.e., 0600 automatically turns into 0666.

clemens.tolboom’s picture

#1)

The files written by PhpStorage are not public files.

I understand these are not public but we need Drush to operate properly and probably other scripts from core/scripts like drupal.sh

To not disable the savvy command line people we need some options to make this work.

I hope for some alternatives to umask() 0777 | 0444 | 0111 pattern but I myself have non yet :(

#2)

Which settings regarding chmod are available?

I haven't found these option. Doing a $ git log sites/default/default.settings.php these come close #1843034: Make Twig settings configurable and #1833516: Add a new top-level global for settings.php - move things out of $conf

Can we add some options through settings.php?

#3)

This means drush works on windows Drupal 8 I guess. (I did not know the bits 0y00 becomes 0yyy)

franskuipers’s picture

I can imagine more Twig users have problems with compiled file permissions.
I found these items:

  • a private solution: http://aknosis.com/2012/10/02/twig-cache-file-permissions/
  • discussion only https://github.com/fabpot/Twig/issues/94
  • chx’s picture

    Status: Needs review » Closed (works as designed)

    If you are in a secure environment use the FileStorage. If you are not then use MTimeProtectedFileStorage.

    chx’s picture

    Title: CM permission settings by MTimeProtectedFastFileStorage and FileStorage make commandline tools like drush unusable. » CM permission settings by FileStorage make commandline tools like drush unusable.
    Status: Closed (works as designed) » Needs work

    Oh. Right, this one is correct. Sorry for premature closing. It shouldn't be 700. Let's discuss what it should be.

    clemens.tolboom’s picture

    Doing grep -r chmod\( core we get a plethora usages like

    core/includes/file.inc:    chmod($uri, 0700);
    core/includes/file.inc:  drupal_chmod($destination);
    core/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/File/UploadedFile.php:                @chmod($target, 0666 & ~umask());
    <code>
    
    Symfony is using <code>& ~umask()

    consistently.

    Symfony + Twig is using the pattern 0xxx while Drupal is using 0x00 instead.

    So my guess is Drupal should use 0xxx and trust umask() settings.

    +++ b/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFastFileStorage.php
    @@ -147,12 +147,12 @@ class MTimeProtectedFastFileStorage extends FileStorage {
    + mkdir($this->directory, 0777 & ~umask(), TRUE);

    This needs to go as mkdir is already respecting umask. See also www.php.net/manual/en/function.mkdir.php

    One side note for the grep done is Drupal now and then uses the error suppressor @chmod which feels weird. Is it?

    chx’s picture

    The error suppressor is necessary because of streams which didn't have chmod until PHP 5.4. Now, I do not know whether & umask() is necessary, file a Symfony PR removing it and if they accept it then we can just use chmod(777) for our dirs. Till then I recommend filing the @chmod(xxx & umask()) patch. I would make the xxx a class constant cos it's different between FileStorage and children.

    Fabianx’s picture

    There are two issues here:

    a) Use FileStorage instead of MTimeProtectedFileStorage via $conf override in settings.php - That will solve the issues with the service container
    b) Even FileStorage (used for config as well) has too tight permissions - this bug is about this.

    I am currently using the following work-around on a DEV machine:

    $conf['php_storage']['default']['class'] = 'Drupal\Component\PhpStorage\FileStorage';
    // @todo: Consider using a directory outside of the webroot to make this secure
    $conf['php_storage']['default']['directory'] = drupal_is_cli()?'sites/default/files/php-drush':'sites/default/files/php';
    

    This is only for a DEV machine, not for production.

    But it works nicely for me and I never had problems with drush since then with the service container :-).

    The bug for FileStorage still needs to be fixed - though.

    Thanks!

    Dave Reid’s picture

    Seems unusual and odd that I'd need specific settings.php modifications in order to have Drupal work on my local install and development. :/ But I guess this is the new Drupal.

    Fabianx’s picture

    Off-topic response to concerns

    Seems unusual and odd that I'd need specific settings.php modifications in order to have Drupal work on my local install and development. :/ But I guess this is the new Drupal.

    I had a discussion with moshe about this on contribute and there is two parts to that:

    1) You may want to tweak settings.php and use FileStorage even on a production system (according to chx) if you ensure that either

    a) directory is outside web root
    b) sys admin takes care to make sure via ACL or such that permissions are enforced securely.

    2) Yes, drush should work out of the box. What might happen is that:

    a) drush is overriding $conf and using its own directory if the storage class is MTimeProtectedFileStorage.
    b) for drush cc all, a flag is set in drupal to delete the files on the next request

    For Twig this would currently already work automatically. (current twig implementation uses a cache item for storing the "last modified time" of the template, such by clearing all caches all templates are also rebuild)

    So for now the only thing missing is clearing the container and for that we might have a flag or something - or maybe even the same solution like with twig.
    This part obviously needs work.

    However the above is off-topic and should be a totally separate issue.

    On-Topic

    The bug at hand here is to fix the permissions of FileStorage.

    clemens.tolboom’s picture

    Status: Needs work » Needs review

    A code review would help moving this forward.

    And maybe some updates to the summary.

    Setting to 'needs review' as it does :p

    greg.1.anderson’s picture

    I made some comments in #1899842: MTimeProtected Directory Grief. My suggested solution (1) is essentially the same as what is proposed in this issue. I propose an alternative solution (2) that is Drush-centric.

    Regarding #17 above, I fear that it is a non-starter to use a different set of files for Drush than are used by Drupal. Perhaps this works now, but what happens in the future when you fix something with a Drush command, but your Drupal instance is still crashing due to a corrupted php cache? If you forget the split environment switch in settings.php, it will cause a lot of angst, and this, I fear, would be just the beginning of the problem. In D6 and D7, Drush goes to a lot of effort to bootstrap Drupal such that code runs in the same environment, regardless of how it is called. We need to maintain this in D8, or it will cause problems further down the road.

    chx’s picture

    Status: Needs review » Closed (works as designed)
    1. Relaxing the 700 in unlink is unnecessary because it is only changed on paths getting deleted. Please reopen the issue if you are aware of a filesystem where you can change the mode of a node without being able to delete it.
    2. We are not changing MTimeProtectedFileStorage.

    The rest can be discussed in the drush queue.

    chx’s picture

    Status: Closed (works as designed) » Needs review
    FileSize
    1 KB

    Eh, again I closed too early. The change that needs to happen is in FileStorage::save. I researched and chmod doesn't support umask so yes the change is 077 & umask.

    There is nothing else to be changed.

    Status: Needs review » Needs work

    The last submitted patch, 1908440_23.patch, failed testing.

    clemens.tolboom’s picture

    chx’s picture

    If you have no interest in finishing the issue you opened please unassign. There is not a lot left, though. (And as for that issue, I will simply assume you wanted to draw my attention to a rather interesting issue and just made a joke on these two being related.)

    clemens.tolboom’s picture

    Assigned: clemens.tolboom » Unassigned

    I'm curled up as the interdiff between #3 and #23 is to big for me to handle :-/

    Fabianx’s picture

    Status: Needs work » Needs review

    #23: 1908440_23.patch queued for re-testing.

    Status: Needs review » Needs work

    The last submitted patch, 1908440_23.patch, failed testing.

    chx’s picture

    Assigned: Unassigned » chx
    Status: Needs work » Needs review
    FileSize
    469 bytes
    1 KB

    There is no interdiff, I wrote it from scratch. And the problem is trivial.

    moshe weitzman’s picture

    Anyone available to review this tiny patch?

    star-szr’s picture

    I can't speak to the technical aspects of the patch, but…

    +++ b/core/lib/Drupal/Component/PhpStorage/FileStorage.phpundefined
    @@ -13,6 +13,19 @@
    +  /**
    +   * ¶
    +
    

    I think these three lines should be removed, unless I'm missing something.

    star-szr’s picture

    FileSize
    464 bytes
    1010 bytes

    And patch.

    chx’s picture

    To review this, you need a setup where the webserver user and the command line user differs. Now set $conf['php_storage']['default']['class'] = 'Drupal\Component\PhpStorage\FileStorage'. Without the patch you won't be able to read the service container. With the patch you should be able to.

    chx’s picture

    Issue summary: View changes

    Added XREFs + minor text changes

    chx’s picture

    Issue summary: View changes

    Updated issue summary.

    chx’s picture

    Issue summary: View changes

    Updated the issue summary

    effulgentsia’s picture

    #33 is a good step, but I have three concerns:

    1) I'm concerned about using the system umask() for PHP files. PHP files require more security care than regular files, so a system umask() that's appropriate for regular files might not be desirable for PHP files. Can we instead use a $configuration['umask'] that could be tweaked independently of the system umask()?

    2) I think it would be ideal if $configuration['umask'] could also be made to work for MTimeProtectedFileStorage. MTimeProtectedFileStorage provides some great security protections independent of whether group permissions are allowed on the file.

    3) While we need Drush to be able to delete PHP files (i.e., execute a complete cache clear), do we want to prevent it from being able to create new ones? Otherwise, we could end up with some PHP files owned by the web server, and others owned by various Drush users, and that seems not ideal. Maybe this is something that needs more discussion in the Drush queue, but I point it out here in case we need to add something to these classes to assist with that.

    This patch implements 1, but not 2 or 3. Looking for feedback on the ideas first. Adding the -do-not-test flag in case this idea is rejected and we want to leave #33 as the latest green patch.

    greg.1.anderson’s picture

    Link to the corresponding Drush issue is up in #21. (Ref: #1899842: MTimeProtected Directory Grief).

    Regarding 3), above, In Linux, a user with w permissions on a directory can add or remove files there, so there's no good way to allow Drush to delete but not add files (save by convention). I discuss the implications of Drush-created files in Drupal's file space in #1899842-12: MTimeProtected Directory Grief.

    chx’s picture

    So I discussed #2 with effulgentsia and the current thought is that we would keep the directory permissions 0100 by default for unknown host environments (practically, shared hosts) but we would allow a setting to override it to 0111. I think we would do an & 0111 to whatever you set so that it's not possible to set a value that compromises the protection in MTimeProtectedFileStorage against move_uploaded_file.

    moshe weitzman’s picture

    #37 sounds good. Further, I think this setting needs to be in the UI and the submit handler has to take care of resetting directory perms when it changes. I say that because the drush or cli user has no perms to make changes to MTime dirs once they are created. Only the webserver can do this ... Deploying devel module just for this change is too convoluted and burdensome on drush users, IMO.

    effulgentsia’s picture

    I was envisioning it as being the same $conf['php_storage']['default']['umask'] = ...; setting that's in the settings.php of #35. Not in the UI. So where the MTimeProtected class currently does chmod 0100, we would instead do chmod 0111 & ~$this->umask. If the umask is the default of 0077, that would result in 0100 as currently in HEAD. But if someone edits their settings.php to set the umask to 0007, that would result in 0110.

    I think this setting needs to be in the UI and the submit handler has to take care of resetting directory perms when it changes

    My initial thought was that this shouldn't be in the UI. Instead, you change it in settings.php, and then in the UI, all you do is clear cache, which should have the effect of deleting everything in PhpStorage, and setting permission correctly on the root PhpStorage folders. However, this thinking was based on everything in PhpStorage being effectively just caches (the DIC and twig files). It doesn't handle the situation of non-rebuildable code in PhpStorage, which we don't have any of in core, but maybe want to consider as a possibility in contrib?

    While we need Drush to be able to delete PHP files (i.e., execute a complete cache clear), do we want to prevent it from being able to create new ones?

    The issue I was thinking through when suggesting this was that the MTimeProtected class creates a wrapper directory for each file, and needs to leave that wrapper directory as no more permissive than 0111, which means only the owner of that directory can subsequently delete it (because the delete operation requires first adding write access, and you can only chmod files you own). However, what might work (though I haven't tried it out yet) is having a "purge" directory (could be /tmp or somewhere else) that we move the directories we want deleted to (since I think moving only requires write permission on the parent directory, not on itself, because you can move directories without emptying them first), and then allowing this "purge" directory to get actually purged by the owner. That step would be made easier by having the owner always be the web server, which would require Drush to not create new files.

    Sorry if any of this isn't clear. Just getting some in-progress thoughts into here. Feedback welcome.

    moshe weitzman’s picture

    That sounds fine to rely on 'clear all caches' to fix up the perms for us.

    I guess core could only clear the service container and twig bins and let other users of phpstorage implement same clearing as needed.

    greg.1.anderson’s picture

    The idea of the "purge" directory will not work; you cannot mv a wrapper directory if its perms are 0111. You would have to first chmod +w it, and then you could move it, so again, only the owner could do this. Strangely enough, though, you can rename a directory if its perms are 0111, so you could conceivably replace the "purge" directory idea with a "purge" filename pattern instead. This seems a bit of an odd solution, but it would work. (Note to perm testers: Linux will allow you to delete directories with 0111 perms if they are completely empty, so create a placeholder file in the directory before chmod'ing it.)

    I am going to make the suggestion that we use 0171 perms for these files. (I am only going to talk about directory permissions; imagine that the files inside have appropriate permissions as well, per the protection level desired for that file). In this scenario, we are assuming that the web server creates the directory, and sets the group to something that the cli user is a member of. With this mode setting, the file owner (the web server) cannot read a directory listing or create new files in the protected directory. Even though the web sever is a member of the directories' group, the "owner" permissions take precedence, and access to the directory is denied. The owner can still change the permissions of the directory, and then modify the file, but this was always true; what we are trying to protect against here is accidental modification (or accidental disclosure, e.g. when someone has found an exploit to read any file, but cannot execute code).

    One downside to this suggestion is that these directories are not protected from accidental modification or deletion from the cli user. However, I think this is not as important, whereas it is very helpful to allow the cli user to delete and modify files here. We still have not solved the problem that it is inconvenient if the cli user creates these files. In the case of drush site-install, we could fix up the permissions via #990812: Add a "permissions" subcommand to fix/set all file permissions afterwards, which is already designed to work with 'sudo' in a couple of different ways. In the case of drush cc all, perhaps we only need to remove files (if we use 0171 permissions) when clearing the cache. If we modify the cache clear operation to set up directories and permissions in advance, though, then we run the risk of making drush cc all dangerous to the web user.

    Another downside to this suggestion is that if the group used on 0171-perm directories has many members, then the files inside might be exposed to other processes running on the system. It would be important to insure that the web files' group was reserved for use by only appropriate users (e.g. the web user and the cli user).

    Despite these limitations, I still think that 0171 (for protected directories) is our best option to allow Drush to continue to function without relying heavily on the use of 'sudo -u webuser' when invoking it.

    effulgentsia’s picture

    Strangely enough, though, you can rename a directory if its perms are 0111, so you could conceivably replace the "purge" directory idea with a "purge" filename pattern instead.

    Can you? I'm not able to do so on a Mac either using the mv command or the rename PHP function. I actually didn't think mv/rename required different permissions for renaming within a parent directory vs. moving to a different parent directory. And seems like both require write permission on both the parent directory and the directory itself. Let me know if you find otherwise.

    Even though the web sever is a member of the directories' group, the "owner" permissions take precedence, and access to the directory is denied.

    Cool! That's awesome, because the primary attack that MTimeProtectedFileStorage was designed to impede is a poorly secured file upload script. So this means relaxing group permissions doesn't weaken us against this attack vector.

    One downside to this suggestion is that these directories are not protected from accidental modification or deletion from the cli user.

    At least for all the core PHP storage (DIC and twig), deletion isn't harmful. See below about accidental or malicious modification.

    Another downside to this suggestion is that if the group used on 0171-perm directories has many members, then the files inside might be exposed to other processes running on the system. It would be important to insure that the web files' group was reserved for use by only appropriate users (e.g. the web user and the cli user).

    So one possibility is we assume that the group can be fully trusted. Another possibility is we enhance MTimeProtectedFileStorage::checkFile() to also require that the file is owned by the currently running process. Because the file won't be writable, so the only way for a rogue group process to attack it is to delete and replace, which makes it the new owner. So this way, even if such an attack occurs, the rogue group process still can't get that PHP code executed as the web server user.

    I still think that 0171 (for protected directories) is our best option

    Especially if we add the extra owner check to MTimeProtectedFileStorage::checkFile(), I'd be comfortable with setting the directory permission to 0177 & ~$this->umask.

    @chx, @greg.1.anderson: what do you think?

    chx’s picture

    So what we have is:

    1. sites/default/php . Currently: 700.
    2. sites/default/php/service_container . Currently: 700.
    3. sites/default/php/service_container/service_container_prod_.php . Currently: 100.
    4. sites/default/php/service_container/service_container_prod_.php/$hash: This file is currently 400.

    What protections do we have?

    1. $hash is secret.
    2. service_container_prod_.php mtime is baked into $hash. The service_container_prod_.php inode is changing when a file is deleted or renamed.
    3. the mtime of the $hash file is smaller than the directory mtime (protection against overwrite in place).

    What we want to allow for? Deletion of $hash. What's necessary for that? The opening of the dir (adding 1 bits to everywhere, ie 711, 111) and the writeability of service_container_prod_ directory. Let's presume the attacker has gained access to the secret key (that's quite bad already) and the server contains a rogue upload script. By the latter we mean a simple script that takes the filename from the request, not arbitrary execution. Olympus still won't fall because though you can bust protection 1. and 2. by writing a file inside the directory (this is now possible) controlling mtime and calculating $hash. However, 3. will still hold.

    What about MTimeProtectedFastFileStorage? It is still a viable protection -- if the secret is not know but one $hash gets revealed (possible through bad error reporting and a bad php file) it's still not possible to create the next $hash even if the directory mtime is controllable.

    To sum up:

    1. sites/default/files/php needs to be 0711.
    2. sites/default/files/php/service_container needs to be 0711.
    3. sites/default/files/php/service_container/service_container_prod_.php . Needs to be 0333.
    4. sites/default/files/php/service_container/service_container_prod_.php/$hash: Who cares when the directory is writeable and the file is chmoddable?
    greg.1.anderson’s picture

    I'll take a stab at addressing this. I agree with #43 insofar as:

    1. the web server can create protected files, and they are secure
    2. drush can delete $hash without opening a security hole

    However, I think we can still run into problems if Drush is allowed to create protected files. In that case, containers which are protected with 0711 will be owned by the Drush user, and therefore no longer writable or chmod-able by the web user.

    Key question: Are we going to allow Drush, running as a user other than the web user, to create files in the Drupal instance?

    If the answer to this question is "no", then I believe that implies 1899842-#12 (option "2") is necessary. We can proceed here as suggested.

    If we are going to continue to allow Drush to run as a separate user, and still create files inside of Drupal's space, including the files mentioned in #43, that complicates the situation slightly. I think that, if we adjust the advice in #43 to use 0771 in place of 0711, and if we make sure that Drupal always chmod's via a wrapper function that will be resilient in situations where (a) someone else owns the file, and (b) the permissions are already correct, then things might work out. One potential wrinkle with this option is that external libraries that include calls to chmod might not (probably would not) follow these same conventions. However, if we are only talking about the files mentioned in #43 at the moment, then perhaps that is a separate issue.

    chx’s picture

    FileSize
    3.93 KB
    chx’s picture

    Did everyone lost interest?

    greg.1.anderson’s picture

    Perhaps folks are waiting for me to follow up on #44. However, since #45 is the same as #43, except in code form, I don't have any further comments. #45 seems to improve the situation well enough to solve some problems that some folks are having. Perhaps that is good enough for now, and we can revisit the issues in #44 at such a time as effects such as these are actually observed and found to be a problem.

    chx’s picture

    #45 is #43 in code form but look at the patch name, however weird it might sound given the time frame, it's a crosspost with #44. I have missed your post.

    That said, I am uncertain what #44 wants actually.

    greg.1.anderson’s picture

    Two things.

    a) Use 0771 everywhere that #45 uses 0711, and let folks use umask to get rid of the group perms if they don't want them

    b) Use @chmod wherever chmod is currently used. Optionally, you could warn when chmod failed iff the file is also chmod'ed incorrectly, but you should never fail when chmod fails, and should not warn if the chmod bits are already correct.

    This allows for the situation where a user runs a Drush command that creates a file (now owned by someone other than the web user), and Drupal later comes by and decides to chmod it again. As long as we have a) and b), then Drupal should be able to continue to use (write to) the file without interference.

    I realize that doing b) is a little tedious, and may not be possible (or at least not quick) when the chmod is in an upstream library. However, without a) and b), using Drush with Drupal will be much less convenient.

    In an ideal world, I'd submit a patch for this; however, I am currently late on two milestones, and do not have time to make a D8 detour right now. If no one wants to take this on, we could go with #45, and continue with the chmod protection when lack of same bothers someone.

    chx’s picture

    So we got here. My take is a "no" on 77x anywhere. We were discussing, instead, a "container invalid" flag presumedly in config. What about that instead? 711 lets you read, the flag would let you instruct Drupal to rebuild. We could tuck it in the modules enabled config object to avoid another performance penalty.

    greg.1.anderson’s picture

    If the file is 711 and owned by the Drush user, then Drupal won't be able to rebuild it, because it won't be able to write it or chmod it.

    We can proceed here as suggested, but it implies 1899842-#12 (option "2") - namely, Drush will need to insure that it runs as the web user whenever it needs to write or create files. That is much less usable than what we have now, but if it is necessary, then so be it.

    greg.1.anderson’s picture

    Oh, I see, by rebuild you mean for cache files only - they can be deleted if Drupal can write to the parent, right? I didn't test. My concern is a little broader, but since this issue is only about cache files, I think your analysis is correct.

    Okay, I think that's fine.

    greg.1.anderson’s picture

    The thing I don't like about this, though, is that if you signal a rebuild via Drush, then you cannot run subsequent Drush commands that depend on the rebuild having taken place until after you hit the site with the web server. It's going to be a while before I will be able to test things enough to satisfy myself one way or the other, so again, I suggest we proceed here to others satisfaction, and I'll open a new issue later if I have a better suggestion.

    clemens.tolboom’s picture

    According to the current issue summary

    Drush will always use the FileStorage.

    contradicts with

    Remaining tasks

    Once this is in we need to spread the word far and wide that you want to use FileStorage during development.

    I'm confused about this.

    Furthermore if @greg.1.anderson is right in "we need to hit the webserver first" my guess is we cannot do configuration management commands like staging a view from dev to tst from the command-line right?

    What is wrong with using 771 in combination with $ chmod +t sites/default/files/php?

    chx’s picture

    Sorry I got confused, I think. Forget everything I said and let's get back to:

    1. sites/default/files/php/service_container/service_container_prod_.php . Needs to be 0333.
    2. sites/default/files/php/service_container/service_container_prod_.php/$hash: Who cares when the directory is writeable and the file is chmoddable?

    You can write a new file inside service_container_prod_.php because the directory has write bits for everyone. You can delete files for the same reason. You can not, however, list it. If drush writes a new container and Drupal needs to wite another one, no problems?

    greg.1.anderson’s picture

    I am happy with any proposed solution where the group perms are the same as the owner perms, and chmod happens only at file creation time. So, if $hash in #55 is 0660, then I think it is good. If you never ever need to rewrite $hash (except by deleting and recreating it), then perhaps it would be even better / clearer to make it 0440.

    For files that need to be written without being deleted and recreated, then the discussion gets more complicated per above comments.

    chx’s picture

    The $hash file permissions are irrelevant as the holding directory is 333 -- chmod at your heart's content. I think we will keep it locked down for rogue upload scripts as a last resort but -- as just said, that's irrelevant because clueful scripts can chmod.

    greg.1.anderson’s picture

    #57 is true from a security standpoint; however, we must also consider usability. If we use 0440 (or 0444 w/ umask) and voluntarily omit the chmod, then Drupal must delete and recreate the $hash file any time it changes, regardless of whether the file was created by Drush or Drupal. This will increase the usability of Drush. We want to strive to maintain an environment where we can run the same Drupal code from Drush as we run from the web server. I think that #55 can maintain that for this particular use case.

    chx’s picture

    I have no idea what you are talking about re deletes. You can just chmod to your heart's content if the holder directory allows for that.

    greg.1.anderson’s picture

    From http://en.wikipedia.org/wiki/File_system_permissions:

    • The read permission grants the ability to read a file. When set for a directory, this permission grants the ability to read the names of files in the directory (but not to find out any further information about them such as contents, file type, size, ownership, permissions, etc.)
    • The write permission grants the ability to modify a file. When set for a directory, this permission grants the ability to modify entries in the directory. This includes creating files, deleting files, and renaming files.
    • The execute permission grants the ability to execute a file. This permission must be set for executable binaries (for example, a compiled C++ program) or shell scripts (for example, a Perl program) in order to allow the operating system to run them. When set for a directory, this permission grants the ability to access file contents and metainfo if its name is known, but not list files inside the directory (unless read is set).

    That's not the best quote we could hope for, as it does not specifically call out what the restrictions on file permissions changes are when a directory has write + execute permissions. We can demonstrate with a brief test, though:

    $ mkdir chmodtest
    $ chmod 777 chmodtest
    $ cd chmodtest
    $ touch filetest
    $ chmod 444 filetest
    $ sudo chown root:root filetest
    $ chmod 777 filetest
    chmod: changing permissions of `filetest': Operation not permitted
    $ sudo chgrp ga filetest
    $ chmod 777 filetest
    chmod: changing permissions of `filetest': Operation not permitted
    $ sudo chown ga filetest
    $ chmod 777 filetest
    

    By changing the permissions of a directory, you can make it possible for a given uid to delete and then recreate a file in that directory; however, you cannot make it possible for a given uid to chmod a file simply by setting the permissions of the directory that it is in. Only the owner of a file (and root) can chmod it.

    If we want to allow the web user and the cli user to run under different uids, and if we want to be able to run the same code either ad the web user or as the cli user, and in either order (sometimes cli first, then web user, sometimes web user first, then cli user), then we need to be very careful about how we create files, modify files, and chmod files. You have to be prepared for the chmod to fail; therefore, if you create the file read-only, you should delete and recreate it if it needs to be modified. What I would recommend is:

    a) only chmod at file-create time (you just created the file, so you know you are the owner and can chmod it)
    b) code that might run at file-create time or file-modify time that chmod's should silently ignore errors from chmod
    c) always insure that owner perms == group perms for files and dirs

    If we do not care about running the web user and the cli user under different uids (that is, cli tools must sudo to the web user before running any Drupal code that might modify a file), then it is okay to create files and directory where the owner permissions are not the same, or to require a chmod prior to a modify or delete.

    chx’s picture

    eh.

    ➜  /tmp  ✗ mkdir chmodtest
    ➜  /tmp  ✗ cd chmodtest
    ➜  chmodtest  ✗ touch filetest
    ➜  chmodtest  ✗ sudo chown root:root filetest
    ➜  chmodtest  ✗ chmod 777 filetest
    chmod: changing permissions of ‘filetest’: Operation not permitted
    ➜  chmodtest  ✗ cp filetest filetest1
    ➜  chmodtest  ✗ chmod 777 filetest1
    ➜  chmodtest  ✗ rm filetest
    rm: remove write-protected regular empty file ‘filetest’? y
    ➜  chmodtest  ✗ mv filetest1 filetest
    
    greg.1.anderson’s picture

    Hm, I tried to reply to #61 on 18 May, but it appears that my comment was not saved. :?

    Anyway, #61 is exactly the point I was trying to make in #58. If the code is written to delete and rewrite, it will work; if it tries to chmod, and the chmod fails (because the file was created by another user), that would be a problem if the code bails on chmod failures. This is what motivated the "rules" (a, b and c) in #60.

    xjm’s picture

    Priority: Normal » Major
    xjm’s picture

    Issue summary: View changes

    updated again

    chx’s picture

    Issue summary: View changes

    rewrote the summary

    chx’s picture

    Issue summary: View changes

    Updated issue summary.

    chx’s picture

    Issue summary: View changes

    Updated issue summary.

    chx’s picture

    Assigned: chx » Unassigned

    I let someone else code this :)

    YesCT’s picture

    Assigned: Unassigned » chx

    the issue summary was updated. (thanks @chx)
    and a proposed resolution is explained.
    per the issue summary, next step is a new patch to implement the proposed resolution.

    YesCT’s picture

    Assigned: chx » Unassigned
    YesCT’s picture

    Issue summary: View changes

    Updated issue summary.

    chx’s picture

    Status: Needs review » Active

    We can treat that patch as if it wouldn't exist.

    greg.1.anderson’s picture

    I think #45 is helpful; there are potentially a number of additional places in Drupal core that would need to change to conform to the suggested file handling proposal (delete and then rewrite), though.

    Juampy suggested an alternative solution, to fix this in Drush (or more specifically, to fix in Drush documentation) that might work out. See #1899842: MTimeProtected Directory Grief, starting with #16. We will post results here if any conclusions are reached. My initial impression is that it is still better to fix this in core as suggested in the issue summary, for files that need to be chmod'ed to a non-writable state.

    pwolanin’s picture

    Here's an alternative thought in line with the initial recovery.php patch from chx. The work-around is to create a web request using a security created from the command line.

    Drupal 8 is supposed to be a better web service endpoint, so perhaps we could/should build in a single-use-token-protected endpoint for running these sorts of commands from drush?

    moshe weitzman’s picture

    Its a novel idea, but I don't want drush to rely on a web server at all. Thats a huge regression IMO.

    Owen Barton’s picture

    I need to read this in more detail, but one thing to note is that Drupal does have 2 existing variables that chmod files/directories created by php intended to be readable by the webserver. Using a configurable umask (rather than chmod value) here is perhaps inconsistent with that - I think chmod values are slightly more intuitive to most users than umasks also. I guess the challenge is that we need several values here?

    clemens.tolboom’s picture

    FWIW another 'use case' is Drush cannot write config files (while working on #2012586: config-import is broken due to removal of config_sync_get_changes)

    WD config_import: Drupal\Core\Config\StorageException: Failed to write configuration file: sites/site-x/files/config_hash/active/image.style.medium.yml in Drupal\Core\Config\FileStorage->write()
    

    In #1899842-16: MTimeProtected Directory Grief @juampy added a link to http://symfony.com/doc/master/book/installation.html (sectionSetting up Permissions) which helped me a little.

    I currently have an environment where apache is running _as_ the current user which makes me less 'facing palms' regarding the current situation.

    I have no clue how this should be solved properly due to lack of sysops skills and the pletoria of different system layouts but hope this issue get's solved the sooner the better :)
    (my 2cnts)

    pwolanin’s picture

    @moshe - does it really make sense that drush will be functional if you don't have a webserver? Also, I think there could be some very interesting applications for drush commands being available via a RPC framework?

    moshe weitzman’s picture

    Yes, it really does make sense ... Could you elaborate on your RPC idea. I don't know what you mean there (beyond our current SSH API), and how it relates to this issue.

    I encourage someone to implement whats been written in the (recently updated) Summary. It is a relatively simple patch.

    heddn’s picture

    Status: Active » Needs review
    FileSize
    4.74 KB

    If I understand the issue summary correctly, this patch incorporates what it documented.

    chx’s picture

    Thanks for the work!

    > Code this: change the permissions as listed and use delete-write instead of trying to chmod existing things.

    The permissions have changed but apparently the delete-write thing didn't happen. I do not think this is a problem for MTimeProtectedFastFileStorage but it is for FileStorage: in save() please delete the file before writing it out.

    mheinke’s picture

    Status: Needs review » Reviewed & tested by the community

    patch applied and ran cleanly,
    my permissions adjusted the way they should according to the patch.

    im still a little wary with some of the "looseness" permission changes, but that might just be because i wear a tinfoil hat..

    Owen Barton’s picture

    I didn't see any responses to my suggestion in #71 - it seems that if the web PHP user and CLI user share a group, the host or site owner could set the permissions to 0710 and 0330 and everything would still work. It seems like this would be a useful hardening, particularly in a shared hosting environment where other vhosts are untrusted (even though the other protections apply).

    chx’s picture

    Did anyone actually test this in a usual user-http setup (say Ubuntu) with drush whether things (like cc) work as they should...?

    mheinke’s picture

    i threw the patch on my development webserver (redhat enterprise linux release 6.4)
    everything seems to be working as normal.

    alexpott’s picture

    Status: Reviewed & tested by the community » Needs review

    in #76 @chx says...

    The permissions have changed but apparently the delete-write thing didn't happen. I do not think this is a problem for MTimeProtectedFastFileStorage but it is for FileStorage: in save() please delete the file before writing it out.

    That sounds like there is still work to do...

    chx’s picture

    It also is not clear whether anyone tried to get drush to rebuild a container and then run with it Drupal under the typical user-different different http user.

    steinmb’s picture

    Status: Needs review » Needs work

    D8 HEAD
    Patch: #75
    OS X 10.8.x
    Server: Apache 2.2.22
    PHP 5.4.16

    Applied patch in #75 and did a clean install of Drupal through the UI. Still having problems

    drush -v cc all
    Initialized Drupal 8.0-dev root directory at /Users/steinmb/apache/htdocs/d8/drupal
    Initialized Drupal site default at sites/default
    WD DrupalKernel: Container cannot be written to disk
    Object of class Drupal\Core\Entity\Field\Field could not be converted to string drupal.inc:158
    Object of class Drupal\Core\Entity\Field\Field could not be converted to string drupal.inc:158
    WD DrupalKernel: Container cannot be written to disk
    WD php: Drupal\Core\Config\StorageException: Failed to write configuration file:
    sites/default/files/config_-ZitrBXnj9vm7IjYJ9nkyvo0Rn_luLYE5t4tsPOiCpk/active/block.block.bartik.breadcrumbs.yml in Drupal\Core\Config\FileStorage->write()
    (line 115 of /Users/steinmb/apache/htdocs/d8/drupal/core/lib/Drupal/Core/Config/FileStorage.php).
    Drupal\Core\Config\StorageException: Failed to write configuration file: sites/default/files/config_-ZitrBXnj9vm7IjYJ9nkyvo0Rn_luLYE5t4tsPOiCpk/active/block.block.bartik.breadcrumbs.yml in Drupal\Core\Config\FileStorage->write() (line 115 of /Users/steinmb/apache/htdocs/d8/drupal/core/lib/Drupal/Core/Config/FileStorage.php).
    Drush command terminated abnormally due to an unrecoverable error.
    

    Running as the apache users remove the exception but still throw 'DrupalKernel: Container cannot be written to disk'.

    sudo -u _www drush -v cc all
    Initialized Drupal 8.0-dev root directory at /Users/steinmb/apache/htdocs/d8/drupal
    Initialized Drupal site default at sites/default
    WD DrupalKernel: Container cannot be written to disk
    Object of class Drupal\Core\Entity\Field\Field could not be converted to string drupal.inc:158
    Object of class Drupal\Core\Entity\Field\Field could not be converted to string drupal.inc:158
    WD DrupalKernel: Container cannot be written to disk
    'all' cache was cleared in self
    Command dispatch complete
    
    chx’s picture

    That might be a drush issue of not deleting first but trying to write? I do not know.

    greg.1.anderson’s picture

    Drush is just calling drupal_flush_all_caches() to clear the cache. Haven't had time to investigate this issue, though.

    chx’s picture

    Status: Needs work » Needs review
    FileSize
    7.14 KB
    902 bytes

    Testing this requires nuking the override in drush; patch attached. Reminder: we do not want a rogue upload script to be able to write a valid file. It still can't.

    Status: Needs review » Needs work

    The last submitted patch, 1908440_86.patch, failed testing.

    chx’s picture

    Status: Needs work » Needs review
    FileSize
    7.28 KB
    816 bytes
    chx’s picture

    FileSize
    3.51 KB
    7.37 KB

    Here's some simplification.

    damiankloip’s picture

    All I will say is DREDITOR!!! It totally screwed up twice in a row trying to review this. Looks much better from our IRC discussion for starters..

    This snippet ((fileperms($path) & 0777) == 0333) seems used enough to just be moved into it's own method? I think it would make for easier code reading too.

    Overridden unlink method:
    - can use {@inheritdoc}
    - It would have been nice to also pass the FileInfo object to here aswell, so we could to $file->getPerms() & 777 ... and $file->isWritable() instead. but I guess we can't really as it's overridding?

    chx’s picture

    FileSize
    8.22 KB
    2.86 KB

    Added {@inheritdoc}, helper, constant for 0333.

    chx’s picture

    Assigned: Unassigned » effulgentsia

    Assigning to Alex for a final review; this should be ready if he is OK with the security implications. Note that the recursive mkdir 0777 will obey umask therefore usually 0755 will be created and also it seems that you can touch() a directory even if you can't write the parent directory; so 0755 is enough. 0777 allows for actual file removals, 0755 allows for invalidating the current container and the next time the owner of the directory comes can dump a new container useable for everyone.

    chx’s picture

    Issue summary: View changes

    lets save people the heart ache of reading all the comments. and be clear about the updated summary. -c

    Status: Needs review » Needs work

    The last submitted patch, 1908440_91.patch, failed testing.

    chx’s picture

    Status: Needs work » Needs review

    Bot fluke.

    chx’s picture

    Issue summary: View changes

    Updated issue summary.

    chx’s picture

    Issue summary: View changes

    Updated issue summary.

    YesCT’s picture

    Issue summary: View changes

    attempt to clarify steps to test.

    YesCT’s picture

    Issue summary: View changes

    more detail (might not help, as it is very specific)

    YesCT’s picture

    Issue summary: View changes

    more detail.

    webchick’s picture

    Priority: Major » Critical

    I'm tentatively bumping this to critical. It would be a major regression not to let people switch between CLI tools and UI tools interchangeably.

    Thank you SO much for working on this, chx!!!

    YesCT’s picture

    I tried the updated instructions. after no drupal patch, installing in the ui, and then trying to use drush to clear caches, it does not work.
    and installing in the ui results in a php directory without permissions for my command line user to write to it.

    $ ls -l sites/default/files
    total 8
    drwxrwxr-x  6 _www    staff  204 Sep 18 19:57:02 2013 ./
    drwxrwxrwx  5 ctheys  staff  170 Sep 18 19:54:33 2013 ../
    -r--r--r--  1 _www    staff   93 Sep 18 19:54:57 2013 .htaccess
    drwxrwxr-x  4 _www    staff  136 Sep 18 19:54:56 2013 config_eHt8jDNZe4tXfGz7CDB-lBUTtFZGGkij_2NGJdAgWJ4/
    drwx------  3 _www    staff  102 Sep 18 19:57:02 2013 php/
    drwxrwxr-x  2 _www    staff   68 Sep 18 19:55:52 2013 styles/
    

    Not sure why. chx has a different result (and a different system) I have mac, with MAMP.

    joelpittet’s picture

    Not sure if this helps but this is a before and after shot of the same directory @YesCT did.
    Yet both installs were done with drush si

    -drwxrwxrwx  6 root    staff  204 Sep 18 09:31 .
    -dr-xr-xr-x  7 joel    staff  238 Sep 18 09:22 ..
    --r--r--r--  1 joel    staff   93 Sep 18 09:29 .htaccess
    -drwxrwxr-x  4 joel    staff  136 Sep 18 09:29 config_cQV3AYpOoNgEkwJvyXKgF7SWNepjgAC33wksi3RcCI0
    -drwx------  4 daemon  staff  136 Sep 18 09:31 php
    -drwxrwxr-x  2 joel    staff   68 Sep 18 09:29 styles
    
    +drwxrwxrwx  5 root  staff  170 Sep 18 16:57 .
    +dr-xr-xr-x  7 joel  staff  238 Sep 18 16:56 ..
    +-r--r--r--  1 joel  staff   93 Sep 18 16:56 .htaccess
    +drwxrwxr-x  4 joel  staff  136 Sep 18 16:56 config_IAAAUISVM5_kC3NsWxhZ_u6C6U1lAWJeABXgVBarPnM
    +drwxr-xr-x  4 daemon  staff  136 Sep 18 16:56 php
    +drwxrwxr-x  2 joel  staff   68 Sep 18 16:57 styles
    

    Mac + Homebrew'd apache/php/mysql

    chx’s picture

    FileSize
    4.12 KB
    9.88 KB

    Try this.

    joelpittet’s picture

    Looks like 777 on php now. @YesCT want to give it another go?

    drwxrwxrwx  6 root    staff  204 Sep 18 23:27 .
    dr-xr-xr-x  7 joel    staff  238 Sep 18 23:25 ..
    -r--r--r--  1 joel    staff   93 Sep 18 23:25 .htaccess
    drwxrwxr-x  4 joel    staff  136 Sep 18 23:25 config_WD9F3SXeAVuqlbORgXlFWWUChjUbrHih1-dR6aI75FI
    drwxrwxrwx  4 daemon  staff  136 Sep 18 23:28 php
    drwxrwxr-x  2 joel    staff   68 Sep 18 23:25 styles
    YesCT’s picture

    I tried the patch in 91 again. I think I was confused about when I should have seen the error. So. I'm updating the review instructions in the summary.

    Here is what happened,

    without the patch, install in the UI, (permissions on php were only for user: drwx------ 2 _www staff 68 Sep 19 09:25:27 2013 php/) then drush cc.
    got warning (twice):

    mkdir(): Permission denied MTimeProtectedFastFileStorage.php:148                                                                                    [warning]
    chmod(): Permission denied MTimeProtectedFastFileStorage.php:150                                                                                    [warning]
    file_put_contents(/Users/ctheys/foo/drupal/sites/default/files/php/service_container/.htaccess): failed to open stream: Permission denied           [warning]
    MTimeProtectedFastFileStorage.php:152
    

    but also got success:

    'all' cache was cleared in self                                                                                                                     [success]
    

    with the patch, install in the UI, permissions on php: drwxr-xr-x) then drush cc
    got warning (twice):

    mkdir(): Permission denied MTimeProtectedFastFileStorage.php:159                                                                                    [warning]
    file_put_contents(/Users/ctheys/foo/drupal/sites/default/files/php/service_container/.htaccess): failed to open stream: No such file or directory   [warning]
    MTimeProtectedFastFileStorage.php:162

    but also got success:

    'all' cache was cleared in self                                                                                                                     [success]
    

    with the patch in 98
    install in the ui
    drwxrwxrwx 2 _www staff 68 Sep 19 09:30:13 2013 php/

    $ drush -y cc all
    'all' cache was cleared in self                                                                                                                     [success]
    

    and no warnings.

    [note I've only tested install in the ui, then command line clear caches. still needs tested the other way around: drush install and ui clear caches.]

    YesCT’s picture

    Issue summary: View changes

    clarify sudo only for rm.

    YesCT’s picture

    I'm not sure about this part from the summary:

    Right now, the only real good way to test whether cleaning has occurred is to put a debug statement (or a breakpoint) in DrupalKernel line 383 $this->storage()->load($cache_file); to see it does not succeed. Other ways won't work because the directories often won't actually be cleaned just invalidated. Other methods (putting bananas() in the class file for example with a sudo'd editor) won't really indicate anything as it invalidates the file immediately without this patch anyways.

    I didn't do anything to put a debug in. Do reviewers/testers need to do that?

    YesCT’s picture

    FileSize
    9.63 KB
    3.15 KB

    I read each line. Makes sense as I read it, but cannot speak to security or the technical approach.

    Found a few things with docs.

    1. +++ b/core/lib/Drupal/Component/PhpStorage/FileStorage.php
      @@ -53,14 +53,36 @@ public function load($name) {
      +   * @param string $directory
      +   *   The directory path.
      +   */
      +  protected function ensureDirectory($directory = NULL, $permission = 0777) {
      

      added the doc for the permission param. also, chx in irc recommended renaming to mode, since that is what it is, and what chmod calls it. http://cz2.php.net/chmod

    2. +++ b/core/lib/Drupal/Component/PhpStorage/FileStorage.php
      @@ -53,14 +53,36 @@ public function load($name) {
      +      // mkdir() obeys umask() so we need to mkdir() and chhmod() manually.
      

      fixed chmod typo.

    3. +++ b/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFastFileStorage.php
      @@ -216,4 +222,54 @@ protected function getUncachedMTime($directory) {
      +   * @param $path
      +   *   The path to the holder directory.
      ...
      +   * @param $directory
      +   *   A directory path.
      

      adding type here: string

    4. +++ b/core/tests/Drupal/Tests/Component/PhpStorage/MTimeProtectedFileStorageTest.php
      @@ -84,7 +84,6 @@ function testSecurity() {
      -      $storageFactory = new PhpStorageFactory();
      

      I was wondering why taking out the new, but then I saw it was just an unused variable. ok.

      I'm making some other docs changes.. so taking this out. That way we dont have to reroll this if #2080367: Remove Unused local variable $storageFactory from core/tests/Drupal/Tests/Component/PhpStorage/MTimeProtectedFileStorageTest.php gets in.

    YesCT’s picture

    Issue summary: View changes

    updated location reviewers should see errors.

    chx’s picture

    Issue summary: View changes

    Updated issue summary.

    chx’s picture

    Issue summary: View changes

    Updated issue summary.

    chx’s picture

    Issue summary: View changes

    Updated issue summary.

    mradcliffe’s picture

    I hope nobody minded, but I found it hard to read the review section. I formatted as numbered list, which meant changing from 4a to 4.1 in the instructions. Also wrapped commands in code blocks. Revert, if necessary.

    mradcliffe’s picture

    Issue summary: View changes

    Trying to improve readability of numbered list. Can't use li type attribute because we're XHTML 4, not HTML 5 on d.o so 4)a becomes 4.1.

    Added code blocks around commands for readability.

    pwolanin’s picture

    This code comment seems to need updating:

    // Leave the directory neither readable nor writable. Since the file
    

    In fact we are leaving the directory as world writeable so that this touch to invalidate works?

    pwolanin’s picture

    Issue summary: View changes

    Updated issue summary.

    chx’s picture

    Issue summary: View changes

    Updated issue summary.

    YesCT’s picture

    Issue summary: View changes

    add reminder to post results

    chx’s picture

    Issue summary: View changes

    Updated issue summary.

    chx’s picture

    Issue summary: View changes

    Updated issue summary.

    willyk’s picture

    I completed some review and testing as well, but wasn't able to fully complete doing so today. I'll try to do tomorrow. I've made some revisions to the summary and will make a few more tomorrow. Please revert as you see fit.

    chx’s picture

    Title: CM permission settings by FileStorage make commandline tools like drush unusable. » Relax MTimeProtectedFileStorage for DX, drush integration and world dominitation
    Assigned: effulgentsia » Unassigned
    FileSize
    7.88 KB

    We are now allowing the listing of the holder directory. Still the file mtime vs directory mtime and the hash based on mtime and secret protects enough.

    chx’s picture

    Issue summary: View changes

    Modifying instructions slightly re MAMP Pro

    chx’s picture

    Issue summary: View changes

    Updated issue summary.

    chx’s picture

    Issue summary: View changes

    Updated issue summary.

    chx’s picture

    Issue summary: View changes

    Updated issue summary.

    Status: Needs review » Needs work

    The last submitted patch, 1908440_106.patch, failed testing.

    chx’s picture

    Status: Needs work » Needs review
    FileSize
    8.11 KB
    chx’s picture

    Title: Relax MTimeProtectedFileStorage for DX, drush integration and world dominitation » Relax MTimeProtectedFileStorage permissions for DX, drush integration and world domination

    Status: Needs review » Needs work

    The last submitted patch, 1908440_108.patch, failed testing.

    chx’s picture

    Status: Needs work » Needs review
    FileSize
    8.15 KB

    Our phpunit integration leaves a lot to be desired, including documentation and testbot integration.

    Anonymous’s picture

    Just making a note, the patch in 111 worked for me. I told the drush folks who are going to give this a code review.

    alexpott’s picture

    I've done the following tests. It is looking good so far.

    • A user other than the user that is running the web service can view the compiled container
    • Drush run by a user that is not the web service user can invalidate the container
    • Editing the container from the command line results in a container rebuild

    Still reviewing...

    greg.1.anderson’s picture

    I have done only one test so far (darn network!). It was a bad test, because I set things up differently than expected. I set up my initial file permissions the way I always do: some other user (e.g. www-admin) owns all of the files, and the webserver is a member of the files groups. Usually, I only make certain directories writable by the webserver, but for this run, I started off with 664 / 775 permissions everywhere. This configuration gave an internal server error installing Drupal from the GUI. I had #111 applied, and the Drush hack-removing patch applied.

    This configuration was supported in D7, but it is not allowed in D8 with #111. I haven't done much with D8 recently, and haven't yet tested to see if the install works in D8 without this patch. The next thing I'm going to do is test as the patch intended, with the webserver owning all documents, and see how that works.

    chx’s picture

    > an internal server error installing Drupal from the GUI

    what error, exactly? Did this happen without the patch as well?

    greg.1.anderson’s picture

    I think I still have the internal server error message on an open window on my other computer. I'll check that later. I'll also try to reproduce the same problem without this patch -- although if I don't remove the Drush hack (drush patch from this issue applied), I might not get to the failure point, and if I leave it in (drush patch from this issue not applied), the failure might be avoided altogether -- so I don't know if there's much to be learned from testing those scenarios.

    I ran one more bad test (install from GUI, then cache clear from Drush), where I did not remember to clear out all of the files described in the issue summary. That resulted in the following error:

    /srv/www/d8.org/htdocs/core/lib/Drupal/Core/Config/FileStorage.php).
    Drupal\Core\Config\StorageException: Failed to write configuration file: sites/default/files/config_zsFwHE0L2HINrK21Xx7t4V4WxCoAVohTIaQqYRJwfN4/active/block.block.bartik.breadcrumbs.yml in Drupal\Core\Config\FileStorage->write() (line 115 of /srv/www/d8.org/htdocs/core/lib/Drupal/Core/Config/FileStorage.php).
    

    I mention it because it seems that the config files should not be touched by cache clear. This is likely a separate issue in any event. I'm going to re-run that test after properly resetting the site.

    greg.1.anderson’s picture

    Hm, I just got the same error as in #114 when re-testing #116.

    The error is:

    An AJAX HTTP error occurred.
    HTTP Result Code: 500
    Debugging information follows.
    Path: http://d8.org/core/install.php?langcode=en&profile=standard&id=1&op=do_nojs&op=do
    StatusText: Internal Server Error
    ResponseText:
    

    It's just an AJAX error, probably unrelated. Ignoring it shows no ill effect to the Drupal site that was installed. I ran the test another time and did not get this error. Couldn't say if this is an intermittent problem, or if I just failed to clean the site correctly a couple of times.

    Re-testing #116, with the webserver set to initially own all of the files, and the Drush user being a member of the file group, I got the same error on drush cc all. The file in question does exist (block.block.bartik.breadcrumbs.yml), and is chmod u+rw, with read-only access given to the group and others. The result is that Drush reports a fatal error; however, the php directory is cleared correctly. Since apparently cc all is not supposed to rewrite config files, perhaps this is a separate issue. Is it a requirement that the Drush user and the web user be members of a shared group? Requiring that would probably also solve this problem, but the file would then need to be 664 instead of 644, as it is now.

    I still need to test installing with Drush, and clearing the cache in the GUI.

    chx’s picture

    Status: Needs review » Reviewed & tested by the community

    Based on #112 and #117 saying "the php directory is cleared correctly" I will do the honors and then let the committers decide.

    The config directory and drush is a completely different issue.

    greg.1.anderson’s picture

    I also did a test where I installed using drush site-install. My intention was to see if the cache could be cleared from the GUI. However, nothing in the php cache was created by site-install; it was all initialized by the subsequent page loads, so all of the files were owned by the webserver, making this test essentially the same as #117. Certainly the twig cache will always be written during page load; are there any code paths that Drush might call that would create the service container caches?

    In any event, in the context of the common scenarios tests, I agree that this patch correctly handles the shared php cache, and is RTBC.

    willyk’s picture

    @greg.1.anderson WRT to the ajax error that you mentioned in #117 during install, I encountered that same in my testing in #115 and confirmed those errors arose during install both due to memory and timeout limit issues. I modified this issue summary/instructions above to reflect the settings needed which should avoid that error. Just posting this to confirm that those error are unrelated the issue/patch in question. Additionally, FWIW in my testing I wasn't able to replicate the error on a Mac w/different users, so was not able to confirm whether the patch was working. Great to see that you and alex were successful; if we need another test, please ping me IRC and I can tweak my testing to mirror yours and complete another test (I'm running OS X).

    greg.1.anderson’s picture

    I am pretty confident that the current patch works well for the specific use cases that it addresses. We will have follow-up issues for CMI, but we can address those separately. The concerns I have vis-a-vis the remaining problems are operational, not security-related.

    I have just one more comment to make regarding security. Well, actually, my one more comment is actually contingent on two questions.

    1. If some foreign code or process or agent ran arbitrary 'sudo chmod' commands on the files/php directory or its contents (or anywhere else), are there any specific modes that could cause serious security problems?

    2. Specifically, if a user followed the instructions from https://drupal.org/node/244924/, could it cause a security vulnerability?

    I did not have time to review this aspect thoroughly, so I'm not certain there is even a problem here. This is probably a follow-up documentation issue at best -- unless of course someone has already written the Drupal 8 version of the above instructions, which were of course written for D7. So, I think we're still RTBC here, but I did want to bring up the documentation issue.

    chx’s picture

    There's no permission in the world that could affect the MTimeProtectedFileStorage security: a) just using fopen() and writing the container will immediately invalidate it b) deleting it and writing it back the same name affects directory mtime so the hash is invalid. (Try mkdir x ; php -r 'print filemtime("x")."\n"; sleep(1);'; touch x/y.php; php -r 'print filemtime("x")."\n";'.)

    To be clear: if you can read settings.php then with this patch you can escalate that to running arbitrary PHP and this is not possible in HEAD -- but if you can read settings.php you have the database credentials and it's plausible to think you can then connect to the db and then do whatever you want...

    alexpott’s picture

    Title: Relax MTimeProtectedFileStorage permissions for DX, drush integration and world domination » Change notice: Relax MTimeProtectedFileStorage permissions for DX, drush integration and world domination
    Priority: Critical » Major
    Status: Reviewed & tested by the community » Active
    Issue tags: +Needs change record

    Committed 701d4e6 and pushed to 8.x. Thanks!

    I think we should have a change notice here - I can't find any change notice for the original mtime.

    jibran’s picture

    Old change notice for this was http://drupal4hu.com/node/331

    chx’s picture

    Title: Change notice: Relax MTimeProtectedFileStorage permissions for DX, drush integration and world domination » Relax MTimeProtectedFileStorage permissions for DX, drush integration and world domination
    Status: Active » Fixed
    Issue tags: -Needs change record
    chx’s picture

    pwolanin’s picture

    @chx - can you explain more clearly the risk
    "if you can read settings.php then with this patch you can escalate that to running arbitrary PHP"?

    Is this only using the "secret salt" and not the actual drupal secret from the DB or config?

    chx’s picture

    This is used to boot the kernel. What DB :) I will add more.

    tonku’s picture

    Status: Fixed » Needs work

    Environment: Win7, Acquia dev desktop.

    We installed Drupal via browser and installation went ok. After visiting the site first time we are getting the following warning:
    Warning: rename(C:\Users\Username\Sites\drupal/sites/d8.localhost/files/php/service_container/.service_container_prod.php,C:\Users\Username\Sites\drupal/sites/d8.localhost/files/php/service_container/service_container_prod.php/58a1afbbaaba3ad71b02586ce30febed3a8bc77c1d45cc8c2f8ce18d067a17b4.php) [function.rename]: Access is denied. (code: 5) in Drupal\Component\PhpStorage\MTimeProtectedFastFileStorage->save() (line 124 of core\lib\Drupal\Component\PhpStorage\MTimeProtectedFastFileStorage.php).

    We did git revert 701d4e6 and reinstalled Drupal from scratch again. The warning message didn't appear again.

    chx’s picture

    Status: Needs work » Fixed

    Please file a separate issue. This sounds a very weird and very likely a Windows-specific issue -- and it's entirely possible that previously you were not getting a container at all and were building one on every load.

    Edit: I filed #2099671: Windows 7 and MTimeProtectedFileStorage has problems for you.

    Status: Fixed » Closed (fixed)

    Automatically closed -- issue fixed for 2 weeks with no activity.

    Anonymous’s picture

    Issue summary: View changes

    Updated issue summary.