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.