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.
Comment | File | Size | Author |
---|---|---|---|
#7 | sweet-sweet-success.png | 68.29 KB | jenlampton |
#6 | drupal_no_autopermissions-1998698.patch | 3.4 KB | quicksketch |
#3 | drupal_no_autopermissions-1998698.patch | 4.06 KB | quicksketch |
Comments
Comment #1
jenlamptonUpdating title, because the need to sudo rm -rf my sites/default/files/php may be even more annoying.
Comment #2
jenlamptontypo.
Comment #3
quicksketchThis 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:
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.
Comment #5
quicksketch#3: drupal_no_autopermissions-1998698.patch queued for re-testing.
Comment #6
quicksketchOops, looks like I removed README.txt from sites/default. File restored in this one.
Comment #7
jenlamptonZOMG, this is so much better!!!!
Comment #9
tim.plunkett#6: drupal_no_autopermissions-1998698.patch queued for re-testing.
Comment #10
tim.plunkett(Random failure)
Discussed this during our sprint at Stanford today, it is a great idea.
Comment #11
alexpottI 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.
Comment #12
chx CreditAttribution: chx commentedThere 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.
Comment #13
Pancho#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.
Comment #14
PanchoReally sorry for unassigning effulgentia by accident, I can't revert this though...
Comment #15
swentel CreditAttribution: swentel commentedAssinging back.
Comment #16
PanchoThanks and sorry again!
Comment #17
quicksketch@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.
Comment #18
PanchoDidn'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.
Comment #19
effulgentsia CreditAttribution: effulgentsia commentedWhat'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.
Comment #20
effulgentsia CreditAttribution: effulgentsia commentedAlso, 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.
Comment #21
quicksketchThanks @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.
Comment #22
chx CreditAttribution: chx commentedI 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.
Comment #23
larowlanSome 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.
Comment #24
amateescu CreditAttribution: amateescu commentedFWIW, 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?
Comment #25
Pancho:)
Comment #26
quicksketchSo 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.
Comment #27
effulgentsia CreditAttribution: effulgentsia commentedYou'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.
That's a fair point though.
Comment #28
chx CreditAttribution: chx commented> 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.
Comment #29
Dries CreditAttribution: Dries commentedTwo 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.
Comment #30
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #31
quicksketchFake security++
I guess this leaves us only with #1232572: Add a variable to disable fixing file permissions during system_requirements() "runtime" checks then.
Comment #32
quicksketch.
Comment #33
jenlamptonI'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...
lesigh.
Comment #34
chx CreditAttribution: chx commentedOn 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.
Comment #34.0
chx CreditAttribution: chx commentedcorrected to avoid mentioning permissions change on settings.php - include mention of default directory
Comment #35
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedIt'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.
Comment #41
sunThis is still an issue today and it gets ridiculous:
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.
Comment #42
michaelmallett CreditAttribution: michaelmallett commentedI still run into this daily despite having $settings['skip_permissions_hardening'] = TRUE;
It's incredibly frustrating.
Comment #43
alexpott#2869916: SiteConfigureForm ignores 'skip_permissions_hardening' setting will fix some of the pain because the installer currently does not respect this setting.
Comment #44
RustedBucket CreditAttribution: RustedBucket commentedThis 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.
Comment #52
philosurfer CreditAttribution: philosurfer as a volunteer and at Pantheon for Pantheon commentedbump... *grabs popcorn*