Right now Drupal is unusable with filesystem (POSIX) ACLs. It's been broken for years. This was previously #944582: ./sites/default/files directory permission check is incorrect during install AND status report and #1333390: file_prepare_directory() / is_writable() on Linux don't support ACLs which are/were doomed to failure.

Guidelines to reproduce:

  1. build a clean Drupal 8 development environment
  2. In sites/default,
    • chown -R nobody files
    • chmod -R 700 files
    • setfacl -R -m user:apache:rwx files/
    • setfacl -Rd -m user:apache:rwx files/
  3. Install Drupal 8
  4. Add a new Article (uploading an image should fail).

Notes:

  • change apache in step 2. for the username of your httpd server.
  • step 4. needs to be confirmed or an alternative method proposed.
  • last 8.x patch was #4

Comments

Cottser’s picture

Version:7.x-dev» 8.x-dev
Issue tags:+Novice, +Needs manual testing, +needs backport to D7

Thanks @mjorlitzky, at a glance this looks pretty good. This needs to be fixed in 8.x first then backported to 7.x per the backport policy.

The 8.x patch from #944582-56: ./sites/default/files directory permission check is incorrect during install AND status report will need to be rerolled against 8.x minus the installer changes.

Instructions for rerolling patches.

Cottser’s picture

Issue tags:+Needs reroll

Tagging, this would still need a forward port/reroll to apply to 8.x.

joates’s picture

Assigned:Unassigned» joates
Status:Needs review» Needs work

i will reroll this patch against 8.x

juan.brein’s picture

Assigned:joates» juan.brein
Status:Needs work» Needs review
StatusFileSize
new7.6 KB
PASSED: [[SimpleTest]]: [MySQL] 52,391 pass(es).
[ View ]

We worked on this in the Drupal Spring London with Pasive and joates. We basically took the patch provided for D7 in this issue summary and ported it to D8 up to date. Thanks to Joates to guide us through the process!

cam8001’s picture

Status:Needs review» Reviewed & tested by the community

Code looks great!

Damien Tournoud’s picture

Status:Reviewed & tested by the community» Needs work

Many things look wrong in there.

First, the original post mentions that Drupal is broken when ACLs are in use, but doesn't mention why checking the execute bit (which has nothing to do with ACLs) would fix anything. Other then parsing the ACLs manually, the only way to check if you can write a file in a given directory is to try to write it, and that's what #1333390: file_prepare_directory() / is_writable() on Linux don't support ACLs actually tried to do.

Second, using drupal_realpath() without checking the return value is *really wrong*. Remote stream wrappers do not implement ->realpath(), so this is going to return FALSE.

Third, I don't see at all why this is necessary, because both is_executable() and is_writable() are basically wrappers around stat() and as a consequence they *actually work* with stream wrappers (as long as they implement ->stat()).

Finally, I doubt is_executable() on a directory returns what you expect it to return on Windows.

joates’s picture

ty for the feedback.

possible alternative to drupal_realpath() (deprecated) is discussed here..
#1201024: drupal_realpath() should describe when it should be used

[edit] in the case of a remote resource ... [/edit]
returning FALSE is certainly not useful, should probably at least test for this & log an error when it occurs.

juan.brein’s picture

Hi Damien and Joates. Thank you guys for the feedback. As far as I can tell at this point this issue should have never existed? Would you suggest another alterantive or we'll just rely on the fact that drupal does not support ACLs full stop.

Please let me know as I'm new at drupal issues so sincerely I don't know how to proceed from here.

Cheers!

Juan B

Berdir’s picture

From reading Damien's response, I think the first thing to do here is to provide detailed, reproducable steps on how to get Drupal to fail when it should work.

orlitzky’s picture

