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!
- 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)
- 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.) - Patch drush (from github) with https://drupal.org/files/drush-do_not_test.patch this actually removes a currently necessary hack from drush.
curl -O https://drupal.org/files/drush-do_not_test.patch
(or use wget instead of curl -O)git checkout -b somedrushbranchname
git apply --index drush-do_not_test.patch
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.
- 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";
- Do not patch drupal (so you should see an error in step 7).
- Patch drupal. patch (from #98 or more recent. copy the address into paste buffer and use it in the curl or wget.)
-
curl -O https://drupal.org/files/1908440_102.patch
git checkout -b cm-perm
git apply --index 1908440_102.patch
git commit -m "cm permissions patch 102"
-
---- now we can test -----
- 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. - 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. - 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.
Comment | File | Size | Author |
---|---|---|---|
#111 | 1908440_111.patch | 8.15 KB | chx |
#108 | 1908440_108.patch | 8.11 KB | chx |
#106 | 1908440_106.patch | 7.88 KB | chx |
#102 | interdiff-98-102.txt | 3.15 KB | YesCT |
#102 | 1908440_102.patch | 9.63 KB | YesCT |
Comments
Comment #1
clemens.tolboomThe attached patch makes it possible to work again by doing some directory tweaking like
[edit]Note:
The user running drush is a member of the same group as webserver
clemens is member of group www-data
[/edit]
Comment #3
clemens.tolboomI changed the test to reflect the same attributes and respect the current UMASK.
Comment #4
clemens.tolboomAre other scripts like Running Tests Through command-line failing too?
Comment #5
clemens.tolboomThe 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
Doing a grep on core as a whole
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 :(
Comment #6
clemens.tolboomHmmm ... I should read #1668892: Enable secure compiled information on disk too.
Comment #7
effulgentsia CreditAttribution: effulgentsia commentedI 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.Comment #8
effulgentsia CreditAttribution: effulgentsia commentedDidn't intend to change status.
Comment #9
clemens.tolboom@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
Is umask ignored on windows at all? I'm not sure what that means to our current implementation.
)
Comment #10
sunTwo 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.
Comment #11
clemens.tolboom#1)
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 $confCan 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)
Comment #12
franskuipers CreditAttribution: franskuipers commentedI can imagine more Twig users have problems with compiled file permissions.
I found these items:
Comment #13
chx CreditAttribution: chx commentedIf you are in a secure environment use the FileStorage. If you are not then use MTimeProtectedFileStorage.
Comment #14
chx CreditAttribution: chx commentedOh. Right, this one is correct. Sorry for premature closing. It shouldn't be 700. Let's discuss what it should be.
Comment #15
clemens.tolboomDoing
grep -r chmod\( core
we get a plethora usages likeconsistently.
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?
Comment #16
chx CreditAttribution: chx commentedThe 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.
Comment #17
Fabianx CreditAttribution: Fabianx commentedThere 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:
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!
Comment #18
Dave ReidSeems 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.
Comment #19
Fabianx CreditAttribution: Fabianx commentedOff-topic response to concerns
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.
Comment #20
clemens.tolboomA code review would help moving this forward.
And maybe some updates to the summary.
Setting to 'needs review' as it does :p
Comment #21
greg.1.anderson CreditAttribution: greg.1.anderson commentedI 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.
Comment #22
chx CreditAttribution: chx commentedunlink
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.MTimeProtectedFileStorage
.The rest can be discussed in the drush queue.
Comment #23
chx CreditAttribution: chx commentedEh, 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.
Comment #25
clemens.tolboomIs this related to #1899126: [D7] Add wrappers to fix permission checks
Comment #26
chx CreditAttribution: chx commentedIf 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.)
Comment #27
clemens.tolboomI'm curled up as the interdiff between #3 and #23 is to big for me to handle :-/
Comment #28
Fabianx CreditAttribution: Fabianx commented#23: 1908440_23.patch queued for re-testing.
Comment #30
chx CreditAttribution: chx commentedThere is no interdiff, I wrote it from scratch. And the problem is trivial.
Comment #31
moshe weitzman CreditAttribution: moshe weitzman commentedAnyone available to review this tiny patch?
Comment #32
star-szrI can't speak to the technical aspects of the patch, but…
I think these three lines should be removed, unless I'm missing something.
Comment #33
star-szrAnd patch.
Comment #34
chx CreditAttribution: chx commentedTo 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.Comment #34.0
chx CreditAttribution: chx commentedAdded XREFs + minor text changes
Comment #34.1
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #34.2
chx CreditAttribution: chx commentedUpdated the issue summary
Comment #35
effulgentsia CreditAttribution: effulgentsia commented#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.
Comment #36
greg.1.anderson CreditAttribution: greg.1.anderson commentedLink 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.
Comment #37
chx CreditAttribution: chx commentedSo 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.
Comment #38
moshe weitzman CreditAttribution: moshe weitzman commented#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.
Comment #39
effulgentsia CreditAttribution: effulgentsia commentedI 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 doeschmod 0100
, we would instead dochmod 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.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?
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.
Comment #40
moshe weitzman CreditAttribution: moshe weitzman commentedThat 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.
Comment #41
greg.1.anderson CreditAttribution: greg.1.anderson commentedThe 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 ofdrush 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 makingdrush 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.
Comment #42
effulgentsia CreditAttribution: effulgentsia commentedCan you? I'm not able to do so on a Mac either using the
mv
command or therename
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.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.
At least for all the core PHP storage (DIC and twig), deletion isn't harmful. See below about accidental or malicious modification.
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.
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?
Comment #43
chx CreditAttribution: chx commentedSo what we have is:
What protections do we have?
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:
Comment #44
greg.1.anderson CreditAttribution: greg.1.anderson commentedI'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.
Comment #45
chx CreditAttribution: chx commentedComment #46
chx CreditAttribution: chx commentedDid everyone lost interest?
Comment #47
greg.1.anderson CreditAttribution: greg.1.anderson commentedPerhaps 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.
Comment #48
chx CreditAttribution: chx commented#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.
Comment #49
greg.1.anderson CreditAttribution: greg.1.anderson commentedTwo 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.
Comment #50
chx CreditAttribution: chx commentedSo 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.
Comment #51
greg.1.anderson CreditAttribution: greg.1.anderson commentedIf 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.
Comment #52
greg.1.anderson CreditAttribution: greg.1.anderson commentedOh, 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.
Comment #53
greg.1.anderson CreditAttribution: greg.1.anderson commentedThe 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.
Comment #54
clemens.tolboomAccording to the current issue summary
contradicts with
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
?Comment #55
chx CreditAttribution: chx commentedSorry I got confused, I think. Forget everything I said and let's get back to:
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?Comment #56
greg.1.anderson CreditAttribution: greg.1.anderson commentedI 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.
Comment #57
chx CreditAttribution: chx commentedThe $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.
Comment #58
greg.1.anderson CreditAttribution: greg.1.anderson commented#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.
Comment #59
chx CreditAttribution: chx commentedI 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.
Comment #60
greg.1.anderson CreditAttribution: greg.1.anderson commentedFrom http://en.wikipedia.org/wiki/File_system_permissions:
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:
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.
Comment #61
chx CreditAttribution: chx commentedeh.
Comment #62
greg.1.anderson CreditAttribution: greg.1.anderson commentedHm, 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.
Comment #63
xjmComment #63.0
xjmupdated again
Comment #63.1
chx CreditAttribution: chx commentedrewrote the summary
Comment #63.2
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #63.3
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #64
chx CreditAttribution: chx commentedI let someone else code this :)
Comment #65
YesCT CreditAttribution: YesCT commentedthe 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.
Comment #66
YesCT CreditAttribution: YesCT commentedComment #66.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary.
Comment #67
chx CreditAttribution: chx commentedWe can treat that patch as if it wouldn't exist.
Comment #68
greg.1.anderson CreditAttribution: greg.1.anderson commentedI 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.
Comment #69
pwolanin CreditAttribution: pwolanin commentedHere'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?
Comment #70
moshe weitzman CreditAttribution: moshe weitzman commentedIts a novel idea, but I don't want drush to rely on a web server at all. Thats a huge regression IMO.
Comment #71
Owen Barton CreditAttribution: Owen Barton commentedI 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?
Comment #72
clemens.tolboomFWIW 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)
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)
Comment #73
pwolanin CreditAttribution: pwolanin commented@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?
Comment #74
moshe weitzman CreditAttribution: moshe weitzman commentedYes, 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.
Comment #75
heddnIf I understand the issue summary correctly, this patch incorporates what it documented.
Comment #76
chx CreditAttribution: chx commentedThanks 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.
Comment #77
mheinke CreditAttribution: mheinke commentedpatch 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..
Comment #78
Owen Barton CreditAttribution: Owen Barton commentedI 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).
Comment #79
chx CreditAttribution: chx commentedDid anyone actually test this in a usual user-http setup (say Ubuntu) with drush whether things (like cc) work as they should...?
Comment #80
mheinke CreditAttribution: mheinke commentedi threw the patch on my development webserver (redhat enterprise linux release 6.4)
everything seems to be working as normal.
Comment #81
alexpottin #76 @chx says...
That sounds like there is still work to do...
Comment #82
chx CreditAttribution: chx commentedIt 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.
Comment #83
steinmb CreditAttribution: steinmb commentedD8 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
Running as the apache users remove the exception but still throw 'DrupalKernel: Container cannot be written to disk'.
Comment #84
chx CreditAttribution: chx commentedThat might be a drush issue of not deleting first but trying to write? I do not know.
Comment #85
greg.1.anderson CreditAttribution: greg.1.anderson commentedDrush is just calling
drupal_flush_all_caches()
to clear the cache. Haven't had time to investigate this issue, though.Comment #86
chx CreditAttribution: chx commentedTesting 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.
Comment #88
chx CreditAttribution: chx commentedComment #89
chx CreditAttribution: chx commentedHere's some simplification.
Comment #90
damiankloip CreditAttribution: damiankloip commentedAll 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?
Comment #91
chx CreditAttribution: chx commentedAdded {@inheritdoc}, helper, constant for 0333.
Comment #92
chx CreditAttribution: chx commentedAssigning 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.
Comment #92.0
chx CreditAttribution: chx commentedlets save people the heart ache of reading all the comments. and be clear about the updated summary. -c
Comment #94
chx CreditAttribution: chx commentedBot fluke.
Comment #94.0
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #94.1
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #94.2
YesCT CreditAttribution: YesCT commentedattempt to clarify steps to test.
Comment #94.3
YesCT CreditAttribution: YesCT commentedmore detail (might not help, as it is very specific)
Comment #94.4
YesCT CreditAttribution: YesCT commentedmore detail.
Comment #95
webchickI'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!!!
Comment #96
YesCT CreditAttribution: YesCT commentedI 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.
Not sure why. chx has a different result (and a different system) I have mac, with MAMP.
Comment #97
joelpittetNot 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
Mac + Homebrew'd apache/php/mysql
Comment #98
chx CreditAttribution: chx commentedTry this.
Comment #99
joelpittetLooks like 777 on php now. @YesCT want to give it another go?
Comment #100
YesCT CreditAttribution: YesCT commentedI 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):
but also got success:
with the patch, install in the UI, permissions on php: drwxr-xr-x) then drush cc
got warning (twice):
but also got success:
with the patch in 98
install in the ui
drwxrwxrwx 2 _www staff 68 Sep 19 09:30:13 2013 php/
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.]
Comment #100.0
YesCT CreditAttribution: YesCT commentedclarify sudo only for rm.
Comment #101
YesCT CreditAttribution: YesCT commentedI'm not sure about this part from the summary:
I didn't do anything to put a debug in. Do reviewers/testers need to do that?
Comment #102
YesCT CreditAttribution: YesCT commentedI read each line. Makes sense as I read it, but cannot speak to security or the technical approach.
Found a few things with docs.
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
fixed chmod typo.
adding type here: string
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.
Comment #102.0
YesCT CreditAttribution: YesCT commentedupdated location reviewers should see errors.
Comment #102.1
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #102.2
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #102.3
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #103
mradcliffeI 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.
Comment #103.0
mradcliffeTrying 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.
Comment #104
pwolanin CreditAttribution: pwolanin commentedThis code comment seems to need updating:
In fact we are leaving the directory as world writeable so that this touch to invalidate works?
Comment #104.0
pwolanin CreditAttribution: pwolanin commentedUpdated issue summary.
Comment #104.1
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #104.2
YesCT CreditAttribution: YesCT commentedadd reminder to post results
Comment #104.3
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #104.4
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #105
willyk CreditAttribution: willyk commentedI 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.
Comment #106
chx CreditAttribution: chx commentedWe 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.
Comment #106.0
chx CreditAttribution: chx commentedModifying instructions slightly re MAMP Pro
Comment #106.1
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #106.2
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #106.3
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #108
chx CreditAttribution: chx commentedComment #109
chx CreditAttribution: chx commentedComment #111
chx CreditAttribution: chx commentedOur phpunit integration leaves a lot to be desired, including documentation and testbot integration.
Comment #112
Anonymous (not verified) CreditAttribution: Anonymous commentedJust making a note, the patch in 111 worked for me. I told the drush folks who are going to give this a code review.
Comment #113
alexpottI've done the following tests. It is looking good so far.
Still reviewing...
Comment #114
greg.1.anderson CreditAttribution: greg.1.anderson commentedI 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.
Comment #115
chx CreditAttribution: chx commented> an internal server error installing Drupal from the GUI
what error, exactly? Did this happen without the patch as well?
Comment #116
greg.1.anderson CreditAttribution: greg.1.anderson commentedI 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:
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.
Comment #117
greg.1.anderson CreditAttribution: greg.1.anderson commentedHm, I just got the same error as in #114 when re-testing #116.
The error is:
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.
Comment #118
chx CreditAttribution: chx commentedBased 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.
Comment #119
greg.1.anderson CreditAttribution: greg.1.anderson commentedI 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.
Comment #120
willyk CreditAttribution: willyk commented@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).
Comment #121
greg.1.anderson CreditAttribution: greg.1.anderson commentedI 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.
Comment #122
chx CreditAttribution: chx commentedThere'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...
Comment #123
alexpottCommitted 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.
Comment #124
jibranOld change notice for this was http://drupal4hu.com/node/331
Comment #125
chx CreditAttribution: chx commentedComment #126
chx CreditAttribution: chx commentedHandbook page added https://drupal.org/node/2097351
Comment #127
pwolanin CreditAttribution: pwolanin commented@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?
Comment #128
chx CreditAttribution: chx commentedThis is used to boot the kernel. What DB :) I will add more.
Comment #129
tonku CreditAttribution: tonku commentedEnvironment: 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.Comment #130
chx CreditAttribution: chx commentedPlease 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.
Comment #131.0
(not verified) CreditAttribution: commentedUpdated issue summary.