It's common for sites in version control to include a "settings.inc" file in sites/default that is used to version control site configuration, while keeping sensitive information (keys, passwords, etc) in settings.php. When system_requirements() runs, it will automatically change the permissions of the site directory to remove write permissions. Then, when running git pull, the merge will break because write permissions are missing on the directory.

Here's a patch that adds a "drupal_install_fix_file_disable" variable that can be used to disable chmod'ing files. system_requirements() still throws an error, but I think that's desirable. I'd be open to downgrading the requirement to a warning if this variable is set.

If this approach is sound, we'd still need to add an example to settings.php so it's known that the variable exists.

CommentFileSizeAuthor
#93 1232572-92.patch9 KBmcdruid
#93 interdiff-1232572-91-92.txt2.5 KBmcdruid
#91 1232572-91.patch7.87 KBmcdruid
#91 interdiff-1232572-90-91.txt715 bytesmcdruid
#90 1232572-90.patch7.86 KBmcdruid
#90 interdiff-1232572-88-90.txt7.19 KBmcdruid
#88 1232572-88.patch6.33 KBmcdruid
#87 1232572-n87.patch6.28 KBhanoii
#86 1232572-86.patch6.36 KBcsmdgl
#85 1232572.patch6.29 KBcman9090
#80 1232572-n80.patch6.26 KBron_s
#79 1232572-n79.patch6.26 KBhanoii
#76 1232572-76.drupal-7.disable-file-permissions-fix.patch6.33 KBLennard Westerveld
#72 1232572-72.drupal-7.disable-file-permissions-fix.patch5.49 KBLennard Westerveld
#71 1232572-71.drupal-7.disable-file-permissions-fix.patch29.85 KBLennard Westerveld
#67 interdiff-1232572-50-67.txt3 KBesod
#67 1232572-67.drupal.disable-file-permissions-fix.patch7.78 KBesod
#57 1232572-57.drupal.disable-file-permissions-fix.patch8.13 KBdeviantintegral
#57 interdiff.1232572.55-57.txt2.59 KBdeviantintegral
#55 interdiff.1232572.50-55.txt2.64 KBjoachim
#55 1232572-55.drupal.disable-file-permissions-fix.patch8.12 KBjoachim
#50 interdiff-1232572-48-50.txt2.96 KBjoachim
#50 1232572-50.drupal.disable-file-permissions-fix.patch7.72 KBjoachim
#48 interdiff-1232572-45-48.txt7.67 KBjoachim
#48 1232572-48.drupal.disable-file-permissions-fix.patch7.87 KBjoachim
#46 1232572-46-no-sites-changes-do-not-test.patch6.91 KBcweagans
#45 interdiff-1232572-40-45.txt8.95 KBjohnennew
#45 1232572-45.patch7.82 KBjohnennew
#43 1232572.43-sites-dir-hardening.patch7.82 KBdeviantintegral
#43 1232572.43-interdiff-40.patch8.95 KBdeviantintegral
#40 1232572_40.patch6.64 KBcweagans
#40 interdiff.txt4.47 KBcweagans
#39 1232572-37.patch6.65 KBwebflo
#39 1232572-37.interdiff.txt1.27 KBwebflo
#34 add_a_variable_to-1232572-34.patch6.63 KBsriharsha.uppuluri
#32 site-dir-hardening-1232572-32.patch6.63 KBcweagans
#27 interdiff.txt1.24 KBbleen
#27 site-dir-hardening-1232572-27.patch6.63 KBbleen
#23 system.install-1232572.23.patch6.64 KBdeviantintegral
#18 interdiff-1232572-17-18.txt619 bytesElijah Lynn
#18 system.install-1232572.18.patch3.99 KBElijah Lynn
#17 system.install-1232572.17.patch3.99 KBdeviantintegral
#16 system.install-1232572.16.patch3.99 KBdeviantintegral
#11 system.install-1232572.11.patch6.47 KBdeviantintegral
#10 system.install-1232572.10.patch2.6 KBdeviantintegral
#9 system.install-1232572.patch2.52 KBkenorb
#7 system.install-1232572.patch2.25 KBkenorb
#2 1232572.2-disable-site-directory-permission-check.patch2.05 KBdeviantintegral
drupal_fix_file_disable.patch556 bytesdeviantintegral
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

q0rban’s picture

Yes, this is a common problem, and essentially breaks upgrades during deployment. I think the patch needs work, though, as it looks like drupal_verify_install_file() is used on more things than just the site directory.

deviantintegral’s picture

Status: Active » Needs review
FileSize
2.05 KB