Steps to reproduce in drupal-7.20 (swap apache with your website user):

  1. Download the drupal tarball, extract it.
  2. Do whatever you need to do in settings.php
  3. In sites/default,
    1. mkdir files
    2. chmod 700 files
    3. setfacl -m user:apache:rwx files/
    4. setfacl -d -m user:apache:rwx files/
  4. Install Drupal (minimal)
  5. Download and install IMCE
  6. Open the file browser from your "My Account" page.

IMCE will fail, claiming that the directory (sites/default/files) is not writable. This is due to file_prepare_directory('public://', FILE_CREATE_DIRECTORY) failing, and is fixed by the patch which modifies file_prepare_directory().

juan.brein’s picture

Thanks, I'll try this on D8 and get back with the results here.

joates’s picture

Assigned:Unassigned» juan.brein
Priority:Normal» Major
Status:Needs review» Needs work
Issue tags:+Novice, +Needs reroll

in response to #10 Posted by mjorlitzky..

it would be more constructive if we can all agree that this is an 8.x issue now,
there is no 8.x release of IMCE

Revised guidelines to re-create the bug:

  1. build a clean Drupal 8 development environment
  2. In sites/default,
    • chown -R nobody files
    • chmod -R 700 files
    • setfacl -R -m user:apache:rwx files/
    • setfacl -Rd -m user:apache:rwx files/
  3. Install Drupal 8
  4. Add a new Article (uploading an image should fail).

Notes:

  • change apache in step 2. for the username of your httpd server.
  • step 4. needs to be confirmed or an alternative method proposed.

[edit]
// 28-Feb-2013
Modified the instructions to implement full excluded file access to the entire files/ sub-tree (user = nobody)
and added recursive ACL permissions as Drupal needs to store file uploads in several sub-folder locations.
[/edit]

orlitzky’s picture

You don't really need IMCE to reproduce the issue in 8.x (if it exists). Just call file_prepare_directory('public://', FILE_CREATE_DIRECTORY) somewhere and see if it works. IMCE is just the reason why this is a complete show-stopper for us.

Damien Tournoud’s picture

Ok, I see what's happening. PHP has a special case for local files in which it uses access(2) or the equivalent Windows call. This supports ACLs. If called on a stream-wrapper, it defaults to a (very complicated) process based on the stat(2) structure returned by the stream-wrapper implementation.

