Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[25.0 backport] daemon: overlay2: remove world writable permission from the lower file #47513

Merged
merged 1 commit into from Mar 6, 2024

Conversation

vvoland
Copy link
Contributor

@vvoland vvoland commented Mar 6, 2024

In de2447c, the creation of the 'lower' file was changed from using os.Create to using ioutils.AtomicWriteFile, which ignores the system's umask. This means that even though the requested permission in the source code was always 0666, it was 0644 on systems with default umask of 0022 prior to de2447c, so the move to AtomicFile potentially increased the file's permissions.

This is not a security issue because the parent directory does not allow writes into the file, but it can confuse security scanners on Linux-based systems into giving false positives.

Reproduction steps:

  1. Install a docker version that contains de2447c (I used docker-24.0.7_ce).
  2. In one terminal start any container (I used alpine/sh for simplicity).
  3. In a second terminal, execute:
germ85:~ # find /var/lib/docker/overlay2/ -type f -perm -222 -ls
   563855      4 -rw-rw-rw-   1 root     root           28 Mar  5 13:58 /var/lib/docker/overlay2/faeafe7e6f15289f0825aeffa2597f0b7ecf5807d7c21d5a9a2d3383090693b7-init/lower
   563878      4 -rw-rw-rw-   1 root     root           57 Mar  5 13:58 /var/lib/docker/overlay2/faeafe7e6f15289f0825aeffa2597f0b7ecf5807d7c21d5a9a2d3383090693b7/lower

With this patch added to the same docker version (24.0.7_ce):

germ85:~ # find /var/lib/docker/overlay2/ -type f -ls | grep lower
   564922      4 -rw-r--r--   1 root     root           28 Mar  5 14:46 /var/lib/docker/overlay2/488b492ef97cfdaa2a531d9e57aa5bd32425ca6bd9154b985eaed89a4fb00dc5-init/lower
   564945      4 -rw-r--r--   1 root     root           57 Mar  5 14:46 /var/lib/docker/overlay2/488b492ef97cfdaa2a531d9e57aa5bd32425ca6bd9154b985eaed89a4fb00dc5/lower

Output from the same system with docker-24.0.5_ce-150000.185.1 (before de2447c was added) installed shows the same behavior as 27.0.7_ce with this patch:

germ85:~ # rpm -qa docker
docker-24.0.5_ce-150000.185.1.x86_64
germ85:~ # find /var/lib/docker/overlay2/ -type f -ls | grep lower
   565976      4 -rw-r--r--   1 root     root           28 Mar  5 14:51 /var/lib/docker/overlay2/d16828005bf086a36eaf4d5d3d4822f652b34bfb0e3323b9c1173dae0d0418e7-init/lower
   565999      4 -rw-r--r--   1 root     root           57 Mar  5 14:51 /var/lib/docker/overlay2/d16828005bf086a36eaf4d5d3d4822f652b34bfb0e3323b9c1173dae0d0418e7/lower

The patch from this PR restores the behavior from 24.0.5_ce where the lower files were not world writable.

In de2447c, the creation of the 'lower' file was changed from using
os.Create to using ioutils.AtomicWriteFile, which ignores the system's
umask. This means that even though the requested permission in the
source code was always 0666, it was 0644 on systems with default
umask of 0022 prior to de2447c, so the move to AtomicFile potentially
increased the file's permissions.

This is not a security issue because the parent directory does not
allow writes into the file, but it can confuse security scanners on
Linux-based systems into giving false positives.

Signed-off-by: Jaroslav Jindrak <dzejrou@gmail.com>
(cherry picked from commit cadb124)
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@vvoland vvoland added this to the 25.0.4 milestone Mar 6, 2024
@vvoland vvoland self-assigned this Mar 6, 2024
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vvoland vvoland merged commit 061aa95 into moby:25.0 Mar 6, 2024
125 of 126 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants