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

Security Extended Attributes are not preserved using buildah #2127

Closed
jeffh-id opened this issue Jan 31, 2020 · 21 comments · Fixed by containers/storage#657
Closed

Security Extended Attributes are not preserved using buildah #2127

jeffh-id opened this issue Jan 31, 2020 · 21 comments · Fixed by containers/storage#657
Assignees
Labels
Good First Issue This issue would be a good issue for a first time contributor to undertake. kind/bug Categorizes issue or PR as related to a bug. locked - please file new issue/PR

Comments

@jeffh-id
Copy link

Description

Steps to reproduce the issue:

  1. Sign a file using evmctl
  2. Include a copy of the file in a buildah image. For example, a Dockerfile with "COPY mycmd.sh /myapp, where mycmd.sh is signed.
  3. build the image using buildah. For example, "buildah bud -t myapp ."
  4. Find the layer directory where the mycmd.sh file is added to the image and execute "getfattr -n security.ima mycmd.sh"
  5. Note that the security.ima extended attribute is not present on the copy of the file.

Describe the results you received:

No such attribute from getfattr on the file

Describe the results you expected:

Should have returned a security.ima=<signature>

Output of rpm -q buildah or apt list buildah:

buildah/bionic,now 1.10.1-1~ubuntu18.04~ppa1 amd64 [installed]

Output of buildah version:

Version:         1.10.1
Go Version:      go1.10.4
Image Spec:      1.0.1
Runtime Spec:    1.0.1-dev
CNI Spec:        0.4.0
libcni Version:  
Git Commit:      
Built:           Thu Aug  8 16:29:48 2019
OS/Arch:         linux/amd64

Output of podman version if reporting a podman build issue:

(paste your output here)

