Tighten permissions on created directory by rhatdan · Pull Request #743 · containers/storage

rhatdan · 2020-10-12T12:30:21Z

The mount points of containers are being marked as 755 where
a lot of images now come with the default for "/", as 555.

The tighter controls on / means that a root process can not
modify the / directory without DAC_OVERRIDE. And some consider
this better security. Bottom line is tools like rpm -qVf /
are reporting failures on permissions on / in lots of RPM
based images.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1854566

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

rhatdan · 2020-10-12T12:31:20Z

@nalind @mtrmac @vrothberg @rhvgoyal @saschagrunert PTAL
I am not sure if I went too far.

mtrmac · 2020-10-12T12:47:13Z

Conceptually, I would expect the permissions of the / directory, as visible from inside the container, to come from the permissions set in the corresponding layer tarball — and with a trivial test with a loopback-mounted filesystem, the permissions of the mount point are indeed set from the mounted filesystem, not from the parent.

So I don’t understand how the PR description and the code really relates; at best changing one hard-coded value for another replaces one set of images with non-matching permissions with another set. With my very minimal understanding of overlay, are all of those directories even container-observable? I thought that the permissions of mount points before the mount happens basically don’t matter.

Either way, I was expecting some way to apply the permissions from the layer tarball to the root of the extracted tree (or, even, some code that removes something that prevents such permission application, which should be the default behavior).

This may well be correct but it is surprising to me at a first glance, and I don’t know enough about overlay or c/storage to immediately say one way or another — I’ll defer to experts.

giuseppe · 2020-10-13T16:08:50Z

agree with @mtrmac. We should probably set the same permissions as the rootfs in the image, otherwise switching a hardcoded value for another will silent the rpm warning but can introduce other regressions. As you pointed out, running without CAP_DAC_OVERRIDE and not being able to create dir/files in the root directory.

Reinstalling filesystem from the container fixes the issue with /, but there are other errors reported by rpm -qV, such as /proc and /sys having the wrong ownership, that cannot be fixed from a user namespace.
While we can think of fixing the root permissions to follow what the rootfs tarball specifies, I think these other issues must be addressed in the rpm tool itself and make it aware of running in a container

rhatdan · 2020-10-13T17:28:51Z

@nalind Didn't we talk about this a while ago, and figured out there was an issue in / of the container.

nalind · 2020-10-13T18:30:00Z

The root directory of a layer is frequently omitted from layer blobs in images (for example, docker.io/library/fedora), though the one in the bug report appears to be an exception to that. In the case of overlay, if I'm reading the docs right, the permissions on the upper ("diff") directory are the ones that are visible for the mountpoint.

nalind · 2020-10-13T18:30:19Z

drivers/overlay/overlay.go

+		_ = idtools.MkdirAs(lower1Dir, 0500, rootUID, rootGID)
+		_ = idtools.MkdirAs(lower2Dir, 0500, rootUID, rootGID)
+		_ = idtools.MkdirAs(upperDir, 0500, rootUID, rootGID)
+		_ = idtools.MkdirAs(workDir, 0500, rootUID, rootGID)


This is run-time feature test code. We delete these directories when the test is finished. What's the purpose of this hunk?

Just globally changed the permissions.

nalind · 2020-10-13T18:43:26Z

As an alternate approach that I haven't tested, does defaulting the permissions of the "diff" directory for a layer to the permissions of its parent layer's "diff" directory, if it has one, provide the desired result? I'd expect that to bring us more in line the device mapper driver.

rhatdan · 2020-10-29T19:55:36Z

@giuseppe @nalind @mtrmac PTAL, I think I got it correctly, Tested this with Podman and it seems to work correctly.

The mount points of containers are being marked as 755 where a lot of images now come with the default for "/", as 555. The tighter controls on / means that a root process can not modify the / directory without DAC_OVERRIDE. And some consider this better security. Bottom line is tools like `rpm -qVf /` are reporting failures on permissions on / in lots of RPM based images. Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

rhatdan · 2020-10-29T20:04:49Z

@giuseppe @mtrmac PTAL

rhatdan · 2020-10-29T20:07:40Z

Nalin points out that most/alot of container images do not come with ./ inside of them.

IE the tar ball contains. (Fedora comes this way)
tmp
usr
var
as opposed to (UBI8 comes this way)
./tmp
./usr
./var

For these containers need a safe default. Fedora, Centos, RHEL have decided that the safe default for /, is 555. So we should go with that.

This PR attempts to look at the parent directory or the directory of lower layers and use it.

vrothberg

LGTM ... Ubuntu is sick atm

rhatdan · 2020-10-30T09:48:50Z

@giuseppe @vrothberg Please /approve this PR so testing will happen

giuseppe · 2020-10-30T12:42:52Z

/approve

mtrmac · 2020-11-02T19:11:02Z

drivers/overlay/overlay.go

 		if err != nil {
 			return err
 		}
+		perms = os.FileMode(st.Mode())


In all instances: AFAICS st.Mode() is already an os.FileMode, so the cast is unnecessary; OTOH the value also includes the file type bits.

So AFAICS this should be perms = st.Mode().Perms().

if I pass st.Mode() I get this error.

make GO111MODULE=on go build -mod=vendor -compiler gc -tags " " ./cmd/containers-storage # github.com/containers/storage/drivers/overlay drivers/overlay/overlay.go:582:9: cannot use st.Mode() (type uint32) as type os.FileMode in assignment make: *** [Makefile:53: containers-storage] Error 2

Which indicates that st.Mode() has no value of perms and needs the cast.

Note this is a system.Stat not an os.Stat.

Ah, my mistake.