Here's a different approach that only disables permission checking for the site directory. I've also include the update to default.settings.php.

q0rban’s picture

This looks MUCH better. Great job!

kenorb’s picture

kenorb’s picture

Status: Needs review » Reviewed & tested by the community

Tested and it works fine.

kenorb’s picture

Status: Reviewed & tested by the community » Needs review

What about settings.php file:

$conf_file = drupal_verify_install_file(conf_path() . '/settings.php', FILE_EXIST|FILE_READABLE|FILE_NOT_WRITABLE);

Should we have another variable 'ignore_site_settings_permissions'?
I'm not interested in changing the permissions on my local, because it's breaking my git environment.

kenorb’s picture

My version of patch which includes settings.php file and in my opinion it should show in Reports that the site permissions are not protected. But it's for 7.x

kenorb’s picture

kenorb’s picture

Version against the current 8.x

deviantintegral’s picture

Status: Needs review » Needs work
FileSize
2.6 KB

The 8.x patch needs to be converted to CMI.

Here's a 7.x patch that downgrades the error to a warning if the variable is used. As well, it changes the message for the warning which I think will make sense for 8.x but I'm not sure if we can add that string into 7.x

deviantintegral’s picture

Status: Needs work » Needs review
FileSize
6.47 KB
  • Converts to using CMI, with a key of system.site:sites_permissions.
  • Adds tests! Including a test for the initial permissions of files after install, which we don't appear to have yet.
  • Downgrades the error to a warning if permissions checking is disabled.
chx’s picture

This should be Settings() and not $conf

jenlampton’s picture

#11: system.install-1232572.11.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, system.install-1232572.11.patch, failed testing.

bsztreha’s picture

I have problem with that too, i am happy if will be a solution. I think many developers dont need to change settings.php chmod, if visit the report page... GIT, deployment etc..

deviantintegral’s picture

Issue summary: View changes
FileSize
3.99 KB

Here's an update against the 7.x patch that also avoids changing permissions during site installs.

deviantintegral’s picture

And following the above, let's skip settings.php too.

Elijah Lynn’s picture

Patch in #17 works great, yay!

If this gets committed I fixed a duplicate word in the doc block.

bleen’s picture

Status: Needs work » Reviewed & tested by the community

This has been working for us for a little while now. RTBC++

The last submitted patch, 16: system.install-1232572.16.patch, failed testing.

The last submitted patch, 17: system.install-1232572.17.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: system.install-1232572.18.patch, failed testing.

deviantintegral’s picture

Here's a reroll for 8.x, as well as a conversion to use Settings().

deviantintegral’s picture

Status: Needs work » Needs review

Shivam Agarwal’s picture

Status: Needs review » Reviewed & tested by the community

Working now! Great work by #24.

bleen’s picture

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

....just a couple grammar improvements in the comment. This patch does not change any functionality so if it passes tests it should go right back to RTBC.

bsztreha’s picture

What about Drupal 7? Will ever committed to core? #17 looks good

cweagans’s picture

Man, that's a really long running test.

Status: Needs review » Needs work

The last submitted patch, 27: site-dir-hardening-1232572-27.patch, failed testing.

cweagans’s picture

Status: Needs work » Needs review
FileSize
6.63 KB

Re-uploading patch to run the test on Drupal CI, instead of the legacy testbots.

Status: Needs review » Needs work

The last submitted patch, 32: site-dir-hardening-1232572-32.patch, failed testing.

sriharsha.uppuluri’s picture

Status: Needs work » Needs review
FileSize
6.63 KB

Previous patch failing to apply.

Status: Needs review » Needs work

The last submitted patch, 34: add_a_variable_to-1232572-34.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review

The last submitted patch, 32: site-dir-hardening-1232572-32.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 34: add_a_variable_to-1232572-34.patch, failed testing.

cweagans’s picture

FileSize
4.47 KB
6.64 KB

I changed the variable name to be more descriptive and moved it to example.settings.local.php, which is probably a reasonable place for this to live by default. It's also now turned on by default in that file.

joachim’s picture

I think it would be good if the docs for the settings also mentioned the other use case for this, which is probably more common: if you're working on Drupal core itself, your git checkout of core gets broken when a git pull changes any of the tracked files in that directory.

cweagans’s picture

@joachim, This is already covered in the patch, I think:

+    // Allow system administrators to ignore permissions hardening for the site
+    // directory. This allows additional files in the site directory to be
+    // updated when they are managed in a version control system.
[...]
+ * For
+ * sites that are managed with a version control system, this can cause problems
+ * when files in that directory (such as a "settings.inc" containing additional
+ * configuration) are updated, because the user pulling in the changes won't
+ * have permissions to modify files in the directory.
deviantintegral’s picture

