I know that we change the permissions on settings.php (actually, the default directory) for "security reasons" but it's a change that makes me very mad at drupal on a weekly basis.

Not only does this behavior end up locking unsuspecting newbies out of thier own files - but it also frustrates people working on core (or trying to work on core) since doing a git pull will fail miserably when it gets to a file or directory that's had it's permissions changed - crapping out a bunch of half pulled files all over your hard work, and making it nearly impossible to get back to the pre-crapped-on state.

If we want more contributors to core, we need to fix this annoying behavior of Drupal thinking it knows better than we do about our file permissions.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jenlampton’s picture

Title: Stop locking me out of my own settings.php file » Stop locking me out of my own sides/default directory

Updating title, because the need to sudo rm -rf my sites/default/files/php may be even more annoying.

jenlampton’s picture

Title: Stop locking me out of my own sides/default directory » Stop locking me out of my own sites/default directory

typo.

quicksketch’s picture

Status: Active » Needs review
FileSize
4.06 KB

This patch takes the reasonable approach of simply checking if Drupal automatically set any permissions on the conf directory. If it made changes to the permissions (to make the directory writable) then it will automatically set the directory to be read-only when done. However if the directory did NOT need to have its permissions modified, we don't automatically change the permissions to something else. Instead if the directory is still writable after settings.php has been written to disk, it displays a message requesting the user change the permissions (the same thing it did previously if it didn't have permissions to chmod the directory at all).

So in short this does the following in each situation:

  1. If the directory permissions were automatically changed to make it writable, the permissions are automatically changed back to read-only when finished (same as before).
  2. If the directory permissions may not be set by Drupal at all, a message is shown as before asking the user to set permissions manually (same as before).
  3. If the directory permissions were not automatically changed, the same message is shown as situation #2, asking the user to set permissions manually (this is the new part).

So basically it's all-or-nothing automatic behavior. Rather than right now where Drupal is monkeying around with file permissions when it shouldn't be.

Status: Needs review » Needs work

The last submitted patch, drupal_no_autopermissions-1998698.patch, failed testing.

quicksketch’s picture

Status: Needs work » Needs review
quicksketch’s picture

Oops, looks like I removed README.txt from sites/default. File restored in this one.

jenlampton’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
68.29 KB

ZOMG, this is so much better!!!!
sweet, sweet success

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal_no_autopermissions-1998698.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

(Random failure)

Discussed this during our sprint at Stanford today, it is a great idea.

alexpott’s picture

Issue tags: +Needs security review

I guessing a plus 1 from a security team member would be nice. Personally I'm all for empowering the user and not doing stuff automagically.

chx’s picture

Assigned: Unassigned » effulgentsia
Status: Reviewed & tested by the community » Needs review

There are two people among the Drupal core maintainers who are widely renowned to bring common sense to the table (the other being David Rothstein). Assinging to one for further review. I do not trust my judgement over this one.

Pancho’s picture

Assigned: effulgentsia » Unassigned
Status: Needs review » Active
Issue tags: -Needs security review

#6 does the job, but here's just another crazy idea:

We might want to introduce different modes like a "dev mode" and a "production mode".
D8 would install in "dev mode" leaving the admin as much freedom as possible. Only when at a later point the admin switches to "production mode", a number of security, performance and possibly data integrity settings would come into place, such as:
- trigger a backup
- change permissions
- aggregate JS and CSS files
- use page cache and possibly APC
- disable/uninstall certain insecure or unnecessary admin modules
- etc.
vice versa.
Regarding permissions - which is the problem here - we'd need to find some solution to make the change reversible.

The modes should be configurable, to give the admin full control, and extendable by contrib. I can imagine this being a major DX improvement, even more now that the disabled state of modules gets removed.

Pancho’s picture

Status: Active » Needs review
Issue tags: +Needs security review

Really sorry for unassigning effulgentia by accident, I can't revert this though...

swentel’s picture

Assigned: Unassigned » effulgentsia

Assinging back.

Pancho’s picture

Thanks and sorry again!

quicksketch’s picture

@Pancho there is already a separate (bikeshedding) issue for a development mode toggle at #1537198: Add a Production/Development Toggle To Core. Let's keep this issue focused on the original problem. This issue doesn't require any settings, it's just about doing less automatic behavior during the installer.

Pancho’s picture

Didn't notice or find that one - thanks for pointing me to it.
The patch in #3/#6 is very reasonable - although it's over-optimistic to assume the admin will manually tighten permissions. Some will, many will sometimes, and others will never bother with that. However, the current D8 behaviour is definitely too bold now. Extra security measures need quite some more considerations. So, +1.

effulgentsia’s picture

Assigned: effulgentsia » Dries
Issue tags: +Needs committer feedback

