oci: layer: correctly handle trusted.overlay. xattr escaping

Signed-off-by: Aleksa Sarai <[email protected]>

Author: cyphar

CHANGELOG.md

@@ -90,6 +90,29 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
     extract plain whiteouts within an opaque whiteout directory in the same
     layer. (As per the OCI spec requirements, this is regardless of the order
     of the opaque whiteout and the regular whiteout in the layer archive.)
+- `UnpackLayer` and `Generate(Insert)Layer` now correctly handle
+  `trusted.overlay.*` xattr escaping when extracting and generating layers with
+  the overlayfs on-disk format. This escaping feature [has been supported by
+  overlayfs since Linux 6.7][linux-overlayfs-escaping-dad02fad84cbc], and
+  allows for you to created images that contain an overlayfs layout inside the
+  image (nested to arbitrary levels).
+  * If an image contains `trusted.overlay.*` xattrs, `UnpackLayer` will
+    rewrite the xattrs to instead be in the `trusted.overlay.overlay.*`
+    namespace, so that when merged using overlayfs the user will see the
+    expected xattrs.
+  * If an on-disk overlayfs directory used with `Generate(Insert)Layer`
+    contains escaped `trusted.overlay.overlay.*` xattrs, they will be rewritten
+    so that the generated layer contains `trusted.overlay.*` xattrs. If we
+    encounter an unescaped `trusted.overlay.*` xattr they will not be included
+    in the image (though they may cause the file to be converted to a whiteout
+    in the image) because they are considered to be an internal aspect of the
+    host on-disk format (i.e. `trusted.overlay.origin` might be automatically
+    set by whatever tool is using the overlayfs layers).
+  Note that in the regular extraction mode, these xattrs will be treated like
+  any other xattrs (this is in contrast to the previous behaviour where they
+  would be silently ignored regardless of the on-disk format being used).
+
+[linux-overlayfs-escaping-dad02fad84cbc]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=dad02fad84cbce30f317b69a4f2391f90045f79d
 
 ## [0.4.7] - 2021-04-05 ##
 

oci/layer/tar_extract.go

