layer: Xattr unpack code is broken on Fedora/RHEL

[runcom]

just have a busybox image copied with skopeo to OCI format, then:

$ umoci unpack --image busybox bundle
INFO[0000] parsed mappings                               map.gid=[] map.uid=[]
FATA[0000] create runtime bundle: chown rootfs: lchown bundle/rootfs: operation not permitted

$ sudo $GOPATH/bin/umoci unpack --image busybox bundle
INFO[0000] parsed mappings                               map.gid=[] map.uid=[]
INFO[0000] unpack manifest: unpacking layer sha256:56bec22e355981d8ba0878c6c2f23b21f422f30ab0aba188b54f1ffeff59c190  diffid="sha256:e88b3f82283bc59d5e0df427c824e9f95557e661fcb0ea15fb0fb6f97760f9d9"
FATA[0000] create runtime bundle: unpack layer: unpack entry: bin: apply hdr metadata: clear xattr metadata: /home/amurdaca/go/src/github.com/docker/containerd/bundle/rootfs/bin: lclearxattrs: lremovexattr(/home/amurdaca/go/src/github.com/docker/containerd/bundle/rootfs/bin, security.selinux): permission denied

[runcom]

if I add --rootless then everything appears to work

[cyphar]

Yup. I have a comment about "this probably breaks on RedHat" 😉. I'm not super familiar about Xattrs and what I should be doing here.

What I currently do is this:

1. Remove all xattrs that exist on a file if it already exists.
2. Set all xattrs that are specified in the header.

This is clearly not correct, so what should I be doing? Should I be modifying xattrs in place? What should I do about xattrs that are set on files that were not present in the image?

[runcom]

Set all xattrs that are specified in the header.

clearly my command fails there right? I would test if setting xattrs directly solves this
As for, what to do with xattrs present on file but not in the image, well, idk, is the image-spec requiring something specific to be done in this case?

[cyphar]

clearly my command fails there right?

No, it's failing when clearing. You can pass --debug to get the full backtrace from pkg/errors. But the wrapped error is "clear xattr metadata".

create runtime bundle: unpack layer: unpack entry: bin: apply hdr metadata: clear xattr metadata: /home/amurdaca/go/src/github.com/docker/containerd/bundle/rootfs/bin: lclearxattrs: lremovexattr(/home/amurdaca/go/src/github.com/docker/containerd/bundle/rootfs/bin, security.selinux): permission denied

I think the issue is that lremovexattr(security.selinux) won't work in SELinux because that would be a slight security issue. :P

I would test if setting xattrs directly solves this

It probably does. I will try it out, but is there a nice environment I can use to test SELinux? Will a Fedora VM work out-of-the box with SELinux in enforcing mode?

As for, what to do with xattrs present on file but not in the image, well, idk, is the image-spec requiring something specific to be done in this case?

Not sure. The spec is quite hazy about xattrs, let alone what to do in this case. My concern is that we accidentally include Xattrs that we shouldn't be into an image that then gets distributed.

[runcom]

It probably does. I will try it out, but is there a nice environment I can use to test SELinux? Will a Fedora VM work out-of-the box with SELinux in enforcing mode?

I usually spin up fedora vms out of qcow2 images (fedora cloud images) and everything works great with Selinux and the rest

[cyphar]

Okay, it "works" if I just remove the Lclearxattrs call. But I wonder if Lsetxattr will work if you actually include Xattrs. Here's the patch:

diff --git a/oci/layer/tar_extract.go b/oci/layer/tar_extract.go
index cb017d86f869..622b550e6d38 100644
--- a/oci/layer/tar_extract.go
+++ b/oci/layer/tar_extract.go
@@ -99,11 +99,7 @@ func (te *tarExtractor) restoreMetadata(path string, hdr *tar.Header) error {
        // Apply xattrs. In order to make sure that we *only* have the xattr set we
        // want, we first clear the set of xattrs from the file then apply the ones
        // set in the tar.Header.
-       // XXX: This will almost certainly break horribly on RedHat.
        if !te.mapOptions.Rootless {
-               if err := system.Lclearxattrs(path); err != nil {
-                       return errors.Wrapf(err, "clear xattr metadata: %s", path)
-               }
                for name, value := range hdr.Xattrs {
                        if err := system.Lsetxattr(path, name, []byte(value), 0); err != nil {
                                return errors.Wrapf(err, "restore xattr metadata: %s", path)

[runcom]

@rhatdan do you have any idea here?

[rhatdan]

I think it is fine to remove the xattrs or we could add a check for the selinux class and only remove the ones that are not security attributes.

[rhatdan]

This probably would not happen on devicemapper back end.

[cyphar]

@rhatdan Do you think an EPERM check would be sufficient? The only question is whether we should be including security. attributes inside OCI images (my gut says no, but I'm not sure how we should blacklist things).

This probably would not happen on devicemapper back end.

This is being done into a plain directory (I'm not using any storage drivers, it's just purely tar unpacking implemented in Go with some of my own tricks to ensure that the output is consistent).

[rhatdan]

I would not include security attributes, since those can vary from machine to machine, and could be different based on the paths you install.

[cyphar]

Are there any other xattrs you can think of that I shouldn't include? I'm a leaning on the side of not including any until the OCI spec figures out what xattrs should be included (/ping @vbatts since he probably has an opinion on this).

[rhatdan]

I think there is very little use of xattrs, other then for security. At least that I am aware of.

[cyphar]

Can you guys PTAL at #51 to see if that fixes the issue for you (it fixes the issue for me).

[cyphar]

I was looking it up, and security.capability attributes are used for set-capability binaries (which we definitely want to preserve the xattrs of). So I'll just blacklist SELinux for now (when generating an image).

[rhatdan]

LGTM

[cyphar]

By the way @runcom, the reason that oci-image-unpack doesn't work in your blog post is because of CAP_DAC_OVERRIDE (both fedora and opensuse have directories where the owner doesn't have rwx permissions -- which causes an unprivileged user problems).

umoci implements some tricks in --rootless mode that mean you can do image unpacking as an unprivileged user in these cases (and I also added a bunch of tests to make sure they work). The tricks are in the pkg/unpriv library, and it basically boils down to changing the mode of all of the directories so that you can get access to unpack a particular file and then reverting the mode change (and the mtime change) so that it all appears transparent.

Just in case you were still wondering why oci-image-tool unpack wasn't working properly. I would want to integrate this into oci-image-tool but I get the feeling they won't want all of these clever tricks in the image tooling. 😉

[runcom]

@cyphar cool. I don't feel strongly about having that in OCI image tools. I do feel umoci is great on unpacking from my experience so I'll stick with it for now. Of course, pushing missing functionalities on image-tools would be great as well.

[vbatts]

security is the most standard use xattrs, but there a number of projects that store arbitrary data in the user xattrs. but those are probably more "nice to have" rather than "must have".

[lukasheinrich]

Hi @cyphar I still see this issue on a RHEL7-based (CERN has its own flavor CERN Centos 7) distro:

   ⨯ create runtime bundle: unpack layer: unpack entry: .: apply hdr metadata: clear xattr metadata: unpacked_atlas/rootfs: unpriv.lclearxattrs: unpriv.lremovexattr: operation not supported

on fedora-25 vagrant box it works fine. If that's a too niche use-case I understand, but if you have any ideas on what to look at e.g. in the debug output to try to fix it, I'd be happy to take a look

(@traylenator -- perhaps this is interesting to you)

[cyphar]

@lukasheinrich Can you give me the list of xattrs that are applied for the root directory? I might need to add whatever xattr it is to the blacklist. Is it IMA or something similarly odd?

[lukasheinrich]

Ah -- I think I figured it out. I was trying to unpack within a AFS share, which probably has all kinds of xattrs defined. On the local disk of that machine it works.