What's the recommendation for system_requirements() then? Do we let that continue automagically remove write permission? #1232572: Add a variable to disable fixing file permissions during system_requirements() "runtime" checks suggests a config setting to control that, but maybe there's a better option than a config setting?

Also, assigning to Dries, because whether we want to retain some kind of automagic seems like a product-level decision that needs his input. Perspectives from people on the security team would also be helpful in the meantime.

effulgentsia’s picture

Priority: Major » Normal

Also, I don't get why this issue needs to be major, and count against our thresholds. The behavior in HEAD is by design, and people have managed to contribute to D6 and D7 and D8 to date. I agree it's an annoyance though, and valuable for Dries to evaluate.

quicksketch’s picture

Thanks @effulgentsia I wasn't aware of #1232572: Add a variable to disable fixing file permissions during system_requirements() "runtime" checks. Perhaps we should keep the two issue separate? I agree they're definitely related, but they don't seem to overlap. I think if this patch individually is approved, we should remove the automatic behavior from system_requirements() in the other issue and change it to just be a *check*, rather than a check + magic-fix.

chx’s picture

I consider the rogue upload script, the kind that takes the filename from $_GET one of the biggest dangers (cos most others we have APIs against: Twig autoescape and filter_xss for XSS, FAPI for CSRF, DBTNG for SQL injection). Often these are not inside modules. Just sit there. Reminder: https://drupal.org/node/1679442 was the SA and http://drupalcode.org/project/dragdrop_gallery.git/blob/refs/heads/6.x-1... is the vulnerability.

Because of this, I think we should not have a directory writeable that has PHP files included by Drupal / served by the webserver.

It is up to Dries to say "this sort of protection is not the job of core". I do not agree with that, but I am just one voice. There's nothing wrong with changing this. Times change, priorities shift, it's entirely possible that among the many worldview changes of Drupal 8 this also changed. If it did, then #2022049: Remove phpstorage is the next step and my resignation from the security team.

larowlan’s picture

Some things to consider:
1) you shouldn't pull with uncommitted files, our git config advocates setting up always rebase when pulling, and that isn't compatible with uncommitted files. i.e. git will say 'you have unstaged commits you cannot pull'

2) if something does error because of file permissions during a pull because you have committed or stashed your changes (refer point 1) then you can set the file permissions manually and then use git fetch origin && git reset --hard origin/8.x && git clean -df and be back to a clean slate. This is assuming that your issue occurred whilst you were on 8.x and trying to pull and had no new work. If not, I would advocate that you should be using local branches. In which case the error would have occurred during a merge or a rebase. In which case git rebase --abort && git reset --hard HEAD && git clean -df would have the same effect.

It is not that inconvenient and I think reversing this important feature, particularly for those with no concept of file permissions or how to change them on their server, is a big price to pay for the sake of a minor inconvenience.

amateescu’s picture

FWIW, I fully agree with chx and larowlan.

And to bring something helpful to the discussion, although it was mentioned in #17 (and quickly dismissed), we might also consider #1537198: Add a Production/Development Toggle To Core a top priority task for D8 and only enforce file permissions if we are on 'prod'. Then comes the question.. what if people don't switch that toggle after putting their site live.. well, they're out of luck? Or we can even make 'prod' the default?

Pancho’s picture

:)

quicksketch’s picture

So some additional feedback I had in IRC that isn't mentioned here:

- In the case of the sites/default directory, if the web server can chmod() the directory to make it readable/writable, the chmod() is not a real layer of security. It's an illusion of security that a directory isn't writable by the web-server when it only takes a single chmod() to make it writable again. Any of the "bad scripts" that are included as examples in various libraries or a truly malicious script only require a single chmod() line to overcome this "security" that we're supposed to be ensuring our users.

- In the case of all other directories, if Drupal can change write to the sites/default directory out of the box, then it's extremely likely that it can *also* write anywhere else in the Drupal root (i.e. the web server user is the same as the user who uploaded all the files). Artificially "locking" the sites/default directory provides no protection when the other hundred directories of Drupal are all writable too. It's *only* the sites/default directory that gets this special treatment and it doesn't make sense. If the user has made the owner of all Drupal the same as the web user, it's both their prerogative and responsibility to set the permissions on ALL the directories. Securing all directories is more the territory of #1232572: Add a variable to disable fixing file permissions during system_requirements() "runtime" checks than this issue. Pretending that they're secure because Drupal has artificially "locked" a single directory is not real security.

effulgentsia’s picture

Any of the "bad scripts" that are included as examples in various libraries or a truly malicious script only require a single chmod() line to overcome this "security"

You're right about truly malicious scripts, but not about accidentally vulnerable scripts being exploited by malicious people, as in #22. Since those scripts typically don't chmod(), permissions are a useful layer of protection.

Artificially "locking" the sites/default directory provides no protection when the other hundred directories of Drupal are all writable too.

That's a fair point though.

chx’s picture