@@ -168,15 +168,15 @@ func (te *TarExtractor) restoreMetadata(path string, hdr *tar.Header) error {
 	// want, we first clear the set of xattrs from the file then apply the ones
 	// set in the tar.Header.
 	err := te.fsEval.Lclearxattrs(path, func(xattrName string) bool {
-		_, ok := ignoreXattrs[xattrName]
-		return ok
+		filter, isComplex := findComplexXattrFilter(xattrName)
+		return isComplex && !filter.UnpackShouldClear(te.whiteoutMode, hdr.Name, xattrName)
 	})
 	if err != nil {
 		if !errors.Is(err, unix.ENOTSUP) {
 			return fmt.Errorf("clear xattr metadata: %s: %w", path, err)
 		}
 		if !te.enotsupWarned {
-			log.Warnf("xattr{%s} ignoring ENOTSUP on clearxattrs", path)
+			log.Warnf("xattr{%s} ignoring ENOTSUP on clearxattrs", hdr.Name)
 			log.Warnf("xattr{%s} destination filesystem does not support xattrs, further warnings will be suppressed", path)
 			te.enotsupWarned = true
 		} else {
@@ -187,26 +187,37 @@ func (te *TarExtractor) restoreMetadata(path string, hdr *tar.Header) error {
 	for name, value := range hdr.Xattrs {
 		value := []byte(value)
 
-		// Forbidden xattrs should never be touched.
-		if _, skip := ignoreXattrs[name]; skip {
-			// If the xattr is already set to the requested value, don't bail.
-			// The reason for this logic is kinda convoluted, but effectively
-			// because restoreMetadata is called with the *on-disk* metadata we
-			// run the risk of things like "security.selinux" being included in
-			// that metadata (and thus tripping the forbidden xattr error). By
-			// only touching xattrs that have a different value we are somewhat
-			// more efficient and we don't have to special case parent restore.
-			// Of course this will only ever impact ignoreXattrs.
-			if oldValue, err := te.fsEval.Lgetxattr(path, name); err == nil {
-				if bytes.Equal(value, oldValue) {
-					log.Debugf("restore xattr metadata: skipping already-set xattr %q: %s", name, hdr.Name)
-					continue
+		// Some xattrs need to be skipped for sanity reasons, such as
+		// security.selinux, because they are very much host-specific and
+		// extracting them from layers would be a really bad idea. Other xattrs
+		// need to be remapped (such as trusted.overlay.* xattrs when in
+		// overlayfs mode) to have correct values.
+		mappedName := name
+		if filter, isComplex := findComplexXattrFilter(name); isComplex {
+			if newName := filter.UnpackEntry(te.whiteoutMode, hdr.Name, name); newName == nil {
+				// If the xattr is already set to the requested value, skip it
+				// more silently. The reason for this logic is kinda
+				// convoluted, but effectively because restoreMetadata is
+				// called with the *on-disk* metadata we run the risk of things
+				// like "security.selinux" being included in that metadata (and
+				// thus tripping the forbidden xattr error). By only touching
+				// xattrs that have a different value we are somewhat more
+				// efficient and we don't have to special case parent restore.
+				// Of course this will only ever impact ignoreXattrs.
+				if oldValue, err := te.fsEval.Lgetxattr(path, name); err == nil {
+					if bytes.Equal(value, oldValue) {
+						log.Debugf("xattr{%s} restore xattr metadata: skipping already-set xattr %q", hdr.Name, name)
+						continue
+					}
 				}
+				log.Warnf("xattr{%s} ignoring forbidden xattr: %q", hdr.Name, name)
+				continue
+			} else if *newName != name {
+				mappedName = *newName
+				log.Debugf("xattr{%s} remapping xattr %q to %q during extraction", hdr.Name, name, mappedName)
 			}
-			log.Warnf("xattr{%s} ignoring forbidden xattr: %q", hdr.Name, name)
-			continue
 		}
-		if err := te.fsEval.Lsetxattr(path, name, value, 0); err != nil {
+		if err := te.fsEval.Lsetxattr(path, mappedName, value, 0); err != nil {
 			// In rootless mode, some xattrs will fail (security.capability).
 			// This is _fine_ as long as we're not running as root (in which
 			// case we shouldn't be ignoring xattrs that we were told to set).
@@ -216,15 +227,15 @@ func (te *TarExtractor) restoreMetadata(path string, hdr *tar.Header) error {
 			//       unprivileged users (we also would need to translate them
 			//       back when creating archives).
 			if te.partialRootless && errors.Is(err, os.ErrPermission) {
-				log.Warnf("rootless{%s} ignoring (usually) harmless EPERM on setxattr %q", hdr.Name, name)
+				log.Warnf("rootless{%s} ignoring (usually) harmless EPERM on setxattr %q", hdr.Name, mappedName)
 				continue
 			}
 			// We cannot do much if we get an ENOTSUP -- this usually means
 			// that extended attributes are simply unsupported by the
 			// underlying filesystem (such as AUFS or NFS).
 			if errors.Is(err, unix.ENOTSUP) {
 				if !te.enotsupWarned {
-					log.Warnf("xattr{%s} ignoring ENOTSUP on setxattr %q", hdr.Name, name)
+					log.Warnf("xattr{%s} ignoring ENOTSUP on setxattr %q", hdr.Name, mappedName)
 					log.Warnf("xattr{%s} destination filesystem does not support xattrs, further warnings will be suppressed", path)
 					te.enotsupWarned = true
 				} else {
@@ -555,7 +566,23 @@ func (te *TarExtractor) UnpackEntry(root string, hdr *tar.Header, r io.Reader) (
 				if err != nil {
 					return fmt.Errorf("get xattr: %w", err)
 				}
-				dirHdr.Xattrs[xattr] = string(value)
+				// Because restoreMetadata will re-apply these xattrs
+				// (potentially remapping them if we have complexXattrs
+				// filters) we need to map their names to match what we would
+				// get from an actual archive. If the xattr should be ignored
+				// we can safely skip it here because UnpackShouldClear will
+				// also stop them from being cleared.
+				mappedName := xattr
+				if filter, isComplex := findComplexXattrFilter(xattr); isComplex {
+					if newName := filter.GenerateEntry(te.whiteoutMode, unsafeDir, xattr); newName == nil {
+						log.Warnf("xattr{%s} ignoring forbidden xattr: %q", unsafeDir, xattr)
+						continue
+					} else if *newName != xattr {
+						mappedName = *newName
+						log.Debugf("xattr{%s} remapping xattr %q to %q for later restoreMetadata", unsafeDir, xattr, mappedName)
+					}
+				}
+				dirHdr.Xattrs[mappedName] = string(value)
 			}
 		}
 

oci/layer/tar_extract_linux_test.go

@@ -223,3 +223,137 @@ func TestUnpackEntry_OverlayFSWhiteout_Nested(t *testing.T) {
 	assertIsOpaqueWhiteout(t, filepath.Join(dir, "opaque-nested/a/b"))
 	assertIsOpaqueWhiteout(t, filepath.Join(dir, "opaque-nested/a/b/c/d"))
 }
+
+func getAllXattrs(t *testing.T, path string) map[string]string {
+	names, err := system.Llistxattr(path)
+	require.NoErrorf(t, err, "fetch all xattrs for %q", path)
+	xattrs := map[string]string{}
+	for _, name := range names {
+		value, err := system.Lgetxattr(path, name)
+		require.NoErrorf(t, err, "fetch xattr %q for %q", name, path)
+		xattrs[name] = string(value)
+	}
+	return xattrs
+}
+
+func TestUnpackEntry_ComplexXattr_OverlayFS(t *testing.T) {
+	dir := t.TempDir()
+
+	testNeedsMknod(t)
+	testNeedsTrustedOverlayXattrs(t)
+
+	headers := []struct {
+		pseudoHdr
+		setXattrs, remappedXattrs map[string]string
+	}{
+		// Set a fake overlayfs xattr in the trusted.overlay namespace on a
+		// directory that contains entries. Since restoreMetadata() gets called
+		// on all parent directories when unpacking, this will cause
+		// restoreMetadata() to be run on the same directory multiple times.
+		// This lets us test that restoreMetadata will not re-apply the xattr
+		// escaping even after being called multiple times.
+		{
+			pseudoHdr{"foo", "", tar.TypeDir},
+			map[string]string{"trusted.overlay.fakexattr": "fakexattr"},
+			map[string]string{"trusted.overlay.overlay.fakexattr": "fakexattr"},
+		},
+		// Some subpaths with dummy overlayfs xattrs.
+		{
+			pseudoHdr{"foo/bar", "file", tar.TypeReg},
+			map[string]string{"trusted.overlay.whiteout": "foo"},
+			map[string]string{"trusted.overlay.overlay.whiteout": "foo"},
+		},
+		{
+			pseudoHdr{"foo/baz", "", tar.TypeDir},
+			map[string]string{"trusted.overlay.opaque": "y"},
+			map[string]string{"trusted.overlay.overlay.opaque": "y"},
+		},
+		// Several levels nested overlayfs xattrs.
+		{
+			pseudoHdr{"foo/extra-nesting", "", tar.TypeDir},
+			map[string]string{
+				"trusted.overlay.overlay.opaque":                                "x",
+				"trusted.overlay.overlay.overlay.whiteout":                      "foobar",
+				"trusted.overlay.overlay.overlay.overlay.overlay.overlay.dummy": "dummy xattr",
+			},
+			map[string]string{
+				"trusted.overlay.overlay.overlay.opaque":                                "x",
+				"trusted.overlay.overlay.overlay.overlay.whiteout":                      "foobar",
+				"trusted.overlay.overlay.overlay.overlay.overlay.overlay.overlay.dummy": "dummy xattr",
+			},
+		},
+		{
+			pseudoHdr{"foo/extra-nesting/reg", "reg", tar.TypeReg},
+			map[string]string{
+				"trusted.overlay.overlay.overlay.overlay.overlay.dummy123": "dummy xattr 123",
+			},
+			map[string]string{
+				"trusted.overlay.overlay.overlay.overlay.overlay.overlay.dummy123": "dummy xattr 123",
+			},
+		},
+	}
+
+	// With extraction using OverlayFSWhiteout we expect to get the remapped
+	// xattrs.
+	t.Run("OverlayFSWhiteout", func(t *testing.T) {
+		unpackOptions := UnpackOptions{
+			MapOptions: MapOptions{
+				Rootless: os.Geteuid() != 0,
+			},
+			WhiteoutMode: OverlayFSWhiteout,
+		}
+
+		te := NewTarExtractor(unpackOptions)
+
+		for _, ph := range headers {
+			hdr, rdr := fromPseudoHdr(ph.pseudoHdr)
+			hdr.Xattrs = ph.setXattrs
+
+			err := te.UnpackEntry(dir, hdr, rdr)
+			assert.NoErrorf(t, err, "UnpackEntry %s", hdr.Name)
+		}
+
+		for _, ph := range headers {
+			path := ph.pseudoHdr.path
+			fullPath := filepath.Join(dir, path)
+
+			xattrs := getAllXattrs(t, fullPath)
+			assert.Equalf(t, ph.remappedXattrs, xattrs, "UnpackEntry(%q): expected to see %#v remapped properly", path, ph.setXattrs)
+
+			// Make sure none of the files are whiteouts.
+			_, isWo, err := isOverlayWhiteout(fullPath, fseval.Default)
+			require.NoErrorf(t, err, "isOverlayWhiteout(%q)", path)
+			assert.Falsef(t, isWo, "isOverlayWhiteout(%q): regular entries with overlayfs xattrs should not end up being unpacked with overlayfs whiteout xattrs", path)
+		}
+	})
+
+	// With extraction using OverlayFSWhiteout we expect to get the remapped
+	// xattrs.
+	t.Run("OCIStandardWhiteout", func(t *testing.T) {
+		unpackOptions := UnpackOptions{
+			MapOptions: MapOptions{
+				Rootless: os.Geteuid() != 0,
+			},
+			WhiteoutMode: OCIStandardWhiteout,
+		}
+
+		te := NewTarExtractor(unpackOptions)
+
+		for _, ph := range headers {
+			hdr, rdr := fromPseudoHdr(ph.pseudoHdr)
+			hdr.Xattrs = ph.setXattrs
+
+			err := te.UnpackEntry(dir, hdr, rdr)
+			assert.NoErrorf(t, err, "UnpackEntry %s", hdr.Name)
+		}
+
+		for _, ph := range headers {
+			path := ph.pseudoHdr.path
+			fullPath := filepath.Join(dir, path)
+
+			xattrs := getAllXattrs(t, fullPath)
+			assert.Equalf(t, ph.setXattrs, xattrs, "UnpackEntry(%q): expected to see %#v not be remapped", path, ph.setXattrs)
+			assert.NotEqualf(t, ph.remappedXattrs, xattrs, "UnpackEntry(%q): expected to see %#v not be remapped", path, ph.setXattrs)
+		}
+	})
+}