I went to update this to handle the string format changes and fix a comment copy-paste typo, and ended up finding a few new bugs with this and some test cleanup that needed to be done. Compared to #40:

  • Uses StringTranslationTrait instead of global t()
  • Removes unused getInfo() method
  • Fixes ! placeholders
  • Fixes an issue with 0744 permissions (should have been 0755)
  • Adds a few protected methods to remove duplicate code
  • Fixes an issue where we were still issuing REQUIREMENT_ERROR even if protection was disabled

The last submitted patch, 43: 1232572.43-interdiff-40.patch, failed testing.

johnennew’s picture

Think the fail was just the interdiff having a .patch extension.

Rerolled the patch in #43 against latest 8.0.x branch and provided an interdiff against #40

cweagans’s picture

Re-uploading patch without the /sites changes for people using subtree splits of /core + cweagans/composer-patches. Please disregard.

Berdir’s picture

Status: Needs review » Needs work

Yes please!

Nitpick review:

  1. +++ b/core/modules/system/src/Tests/System/SitesDirectoryHardeningTest.php
    @@ -0,0 +1,115 @@
    + * @file
    + * Definition of Drupal\system\Tests\System\SitesDirectoryHardeningTest.
    

    Contains \Drupal\...

  2. +++ b/core/modules/system/src/Tests/System/SitesDirectoryHardeningTest.php
    @@ -0,0 +1,115 @@
    +  use StringTranslationTrait;
    +
    +
    

    unnecessary double empty line.

  3. +++ b/core/modules/system/src/Tests/System/SitesDirectoryHardeningTest.php
    @@ -0,0 +1,115 @@
    +    $this->assertTrue(drupal_verify_install_file($site_path, FILE_NOT_WRITABLE, 'dir'), $this->t('Verified permissions for @file.', array('@file' => $site_path)));
    

    we don't use t() anymore for assertion messages.

  4. +++ b/core/modules/system/src/Tests/System/SitesDirectoryHardeningTest.php
    @@ -0,0 +1,115 @@
    +  /**
    +   * Test that when when directory hardening is disabled that a writable file
    +   * remains writable.
    

    description of a method should be < 80 characters.

  5. +++ b/core/modules/system/src/Tests/System/SitesDirectoryHardeningTest.php
    @@ -0,0 +1,115 @@
    +  /**
    +   * Check system runtime requirements.
    

    Checks, Makes, Returns...

  6. +++ b/sites/example.settings.local.php
    @@ -90,3 +90,17 @@
    + * sites that are managed with a version control system, this can cause problems
    + * when files in that directory (such as a "settings.inc" containing additional
    

    nobody has a settings.inc anymore :)

    I think this should just say settings.php, as the documented standard these days is to have that committed and have a non-committed settings.local.php

  7. +++ b/sites/example.settings.local.php
    @@ -90,3 +90,17 @@
    + *
    + * Remove the leading hash sign to disable permissions hardening.
    + */
    +$settings['skip_permissions_hardening'] = TRUE;
    

    There is no leading hash sign, so just remove this.

joachim’s picture

All the changes from 48 done.

Status: Needs review » Needs work

The last submitted patch, 48: 1232572-48.drupal.disable-file-permissions-fix.patch, failed testing.

joachim’s picture

Whoops. Over-enthusiastic find & replace!

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Ok, this would be a great thing to have, it always causes a lot of confusion, especially with not very experienced git users.

Not sure if it can get into 8.0.x, leaving that for the committers to decide :)

catch’s picture

Title: Add a variable to disable fixing file permissions in drupal_install_fix_file » Add a variable to disable fixing file permissions in drupal_install_fix_file()
Version: 8.0.x-dev » 8.1.x-dev
Status: Reviewed & tested by the community » Needs work

This feels to me like an 8.1.x -> 7.x change to me. New strings, new setting etc.

Moving there for now. We can always cherry-pick to 8.0.x if we change our minds.

