Run PHP as www-data and alter permissions to allow uploads

[culturedsys]

Per #368 , the current user and permissions set-up is less secure than it could be, because PHP currently has write access to the whole web root. This commit fixes that by ensuring that PHP runs as www-data, rather than web_user, and so does not have write access to the web root. It also changes the permissions on the uploads folder, which we do want PHP to have write access to.

[austinpray]

Thank you for this. Looks like this was introduced in culturedsys@6088df5

[swalkinshaw]

This makes sense and is definitely better for security. Just needs some testing 👍

[QWp6t]

This will probably break WordPress update functionality as well as the plugin installer in the admin panel. We should definitely test that, and if we do accept this, it should coincide with some documentation for how to change permissions to restore this WordPress functionality.

If WordPress core functionality breaks as I'm predicting, then I'm undecided on this PR. Part of me wants to follow security best practices of granting least privilege and establishing secure defaults. To that extent, I'm in favor of accepting this PR. But then there's a pragmatic argument to be made that naive developers/users might grant too many permissions to PHP or WordPress in an attempt to restore functionality. (For example, when WordPress can't update itself, it will prompt administrators to enter FTP credentials, which we probably don't want.) So we might consider making this more hardened approach opt-in instead of opt-out.

[louim]

Unless I'm mistaken, those functionalities are already disabled because we disable filemod in bedrock.

[louim]

But thinking about it, it will break the many plugins that write elsewhere than the upload folder. I don't know if it's a good idea…

[culturedsys]

You're right that the change I am proposing would interfere with plugins that want to write files outside of the uploads folder. What I've been doing myself in that situation is adding more entries to project_shared_children so that whatever directories the plugin needs get created with the right permissions at deploy time, but I can see that might be too low-level a solution make the default.

Quite often, plugins that need to write files outside of the upload folder recommend that users temporarily give PHP write permissions to the wp-content (or, in Bedrock's case, app) directory, and then remove those permissions after the plugin has been activated and created whatever files or directories it needs. Perhaps this process could be automated, say with two additional ansible roles, one to relax permissions and one to tighten them up again. However, it might be difficult to get the tighten permissions role right; it couldn't just remove write permissions from the whole of the app folder, because of the case where a plugin creates a folder which it needs continued write permissions to (say, a cache folder).

[louim]

It could also be implemented as a boolean, disabled by default. Calling it harden or something like that. People who who want the most secure environment and know they won't install plugins that needs write permissions outside the uploads folder could enable it. Like the cache setting which isn't on by default, because it may not fit for everyone.

[swalkinshaw]

Looking into this again...

@kulturavashchi @louim could you provide some examples of plugins that write files outside of uploads? And when/how they write them?

I know there's plugins like Batcache where you manually need to copy files but that wouldn't be changed with this PR.

[ismael-benitez]

Maybe you can get some errors with SEO plugins. E.g.: When it is generating the sitemap.xml or robots.txt.

[mindfullsilence]

EWWW Image Optimizer create a new folder in wp-content.
WP Total Cache creates a new folder in wp-content
Many redirect plugins write to htaccess, even WP core writes to htaccess under certain circumstances
As mentioned above, YEOST and others write to robots/sitemap
Countless favicon generator plugins write favicon icons to the root dir, and if they write favicons elsewhere, they have to be able to write a manifest file to the root. (see real favicon generator as example).
Can't think of others, but the above listed plugins are fairly heavily used in the community.

Personally I'm for @louim's suggestion of adding it as a configuration. Or add configuration options for creating exceptions to the hardening. Or both. Definitely foresee some pretty rough repercussions with setting it as default with no options to avoid hardening.

[zessx]

FYI, I've used hardened file permissions with Trellis for years without major issues. You may have to setup additional shared folders depending on the plugins you're using, but that's no big deal.

That said, we noticed an unexpected side-effect that you should be aware of if you're changing PHP FPM user for www-data: as Wordpress WP-Cron is disabled, and your server running a real cron with the web user, files override still can happens. To avoid this, you'll have to change the cron user:

# roles/wordpress-setup/tasks/main.yml
# […]

- name: Setup WP system cron
  cron:
    name: "{{ item.key }} WordPress cron"
    minute: "{{ item.value.cron_interval | default('*/15') }}"
    user: "{{ web_group }}"
    job: "cd {{ www_root }}/{{ item.key }}/{{ item.value.current_path | default('current') }} && wp cron event run --due-now > /dev/null 2>&1"
    cron_file: "wordpress-{{ item.key | replace('.', '_') }}"
    state: "{{ (cron_enabled and not item.value.multisite.enabled) | ternary('present', 'absent') }}"
  with_dict: "{{ wordpress_sites }}"

- name: Setup WP Multisite system cron
  cron:
    name: "{{ item.key }} WordPress network cron"
    minute: "{{ item.value.cron_interval_multisite | default('*/30') }}"
    user: "{{ web_group }}"
    job: "cd {{ www_root }}/{{ item.key }}/{{ item.value.current_path | default('current') }} && wp site list --field=url | xargs -n1 -I \\% wp --url=\\% cron event run --due-now > /dev/null 2>&1"
    cron_file: "wordpress-multisite-{{ item.key | replace('.', '_') }}"
    state: "{{ (cron_enabled and item.value.multisite.enabled) | ternary('present', 'absent') }}"
  with_dict: "{{ wordpress_sites }}"