> it's extremely likely that it can *also* write anywhere else in the Drupal root (i.e. the web server user is the same as the user who uploaded all the files).

Let's then add an installer step nagging against this instead.

> when the other hundred directories of Drupal are all writable too.

We presumed all the time this is not the case. If it is, that's bad. But that's a different issue.

Dries’s picture

Assigned: Dries » Unassigned

Two rules:

1) When it comes to security, it's better to do more than to do less, even if it is not a complete solution. We should always move to greater security for end-users, not to less security.

2) When it comes to security, it's better to optimize for the end-user than for the developer. If there is something that annoys developers, we can offer a switch that disables the security feature.

Given that, I think we should leave the current behavior and possibly introduce a kill-switch that developers can enable at their own risk.

effulgentsia’s picture

Status: Needs review » Postponed
Issue tags: -Needs committer feedback

I think #29 makes this a pretty clear "won't fix", at least without #1537198: Add a Production/Development Toggle To Core getting resolved first. Not sure whether the right status here is "won't fix" or "postponed". Choosing the latter for now.

quicksketch’s picture

Status: Postponed » Closed (won't fix)
Issue tags: +Needs committer feedback
quicksketch’s picture

Issue tags: -Needs committer feedback, -Needs security review

.

jenlampton’s picture

I'm sad that we can't find a way to *actually* make Drupal secure and also remove this annoying behavior that frustrates would-be developers (cause yeah, maybe we have too many right now?), but I guess this is the world we live in...

Pretending that they're secure because Drupal has artificially "locked" a single directory

lesigh.

chx’s picture

Title: Stop locking me out of my own sites/default directory » Stop locking me out of my own sites/default directory if Drupal is already insecure
Status: Closed (won't fix) » Active

On further IRC discussion, we came up with the following: do a writeability check on DRUPAL_ROOT, if it is writeable, we can skip this lock-out because it is pointless. It is NOT pointless in every other case. And, every other case should be normal. But for people on MAMP/WAMP it seems to be default. On the site info page in the installer I would like to see a) a message that such a setup is fundamentally insecure and link to d.o. for explanation b) a checkbox acknowledging the message.

chx’s picture

Issue summary: View changes

corrected to avoid mentioning permissions change on settings.php - include mention of default directory

David_Rothstein’s picture

Issue summary: View changes
Issue tags: +Needs backport to D7

It's a tricky problem, but would love to see this improved.

I think #34, combined with the earlier patch, could probably work. So if I understand this correctly:
1. If Drupal itself changed the permissions, definitely try to change them back.
2. If Drupal didn't change the permissions, but it appears that at the end of the installation the sites/default directory (or things in it) are still writable while the "rest of the installation" (e.g. DRUPAL_ROOT) isn't writable, try to fix the permissions also. I guess this is intended to cover the case where someone made the sites/default directory writable before even starting the install.
3. If Drupal didn't change the permissions and DRUPAL_ROOT itself is writable, don't try to change the permissions back - but still warn about the situation.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

sun’s picture

Issue tags: -Needs backport to D7

This is still an issue today and it gets ridiculous:

~/sites/test/drupal8/htdocs (8.5.x)*$ git reset --hard 
error: unable to unlink old 'sites/default/default.settings.php': Permission denied
fatal: Could not reset index file to revision 'HEAD'.

~/sites/test/drupal8/htdocs (8.5.x)*$ ll sites/default/
total 144
dr-xr-xr-x  6 sun  staff    204 25 Mär  2017 .
drwxr-xr-x  8 sun  staff    272 26 Jan 19:10 ..
-rw-r--r--  1 sun  staff   6762 25 Mär  2017 default.services.yml
-rw-r--r--  1 sun  staff  31180 25 Mär  2017 default.settings.php

Drupal 8 claims to be "made for developers". This baby-sitting is the opposite of that.

In the meantime #1232572: Add a variable to disable fixing file permissions during system_requirements() "runtime" checks introduced a configuration setting in 8.1…

But every new manual setting – instead of sane defaults – hurts DX and UX for everyone.

There are 33,000 results for https://www.google.de/search?q=drupal+settings.php+not+writable — Drupal is the only CMS I'm aware of that performs such an operation. Not even WordPress is doing something like this, even though it has 20x more users, and even though the vast majority of them are not developers.

michaelmallett’s picture

I still run into this daily despite having $settings['skip_permissions_hardening'] = TRUE;

It's incredibly frustrating.

alexpott’s picture

#2869916: SiteConfigureForm ignores 'skip_permissions_hardening' setting will fix some of the pain because the installer currently does not respect this setting.

RustedBucket’s picture

This whole methodology is absurd and should be removed. It's insulting that Drupal thinks it needs to babysit everything. There's already appropriate warning about a directory being insecure without the further need to cram this down peoples throats.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

philosurfer’s picture

bump... *grabs popcorn*

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.