A change introduced by [1] makes it impossible to create a directory entry right under container's root directory, so the test case "should enforce a permissive profile" fails. For the purpose of this test it does not matter where we create the file, so let's do it under /tmp. [1] containers/storage#743 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

A change introduced by [1] makes it impossible to create a directory entry right under container's root directory, so the test case "should enforce a permissive profile" fails. For the purpose of this test it does not matter where we create the file, so let's do it under /tmp. [1] containers/storage#743 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> (cherry picked from commit 956463e) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

cgwalters · 2026-01-09T18:24:33Z

I know this is an old PR but all the relevant people are here, and this is now biting me. It took me quite a while to understand that basically container runtimes have always ignored / in the tar stream (if it exists, which per this PR often doesn't, but sometimes it does).

AI-generated summary in https://gist.github.com/cgwalters/c18c9337aa9345d763aa446cc95c7847 (but of course with a lot of guidance and tweaking from me)

I am probably going to work around this in composefs by setting the metadata for / to equal /usr...probably no one is going to ever care, but it sure is ugly.

OCI container layer tars often don't include a root directory entry, and when they do, container runtimes (Podman, Docker) ignore it and use hardcoded defaults. This makes root metadata non-deterministic, causing composefs digest mismatches. For detailed analysis of this issue, see: https://gist.github.com/cgwalters/c18c9337aa9345d763aa446cc95c7847 containers/storage#743 Since the metadata of `/` is ill-defined in the OCI spec, we default to copying it from `/usr` - no one should care if these differ, except for SELinux labels which we handle separately in the boot path. Add new APIs: - copy_root_metadata_from_usr(): copies mode, uid, gid, mtime, xattrs from /usr to root directory - read_container_root(): wrapper that calls read_filesystem() then copy_root_metadata_from_usr() Remove the incomplete have_root_stat/ensure_root_stat() mechanism which only handled mtime, not permissions or other metadata. Update cfsctl commands (compute-id, create-image, create-dumpfile) to use read_container_root() by default. Add --no-propagate-usr-to-root flag to use raw read_filesystem() when root metadata is well-defined. Assisted-by: OpenCode (Opus 4.5)

OCI container layer tars often don't include a root directory entry, and when they do, container runtimes (Podman, Docker) ignore it and use hardcoded defaults. This makes root metadata non-deterministic, causing composefs digest mismatches. For detailed analysis of this issue, see: https://gist.github.com/cgwalters/c18c9337aa9345d763aa446cc95c7847 containers/storage#743 Since the metadata of `/` is ill-defined in the OCI spec, we default to copying it from `/usr` - no one should care if these differ, except for SELinux labels which we handle separately in the boot path. Add new APIs: - copy_root_metadata_from_usr(): copies mode, uid, gid, mtime, xattrs from /usr to root directory - read_container_root(): wrapper that calls read_filesystem() then copy_root_metadata_from_usr() Remove the incomplete have_root_stat/ensure_root_stat() mechanism which only handled mtime, not permissions or other metadata. Update cfsctl commands (compute-id, create-image, create-dumpfile) to use read_container_root() by default. Add --no-propagate-usr-to-root flag to use raw read_filesystem() when root metadata is well-defined. Assisted-by: OpenCode (Opus 4.5) Signed-off-by: Colin Walters <walters@verbum.org>

nalind reviewed Oct 13, 2020

View reviewed changes

rhatdan force-pushed the perms branch 2 times, most recently from 54cca22 to 6d33a94 Compare October 29, 2020 19:48

rhatdan force-pushed the perms branch 2 times, most recently from 3109c8c to ee02f73 Compare October 29, 2020 20:00

rhatdan force-pushed the perms branch from ee02f73 to 7b157ac Compare October 29, 2020 20:02

vrothberg reviewed Oct 30, 2020

View reviewed changes

Merge branch 'master' into perms

5d577c8

rhatdan merged commit 3223e3e into containers:master Oct 30, 2020

mtrmac reviewed Nov 2, 2020

View reviewed changes

kolyshkin mentioned this pull request Nov 11, 2020

vendor: bump containers/storage to v1.24.0 cri-o/cri-o#4334

Merged

This was referenced Nov 12, 2020

pkg/validate/apparmor: fix path kubernetes-sigs/cri-tools#682

Merged

[DO NOT MERGE] testing apparmor critest fix cri-o/cri-o#4366

Closed

nalind mentioned this pull request Jun 8, 2021

The mergedDir has different permission mode on two hosts #937

Closed

cgwalters mentioned this pull request Jan 9, 2026

build-sys: Rework sealing to be one build step bootc-dev/bootc#1898

Merged

Conversation

rhatdan commented Oct 12, 2020

Uh oh!

rhatdan commented Oct 12, 2020

Uh oh!

mtrmac commented Oct 12, 2020

Uh oh!

giuseppe commented Oct 13, 2020

Uh oh!

rhatdan commented Oct 13, 2020

Uh oh!

nalind commented Oct 13, 2020

Uh oh!

nalind Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

rhatdan Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

nalind commented Oct 13, 2020

Uh oh!

rhatdan commented Oct 29, 2020

Uh oh!

rhatdan commented Oct 29, 2020

Uh oh!

rhatdan commented Oct 29, 2020

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Oct 30, 2020

Uh oh!

giuseppe commented Oct 30, 2020

Uh oh!

mtrmac Nov 2, 2020

Choose a reason for hiding this comment

Uh oh!

rhatdan Nov 3, 2020

Choose a reason for hiding this comment

Uh oh!

mtrmac Nov 3, 2020

Choose a reason for hiding this comment

Uh oh!

cgwalters commented Jan 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants