From e09da0b51a40ed59403aa54e79436246cff9d920 Mon Sep 17 00:00:00 2001 From: Ivan Glushkov Date: Fri, 7 Aug 2026 15:41:46 +0000 Subject: [PATCH] builder: explain why an unparsable PAX extended header fails the build parse_entry() aborts the build when a record of a PAX extended header fails to parse, and says only which crate complained: tarball: failed to parse PaxExtension from tar header, malformed pax extension Nothing points at the entry, and nothing says why a build must end over a record the builder never reads. Name the entry, and write down the reason, because it is not the obvious one. A PAX record is "%d %s=%s\n" and its leading length spans the whole record, so a value may hold arbitrary bytes, newlines included. tar 0.4.45 splits the body on newlines before honouring that length, so every record in such a header comes back as an error. The tempting reading is that only SCHILY.xattr.* records are consumed here, so the rest can be skipped with a warning: that would convert npm .bin shims, stored as symlinks whose target is the shim's shell script, which are unconvertible today while docker pull handles them. It would also convert an attacker's symlink. Splitting the body leaves the tail of a value framed as a record of its own, and tar hands that fragment back as one, so a value holding "\n22 linkpath=/etc/evil\n" makes Entry::link_name() report /etc/evil while the ustar header of the entry says harmless. A reader walking the records by their length, Go's archive/tar among them, sees the value verbatim and no linkpath at all. path and size are read from those same bytes. Skipping the errors puts a symlink target, a path or a size of the tarball author's choosing into an image which no longer agrees with the layer it was built from, and does so silently. Nothing at this point can tell a fragment apart from a record, so the entries which cannot be read are the ones which must not be written. Three tests pin that: the error names the entry, the npm shim shape is rejected, and the smuggled linkpath never reaches a node. Walking the records by their leading length belongs in tar. Once that lands and the dependency is bumped, these images convert with their real targets rather than with degraded ones. Signed-off-by: Ivan Glushkov --- builder/src/tarball.rs | 108 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 98 insertions(+), 10 deletions(-) diff --git a/builder/src/tarball.rs b/builder/src/tarball.rs index eae7bc14b94..a9f3c40cda8 100644 --- a/builder/src/tarball.rs +++ b/builder/src/tarball.rs @@ -331,7 +331,17 @@ impl<'a> TarballTreeBuilder<'a> { } } - // Parse xattrs + // Parse xattrs. + // + // A failed record fails the build, though only `SCHILY.xattr.*` is read here: tar 0.4.45 + // splits the body on newlines before honouring a record's leading length, so the tail of + // a value comes back as a record of its own. A value holding "\n22 linkpath=/etc/evil\n" + // makes `entry.link_name()` report /etc/evil over the `harmless` in the ustar header, + // while a length-walking reader — Go's `archive/tar` — sees no `linkpath` at all. `path` + // and `size` come from those bytes too. See + // test_pax_record_smuggled_inside_a_value_cannot_reach_the_image. + // + // The fix belongs in tar; until it lands, such images do not convert. let mut xattrs = RafsXAttrs::new(); if let Some(exts) = entry.pax_extensions()? { for p in exts { @@ -346,7 +356,8 @@ impl<'a> TarballTreeBuilder<'a> { } Err(e) => { return Err(anyhow!( - "tarball: failed to parse PaxExtension from tar header, {}", + "tarball: failed to parse the PAX extended header of {}, {}", + path.display(), e )) } @@ -1048,26 +1059,103 @@ mod tests { assert!(err.to_string().contains("unknown target"), "{}", err); } + // Append a PAX extended header holding `body` verbatim, so that a record the tar crate + // writes correctly can be malformed on purpose. + fn append_pax_header(tar: &mut tar::Builder, name: &str, body: &str) { + let mut header = Header::new_ustar(); + header.set_entry_type(EntryType::XHeader); + header.set_mode(0o644); + header.set_mtime(0); + header.set_size(body.len() as u64); + tar.append_data(&mut header, format!("PaxHeaders/{}", name), body.as_bytes()) + .unwrap(); + } + + // Frame `record` as "%d %s", where the leading decimal counts the whole record including the + // digits of the count itself. That is self-referential, so widening the count can widen the + // record: settle it by feeding the framed length back until it stops growing. + fn pax_record(record: &str) -> String { + let mut len = record.len() + 1; + loop { + let framed = format!("{} {}", len, record); + if framed.len() == len { + return framed; + } + len = framed.len(); + } + } + #[test] fn test_malformed_pax_extension_is_rejected() { let tmp_dir = vmm_sys_util::tempdir::TempDir::new().unwrap(); let source_path = tmp_dir.as_path().join("bad-pax.tar"); let mut tar = tar::Builder::new(File::create(&source_path).unwrap()); // A PAX record starts with its own length in decimal, here reported too long. - let pax = "99 SCHILY.xattr.user.key=value\n"; + append_pax_header(&mut tar, "foo", "99 SCHILY.xattr.user.key=value\n"); + append_entry(&mut tar, "foo", EntryType::Regular, None, b"hello"); + tar.finish().unwrap(); + + let mut ctx = create_context(source_path, RafsVersion::V6); + let err = build_tree(&mut ctx).err().unwrap(); + // The entry is named, so an unconvertible image can be traced to what makes it one. + assert!(err.to_string().contains("/foo"), "{}", err); + } + + // The npm .bin shim shape: a symlink whose `linkpath` value carries newlines, which is legal + // — the leading length is what makes it legal — and which tar 0.4.45 still reports as + // malformed. Rejected on purpose; converting it needs the fix in tar. + #[test] + fn test_pax_record_containing_newline_is_rejected() { + let tmp_dir = vmm_sys_util::tempdir::TempDir::new().unwrap(); + let source_path = tmp_dir.as_path().join("newline-pax.tar"); + let mut tar = tar::Builder::new(File::create(&source_path).unwrap()); + let target = "#!/bin/sh\nbasedir=$(dirname \"$(echo \"$0\" | sed -e 's,\\\\,/,g')\")\n\n\ + case `uname` in\n *CYGWIN*) basedir=`cygpath -w \"$basedir\"`;;\nesac\n"; + assert!(target.len() > 100, "the target must need a linkpath record"); + let body = pax_record("SCHILY.xattr.user.key=value\n") + + &pax_record(&format!("linkpath={}", target)); + append_pax_header(&mut tar, "acorn", &body); let mut header = Header::new_ustar(); - header.set_entry_type(EntryType::XHeader); - header.set_mode(0o644); + header.set_entry_type(EntryType::Symlink); + header.set_mode(0o777); header.set_mtime(0); - header.set_size(pax.len() as u64); - tar.append_data(&mut header, "PaxHeaders/foo", pax.as_bytes()) - .unwrap(); - append_entry(&mut tar, "foo", EntryType::Regular, None, b"hello"); + header.set_size(0); + header.set_link_name(&target[..99]).unwrap(); + tar.append_data(&mut header, "acorn", &[][..]).unwrap(); + tar.finish().unwrap(); + + let mut ctx = create_context(source_path, RafsVersion::V6); + let err = build_tree(&mut ctx).err().unwrap(); + assert!(err.to_string().contains("/acorn"), "{}", err); + } + + // Why the error may not be downgraded to a warning: `22 linkpath=/etc/evil` lives inside the + // value of user.a, yet it is what `Entry::link_name` reports, over the `harmless` in the + // ustar header. Warning instead would put a symlink target of the tarball author's choosing + // into an image which disagrees with the layer it came from. + #[test] + fn test_pax_record_smuggled_inside_a_value_cannot_reach_the_image() { + let tmp_dir = vmm_sys_util::tempdir::TempDir::new().unwrap(); + let source_path = tmp_dir.as_path().join("smuggled-pax.tar"); + let mut tar = tar::Builder::new(File::create(&source_path).unwrap()); + let smuggled = pax_record("linkpath=/etc/evil\n"); + append_pax_header( + &mut tar, + "shim", + &pax_record(&format!("SCHILY.xattr.user.a=AAA\n{}", smuggled)), + ); + let mut header = Header::new_ustar(); + header.set_entry_type(EntryType::Symlink); + header.set_mode(0o777); + header.set_mtime(0); + header.set_size(0); + header.set_link_name("harmless").unwrap(); + tar.append_data(&mut header, "shim", &[][..]).unwrap(); tar.finish().unwrap(); let mut ctx = create_context(source_path, RafsVersion::V6); let err = build_tree(&mut ctx).err().unwrap(); - assert!(err.to_string().contains("PaxExtension"), "{}", err); + assert!(err.to_string().contains("/shim"), "{}", err); } #[test]