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.

Files: 
CommentFileSizeAuthor
#7 sweet-sweet-success.png68.29 KBjenlampton
#6 drupal_no_autopermissions-1998698.patch3.4 KBquicksketch
PASSED: [[SimpleTest]]: [MySQL] 57,571 pass(es).
[ View ]
#3 drupal_no_autopermissions-1998698.patch4.06 KBquicksketch
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Comments

Title:Stop locking me out of my own settings.php fileStop 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.

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

typo.

Status:Active» Needs review
StatusFileSize
new4.06 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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.

Status:Needs work» Needs review

StatusFileSize
new3.4 KB
PASSED: [[SimpleTest]]: [MySQL] 57,571 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new68.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.

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community

(Random failure)

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

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.

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.

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.

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

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

Assigned:Unassigned» effulgentsia

Assinging back.

Thanks and sorry again!

@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.

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.

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 in drupal_install_fix_file 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.

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.

Thanks @effulgentsia I wasn't aware of #1232572: Add a variable to disable fixing file permissions in drupal_install_fix_file. 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.

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.

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.

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?

:)

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 in drupal_install_fix_file than this issue. Pretending that they're secure because Drupal has artificially "locked" a single directory is not real security.

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.

> 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.

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.

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.

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

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.

Title:Stop locking me out of my own sites/default directoryStop 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.

Issue summary:View changes

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