+++ b/core/modules/system/system.install
@@ -365,13 +365,22 @@ function system_requirements($phase) {
+        $severity = min($severity, REQUIREMENT_ERROR);

Shouldn't this be max()?

joachim’s picture

Yup. REQUIREMENTS_FOO constants are integers, and larger ones are worse, so we should always take the maximum.

Although:

    if (Settings::get('skip_permissions_hardening')) {
      $conf_errors[] = t('Protection disabled');
      $severity = REQUIREMENT_WARNING;
    }
    elseif (!drupal_verify_install_file($site_path, FILE_NOT_WRITABLE, 'dir')) {
      $conf_errors[] = t("The directory %file is not protected from modifications and poses a security risk. You must change the directory's permissions to be non-writable.", array('%file' => $site_path));
      $severity = REQUIREMENT_ERROR;
    }
    foreach (array('settings.php', 'settings.local.php', 'services.yml') as $conf_file) {
      $full_path = $site_path . '/' . $conf_file;
      if (file_exists($full_path) && (Settings::get('skip_permissions_hardening') || !drupal_verify_install_file($full_path, FILE_EXIST|FILE_READABLE|FILE_NOT_WRITABLE))) {
        $conf_errors[] = t("The file %file is not protected from modifications and poses a security risk. You must change the file's permissions to be non-writable.", array('%file' => $full_path));
        $severity = min($severity, REQUIREMENT_ERROR);
      }
    }

By the time we get to the min() line, $severity has been set to either REQUIREMENT_WARNING or REQUIREMENT_ERROR anyway, so if at the min() line we want REQUIREMENT_ERROR, we can just set that.

But there's more, I think: we have an 'if() elseif()'. So it's possible for $severity to not be set at all at this point -- wouldn't that cause the min() to produce an error?

And finally, this function uses the variable $severity again further on, in a different context. That's a potential bug, I think. Changing this to $settings_permissions_severity would avoid that.

joachim’s picture

Assigned: Unassigned » joachim

> Shouldn't this be max()?

> Yup. REQUIREMENTS_FOO constants are integers, and larger ones are worse, so we should always take the maximum.

Looking at the code again, no, min() is correct.

The reasoning is this:

- if the 'skip_permissions_hardening' config is set, then file permissions being writable are merely a warning, not an error, because it's assumed the site administrator has purposefully set this. So remind them of the situation as a warning, but don't make it an error.
- if 'skip_permissions_hardening' isn't set, then it's an error if settings files are writable.

That said, I don't find the use of min() to be really clear. What's actually going on here is:

- is skip_permissions_hardening set? If so, all file permissions issues should be warnings. If not, they're errors.
- check dirs and files and flag any issues, using for each one the level already set

I'll reroll for 8.1.x.

joachim’s picture

I've changed the variable name, changed the logic around the error level, and rebased to 8.1.x.

joachim’s picture

Status: Needs work » Needs review
deviantintegral’s picture

The above changes look good to me.

I've replaced SafeMarkup in the test with FormattableMarkup since SafeMarkup is depreciated. Almost there?

Saphyel’s picture

Status: Needs review » Reviewed & tested by the community

Works like a charm! Tested on my local.

Saphyel’s picture

Status: Reviewed & tested by the community » Needs work

I tested again and if you use $settings['skip_permissions_hardening'] = FALSE; it's still doing the same that TRUE..

I tested with Drush.

Berdir’s picture

Status: Needs work » Reviewed & tested by the community

I don't know what you tested, this works fine for me. Note that only certain operations, like visiting admin/reports/status triggers the file permission check and automated attempt at fixing it. With it not set or set to FALSE, write permission is removed for me, with TRUE, I get a warning and it's not changed. Exactly as it should be.

catch’s picture

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

Committed/pushed to 8.1.x, thanks!

Moving to 7.x for backport.

catch’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

  • catch committed 8548982 on 8.1.x
    Issue #1232572 by deviantintegral, joachim, cweagans, kenorb, bleen,...
joachim’s picture

We should add something about this to our developer documentation on d.org, but I'm not sure where. The only page I can find that mentions using local.settings.php is in the theme guide: https://www.drupal.org/node/1903374

David_Rothstein’s picture

Title: Add a variable to disable fixing file permissions in drupal_install_fix_file() » Add a variable to disable fixing file permissions during system_requirements() "runtime" checks
Issue tags: +Needs backport to D7
Related issues: +#1925822: Stop locking me out of my own sites/default directory if Drupal is already insecure

Retitling a bit to match what the patch does.

I have to ask though: Instead of adding a variable, are we sure we can't remove this behavior entirely?

It makes sense that Drupal tries to fix this for you at the end of the installer (and I'm linking to a related issue regarding that)...

But doing it randomly during normal site operation whenever the requirements check happens to run - that seems pretty weird.

I guess it's possible that we're trying to protect against the situation where someone went into the operating system and made the file writeable (in order to add something to settings.php, e.g. for a module they just installed) and then forgot to change the permissions back, but why isn't the warning enough in that case?

deviantintegral’s picture

It looks like the code hasn't changed much since the initial D5 installer patch over at #68926: Installer for Drupal Core.

I am OK with just removing it - but is there any value for this for those using ftp / sftp / rsync to push up Drupal? Shared hosting? I have no idea to be honest, but I also haven't dealt with a small scale site in years. Anyone from the security team have comments on removing this?

esod’s picture

For use in 8.0.x, I rerolled the patch in #50 against latest 8.0.x branch and provided an interdiff against #50.

joachim’s picture

Assigned: joachim » Unassigned

  • catch committed 8548982 on 8.3.x
    Issue #1232572 by deviantintegral, joachim, cweagans, kenorb, bleen,...

  • catch committed 8548982 on 8.3.x
    Issue #1232572 by deviantintegral, joachim, cweagans, kenorb, bleen,...
Lennard Westerveld’s picture

Adding a 7.x fix for this issue a reroll of the latest 7.x patch.
Patch is tested on version: 7.53

Lennard Westerveld’s picture

Oops removed code formatting of the patch...

  • catch committed 8548982 on 8.4.x
    Issue #1232572 by deviantintegral, joachim, cweagans, kenorb, bleen,...

  • catch committed 8548982 on 8.4.x
    Issue #1232572 by deviantintegral, joachim, cweagans, kenorb, bleen,...
joachim’s picture

Status: Patch (to be ported) » Needs work

Patch is working well, just a couple of details:

+++ b/modules/system/system.install
@@ -236,27 +236,43 @@ function system_requirements($phase) {
       $requirements['settings.php'] = array(
-        'value' => $t('Protected'),
+        'title' => $t('Configuration file'),
+        'value' => $t('Protection disabled'),
+        'severity' => REQUIREMENT_WARNING,
+        'description' => 'The protection is disable by the setting "ignore_site_directory_permissions" in your settings.php',

This bit isn't in the D8 patch, so should it be in this backport?

At any rate, the text has typos: 'disableD', and missing a final full stop.

Also, the text that the D8 patch adds to sites/example.settings.local.php should probably be added to default.settings.php for documentation. That already has other developer-only config examples in it, such as theme_debug.

Lennard Westerveld’s picture

- Variable is now skip_permissions_hardening same as in D8
- Fixed some typo's
- Added variable description to default.settings.php

Lennard Westerveld’s picture

For me is it running nicely in several production websites.
Some people more can review this? instead of reviewing my own patch :)

Rene Bakx’s picture

Been using the #76 patch on development for a few weeks, works like advertised. RTBC for me.

hanoii’s picture

Re-roll for 7.64+

ron_s’s picture

There is an unnecessary extra line break in the install_configure_form changes.

See the attached update to #79.

ron_s’s picture

I'm not having a problem applying the patch in #80... can anyone else confirm if there is an issue?

hkirsman’s picture

The last patch does not work for me on 7.66.

 patch -p1 < 1232572-n80.patch                                        
patching file includes/install.core.inc
patch: **** malformed patch at line 30: diff --git a/modules/system/system.install b/modules/system/system.install

But the #79 does

 patch -p1 < 1232572-n79.patch 
patching file includes/install.core.inc
patching file modules/system/system.install
patching file sites/default/default.settings.php
hanoii’s picture

Let's stick with #79 for now, not sure if an extra line needs a re-roll, but otherwise feel free to send a fixed one.

AaronBauman’s picture

bump

cman9090’s picture

Reroll for 7.78

csmdgl’s picture

Reroll for 7.79

mcdruid’s picture

Reroll of the D7 patch.

It doesn't look like D8/9 have a stanza in default.settings.php for the skip_permissions_hardening setting.

I'm not sure if that's deliberate; if not, I think we need a follow-up to add it to D9.

mcdruid’s picture

Status: Needs review » Needs work

We should backport the changes from #2950851: invalid conf file warnings when skip_permissions_hardening is on to this patch.

mcdruid’s picture

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC - I looked very carefully at the logic for the boolean change, but it makes perfect sense.

mcdruid’s picture

  • mcdruid committed be2c076 on 7.x
    Issue #1232572 by deviantintegral, mcdruid, joachim, cweagans, Lennard...

mcdruid credited alexpott.

mcdruid credited anavarre.

mcdruid’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs change record

Thank you everyone!

This backport could do with a CR for D7.

Status: Fixed » Closed (fixed)

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

Collins405’s picture

This caused me a lot of grief - we have a multisite environment that uses bash scripts to create new instances programmatically by cloning the default folder and editing settings.php. Each time cron ran, it would change settings.php to 444 and stop it from being edited by our script, which relies on it being 644.

The fix for anyone running into an issue with Drupal automatically updating the permissions for settings.php is to set this variable... variable_set('skip_permissions_hardening', TRUE);