search

serversideup / docker-php

🐳 Production-ready Docker images for PHP. Optimized for Laravel, WordPress, and more!
https://serversideup.net/open-source/docker-php/
GNU General Public License v3.0
1.65k stars 108 forks source link

Configure permissions on /run directory for compatibility with cloud providers with restricted container permissions #376

Closed hajekj closed 2 months ago

hajekj commented 2 months ago

Fixes #372, #360

jaydrogers commented 2 months ago

I finally had a chance to take a look at this. Thanks for your patience!

Looking at the suggestions, I'm worried this could open up some security issues.

I ran it through ChatGPT just as a smoke test, and it came back with the same thoughts: https://chatgpt.com/share/3e1a31af-d312-4df1-8775-45125aebcbf9

How confident are you in the security implications of this change?

hajekj commented 2 months ago

I have based this change based on the permissions which are used when I start the container in my own local Docker on the official serversideup/php:8.1-fpm-apache image:

$ ls -l /
total 80
lrwxrwxrwx   1 root     root         7 May 13 00:00 bin -> usr/bin
drwxr-xr-x   2 root     root      4096 Jan 28 21:20 boot
drwxr-xr-x   2 root     root     12288 Nov 20  2023 command
drwxr-xr-x   2 www-data www-data  4096 May 17 17:08 composer
drwxr-xr-x   5 root     root       340 Jun 19 07:13 dev
drwxr-xr-x   1 root     root      4096 Jun 19 07:13 etc
drwxr-xr-x   2 root     root      4096 Jan 28 21:20 home
-rwxr-xr-x   1 root     root      1012 Nov 20  2023 init
lrwxrwxrwx   1 root     root         7 May 13 00:00 lib -> usr/lib
lrwxrwxrwx   1 root     root         9 May 13 00:00 lib64 -> usr/lib64
drwxr-xr-x   2 root     root      4096 May 13 00:00 media
drwxr-xr-x   2 root     root      4096 May 13 00:00 mnt
drwxr-xr-x   2 root     root      4096 May 13 00:00 opt
drwxr-xr-x   6 root     root      4096 Nov 20  2023 package
dr-xr-xr-x 310 root     root         0 Jun 19 07:13 proc
drwx------   1 root     root      4096 May 14 13:47 root
drwxr-xr-x   1 www-data www-data  4096 Jun 19 07:13 run
lrwxrwxrwx   1 root     root         8 May 13 00:00 sbin -> usr/sbin
drwxr-xr-x   2 root     root      4096 May 13 00:00 srv
dr-xr-xr-x  11 root     root         0 Jun 19 07:13 sys
drwxrwxrwt   1 root     root      4096 May 14 13:47 tmp
drwxr-xr-x   1 root     root      4096 May 17 17:08 usr
drwxr-xr-x   1 root     root      4096 May 14 12:35 var

The permission which I set here manually are actually also set by the s6-overlay, specifically here, through s6-overlay-suexec. However since the cloud containers run with the no_new_priv flag (and there is no way to change this), the above will always fail unless the permissions are set.

Since s6 sets the permissions like this anyways, I don't see any issue with setting them in the container image beforehand, since the result will be the same anyways, and any security issue which this would introduce is already there anyways, due to s6 doing the same.

jaydrogers commented 2 months ago

Thanks for the extra info. I am still confused on what is causing this error and why running chown -R www-data:www-data /run fixes it.

My approach

I am being extra cautious here because I want everything to follow the LEAST privilege standard.

Current permissions of /run

They current are set to www-data:

www-data@09b95a829fe3:/$ ls -al /run
total 36
drwxr-xr-x 1 www-data    www-data    4096 Jun 19 16:29 .
drwxr-xr-x 1 root        root        4096 Jun 19 16:28 ..
-rw-r--r-- 1 root        root           0 Jun 19 14:42 adduser
drwxr-x--- 2 Debian-exim Debian-exim 4096 Jun 19 14:42 exim4
drwxrwxrwt 2 root        root        4096 Apr 23 15:00 lock
-rw-r--r-- 1 www-data    www-data       4 Jun 19 16:29 nginx.pid
drwxr-xr-x 5 www-data    www-data    4096 Jun 19 16:29 s6
drwxr-xr-x 2 www-data    www-data    4096 Jun 19 16:28 s6-linux-init-container-results
lrwxrwxrwx 1 www-data    www-data      23 Jun 19 16:28 s6-rc -> s6-rc:s6-rc-init:NCjClN
drwxr-xr-x 3 www-data    www-data    4096 Jun 19 16:29 s6-rc:s6-rc-init:NCjClN
drwxr-xr-x 4 www-data    www-data    4096 Jun 19 16:28 service

Looking at the error more

s6-rmrf: fatal: unable to remove /run/s6: Permission denied

I checked /run/s6, which also had www-data permissions:

www-data@09b95a829fe3:/$ ls -al /run/s6
total 24
drwxr-xr-x 5 www-data www-data 4096 Jun 19 16:29 .
drwxr-xr-x 1 www-data www-data 4096 Jun 19 16:29 ..
drwxr-xr-x 7 www-data www-data 4096 Jun 19 16:28 basedir
drwxr-xr-x 3 www-data www-data 4096 Jun 19 16:28 db
drwxr-xr-x 2 www-data www-data 4096 Jun 19 16:29 legacy-services
-rw-r--r-- 1 www-data www-data   14 Jun 19 16:28 workdir

The official docs of s6-overlay-suexec

I read the official docs and they point out something interesting:

s6-overlay-suexec

This is a small program that can only be run as pid 1; it ensures that a given block of code is always run as root, even if the container is run with the USER directive. After the root block exits, if the USER directive has been given, privileges are dropped again and the rest of the execution proceeds as a normal user.

This is used in s6-overlay to make sure that a portion of the filesystem hierarchy always exists and is owned by the current user.

This binary needs the suid bit enabled so it can gain privileges to execute its argument as root. Since it can only be run as part of the pid 1 process chain, it does not endanger the security of the system; nevertheless, please audit the code until you have full confidence that it is secure. I was pointed to the docs of s6-overlay-suexec

I checked the permissions and it has the suid bit set:

www-data@09b95a829fe3:/$ ls -al /package/admin/s6-overlay-helpers/command
total 76
drwxr-xr-x 2 root root  4096 Nov 20  2023 .
drwxr-xr-x 3 root root  4096 Nov 20  2023 ..
-rwsr-xr-x 1 root root 66624 Nov 20  2023 s6-overlay-suexec

The only thing I see there is that command is owned by root.

Moving forward

I am hesitant to just a chown -R and chmod -R on such an important directory. I feel it gets into a grey area of security.

In the past, I've had a lot of issues with cloud providers on how they run containers. I opened an issue and the creator of S6 Overlay suggested they changed their process: https://github.com/just-containers/s6-overlay/issues/535#issuecomment-1597680218

You can see that here in other discussions specifically related to Azure.

https://learn.microsoft.com/en-us/answers/questions/1153255/s6-overlay-support-in-container-instances

The reason why I'm hesitant on all this stuff is the containers work fine with Docker on every other machine until it hits a PaaS. That's where my gut feeling is it might be something in the PaaS that needs to be configured so it's not causing issues.

Are there options in Azure that can allow S6 Overlay to run? If so, I'd rather document how to configure the PaaS to allow this then give more privileges to an unprivileged user for supporting a certain PaaS.

Thanks for your help on this!

jaydrogers commented 2 months ago

👉 Update: Another thing to try

I just upgraded the S6 Overlay version to v3.2.0.0. Looking at the release notes, they made some changes that could affect what we're trying to resolve.

When this will be available

