From 25de77ec662c7068e816e87e9400b87bf65105af Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Thu, 15 Oct 2026 17:37:32 +0200 Subject: [PATCH] image: Normalize modes again when an image is finalized A layer normalizes what it captures, but a layer travels through Buck as a directory artifact, and Buck stores no mode beyond the executable bit. A layer materialized from the cache therefore has every directory in the mode the host's umask allows: 0775 under a developer's 0002, 0755 under CI's 0022. sshd refuses a group-writable /usr/share/empty.sshd, so an image built from cached layers on such a host boots without a working SSH server, while CI never sees it. finalize.image() restored /tmp's 1777 as the one mode a layer cannot carry, but left the rest to the layers. Normalize the whole mounted stack there, the last point before the image is written, and only chmod what differs so the overlay copies up nothing it need not. The sysext driver writes /opt and the extension-release after that point, so it normalizes what it creates. layer.py uses the shared function; its test moves to finalize_test.py. --- docs/user/images.md | 3 +++ image/finalize.py | 46 +++++++++++++++++++++++++++++++ image/finalize_test.py | 61 ++++++++++++++++++++++++++++++++++++++++++ image/layer.py | 22 +-------------- image/layer_test.py | 32 ---------------------- image_format/sysext.py | 6 ++++- 6 files changed, 116 insertions(+), 54 deletions(-) diff --git a/docs/user/images.md b/docs/user/images.md index 116b34d7..a30b5485 100644 --- a/docs/user/images.md +++ b/docs/user/images.md @@ -12,6 +12,9 @@ provider vocabulary, the delta layer model, and disk composition internals) is d ## Restrictions File permissions in built images are normalized: 0755 for directories and executables, 0644 for files. +A layer normalizes what it captures, and the image is normalized again when it is finalized, because +Buck stores no mode beyond the executable bit and a layer out of the cache carries the umask of the +host that materialized it. This deliberately breaks suid/sgid executables and the sticky bit: these need to be replaced with socket-activated services and other safer alternatives. `/tmp` and `/var/tmp` keep their `1777`, which is restored when the image is finalized. Nothing on a `/usr` partition ought to be a secret, and diff --git a/image/finalize.py b/image/finalize.py index b5c720db..bcb187ca 100644 --- a/image/finalize.py +++ b/image/finalize.py @@ -11,7 +11,9 @@ """ import contextlib +import os import shutil +import stat import subprocess import sys from collections.abc import Generator, Sequence @@ -30,6 +32,47 @@ class ImageSpec(TypedDict): tmpfiles: list[str] +def normalize_mode(path: Path) -> None: + """Normalize one inode's permissions: 0755 for directories and executables, 0644 for files. + + See docs/user/images.md. Symlinks (a meaningless mode), whiteout nodes and anything else that is + not a file or directory are not content, so whatever they are they are not ours to rewrite. + """ + mode = path.lstat().st_mode + if stat.S_ISDIR(mode): + wanted = 0o755 + elif stat.S_ISREG(mode): + wanted = 0o755 if mode & 0o111 else 0o644 + else: + return + # A mode already right is left alone: on an overlay every chmod copies the inode up. + if stat.S_IMODE(mode) != wanted: + path.chmod(wanted) + + +def normalize_modes(tree: Path, *, skip: Sequence[Path] = ()) -> None: + """Normalize every mode beneath `tree`, leaving the paths in `skip` and what is below them alone. + + A layer normalizes what it captures, but Buck stores no mode beyond the executable bit, so a + directory comes back out of the cache with whatever the umask of the host materializing it + allows: 0o775 under a developer's 0o002, which sshd for one refuses in its privilege-separation + directory and CI, under 0o022, never sees. A file a terminal driver writes while finalizing, such + as an extension-release, carries that umask too. Here is the last point before the image is + written, so this is where the modes docs/user/images.md promises are made true. + """ + skipped = {Path(path) for path in skip} + normalize_mode(tree) + for root, dirs, files in os.walk(tree): + parent = Path(root) + for name in [*dirs, *files]: + path = parent / name + if path in skipped: + if name in dirs: + dirs.remove(name) + continue + normalize_mode(path) + + # The directories whose whole point is a mode, so a layer cannot carry them: sticky and # world-writable, which is exactly what Buck cannot store and the tar filter strips. _WORLD_WRITABLE = ("tmp", "var/tmp") @@ -60,6 +103,9 @@ def image( with rootfs.rootfs( "/buildroot", lowers=spec["lower"] if lowers is None else lowers, binds=binds ) as tree: + # The mounted stack carries the modes its layers were materialized with, not the image's. + # A bind is another artifact mounted in for the driver, and not this image's to touch. + normalize_modes(tree, skip=[tree / str(target).lstrip("/") for _, target in binds or ()]) # Before the snippets, so an image that states a mode of its own still wins. world_writable(tree) apply_tmpfiles(tree, spec["tmpfiles"], program=program) diff --git a/image/finalize_test.py b/image/finalize_test.py index ae5b5dfc..ff1ab810 100644 --- a/image/finalize_test.py +++ b/image/finalize_test.py @@ -6,6 +6,7 @@ buck test tine//image:test """ +import os import tempfile import unittest from pathlib import Path @@ -66,6 +67,66 @@ def test_an_uncommented_locale_is_a_request(self) -> None: self.assertTrue(finalize.wants_locales(self.tree)) +class TestNormalizeModes(unittest.TestCase): + @override + def setUp(self) -> None: + self.tree = Path(self.enterContext(tempfile.TemporaryDirectory(prefix="finalize.", dir="/var/tmp"))) + + def test_modes(self) -> None: + tree = self.tree + tree.chmod(0o700) + (tree / "sub").mkdir() + (tree / "sub").chmod(0o1777) + (tree / "sub/file").write_text("file") + (tree / "sub/file").chmod(0o664) + (tree / "sub/secret").write_text("secret") + (tree / "sub/secret").chmod(0o600) + (tree / "sub/script").write_text("script") + (tree / "sub/script").chmod(0o777) + (tree / "sub/mount").write_text("mount") + (tree / "sub/mount").chmod(0o4755) + (tree / "link").symlink_to("sub/secret") + os.mkfifo(tree / "fifo", 0o600) + + finalize.normalize_modes(tree) + + self.assertEqual(tree.stat().st_mode & 0o7777, 0o755) + self.assertEqual((tree / "sub").stat().st_mode & 0o7777, 0o755) + self.assertEqual((tree / "sub/file").stat().st_mode & 0o7777, 0o644) + self.assertEqual((tree / "sub/secret").stat().st_mode & 0o7777, 0o644) + self.assertEqual((tree / "sub/script").stat().st_mode & 0o7777, 0o755) + self.assertEqual((tree / "sub/mount").stat().st_mode & 0o7777, 0o755) + self.assertTrue((tree / "link").is_symlink()) + # not content: whatever it is, it is not ours to rewrite + self.assertEqual((tree / "fifo").lstat().st_mode & 0o7777, 0o600) + + def test_the_directories_a_materialized_layer_comes_back_with(self) -> None: + # Buck keeps no directory mode, so a layer out of the cache has the materializing host's umask. + for name in ("usr", "usr/share", "usr/share/empty.sshd"): + (self.tree / name).mkdir(mode=0o775) + finalize.normalize_modes(self.tree) + for name in ("usr", "usr/share", "usr/share/empty.sshd"): + self.assertEqual((self.tree / name).stat().st_mode & 0o7777, 0o755) + + def test_leaves_a_skipped_subtree_alone(self) -> None: + (self.tree / "run/tine/repart").mkdir(parents=True, mode=0o775) + (self.tree / "run/tine/repart/0.raw").write_text("blocks") + (self.tree / "run/tine/repart/0.raw").chmod(0o664) + (self.tree / "usr").mkdir(mode=0o775) + finalize.normalize_modes(self.tree, skip=[self.tree / "run/tine/repart/0.raw"]) + self.assertEqual((self.tree / "run/tine/repart/0.raw").stat().st_mode & 0o7777, 0o664) + self.assertEqual((self.tree / "run/tine/repart").stat().st_mode & 0o7777, 0o755) + self.assertEqual((self.tree / "usr").stat().st_mode & 0o7777, 0o755) + + def test_the_temporary_directories_get_their_mode_back_afterwards(self) -> None: + # The order image() applies them in: normalization first, the sticky bit restored after. + (self.tree / "tmp").mkdir(mode=0o1777) + finalize.normalize_modes(self.tree) + self.assertEqual((self.tree / "tmp").stat().st_mode & 0o7777, 0o755) + finalize.world_writable(self.tree) + self.assertEqual((self.tree / "tmp").stat().st_mode & 0o7777, 0o1777) + + class TestWorldWritable(unittest.TestCase): @override def setUp(self) -> None: diff --git a/image/layer.py b/image/layer.py index ef4fa734..00287ddf 100755 --- a/image/layer.py +++ b/image/layer.py @@ -9,7 +9,6 @@ import os import re import shutil -import stat import subprocess import sys import tempfile @@ -142,25 +141,6 @@ def _remove_glob(tree: Path, value: str) -> None: util.remove_path(path) -def _normalize_mode(path: Path) -> None: - """Normalize file permissions for path. - - See docs/user/images.md. - """ - mode = path.lstat().st_mode - if stat.S_ISDIR(mode): - path.chmod(0o755) - elif stat.S_ISREG(mode): - path.chmod(0o755 if mode & 0o111 else 0o644) - # ignore symlinks (meaningless mode), whiteout nodes, and non-files - - -def _normalize_modes(tree: Path) -> None: - _normalize_mode(tree) - for path in tree.rglob("*"): - _normalize_mode(path) - - def _apply_filesystem(operation: list[object]) -> None: match operation: case ["mkdir", str(path)]: @@ -271,7 +251,7 @@ def main(argv: list[str] | None = None) -> None: _install(install, target, Path(scratch)) for operation in operations: _apply(operation, target) - _normalize_modes(out) + finalize.normalize_modes(out) print(f"image: applied {len(operations)} ops over {len(lower)} lower(s) -> {out}", file=sys.stderr) diff --git a/image/layer_test.py b/image/layer_test.py index dd0516a2..f4501f71 100644 --- a/image/layer_test.py +++ b/image/layer_test.py @@ -6,7 +6,6 @@ buck test tine//image:test """ -import os import tempfile import unittest from pathlib import Path @@ -66,34 +65,3 @@ def test_pattern_must_be_an_absolute_image_path(self) -> None: for pattern in ("relative/*", "/safe/../outside"): with self.subTest(pattern=pattern), self.assertRaises(SystemExit): layer._remove_glob(Path("/"), pattern) - - -class TestNormalization(unittest.TestCase): - def test_modes(self) -> None: - with tempfile.TemporaryDirectory(prefix="layer-test.", dir="/var/tmp") as scratch: - tree = Path(scratch) - tree.chmod(0o700) - (tree / "sub").mkdir() - (tree / "sub").chmod(0o1777) - (tree / "sub/file").write_text("file") - (tree / "sub/file").chmod(0o664) - (tree / "sub/secret").write_text("secret") - (tree / "sub/secret").chmod(0o600) - (tree / "sub/script").write_text("script") - (tree / "sub/script").chmod(0o777) - (tree / "sub/mount").write_text("mount") - (tree / "sub/mount").chmod(0o4755) - (tree / "link").symlink_to("sub/secret") - os.mkfifo(tree / "fifo", 0o600) - - layer._normalize_modes(tree) - - self.assertEqual(tree.stat().st_mode & 0o7777, 0o755) - self.assertEqual((tree / "sub").stat().st_mode & 0o7777, 0o755) - self.assertEqual((tree / "sub/file").stat().st_mode & 0o7777, 0o644) - self.assertEqual((tree / "sub/secret").stat().st_mode & 0o7777, 0o644) - self.assertEqual((tree / "sub/script").stat().st_mode & 0o7777, 0o755) - self.assertEqual((tree / "sub/mount").stat().st_mode & 0o7777, 0o755) - self.assertTrue((tree / "link").is_symlink()) - # not content: whatever it is, it is not ours to rewrite - self.assertEqual((tree / "fifo").lstat().st_mode & 0o7777, 0o600) diff --git a/image_format/sysext.py b/image_format/sysext.py index f89775d7..8db17a6a 100755 --- a/image_format/sysext.py +++ b/image_format/sysext.py @@ -115,7 +115,11 @@ def main(argv: list[str] | None = None) -> None: release_dir = tree / "usr/lib/extension-release.d" release_dir.mkdir(parents=True, exist_ok=True) content = "".join(f"{key}={value}\n" for key, value in release.items()) - (release_dir / f"extension-release.{spec['name']}").write_text(content) + release_file = release_dir / f"extension-release.{spec['name']}" + release_file.write_text(content) + # Written after the mounted stack was normalized, so these carry this process's umask. + for created in (tree / "opt", release_dir.parent, release_dir, release_file): + finalize.normalize_mode(created) # Here rather than beside the DDI as an action of its own: this is the tree repart is about # to copy, edits and all, and nothing outside this driver has it.