I am going to demonstrate this problem by looking at the case of a system of a settings.php set to 0700.

The function call is:
drupal_verify_install_file($settings_file, FILE_EXIST|FILE_READABLE|FILE_NOT_WRITABLE)

drupal_install_fix_file($file, $mask) is then called up to 3 times, depending on the status checks for the 3 masks above.

Now, assuming the webserver runs as the user that owns the file, the only permission change that will be triggered is FILE_NOT_WRITABLE (since 0700 is writable, and hence we want to fix that).

So lets trace the code:

  $mod = substr(sprintf('%o', fileperms($file)), -4); // Get the file permissions
  $prefix = substr($mod, 0, 1); // Strip the initial 0
  $mod = substr($mod, 1 ,4); // Leaving 777 (string, and valid decimal integer)
  $masks = array(FILE_READABLE, FILE_WRITABLE, FILE_EXECUTABLE, FILE_NOT_READABLE, FILE_NOT_WRITABLE, FILE_NOT_EXECUTABLE);
  foreach ($masks as $m) {
    if ($mask & $m) {
      switch ($m) {
// snip
        case FILE_NOT_WRITABLE:
          if (is_writable($file)) {
            $mod -= 222; // 700 - 222 = 478 (meaningless number for a chmod, and invalid octal!)
          }
          break;
// snip
  if (@chmod($file, intval("$prefix$mod", 8))) // intval(0478, 8) tries to convert 0478 into octal

At this last line things really go bork, since 0478 is impossible to convert into octal. As described in the PHP documentation the invalid digits are simply stripped. This leaves us a chmod that does something entirely different, and breaks the site.

I think the underlying problem here is that the way the code is trying to apply the mask to each digit is completely faulty. Calculations like 700 - 222 are never going to produce the right results (even when it ends up as valid octal!). The faulty assumption is that files masks only ever look like 222, 333, 666 etc, which is just not the case!

My proposed fix is that instead of treating the mask as a single number (which has digits that carry values into different columns, breaking things), we treat each digit separately. This means we would do 7-2=5, 0-2=0, 0-2=0 (-ve is set to 0), giving is a correct value of 500.
Possibly there is a php function that might make all this simpler (any inspiration?), or else it would be easy to write a helper function to take care of the arithmetic.
I haven't given this a whole lot of thought however - and I suspect that there is a good chance that if we think about the problem differently we might rewrite this in a much cleaner and simpler way.

The other big problem with this code is that there is practically no comments explaining what it is trying to do (the same goes for drupal_verify_install_file, and maybe other similar functions too). This problem might be worthy of a separate issue.

This is some quite obtuse, undocumented code - so I may have totally misunderstood how it works - corrections are welcome!

CommentFileSizeAuthor
#9 install_perms_2.patch.txt2.92 KBOwen Barton
#7 install_perms_1.patch2.34 KBChrisKennedy
#6 install_perms_0.patch6.21 KBChrisKennedy
#3 install_perms.patch5.54 KBChrisKennedy
#2 install_21.patch1.75 KBdouggreen
#1 install_20.patch1.14 KBdouggreen
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

douggreen’s picture

douggreen CreditAttribution: douggreen commented
FileSize
install_20.patch1.14 KB

I believe that the correct way to change this is with a logical operator (not arithmetic) as in:

            $mod &= ~0222;

instead of 

            $mod -= 222;

See attached patch. For using the logical operator on all such mod's. I tested on a file with 0700 perms and it correctly set it to 0500.

douggreen’s picture

douggreen CreditAttribution: douggreen commented
FileSize
install_21.patch1.75 KB

I humbly defer to grugnog's comments -- maybe we need to figure out what is needed here first. But I do want to update with a different patch. There were some problems with my last patch using decimal values.

This patch will change the settings.php from 0600 to 0444, still not exactly what we'd like, but what I perceive the developer was trying to do. It would be better in a secure environment if Drupal would change 0600 to 0400, but then the problem arises, did someone accidentally set it to 0600 and thus not have permissions that are desired?

How do we accomodate both (a) novice users who have messed up the permissions, and (b) experienced admins who have intentionally set tight permissions? I would think that we should take into account the process user and group against the file's owner and group and give access only to one of the owner/group/other based on the most powerful permission available.

Sorry if I'm late to the game? Has there been a larger discussion of this somewhere?

ChrisKennedy’s picture

ChrisKennedy CreditAttribution: ChrisKennedy commented
Status: Active » Needs review
FileSize
install_perms.patch5.54 KB

You must not have tested the patch. For one, there's a syntax error on the last modified line (extra parenthesis). Also, this won't result in a settings.php original set to 600 being changed to 444 as you claim - no call is ever made to set settings.php to FILE_READABLE because if settings.php is unreadable drupal will give an error (bootstrap.inc, line 163). And because the 444 permissions are only set if is_readable fails, a file set to 400 won't have any read bits modified.

In cases where we are expanding permissions (FILE_READABLE, FILE_WRITABLE, and FILE_EXECUTABLE) we technically only need to modify the owner permission, because chmod() will only work if the web server is running as the owner. However, just to be safe we can leave the full access changes - the administrator may have only changed the owner of settings.php to the web server for the duration of the installation process.

This patch modifies bootstrap.inc to not assume that settings.php is readable, and attempts to set the readable bit for settings.php if it's unreadable. It displays a correct error message if no settings.php file exists or if the file exists but isn't readable - previously there would be several warning messages and then an error about an unsupported database type. It fixes drupal_verify_install_file() to check that the file exists before attempting to check any of the other masks - before it resulted in a php stat error in drupal_install_fix_file() because fileperms() requires the file to exist.

douggreen’s picture

douggreen CreditAttribution: douggreen commented

Chris, you're right on the extra parenthesis and improper comments. Both our patches change a 0700 settings.php to 0500 and a 0600 settings.php to 0400. Please ignore my late night ramblings about owner/user/group (as I'm sure you already have). I should stick to only submitting patches in the morning when I'm awake!

Dries’s picture

Dries CreditAttribution: Dries commented

I haven't carefully reviewed the patches, but from an aesthetic point of view, doug's patch is cleaner. Chris, are we sure we can't use Doug's approach?

Also, Chris has written down a great analysis of what the code is supposed to do. I think some of that information should make its way into the code comments and/or PHPdoc. It looks like it might help other developers understand what is going on.

ChrisKennedy’s picture

ChrisKennedy CreditAttribution: ChrisKennedy commented
FileSize
install_perms_0.patch6.21 KB

To clarify: my patch uses exactly the same code as douggreen's in drupal_install_fix_file() because the code works great. In my testing of his patch I was able to find various bugs that could be fixed, which are included in my patch.

The attached patch adds more documentation to describe things.

ChrisKennedy’s picture

ChrisKennedy CreditAttribution: ChrisKennedy commented
FileSize
install_perms_1.patch2.34 KB

New patch with just Peter's improvements and the relevant comments. The other part of the patch will be moved to http://drupal.org/node/100476

Owen Barton’s picture

Owen Barton CreditAttribution: Owen Barton commented

This looks (and works!) much better than the original code - nice work guys!

I have added some more documentation to the function header explaining the general approach, and why this approach was taken.

 * Attempt to fix file permissions.
 *
 * The general approach here is that, because we do not know the security setup
 * of the webserver, we apply our permission changes to all three digits of the
 * file permission (i.e. user, group and all).
 * 
 * To ensure that the values behave as expected (and numbers don't carry from
 * one digit to the next) we do the calculation on the octal value using bitwise
 * operations. This lets us remove, for example, 0222 from 0700 and get the correct
 * value of 0500.

One thing that did occur to me - is that we should test this out to make sure we understand what happens when there is a value on the initial digit (i.e. sticky, suid or sgid perms). Are there any implications if this value is stripped off?

Owen Barton’s picture

Owen Barton CreditAttribution: Owen Barton commented
FileSize
install_perms_2.patch.txt2.92 KB

Oh - and the patch!

Egon Bianchet’s picture

Egon Bianchet CreditAttribution: Egon Bianchet commented

Tested with success, it works as expected

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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