More precisely it checks part of the .mode structure depending on:

  • If the current user (as returned by getuid(2)) is the same as the .uid in the stat, it uses the "user" part of the mode;
  • If current group (as returned by getgid(2)) or the supplementary groups (as returned by getgroups(2) is the same as the .gid in the stat, it uses the "group" part of the mode;
  • Else, it falls back to the "others" part of the mode.

So, one way of moving forward here, would be to modify the stat implementation in our local streamwrapper to fake the mode by calling is_readable(), is_writable() and is_executable() manually, and return the proper structure.

clemens.tolboom’s picture

joates’s picture

Status:Needs review» Needs work

Re #14: thanks Damien for locating what seems to be the root cause of this..

  • removing Novice tag
  • priority normal seems more appropriate
  • need a patch to "modify the stat implementation in our local streamwrapper" ?

from core/lib/Drupal/Core/StreamWrapper/LocalStream.php, line 472

<?php
public function url_stat($uri, $flags) {
 
$this->uri = $uri;
 
$path = $this->getLocalPath();
 
// Suppress warnings if requested or if the file or directory does not
  // exist. This is consistent with PHP's plain filesystem stream wrapper.
 
if ($flags & STREAM_URL_STAT_QUIET || !file_exists($path)) {
    return @
stat($path);
  }
  else {
    return
stat($path);
  }
}
?>
joates’s picture

Assigned:juan.brein» Unassigned
Priority:Major» Normal
Status:Needs work» Needs review
Issue tags:-Novice, -Needs reroll
StatusFileSize
new318.78 KB

in response to #9 and #12..

after setting chmod 700 files
you can clearly see from the terminal that my normal user (joates) is group member but cannot gain access.
getfacl shows the ACL permissions (as described in #12)
(screenshot attachment not working?)

on a fresh install of D8 using default configuration (public:// streamwrapper) i am able to upload an image without error.

joates’s picture

Status:Needs work» Needs review
StatusFileSize
new318.78 KB

try again with the screenshot..

link: http://i.imgur.com/G0s2i99.png

afterwards i realised that having www-data as the directory owner is not too helpful :-p

so i did a chown -R joates:joates files cleared cache and tried again to upload the image, and it did indeed FAIL to upload the file
(which sort of feels like causing it to fail when it *should* fail, but maybe i don't understand ACLs correctly -- does setfacl in the top folder apply to every child ?)
Update: even with recursive ACL the file upload still fails.

link: http://i.imgur.com/FesrnJV.png

joates’s picture

can someone with superior ACL sysadmin skills to me please confirm that it seems that Drupal is not providing support for ACLs ?
(perhaps due to the situation Damien describes in #14)

and maybe change the title of this issue if that is the case :)

Cottser’s picture

Assigned:juan.brein» Unassigned
Priority:Major» Normal
Status:Needs work» Needs review
Issue tags:-Novice, -Needs reroll

@joates - Thanks for all your work on this issue. Could you please update the issue summary with the steps to reproduce, that way others can update them too and you don't have to keep editing your comments :)

joates’s picture

Issue summary:View changes

Updated issue summary.

joates’s picture

ok

orlitzky’s picture

The current steps to reproduce probably won't work I'm afraid. Uploading files—in and of itself—has always worked. The filesystem ACLs do really allow you to write the file, so if you try to do it, it will work.

The problem is when Drupal does its heuristic to determine, "will I be able to write this file if the user tries to do it?" That test is currently broken, so the answer is "no" even though it should be "yes" (thanks to the ACL).

orlitzky’s picture

StatusFileSize
new5.69 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-writable-executable-fix-7.22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Updated patch for 7.22.

Status:Needs review» Needs work

The last submitted patch, drupal-writable-executable-fix-7.22.patch, failed testing.

orlitzky’s picture

StatusFileSize
new5.69 KB

Updated patch for 7.23.

orlitzky’s picture

Issue summary:View changes

Updated issue summary.

orlitzky’s picture

Issue summary:View changes
StatusFileSize
new6.29 KB

Updated patch for 7.24.

orlitzky’s picture

StatusFileSize
new6.26 KB

Updated patch for 7.25.

orlitzky’s picture

The patch for 7.25 applies cleanly to 7.26.

orlitzky’s picture

And to 7.27.

orlitzky’s picture

StatusFileSize
new6.27 KB
PASSED: [[SimpleTest]]: [MySQL] 41,126 pass(es).
[ View ]
YesCT’s picture

Issue summary:View changes
orlitzky’s picture

StatusFileSize
new6.28 KB
PASSED: [[SimpleTest]]: [MySQL] 41,126 pass(es).
[ View ]
orlitzky’s picture

StatusFileSize
new6.28 KB
PASSED: [[SimpleTest]]: [MySQL] 41,125 pass(es).
[ View ]
gold.eagle’s picture

mjorlitzky: Just wanted to say "Thank you". We download this patch regularly, whenever we update Core.

orlitzky’s picture

gold.eagle: glad someone else is making use of it!

orlitzky’s picture

StatusFileSize
new6.28 KB
PASSED: [[SimpleTest]]: [MySQL] 41,128 pass(es).
[ View ]
YesCT’s picture

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

to run the tests ... change the version. (you can change it back)

YesCT’s picture

Status:Needs work» Needs review
YesCT’s picture

Version:7.x-dev» 8.0.x-dev
Status:Needs review» Needs work

back to 8.0.x which needs a patch to work on that.

orlitzky’s picture

StatusFileSize
new6.28 KB
orlitzky’s picture

StatusFileSize
new6.28 KB
orlitzky’s picture

StatusFileSize
new6.28 KB