Output of cat /etc/*release:

DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=18.04
DISTRIB_CODENAME=bionic
DISTRIB_DESCRIPTION="Ubuntu 18.04.3 LTS"
NAME="Ubuntu"
VERSION="18.04.3 LTS (Bionic Beaver)"
ID=ubuntu
ID_LIKE=debian
PRETTY_NAME="Ubuntu 18.04.3 LTS"
VERSION_ID="18.04"
HOME_URL="https://www.ubuntu.com/"
SUPPORT_URL="https://help.ubuntu.com/"
BUG_REPORT_URL="https://bugs.launchpad.net/ubuntu/"
PRIVACY_POLICY_URL="https://www.ubuntu.com/legal/terms-and-policies/privacy-policy"
VERSION_CODENAME=bionic
UBUNTU_CODENAME=bionic

Output of uname -a:

Linux jhewett-ubuntu-18 4.15.18-jh-ima-v1 #10 SMP Wed Aug 14 17:08:22 EDT 2019 x86_64 x86_64 x86_64 GNU/Linux

Output of cat /etc/containers/storage.conf:

# storage.conf is the configuration file for all tools
# that share the containers/storage libraries
# See man 5 containers-storage.conf for more information

# The "container storage" table contains all of the server options.
[storage]

# Default Storage Driver
driver = "overlay"

# Temporary storage location
runroot = "/var/run/containers/storage"

# Primary read-write location of container storage
graphroot = "/var/lib/containers/storage"

[storage.options]
# AdditionalImageStores is used to pass paths to additional read-only image stores
# Must be comma separated list.
additionalimagestores = [
]

# Size is used to set a maximum size of the container image.  Only supported by
# certain container storage drivers (currently overlay, zfs, vfs, btrfs)
size = ""

# OverrideKernelCheck tells the driver to ignore kernel checks based on kernel version
override_kernel_check = "true"

@TomSweeneyRedHat TomSweeneyRedHat self-assigned this Jan 31, 2020
@TomSweeneyRedHat
Copy link
Member

Thanks for the report @jeffh-id, looking into it.

@jeffh-id
Copy link
Author

Any progress?

@rhatdan
Copy link
Member

rhatdan commented Apr 27, 2020

Are you seeing this in rootless? Rootful? Both?
@giuseppe Any ideas?

@giuseppe
Copy link
Member

we don't copy xattrs except security.capability and user.*: https://github.com/containers/storage/blob/master/pkg/archive/archive.go#L503-L508

@rhatdan
Copy link
Member

rhatdan commented Apr 29, 2020

@jeffh-id is there a reason you would want the IMA stuff recorded?

@jeffh-id
Copy link
Author

It isn't so much a matter of having measurements (although those are impacted as well). If one enables IMA appraisal using a typical policy that enforces signatures of executables and shared libraries, then loads a container built with buildah, none of those executables in the container will be allowed to execute. Is there a reason the security attributes aren't preserved?

@rhatdan
Copy link
Member

rhatdan commented May 1, 2020

I don't think so, I just don't think many people were using them. As far as SELinux labels, it makes little sense to preserve these.

@jeffh-id
Copy link
Author

jeffh-id commented May 1, 2020

Sure, understand the SELinux bits. There is an increasing amount of IMA usage. Every application team can post-process their containers to add all the signatures back in, but I think we shouldn't need to do that.

@rhatdan
Copy link
Member

rhatdan commented May 1, 2020

@giuseppe Is this something that needs to be fixed in Buildah or container storage?

@jeffh-id
Copy link
Author

jeffh-id commented Jul 2, 2020

Is a fix planned or is the issue still being assessed? Would be helpful to know for planning purposes. Thx!

@rhatdan
Copy link
Member

rhatdan commented Jul 3, 2020

The best way to get this fixed would be to open a PR against containers/storage to add the xattr support.

@rhatdan
Copy link
Member

rhatdan commented Jul 3, 2020

Opened containers/storage#657 to save ima content into the container.

@yangfeiyu20102011
Copy link
Contributor

With the patch in containers/storage#657 , I found that there is something wrong with EA.

func ReadSecurityXattrToTarHeader(path string, hdr *tar.Header) error {
	if hdr.Xattrs == nil {
		hdr.Xattrs = make(map[string]string)
	}
	for _, xattr := range []string{"security.capability", "security.ima"} {
		capability, err := system.Lgetxattr(path, xattr)
		if err != nil && err != system.EOPNOTSUPP && err != system.ErrNotSupportedPlatform {
			return errors.Wrapf(err, "failed to read %q attribute from %q", xattr, path)
		}
		if capability != nil {
			hdr.Xattrs[xattr] = string(capability)
		}
	}
    logrus.Debugf("********path %v and xattr %v",path,hdr.Xattrs)
	return nil
}

File b which will be added to /home/b in the image, the EA as follows:

[root@hghphisprb05903 t]# getfattr -d -m - b
# file: b
security.abc="www"
security.capability1="www"
security.ima="imaok"
security.selinux="unconfined_u:object_r:admin_home_t:s0"
security.www="122121"
trusted.wq="www"
user.ok="www"
user.ok2="www2"

Debug info:

STEP 13: COMMIT bud
DEBU parsed reference into "[overlay@/var/lib/containers/storage+/var/run/containers/storage]localhost/bud:latest" 
DEBU COMMIT "containers-storage:[overlay@/var/lib/containers/storage+/var/run/containers/storage]localhost/bud:latest" 
DEBU committing image with reference "containers-storage:[overlay@/var/lib/containers/storage+/var/run/containers/storage]localhost/bud:latest" is allowed by policy 
DEBU parsed reference into "[overlay@/var/lib/containers/storage+/var/run/containers/storage]@386f9066e7c6f52654d3dea5a3a5d859eafeacbf79b6c268e9ace23cf68380e6" 
DEBU base image "386f9066e7c6f52654d3dea5a3a5d859eafeacbf79b6c268e9ace23cf68380e6" is already present in local storage, no need to copy its layers 
DEBU layer list: ["6eb4c21cc3fcb729a9df230ae522c1d3708ca66e5cf531713dbfa679837aa287" "5f27c9b971244fa0ea044fbb8187c3ecd262c986a1c56a7dfc7e4c5b755d3183"] 
DEBU using "/var/tmp/buildah807198897" to hold temporary data 
DEBU Tar with options on /var/lib/containers/storage/overlay/5f27c9b971244fa0ea044fbb8187c3ecd262c986a1c56a7dfc7e4c5b755d3183/diff 
DEBU ********path /var/lib/containers/storage/overlay/5f27c9b971244fa0ea044fbb8187c3ecd262c986a1c56a7dfc7e4c5b755d3183/diff/bar and xattr map[] 
DEBU ********path /var/lib/containers/storage/overlay/5f27c9b971244fa0ea044fbb8187c3ecd262c986a1c56a7dfc7e4c5b755d3183/diff/etc and xattr map[] 
DEBU ********path /var/lib/containers/storage/overlay/5f27c9b971244fa0ea044fbb8187c3ecd262c986a1c56a7dfc7e4c5b755d3183/diff/etc/resolv.conf and xattr map[] 
DEBU ********path /var/lib/containers/storage/overlay/5f27c9b971244fa0ea044fbb8187c3ecd262c986a1c56a7dfc7e4c5b755d3183/diff/home and xattr map[] 
DEBU ********path /var/lib/containers/storage/overlay/5f27c9b971244fa0ea044fbb8187c3ecd262c986a1c56a7dfc7e4c5b755d3183/diff/home/b and xattr map[] 
DEBU ********path /var/lib/containers/storage/overlay/5f27c9b971244fa0ea044fbb8187c3ecd262c986a1c56a7dfc7e4c5b755d3183/diff/run and xattr map[] 
DEBU ********path /var/lib/containers/storage/overlay/5f27c9b971244fa0ea044fbb8187c3ecd262c986a1c56a7dfc7e4c5b755d3183/diff/run/.containerenv and xattr map[] 
DEBU layer "5f27c9b971244fa0ea044fbb8187c3ecd262c986a1c56a7dfc7e4c5b755d3183" size is 4608 bytes 
.....................................................................
DEBU Using blob info cache at /var/lib/containers/storage/cache/blob-info-cache-v1.boltdb 
DEBU IsRunningImageAllowed for image containers-storage: 
DEBU  Using transport "containers-storage" policy section  
DEBU  Requirement 0: allowed                      
DEBU Overall: allowed                             
DEBU start reading config                         
DEBU finished reading config                      
Getting image source signatures
DEBU Manifest has MIME type application/vnd.oci.image.manifest.v1+json, ordered candidate list [application/vnd.oci.image.manifest.v1+json, application/vnd.docker.distribution.manifest.v2+json, application/vnd.docker.distribution.manifest.v1+prettyjws, application/vnd.docker.distribution.manifest.v1+json] 
DEBU ... will first try using the original manifest unmodified 
DEBU Skipping blob sha256:6eb4c21cc3fcb729a9df230ae522c1d3708ca66e5cf531713dbfa679837aa287 (already present): 
Copying blob 6eb4c21cc3fc skipped: already exists  
DEBU reading layer "sha256:148307db544e2ab625eca581adbda7b35cf6f8eca350c3efcbb1381e25e7b706" 
DEBU No compression detected                      
Copying blob 6eb4c21cc3fc skipped: already exists  
Copying blob 148307db544e done  
DEBU finished reading layer "sha256:148307db544e2ab625eca581adbda7b35cf6f8eca350c3efcbb1381e25e7b706" 
DEBU No compression detected                      
DEBU Using original blob without modification     
Copying config e7fed4184f done  
Writing manifest to image destination
Storing signatures
DEBU Applying tar in /var/lib/containers/storage/overlay/980e9d5da3c403cae204c9a9682dc15433fd20dd90ab1748fb3f58b495542a36/diff
DEBU setting image creation date to 2020-07-25 07:17:04.11807863 +0000 UTC 
DEBU created new image ID "e7fed4184fb33f1909aeaf1c98d4cf2c90cb4ec729bd4442ab9bf954d740129d" 
DEBU set names of image "e7fed4184fb33f1909aeaf1c98d4cf2c90cb4ec729bd4442ab9bf954d740129d" to [localhost/bud:latest] 
DEBU saved image metadata "{\"signatures-sizes\":{\"sha256:05e80d9c1ce82211c34305d9d5af58ad4bde8cf6fe749b2633d9073da276174d\":[]}}" 
--> e7fed4184fb
DEBU printing final image id "e7fed4184fb33f1909aeaf1c98d4cf2c90cb4ec729bd4442ab9bf954d740129d" 
e7fed4184fb33f1909aeaf1c98d4cf2c90cb4ec729bd4442ab9bf954d740129d

/var/lib/containers/storage/overlay/5f27c9b971244fa0ea044fbb8187c3ecd262c986a1c56a7dfc7e4c5b755d3183/diff/home/b should have security. and user. but it not.

getfattr gets the EA in the final path, security. and user. are not preserved.

getfattr -d -m "." /var/lib/containers/storage/overlay/980e9d5da3c403cae204c9a9682dc15433fd20dd90ab1748fb3f58b495542a36/diff/home/b

@rhatdan @TomSweeneyRedHat ,thanks.

@rhatdan
Copy link
Member

rhatdan commented Jul 25, 2020

Is storage writing them as well as reading them?

@yangfeiyu20102011
Copy link
Contributor

Is storage writing them as well as reading them?

I use the build-using-dockerfile cmd, it reads them and then writes them.

@rhatdan rhatdan reopened this Jul 27, 2020
@rhatdan
Copy link
Member

rhatdan commented Jul 27, 2020

@TomSweeneyRedHat PTAL

@rhatdan
Copy link
Member

rhatdan commented Aug 6, 2020

Are you sure Buildah is using the latest code from containers/storage?

@yangfeiyu20102011
Copy link
Contributor

Are you sure Buildah is using the latest code from containers/storage?

I have tried agagin, the problem also exists.
I use buildah(commit 1514dca)
and storage(commit 2c98b45e754bbf5c56a8a7889e0e820ae1df9314)

@rhatdan
Copy link
Member

rhatdan commented Oct 7, 2020

@yangfeiyu20102011 Do we still have the issue?

@rhatdan rhatdan added kind/bug Categorizes issue or PR as related to a bug. Good First Issue This issue would be a good issue for a first time contributor to undertake. labels Oct 7, 2020
@yangfeiyu20102011
Copy link
Contributor

yangfeiyu20102011 commented Oct 10, 2020

@yangfeiyu20102011 Do we still have the issue?

The PR will fix this.@rhatdan
https://github.com/containers/storage/pull/742/files

rhatdan added a commit to rhatdan/buildah that referenced this issue Oct 20, 2020
Fixes: containers#2127

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
rhatdan added a commit to rhatdan/buildah that referenced this issue Oct 20, 2020
Fixes: containers#2127

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
rhatdan added a commit to rhatdan/buildah that referenced this issue Oct 20, 2020
Fixes: containers#2127

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
rhatdan added a commit to rhatdan/buildah that referenced this issue Oct 21, 2020
Fixes: containers#2127

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
@rhatdan
Copy link
Member

rhatdan commented Feb 10, 2021

This should have been fixed in the current release.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Good First Issue This issue would be a good issue for a first time contributor to undertake. kind/bug Categorizes issue or PR as related to a bug. locked - please file new issue/PR
Projects
None yet
5 participants