I have a job building our dev images now: https://github.com/serversideup/docker-php/actions/runs/9585794967

Images to test

Once that job is complete, the images will be made available on our development repo: https://hub.docker.com/r/serversideup/php-dev

Next steps

Test with the latest serversideup/php-dev images and see if that fixes the issue.

hajekj commented 2 months ago

Thanks for the update.

The images with latest s6-overlay fail with:

2024-06-19T17:58:40.736930557Z /package/admin/s6-overlay/libexec/preinit: info: container permissions: uid=33 (www-data), euid=33, gid=33 (www-data), egid=33
2024-06-19T17:58:40.736954767Z /package/admin/s6-overlay/libexec/preinit: info: /run permissions: uid=0 (root), gid=0 (root), perms=oxorgxgruxuwur
2024-06-19T17:58:40.736958490Z /package/admin/s6-overlay/libexec/preinit: fatal: /run belongs to uid 0 instead of 33 and we're lacking the privileges to fix it.
2024-06-19T17:58:40.736935796Z s6-overlay-suexec: fatal: child failed with exit code 100

So it is basically giving the same error, just being a bit more verbose about it.

The reason is that the suid bit works (hence the permissions get overriden by the s6-overlay-suexec). Despites the suid bit being set, the PaaS platforms have it disabled for security - this is a Docker configuration option - no_new_priv which is part of security best practices and prevents privilege escalation. With PaaS platforms (not just Azure), there is no way to change this behavior unfortunately.

I am hesitant to just a chown -R and chmod -R on such an important directory. I feel it gets into a grey area of security.

As you can see, from the ls -l / command, it gets executed on the directory anyways by s6. So I don't think it will make much of a difference.

jaydrogers commented 2 months ago

I just pushed up changes, but let's try it with this only:

RUN chown -R www-data:www-data /run

Once the GitHub Actions completes on this run, give it a try again.

I have a bug in my prefixing of the PR number, to the images are being pushed without a prefix.

Example

docker.io/serversideup/php-dev:8.1.29-fpm-alpine  
docker.io/serversideup/php-dev:8.1-fpm-alpine

Once this job completes if you can try again, that would be greatly appreciated.

I tried to set up Azure Containers myself but I got extremely lost in Azure's UI 😆

hajekj commented 2 months ago

I just looked into the latest published image - https://hub.docker.com/layers/serversideup/php-dev/8.0-fpm-apache/images/sha256-b12f7eb7e1e118fcd01e65ebeb9e608aa7ce24d74b69c2272324f6519727f998?context=explore but the build doesn't seem to contain the changes - it appears to be building the main branch.

jaydrogers commented 2 months ago

I need to move this into my own branch and will open a new PR for you to test (I'm in GitHub Actions Permissions Hell right now 😆)

TheAndrey commented 6 days ago

I ran into the same problem with images where the s6 overlay is present.

The problem is that changing the ID of an existing user using docker-php-serversideup-set-id is a ugly hack. After changing the ID, you need to update all user rights to files and directories in order to overwrite the previous UID/GID stored in the file system. For this reason, in order for access to files from the container and the host to work on both sides, it is enough that the UID and GID match, the names of users and groups may differ.

I am still confused on what is causing this error and why running chown -R www-data:www-data /run fixes it.

This updates the user ID to the current ones. You need to use ls with the -n flag to see the ID instead of the user's display name.

The script docker-php-serversideup-set-id can be improved by replacing the old ID with a new one throughout the file system (ex. find / -uid $OLD_ID -gid $OLD_ID). Then there will be no need for script docker-php-serversideup-set-file-permissions.

jaydrogers commented 6 days ago

Thanks for the feedback! I would agree there is room for improvement on that process as I was trying to prove a concept at the time of writing it. I was just glad I was able to get it to work.

If you'd like to put together some suggestions for improvement, I would love to explore them: https://github.com/serversideup/docker-php/discussions/66